feat(plugin): resumable probe sweeps and origin backoff; v0.56.0 - #131
Conversation
Two safety properties the probe was missing, both the same shape: a background job with no feedback from the system it loads. RESUMABLE SWEEPS. The walk cursor is in memory, so a restart mid-pass re-probed every URL the pass had already covered — hours of origin requests that can only confirm what is already stored, and we restart often. The stored baseline already carries probedAt, so a pass now skips a URL whose baseline is younger than reprobeAfter (default 12h, half the default sweep interval). A restarted pass reaches new work in seconds. The canary never skips, and a canary-triggered reseed never skips. ORIGIN BACKOFF. ratePerSecond is sized with the origin's operator for a healthy origin; it says nothing about one having a bad afternoon, and a sweep holding its configured rate through 429s and 503s adds load to something already failing. Pushback responses (429/502/503/504, connect and read timeouts) now double the pacing window; clean batches halve it back — immediate response, gradual recovery, steady state unchanged. An explicit Retry-After outranks the computed wait, and an origin refusing abortAfterDistress probes in a row ends the pass for the next scheduled one to retry cleanly. A 404 or a per-product 500 is NOT pushback and must not throttle a healthy sweep. 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 features to make change-probe sweeps resumable and resilient to origin distress, including a reprobeAfter option to skip recently probed URLs, and backoff/abort mechanisms (backoffMax, abortAfterDistress) when encountering origin distress. Review feedback recommends handling potential BigInt values during date parsing to avoid crashes, wrapping synchronous invocations of runProbeSweepOnce in try...catch blocks to safely catch synchronous exceptions, and clamping the calculated wait delay to Node.js's maximum timeout limit to prevent overflow warnings.
| if (!row) return null; | ||
| // A Date column can surface as a Date, an epoch number, or a string depending on how the row | ||
| // was written; anything unparseable reads as "age unknown", which probes rather than skips. | ||
| const probedAt = row.probedAt === undefined || row.probedAt === null ? NaN : new Date(row.probedAt).getTime(); |
There was a problem hiding this comment.
If row.probedAt is returned as a BigInt from the database, passing it directly to new Date() will throw a TypeError and crash the entire sweep. Coerce potential BigInt values to Number first, and wrap the instantiation in a try...catch block to safely default to NaN on any unparseable values.
let probedAt = NaN;
if (row.probedAt !== undefined && row.probedAt !== null) {
try {
const val = typeof row.probedAt === 'bigint' ? Number(row.probedAt) : row.probedAt;
probedAt = new Date(val).getTime();
} catch {
probedAt = NaN;
}
}References
- When parsing or coercing date values in JavaScript, always perform a truthiness check first (e.g., ensuring the value is not null or undefined) before passing it to
new Date(). This avoids bugs wherenew Date(null).getTime()evaluates to0(epoch 0) instead ofNaN.
There was a problem hiding this comment.
Good catch, fixed. A Long column surfacing as BigInt is a live trap in this codebase (resolveRenderInterval and resolveEffectiveInterval both coerce for it), and new Date() refusing a BigInt outright would have failed the read, not just the parse — taking the whole sweep down. Now coerced one type earlier.
Skipped the surrounding try/catch: past BigInt, new Date() returns Invalid Date rather than throwing for anything a column can hold, and NaN.getTime() already routes to the safe direction (probe, never skip). Pinned by a test using a BigInt-derived timestamp.
| const chained = sweepInterrupt; | ||
| sweepInterrupt = null; | ||
| if (chained) runProbeSweepOnce({ dryRun: true, label: chained }).catch((e) => logger.error(e)); | ||
| if (chained) runProbeSweepOnce({ dryRun: true, label: chained, reseed: true }).catch((e) => logger.error(e)); |
There was a problem hiding this comment.
When invoking a function that returns a Promise across an interface boundary or asynchronously without awaiting it, wrap the synchronous invocation itself in a try...catch block. If the function throws synchronously, the .catch() handler will be bypassed, propagating an unhandled exception.
if (chained) {
try {
runProbeSweepOnce({ dryRun: true, label: chained, reseed: true }).catch((e) => logger.error(e));
} catch (e) {
logger.error(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.
There was a problem hiding this comment.
Dismissing this one: runProbeSweepOnce is declared async (changeProbe.js:579), and an async function can never throw synchronously — a throw anywhere in its body, including before its first await, becomes a rejected promise. So the .catch() is reachable for every failure mode that exists here, and the try/catch would be unreachable code.
The guidance is sound where a callee might be a plain function returning a promise (the sync-throw path is real there); it just does not apply to an async callee. Same finding was raised and dismissed on the same grounds in #126.
| return { chained: true }; | ||
| } | ||
| runProbeSweepOnce({ dryRun: true, label }).catch((e) => logger.error(e)); | ||
| runProbeSweepOnce({ dryRun: true, label, reseed: true }).catch((e) => logger.error(e)); |
There was a problem hiding this comment.
Wrap the synchronous invocation of runProbeSweepOnce in a try...catch block to ensure that any synchronous exceptions thrown during the call are safely caught and do not bypass the .catch() handler.
try {
runProbeSweepOnce({ dryRun: true, label, reseed: true }).catch((e) => logger.error(e));
} catch (e) {
logger.error(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.
There was a problem hiding this comment.
Dismissing this one: runProbeSweepOnce is declared async (changeProbe.js:579), and an async function can never throw synchronously — a throw anywhere in its body, including before its first await, becomes a rejected promise. So the .catch() is reachable for every failure mode that exists here, and the try/catch would be unreachable code.
The guidance is sound where a callee might be a plain function returning a promise (the sync-throw path is real there); it just does not apply to an async callee. Same finding was raised and dismissed on the same grounds in #126.
| const wait = Math.max(window - elapsed, retryAfterMs); | ||
| if (wait > 0) await pause(wait); |
There was a problem hiding this comment.
Clamp the calculated wait delay to not exceed 2147483647 (the maximum 32-bit signed integer) before passing it to pause(). Exceeding this limit in Node.js causes a TimeoutOverflowWarning and executes the callback immediately (after 1ms), which can trigger unexpected hot loops.
| const wait = Math.max(window - elapsed, retryAfterMs); | |
| if (wait > 0) await pause(wait); | |
| const wait = Math.min(Math.max(window - elapsed, retryAfterMs), 2147483647); | |
| if (wait > 0) await pause(wait); |
References
- In Node.js, configuration options representing timeouts or delays passed to 'setTimeout' must enforce a maximum limit of 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.
There was a problem hiding this comment.
Agreed and applied, with one change of approach: clamped at the call site rather than via a schema max. The repo does cap individual timer options that way (sweepInterval, startDelay, canary.interval), but this window is the PRODUCT of concurrency, 1/ratePerSecond and the live backoff multiplier — three separately-sane values that can multiply past the limit with no single option out of range, so no per-option cap can close it.
The failure mode is worth naming: past the cap setTimeout fires after 1ms instead of waiting, so the backoff would invert into a hot loop against an origin that had just asked for room. Pinned by a test.
…he timer cap Both from PR review. A Long column can surface as BigInt and new Date() refuses rather than coerces it, so an unguarded read would take down the sweep. The pacing wait is the product of concurrency, 1/ratePerSecond and the backoff multiplier, so three separately-sane options can multiply past setTimeout's signed-32-bit delay — past which it fires after 1ms instead of waiting, turning the backoff into a hot loop. Co-Authored-By: Claude Fable 5 <[email protected]>
|
Addressed the review — pushed as a follow-up commit, 835/835 tests green. Applied:
Dismissed, with rationale on each thread:
|
1 participant
Why
Two safety properties the change probe was missing, and they are the same shape as the purge regression fixed in 0.55.1: a background job with no feedback from the system it loads. The purge had no backpressure from the storage engine and ended up 503-ing the render fleet. The probe has none from the origin, and no memory across restarts.
Resumable sweeps
The sweep's walk position is in-memory, so a restart mid-pass re-probes every URL the pass had already covered — hours of origin requests that can only confirm what is already stored. That is not hypothetical: a rollout plus an incident meant several restarts in one evening, and each one reset seeding progress.
The stored baseline already carries
probedAt, so no schema change was needed. A pass now skips a URL whose baseline is younger thanreprobeAfter(default 12h — half the defaultsweepInterval, kept below it so the skip can never eat a real pass's cadence). A restarted pass walks past covered ground in seconds and reaches new work immediately.Deliberate exemptions, both load-bearing:
This required inverting the read/probe order (the pass used to probe first, then read). That trades a node-local point read for an origin request — the right way round, since the origin request is the scarce, externally-visible resource.
Origin backoff
ratePerSecondis sized with the origin's operator for a healthy origin. It says nothing about an origin having a bad afternoon, and a sweep that holds its configured rate through 429s and 503s is adding load to something already failing.Pushback responses — 429, 502, 503, 504, and connect/read timeouts — now double the pacing window; a clean batch halves it back. Immediate response, gradual recovery, steady state unchanged at the configured rate. An explicit
Retry-Afteroutranks the computed wait (the origin named a number; we do not guess under it). An origin that refusesabortAfterDistressprobes in a row (default 50) ends the pass rather than crawling a doomed one into the next window while holding the sweep lock — the next scheduled pass is the retry and it starts clean.A 404 or a per-product 500 is explicitly not pushback. This corpus has a stable ~1.7% API-dark product floor; treating it as distress would throttle a perfectly healthy sweep down to nothing.
Two new
prerender_opsseries:probe_fresh(skipped as already-covered) andprobe_throttled(alert on this — it is the only signal that the probe is loading an origin that cannot take it).Verification
probedAtprobes rather than skips, backoff doubles-then-halves with exact window arithmetic,Retry-Afterwins, a fully-refusing origin aborts early, and a 404 floor never throttles.For the human reviewer
reprobeAftermust stay belowsweepInterval. At the defaults (12h vs 24h) there is a wide margin, but a deployment that raised it near the interval would silently stretch real cadence. The schema text says so; it is not enforced, because the sensible bound depends on how long a pass actually takes on that corpus.sweepIntervalbefore backing off again.Review coverage
Authored by Claude (Fable). Self-review + domain pass only — no outside-model leg ran; flagging per policy.
🤖 Generated with Claude Code