feat(plugin): change-driven re-rendering (changeProbe); v0.53.0 - #126
Conversation
Probe the origin for changes to the fields that actually invalidate a snapshot, and re-render only on change, instead of guessing with an interval: - rules: 'document' mode extracts schema.org JSON-LD Product offers (generic); 'request' mode probes a templated endpoint the page's own client code consults, extracting configured JSON paths. - observations reduce to a signature stored on the Target; the sweep walks each node's owned slice on a paced cadence and files changed URLs due now through the schedule funnel (revalidate semantics). - a small fixed canary cohort probed every few minutes detects mass changes (promotional events) and can record a bulk invalidation, then reseeds signatures without triggering — re-renders refill on cadence plus the invalidation accelerator. - probe failures change nothing by design (accelerator, never a gate); all-null extractions are failures, so an endpoint shape change cannot mass-trigger. Default off, dry-run on. - admin: GET/POST /prerender_admin/change-probe; metrics: prerender_ops probe_* series; config warnings for empty/dropped rule sets. Co-Authored-By: Claude Fable 5 <[email protected]>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a change-driven re-rendering feature called the 'change probe' to the prerender plugin, allowing pages to be re-rendered only when monitored fields (such as price and availability) actually change. It implements two cadences—a rolling sweep for continuous drift and a canary for mass-change events—along with configuration schema updates, metrics tracking, admin API endpoints, and comprehensive tests. Review feedback highlights several robustness and compatibility improvements for Node.js, including replacing URL.parse with new URL() for Node 20 compatibility, clamping timeouts to prevent 32-bit integer overflow in setInterval and setTimeout, wrapping Promise-returning functions in try...catch blocks to capture synchronous exceptions, and yielding the event loop during large database walks to prevent event loop starvation.
| const pathname = URL.parse(url)?.pathname; | ||
| if (pathname === undefined) return null; |
There was a problem hiding this comment.
Avoid using URL.parse as it is not supported in Node.js versions below v22.1.0 (such as Node 20). Use new URL() with appropriate error handling instead.
| const pathname = URL.parse(url)?.pathname; | |
| if (pathname === undefined) return null; | |
| let pathname; | |
| try { | |
| pathname = new URL(url).pathname; | |
| } catch { | |
| return null; | |
| } |
References
- Avoid using URL.parse if the target Node.js version is below v22.1.0, as it is not backported to Node 20. Use new URL() with appropriate error handling or pre-validation instead.
There was a problem hiding this comment.
Dismissing: URL.parse is the established idiom in this codebase (routeClass.js#classifyUrl, bot_request.js:160, three sites in RenderQueue.js — all on main), so the runtime floor is already ≥22.1 in practice, and the deployed runtime is Node 24. If the engines: >=20 field should be raised to match reality, that's a repo-wide one-liner for a separate PR rather than a per-call-site try/catch that would diverge this file from every sibling.
| const pathname = URL.parse(url)?.pathname; | ||
| if (pathname === undefined) return null; |
There was a problem hiding this comment.
Avoid using URL.parse as it is not supported in Node.js versions below v22.1.0 (such as Node 20). Use new URL() with appropriate error handling instead.
let pathname;
try {
pathname = new URL(url).pathname;
} catch {
return null;
}References
- Avoid using URL.parse if the target Node.js version is below v22.1.0, as it is not backported to Node 20. Use new URL() with appropriate error handling or pre-validation instead.
There was a problem hiding this comment.
Same as the sibling thread — URL.parse matches the repo-wide idiom (routeClass.js, RenderQueue.js, bot_request.js); Node 24 is the deployed runtime and the effective floor is already ≥22.1.
| const armIntervals = () => { | ||
| sweepTimer = setInterval(() => runProbeSweepOnce().catch((e) => logger.error(e)), armedSweep); | ||
| sweepTimer.unref?.(); | ||
| if (armedCanary) { | ||
| canaryTimer = setInterval(() => runProbeCanaryOnce().catch((e) => logger.error(e)), armedCanary); | ||
| canaryTimer.unref?.(); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Configuration options representing timeouts or delays passed to setInterval must be clamped to not exceed 2147483647 (the maximum 32-bit signed integer) to prevent unexpected immediate execution and hot loops. Additionally, wrap the Promise-returning function invocation in a try...catch block to handle any synchronous exceptions, and safely handle caught exceptions using e?.message ?? String(e).
const armIntervals = () => {
sweepTimer = setInterval(() => {
try {
runProbeSweepOnce().catch((e) => logger.error(e?.message ?? String(e)));
} catch (e) {
logger.error(e?.message ?? String(e));
}
}, Math.min(2147483647, armedSweep));
sweepTimer.unref?.();
if (armedCanary) {
canaryTimer = setInterval(() => {
try {
runProbeCanaryOnce().catch((e) => logger.error(e?.message ?? String(e)));
} catch (e) {
logger.error(e?.message ?? String(e));
}
}, Math.min(2147483647, armedCanary));
canaryTimer.unref?.();
}
};References
- In Node.js, clamp or validate configuration options representing timeouts or delays passed to setInterval or setTimeout to not exceed 2147483647 (the maximum 32-bit signed integer). Exceeding this limit causes Node.js to emit a 'TimeoutOverflowWarning' and execute the callback immediately (after 1ms), which can lead to unexpected immediate timeouts and system failures.
- When invoking a function that returns a Promise (especially across an interface boundary), wrap the synchronous invocation itself in a 'try...catch' block to capture any synchronous exceptions. If the function throws synchronously, asynchronous error handling (such as 'await' or '.catch()') will be bypassed, which can propagate unhandled exceptions and cause unexpected failures.
- When handling caught exceptions, safely access the error message using optional chaining and fall back to String(e) (e.g., e?.message ?? String(e)) to ensure that the error handling or logging block itself does not throw a TypeError if the caught exception is null, undefined, or a non-object.
There was a problem hiding this comment.
Fixed in e09bd03 (the clamp half): sweepInterval and canary.interval now carry the schema max: 2147483647 — the same guard page.blobReadBudgetMs documents, enforced at config validation rather than at each timer arm. The try/catch half is dismissed: runProbeSweepOnce/runProbeCanaryOnce are async functions, which cannot throw synchronously — a rejected promise is the only failure shape, and .catch covers it.
| bootTimer = setTimeout(() => { | ||
| runProbeSweepOnce().catch((e) => logger.error(e)); | ||
| armIntervals(); | ||
| }, config.changeProbe.startDelay + stagger); |
There was a problem hiding this comment.
Clamp the timeout passed to setTimeout to not exceed 2147483647 to prevent immediate execution, and wrap the Promise-returning function invocation in a try...catch block to handle synchronous exceptions. Ensure that error handling safely handles non-standard exceptions using e?.message ?? String(e) as this is on a critical boot-path operation.
bootTimer = setTimeout(() => {
try {
runProbeSweepOnce().catch((e) => logger.error(e?.message ?? String(e)));
} catch (e) {
logger.error(e?.message ?? String(e));
}
armIntervals();
}, Math.min(2147483647, config.changeProbe.startDelay + stagger));References
- In Node.js, clamp or validate configuration options representing timeouts or delays passed to setInterval or setTimeout to not exceed 2147483647 (the maximum 32-bit signed integer). Exceeding this limit causes Node.js to emit a 'TimeoutOverflowWarning' and execute the callback immediately (after 1ms), which can lead to unexpected immediate timeouts and system failures.
- When invoking a function that returns a Promise (especially across an interface boundary), wrap the synchronous invocation itself in 'try...catch' block to capture any synchronous exceptions. If the function throws synchronously, asynchronous error handling (such as 'await' or '.catch()') will be bypassed, which can propagate unhandled exceptions and cause unexpected failures.
- Ensure that error handling in critical boot-path operations (such as database subscriptions during startup) safely handles non-standard exceptions using
e?.message ?? String(e). ATypeErrorescaping a catch block on the boot path can cause the entire component or application to fail to load, whereas a safely handled error allows the system to fall back to a degraded but functional state (e.g., falling back to a poll instead of a subscription).
There was a problem hiding this comment.
Fixed in e09bd03: startDelay and startJitter are now bounded at 24h each in the schema, so their sum can never exceed setTimeout's signed-32-bit delay. The try/catch is dismissed for the async-function reason on the sibling thread.
| const run = action === 'sweep' ? runProbeSweepOnce({ dryRun }) : runProbeCanaryOnce({ dryRun }); | ||
| run.catch((e) => logger.error(e)); |
There was a problem hiding this comment.
When invoking a Promise-returning function across an interface boundary, wrap the synchronous invocation itself in a try...catch block to capture any synchronous exceptions. If the function throws synchronously, asynchronous error handling (such as .catch()) will be bypassed. Ensure caught exceptions are safely handled using e?.message ?? String(e).
try {
const run = action === 'sweep' ? runProbeSweepOnce({ dryRun }) : runProbeCanaryOnce({ dryRun });
run.catch((e) => logger.error(e?.message ?? String(e)));
} catch (e) {
logger.error(e?.message ?? String(e));
}References
- When invoking a function that returns a Promise (especially across an interface boundary), wrap the synchronous invocation itself in a 'try...catch' block to capture any synchronous exceptions. If the function throws synchronously, asynchronous error handling (such as 'await' or '.catch()') will be bypassed, which can propagate unhandled exceptions and cause unexpected failures.
- When handling caught exceptions, safely access the error message using optional chaining and fall back to String(e) (e.g., e?.message ?? String(e)) to ensure that the error handling or logging block itself does not throw a TypeError if the caught exception is null, undefined, or a non-object.
There was a problem hiding this comment.
Dismissed: both branches call an async function, and an async function cannot throw synchronously — invocation always yields a promise, so the .catch is complete. Wrapping it would be unreachable code.
| // changed set dwarfs maxTriggersPerSweep, so a triggering sweep would leave most signatures | ||
| // stale and then drip re-renders of ALREADY-HEALED pages (a render never updates the | ||
| // signature; only a probe does) pass after pass until the baseline caught up. | ||
| runProbeSweepOnce({ dryRun: true, label: `reseed after invalidating ${scope}` }).catch((e) => logger.error(e)); |
There was a problem hiding this comment.
Wrap the Promise-returning function invocation in a try...catch block to capture any synchronous exceptions and prevent unhandled rejections or crashes. Ensure caught exceptions are safely handled using e?.message ?? String(e).
try {
runProbeSweepOnce({ dryRun: true, label: 'reseed after invalidating ' + scope }).catch((e) => logger.error(e?.message ?? String(e)));
} catch (e) {
logger.error(e?.message ?? String(e));
}References
- When invoking a function that returns a Promise (especially across an interface boundary), wrap the synchronous invocation itself in a 'try...catch' block to capture any synchronous exceptions. If the function throws synchronously, asynchronous error handling (such as 'await' or '.catch()') will be bypassed, which can propagate unhandled exceptions and cause unexpected failures.
- When handling caught exceptions, safely access the error message using optional chaining and fall back to String(e) (e.g., e?.message ?? String(e)) to ensure that the error handling or logging block itself does not throw a TypeError if the caught exception is null, undefined, or a non-object.
There was a problem hiding this comment.
Dismissed for the same reason: requestSweepReseed handles the promise internally with .catch, and the call sits inside runProbeCanaryOnce's own try/catch besides.
| stats.examined++; | ||
| if (!ownershipChecked && ownerOf(row.url) !== hostname) continue; |
There was a problem hiding this comment.
When iterating over large database cursors or registries, yield the event loop unconditionally based on the number of rows scanned to prevent event loop starvation. Use global.setImmediate to avoid potential conflicts with promisified setImmediate imports.
| stats.examined++; | |
| if (!ownershipChecked && ownerOf(row.url) !== hostname) continue; | |
| stats.examined++; | |
| if (stats.examined % 200 === 0) { | |
| await new Promise((resolve) => global.setImmediate(resolve)); | |
| } | |
| if (!ownershipChecked && ownerOf(row.url) !== hostname) continue; |
References
- When iterating over large database cursors or registries in Node.js, yield the event loop (e.g., using setImmediate) unconditionally based on the number of rows scanned, rather than conditionally on writes or specific branches. This prevents event loop starvation, as awaiting on cursors only drains microtasks. Use a consistent count-based trigger across the codebase.
- When importing the promise-based
setImmediatefromnode:timers/promises, ensure that no manualnew Promise(setImmediate)wraps survive in the same scope. Shadowing the globalsetImmediatewith the promisified version causesnew Promise(setImmediate)to passresolveas a value to the promisified function, returning a promise that never settles and leading to silent hangs.
There was a problem hiding this comment.
Fixed in e09bd03: the pass loop now yields every 200 examined rows via the promisified node:timers/promises setImmediate (injected as onYield, the same shape and cadence as util/reconcile.js). You're right that skipped rows never reached the paced flush — a chunk of pure skips ran as one synchronous burst.
…t-and-chain the post-trip reseed Pre-push review findings (Claude advisory + Opus domain adjudication — no independent outside model was available on this machine): - the origin security token and staging-IP pin now attach ONLY when the probe endpoint shares the probed page's origin; a third-party host a rule names gets a plain fetch, never the bypass secret (the repo's own origin-bypass scoping rule). - a canary trip mid-sweep now INTERRUPTS the running sweep and chains the reseed when it stands down — previously the reseed silently no-op'd in exactly the scenario the canary exists for. - HEAD is refused at rule compile (extraction parses the body; a HEAD probe validated and then failed on every probe). - 3xx responses fail with an explicit no-redirects message; documented. - sweep-built canary cohorts are now the lowest-N-by-hash keyspace sample rather than the alphabetical head of the candidate set. - comment dedup; four new tests (same-origin gate, HEAD refusal, cohort sampling, reseed direct + interrupt-and-chain). 807/807 pass. Co-Authored-By: Claude Fable 5 <[email protected]>
Final self-review finding: labels key the canary cohorts, pass records and log lines, and nothing enforced uniqueness — two rules configured with the same label silently shared one cohort and mis-attributed each other's passes. A duplicate is uniquified (label#index) with a warning rather than dropped: losing probe coverage over a naming clash is the worse trade. 808/808 tests. Co-Authored-By: Claude Fable 5 <[email protected]>
…ield the sweep loop on skipped rows Review-bot findings: sweepInterval and canary.interval get the page.blobReadBudgetMs schema max (a delay past 2^31-1 makes setInterval fire immediately — a hot loop, not 'effectively never'); startDelay and startJitter are bounded at 24h each so their sum stays under the cap. The pass loop now yields every 200 examined rows — skipped rows (unowned/unmatched, most of a multi-node registry) never reach the paced flush, so a chunk of pure skips ran as one synchronous burst (util/reconcile.js's cadence). 808/808 tests. Co-Authored-By: Claude Fable 5 <[email protected]>
|
Addressed the review-bot findings in e09bd03: timer-delay clamps via schema |
…nchor empty-replicable databases Signatures move off the replicated Target rows into ProbeState (replicate: false) in its own probe_state database: - the sweep is owner-scoped, so a URL's baseline is only ever read and written by its owner — replication shipped every baseline to three nodes that never consult it. Seeding and post-trip reseeds now cost zero replication and a quarter of the audit. - a lost baseline (ownership move, node rebuild) re-SEEDS on the next pass — the safe direction by construction. Target.delete removes the baseline with the cached pages; a sitemap re-put no longer clears it. - own database so corpus-scale reseed bursts serialize against nothing. Both all-non-replicable databases (probe_state and the existing coordination) gain a deliberately empty REPLICATED anchor table: a database with zero replicable tables spins the replication subscription in a disconnect/retry loop measured at ~4,300 cycles/node/day (harper-pro#685). The anchor turns that into an ordinary idle stream at zero traffic. 808/808 tests. Co-Authored-By: Claude Fable 5 <[email protected]>
|
Design revision from review discussion (latest commit): probe baselines moved off the replicated Target rows into a node-local |
…per worker; v0.54.0 The sweep yielded every 200 rows. That constant was chosen when `bench/queue-index` measured a row at ~2.4us — 200 rows was ~0.5ms of held event loop, invisible beside a ~1.6ms cache hit. On the production corpus a row costs ~55us, so those same 200 rows hold the loop for ~11ms, and the sweep runs on a worker that also serves bot traffic. Every crawler request landing inside a slice waits for it. A row count cannot express "do not stall a request". It now yields on elapsed time (`queue.ready.yieldBudget`, default 2ms — just above the 1.6ms a cache hit takes to serve), so a delayed request waits about as long as it would have taken to answer. The clock is sampled every 32 rows rather than every row: at ~55us/row a per-row read is free, but the reason this is being rewritten at all is that a per-row cost moved 20x, and at 2.4us/row a per-row clock read would be ~2%. This is the same class of bug as the sweepCap waste in v0.52.0: a constant sized against a per-row cost that turned out to be 20x higher. AND THE ATTRIBUTION, because the evidence for the above stops short of proof. Bot-facing `duration` (path: 'p') on the live fleet shows median and p95 identical across every worker (1.6ms / ~2.7ms) while five workers carry a p99 of 13-53ms against 3.6-7.4ms elsewhere. Median flat, p95 flat, p99 blown out 10-30x is the signature of intermittent loop blocking. But the sweep self-gates to workerIndex 0 and the tail covers five workers, so it is CONSISTENT with the sweep and cannot be pinned on it — the queue-status sync, the reconciler, the sitemap refresh and GC are all indistinguishable in that data. `prerender_ops` `event_loop_lag` fixes that. It reports libuv's own histogram per worker (p99 and max, dimensioned by worker index), on every worker rather than pinned to one — which is the entire point, since a cluster-wide number averages away exactly the signal being looked for. Whatever only worker 0 does then shows up only on worker 0. `readLag` is extracted and tested because its failure is silent and fleet-wide: an empty window returns Infinity from percentile(), and recordAnalytics aggregates by mean, so one Infinity makes the merged row's mean Infinity for that period across every worker — the series reads as catastrophic while nothing is wrong. 785 tests pass. The new sweep test pins the invariant that matters for a scheduling change: the yield budget changes WHEN the walk pauses and never what it publishes, verified by running the same corpus at 1ms and at 1e9ms. Co-Authored-By: Claude Opus 5 <[email protected]> Rebased onto main after #126. Version moved 0.53.0 -> 0.54.0 (main took 0.53.0). The only conflict was METRICS.md table reflow — #126 added probe_* to the prerender_ops row, this adds event_loop_lag to the same row; both are present. #126's commit message mentions yielding "the sweep loop", but that is the change-probe's pass loop, not this one — renderSchedule.js is untouched by it, so there is no logic overlap. Also adopts the schema max #126 established for timer delays: management.eventLoopLagInterval is an interval delay, the same shape as the sweepInterval/canary.interval bug that PR fixed, so it now carries the same 2^31-1 cap. The code already clamped; this makes it warn by name.
1 participant
What
A render interval bounds staleness blind: it pays a full headless-Chrome render per page per interval whether or not anything changed, and still misses every change that lands mid-interval. For the fields that actually invalidate a snapshot — price and availability on commerce pages being the canonical case — the origin can answer "did it change?" thousands of times cheaper than a render can.
changeProbe(default off, and dry-run even when on) asks exactly that and re-renders only on change.How
Rules (
changeProbe.rules, firstpathPatternmatch wins):source: document— the generic mode: GET the page itself, extract its schema.org JSON-LDProductoffers (price/currency/availability). Nothing site-specific to configure.source: request— probe an endpoint the page's own client code consults (urlTemplatewith$1..$9from pattern captures, URI-encoded), extracting configured JSON value paths.Extracted values reduce to a signature stored on the Target (
probeSignature, thedemandIntervalwritten-only-on-change discipline;Target.putclears it deliberately). A differing observation expires the URL's cached pages and files every device row due now — the exactTarget.revalidateper-URL shape, through the schedule funnel, withcurrentMinuteMs()taken per trigger.Two cadences for the two ways content changes:
sweepInterval) walks each node's owned slice (reconcile-style: owner-scoped so the funnel's floor lowering covers the keys it writes), in cursor-bounded chunks (no read txn open across probes/writes), paced byratePerSecond/concurrency. Catches continuous drift — availability sell-through, item-level price moves. Triggers per pass are capped (maxTriggersPerSweep); excess changes stay detected and retry next pass (signature deliberately left stale).canary.*) probes a small deterministic cohort (1-in-16 keyspace stride, firstcountper rule per node, rebuilt by each sweep, built lazily after restart) everycanary.interval. Commerce price steps at promotional events — most of a catalog at once — which a sample of hundreds sees within minutes while a sweep is hours away. On a trip: records the rule'sinvalidateScopebulk invalidation (one row; bots get correct origin content immediately) and kicks a reseed sweep (dry-run semantics: re-baseline signatures, trigger nothing — per-URL triggers would be redundant with the invalidation and would otherwise drip re-renders of already-healed pages for weeks, because only a probe updates a signature). Aholdoffprevents re-stamping the epoch while the corpus refills, which would re-invalidate exactly the pages that just healed.Failure semantics — the load-bearing part: a probe failure (fetch error, non-2xx, unparseable body, or an extraction where every path is null) changes nothing — no signature write, no trigger. The probe is an accelerator on top of the baseline cadence, never a gate on it. An endpoint replatforming under a rule surfaces as a
probe_failedshare and a loud log line (>50% failure warning), not as schedule churn — the all-null rule is what stops a shape change from flipping every signature at once and mass-triggering.Surfaces
GET /prerender_admin/change-probe(rules, last sweep/canary records incl. failure samples, cohort sizes),POST /prerender_admin/change-probe{ action: "sweep"|"canary", dryRun? }.prerender_opsprobe_*series (probed/seeded/changed/triggered/deferred/failed, canary_trip, invalidated); catalog + METRICS.md updated, alerting row added.Operational notes
ratePerSecondwith whoever runs the origin before enabling a sweep over a large corpus.renderIntervalcan be raised substantially — the interval then only bounds what the probe cannot see (reviews, images), and the freed render budget is what pays for mass-change refills.Testing
npm run lint/format:checkclean for touched files.packages/console/test/trafficView.test.jshas 3 pre-existingno-unused-varslint errors from the v0.51.0 commit — untouched here, worth a separate one-liner.Version
@harperfast/prerender0.52.0 → 0.53.0 (reserving 0.53.0 for this PR).🤖 Generated with Claude Code
Pre-push review (cross-model, degraded)
Coverage, honestly: no independent outside model was reachable on this machine (codex CLI auth expired; agy/cursor-agent not installed; no GEMINI_API_KEY). The pass that ran was a Claude advisory + Opus domain adjudication — same-family, so it does not count as cross-model coverage. Verdict was CHANGES with 2 majors; all findings are addressed in the follow-up commit:
request-mode host. Now same-origin-gated (isSameProbeOrigin); a third-party endpoint gets a plain fetch. This also answers the review's open judgment call:request.urlTemplateMAY name a third-party host — the token is what's origin-only.requestSweepReseed), instead of silently skipping in exactly the scenario the canary exists for.sort: kept deliberately — keyset pagination needs deterministic order, andPrerenderAdmin.listPagesInneris the precedent forsort+ PK condition.Suite after fixes: 807/807 (4 new tests: same-origin gate, HEAD refusal, cohort sampling, reseed direct + interrupt-and-chain).