fix(seeders): guard S3-mode R2 ops + runSeed fetch-phase deadline (graceful degradation, #4786)#4789
Conversation
…#4786) S3-mode getR2JsonObject/putR2JsonObject had no request timeout while their Cloudflare-R2-API siblings bound every request with AbortSignal.timeout(30s). A stalled client.send, or a Body.transformToString() whose socket is silently reaped by the keep-alive agent, leaves a promise that NEVER settles — no rejection for a try/catch to catch, no open handle to keep the loop alive — so a seeder awaiting R2 at the top level (seed-forecasts prior-trace reads) drains the event loop and Node exits 13 ("Detected unsettled top-level await"): a red Railway badge that is neither a graceful skip nor a catchable failure. Two layers, both in shared modules so every seeder benefits: - _r2-storage.mjs: wrap both S3 sends + transformToString in a settle-timeout (+ per-call abortSignal) and give the cached S3Client a client-level requestTimeout/connectionTimeout. A stall now rejects -> withR2Retry -> caller fallback, instead of hanging. Fixes every R2-reading seeder. - _seed-utils.mjs runSeed: race the fetch phase against a wall-clock deadline (lockTtlMs + margin, per-seed -- never a fixed value, since seeders run from ~1min to 40min) so ANY non-settling await routes into the existing graceful exit-75 path (TTL extended, last-good served, no data lost). Fleet-wide backstop for the whole dangling-await class. Tests reproduce each hang (never-settling send/transformToString, hanging fetchFn) and prove graceful settle; the ordinary-rejection path still exits 75. Claude-Session: https://claude.ai/code/session_01HUHf9mxHGA2ujGvBAQ8m2v
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export function raceFetchDeadline(promise, ms, label) { | ||
| let timer; | ||
| const guard = new Promise((_, reject) => { | ||
| timer = setTimeout( | ||
| () => reject(new Error(`${label} fetch phase exceeded ${ms}ms deadline (likely a non-settling await — see issue #4786)`)), | ||
| ms, | ||
| ); | ||
| }); | ||
| return Promise.race([promise, guard]).finally(() => clearTimeout(timer)); | ||
| } |
There was a problem hiding this comment.
raceFetchDeadline duplicates withSettleTimeout
raceFetchDeadline (here) and withSettleTimeout (in _r2-storage.mjs) are structurally identical — both race a promise against a setTimeout guard and clear the timer in finally. If the timeout behaviour ever needs to change (e.g. unref the timer, adjust the error format) the fix has to be applied in two places. Extracting the shared pattern into a single helper in a shared utility module (e.g. _async-utils.mjs) would keep the two call sites as thin wrappers that only supply a label.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| function __setS3ClientForTests(client) { _s3ClientOverride = client; } | ||
| function __setR2S3TimeoutForTests(ms) { _s3TimeoutMs = ms == null ? R2_S3_TIMEOUT_MS : ms; } |
There was a problem hiding this comment.
__setS3ClientForTests and __setR2S3TimeoutForTests mutate module-level globals (_s3ClientOverride, _s3TimeoutMs) and are part of the module's public export surface. If any consuming code accidentally imports and calls these outside of a test context (or if a test suite fails before afterEach cleans up and the module is shared across test files in the same process), the global state silently persists for all subsequent calls. A comment or a runtime guard (e.g. checking process.env.NODE_ENV) would make the test-only intent explicit at the call site, or these could be scoped behind a secondary test-only entry point.
| await withSettleTimeout( | ||
| client.send(new PutObjectCommand({ | ||
| Bucket: config.bucket, | ||
| Key: key, | ||
| Body: body, | ||
| ContentType: 'application/json; charset=utf-8', | ||
| CacheControl: 'no-store', | ||
| Metadata: metadata, | ||
| }), { abortSignal: AbortSignal.timeout(_s3TimeoutMs) }), | ||
| _s3TimeoutMs, | ||
| `R2 s3 put ${key}`, | ||
| ); |
There was a problem hiding this comment.
client.send, all at the same deadline
client.send is guarded by (1) the client-level requestHandler.requestTimeout: 30s, (2) AbortSignal.timeout(_s3TimeoutMs) passed as the SDK options argument, and (3) the withSettleTimeout wrapper — all at the same _s3TimeoutMs value. Layers 1 and 2 both cause client.send to reject with an abort/timeout error when they fire, so the withSettleTimeout race guard is effectively redundant for the send call (though it is genuinely necessary for transformToString(), which has no abort-signal path). The only scenario where withSettleTimeout adds value on send is an SDK bug that ignores the abort signal, which is unlikely. Consider removing the AbortSignal.timeout argument from client.send so each layer guards a distinct failure mode, keeping the code easier to reason about.
| const fetchDeadlineMs = Number.isFinite(fetchPhaseTimeoutMs) && fetchPhaseTimeoutMs > 0 | ||
| ? fetchPhaseTimeoutMs | ||
| : lockTtlMs + FETCH_PHASE_DEADLINE_MARGIN_MS; |
There was a problem hiding this comment.
fetchDeadlineMs assumes lockTtlMs upper-bounds the fetch phase
When no fetchPhaseTimeoutMs is given, the deadline is lockTtlMs + 120s. The default lockTtlMs is 120 s, so any seeder that omits both opts gets a 240 s fetch-phase budget. The PR's invariant ("a healthy seeder never outlives its lock") holds if every long-running seeder sets a lockTtlMs proportional to its expected fetch time — but this isn't enforced at config time and is easy to miss when adding new seeders. A seeder with a fetch phase that legitimately runs beyond 4 minutes using the default lockTtlMs would now exit 75 on every run, silently serving stale data indefinitely. It's worth scanning the ~70 seeder files to confirm that every seeder whose fetch phase can exceed lockTtlMs + 120s explicitly overrides one or both opts, or surface a console warning when the derived deadline looks implausibly short.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
1 participant
Closes #4786. Fixes the observed
seed-forecastscrash and the whole class of "non-settling await → exit 13" failures fleet-wide, both in shared modules so all ~70 seeders benefit.Problem
S3-mode
getR2JsonObject/putR2JsonObject(scripts/_r2-storage.mjs) had no request timeout, while their Cloudflare-R2-API siblings bound every request withAbortSignal.timeout(30s). A stalledclient.send, or aBody.transformToString()whose socket is silently reaped by the keep-alive agent, leaves a promise that never settles — no rejection for atry/catchto catch, no open handle to keep the loop alive. A seeder awaiting R2 at the top level (seed-forecastsreading prior trace state) then drains the event loop and Node exits 13 (Detected unsettled top-level await) — a red Railway badge that is neither a graceful skip nor a catchable failure. Production runs S3 mode ([Trace] Storage mode=s3).Fix — two layers
1.
_r2-storage.mjs— close the specific holes (every R2-reading seeder):client.sendcalls andBody.transformToString()in awithSettleTimeout(+ per-callabortSignal). A stall now rejects →withR2Retry→ the caller's existing.catch(() => fallback), soseed-forecastsdegrades to "no priors this run" and still publishes.S3Clienta client-levelrequestTimeout: 30s/connectionTimeout: 10s(verified retained by@aws-sdk/[email protected]).2.
_seed-utils.mjsrunSeed— fleet-wide graceful-degradation backstop:lockTtlMs + margin, per-seed — never a fixed value, since seeders run from ~1min to 40min). Any non-settling await now routes into the existing graceful exit-75 path (TTL extended, last-good served, no data lost) instead of hanging into exit 13. Catches R2 and any future unguarded await.Tests (reproduce-first)
tests/r2-storage-s3-timeout.test.mjs— never-settlingsend/transformToStringnow reject; happy path + 404/empty-body unchanged;putR2JsonObjectcovered.tests/seed-runseed-fetch-deadline.test.mjs— a hangingfetchFnexits 75 (not 13) in ~55ms; an ordinary rejection still takes the same graceful path._seed-utils/_r2-storage/seed-contracttests pass (147 tests) undertsx --test.Blast radius
Both edits are additive (new exports, one optional opt, transparent wrappers) — no existing signature changes. Happy paths and the pre-existing graceful/SIGTERM paths are unchanged and covered.
https://claude.ai/code/session_01HUHf9mxHGA2ujGvBAQ8m2v