Skip to content

fix(replication): per-chunk send timeout to unwedge stalled blob streams - #451

Merged
kriszyp merged 5 commits into
mainfrom
fix/blob-sender-chunk-timeout
Jun 23, 2026
Merged

fix(replication): per-chunk send timeout to unwedge stalled blob streams#451
kriszyp merged 5 commits into
mainfrom
fix/blob-sender-chunk-timeout

Conversation

@ldt1996

@ldt1996 ldt1996 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Fixes #450.

Summary

sendBlobs in replication/replicationConnection.ts iterates the local blob with for await (const buffer of blob.stream()). When the underlying read stream stalls (production trigger: ENOENT or confidently-corrupt local blob file), the iterator's next .next() never resolves, the for-await waits forever, no finishing BLOB_CHUNK is sent, and the receiver-side apply consumer wedges at lastReceivedStatus:"Receiving" until its own idle watchdog (core/resources/blob.ts, harper#1443/#1444) fires ~120s later.

Fix

Rewrite the loop with explicit iterator.next() calls wrapped in Promise.race against a setTimeout reject. Gated by HARPER_BLOB_SEND_CHUNK_TIMEOUT_MS (default 0 = off, behavior unchanged for callers that do not set it; replication deploys set 120000ms). On timeout the catch path emits the finishing BLOB_CHUNK with error: 'Blob send chunk timeout after <ms>ms (fileId=<id>)' and errorCode / errorStatus derived from the underlying error, so the receiver can advance the resume cursor past a permanently-missing blob via the existing isPermanentSourceBlobErrorCode classification.

The receive-side idle watchdog (harper#1444) is the rescue when the sender doesn't close the frame; this fix lets the sender close it first, which is the cleaner direction.

Test plan

  • Diff parses cleanly (ts.transpileModule reports no diagnostics on the edited file)
  • Pre-existing TS errors in unrelated files (analytics/profile.ts, core/dataLayer/...) are unaffected
  • Manual: set HARPER_BLOB_SEND_CHUNK_TIMEOUT_MS=2000 on a sender, induce ENOENT on a referenced blob file, verify the receiver gets the finishing error frame within ~2s instead of waiting on its own watchdog
  • Soak: re-run the v4→v5 stage upgrade with both halves of the fix and confirm sender logs Blob send chunk timeout and not the receive-side Blob source stream idle (sender now closes the frame first)

Production observation

Akamai v4→v5 stage cluster (5.1.6-fixes2): nl-ams-1 logged 6077 ENOENT blob-send errors during the post-upgrade catch-up window. The receive-side watchdog (harper#1444) caught 420 of the resulting stalls. With this fix, the sender closes the frame in the configured window rather than depending on the receiver's watchdog.

Companion change

Receive-side idle watchdog: harper#1443 / harper#1444. Both halves are bundled in harperfast/harper-pro:5.1.6-fixes2.

@ldt1996
ldt1996 requested a review from a team as a code owner June 22, 2026 16:11

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces a per-chunk timeout mechanism when streaming blobs over WebSockets to prevent the send loop from hanging indefinitely if an underlying read stalls. The reviewer noted that manually iterating the async iterator and terminating early on timeout can leak resources (such as file descriptors), and suggested wrapping the iteration in a try-catch block to explicitly call iterator.return() for proper cleanup.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread replication/replicationConnection.ts
@claude

claude Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Reviewed; no blockers found.

Prior observations resolved: a0e207a coerces non-numeric HARPER_BLOB_SEND_CHUNK_TIMEOUT_MS to 0 (NaN guard); 55ccc95 calls iterator.return?.() in catch (FD leak closed). The 576b2ba enablement commit wires the default to blobTimeout (120s) so the guard is on out of the box. Code is correct — chunkTimeoutMs computation, Promise.race pattern, finally timer clearance, and catch iterator cleanup all look sound.

Carry-over observation (not a blocker, already raised): no unit test for the iterator.next() timeout race in sendBlobs; the kriszyp premise question about getBlobReadTimeout coverage in FileBackedBlob.stream() is also still open in the PR conversation.

@kriszyp

kriszyp commented Jun 22, 2026

Copy link
Copy Markdown
Member

Nice, surgical fix — racing iterator.next() against the timeout and letting the existing catch emit the finishing frame is exactly right, and it keeps the off-by-default contract clean. 🙏

One blocker before this lands as a fix (vs a dormant capability): it defaults to 0 (off) and I can't find anywhere that sets HARPER_BLOB_SEND_CHUNK_TIMEOUT_MS — not in harper-pro main, not on this branch. So merged as-is it's inert in production.

Live context for why this matters now: a customer cluster preprod 5.1.7 is wedged on exactly this mechanism (tracked in #453). The system-DB base copy is parked because a stalled deployment-payload blob send never emits its finishing frame, the receiver sits at lastReceivedStatus:"Receiving", COPY_COMPLETE never arrives, and replicated deploys are blocked — ~3h and counting, no self-heal. With this timeout enabled it would close the frame at 120s and converge.

Suggestion: mirror the existing blobTimeout right above (replicationConnection.ts:1003: env.get(CONFIG_PARAMS.REPLICATION_BLOBTIMEOUT) ?? 120000) — wire this to a config param with a sensible non-zero default in the replication path so it's on by default, rather than relying on an env var that nothing sets. Happy to add that enablement (here or as a follow-up) so #453 / a 5.1.8 patch actually resolves the wedge.

— Claude (Opus 4.8), from live a customer cluster investigation

…BTIMEOUT)

The per-chunk send timeout added in this PR defaulted to 0 (off), with nothing
setting HARPER_BLOB_SEND_CHUNK_TIMEOUT_MS anywhere — so it shipped inert and a
stalled blob send could still silently wedge a base copy (observed live on JJill
preprod 5.1.7, harper-pro#453: a missing system deployment-payload blob hung
sendBlobs, no finishing frame, the receiver stuck at "Receiving"/ver=0 forever,
COPY_COMPLETE never reached, replicated deploys blocked).

Default it to the existing replication blob timeout (REPLICATION_BLOBTIMEOUT,
120000ms default) so it's on out of the box and shares one operator knob with the
receiver-side blob timeout. HARPER_BLOB_SEND_CHUNK_TIMEOUT_MS still overrides it;
set it to 0 to disable.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@kriszyp

kriszyp commented Jun 22, 2026

Copy link
Copy Markdown
Member

Pushed an enablement commit (576b2ba) per @kriszyp's call: defaults chunkTimeoutMs to the existing replication blob timeout (blobTimeout = REPLICATION_BLOBTIMEOUT ?? 120000) instead of 0, so it's on out of the box and shares one operator knob with the receiver-side blob timeout. HARPER_BLOB_SEND_CHUNK_TIMEOUT_MS still overrides (set 0 to disable), so your opt-out path is preserved. Diff is one hunk; left your reject(...) formatting untouched. This is the half that directly unwedges a customer cluster (#453) — the leader's stalled system blob send now closes the frame at 120s and the followers advance.

— Claude (Opus 4.8)

Comment thread replication/replicationConnection.ts Outdated
@kriszyp

kriszyp commented Jun 22, 2026

Copy link
Copy Markdown
Member

@ldt1996 — a question on whether this is still needed, prompted by the #453 investigation. 🙏

Tracing the sender path on current core, sendBlobsblob.stream() (FileBackedBlob) already looks bounded by getBlobReadTimeout (storage_blobReadTimeout, 20s) at both points it could stall:

  • the open / ENOENT-retry loop — resources/blob.ts:351
  • the chunk-read watcher-stall timer — resources/blob.ts:514

So for a missing / corrupt / mid-write local blob, the for await rejects within ~20s, hits the catch, and sends the finishing error frame — it shouldn't hang indefinitely. The per-chunk iterator.next() timeout here would then be duplicating that guard (and it wraps iterator.next(), not the one genuinely-unbounded await in sendBlobs — the ws._socket.once('drain') backpressure wait).

Before I suggest closing this as redundant: was the Akamai v4→v5 stage-cluster case that prompted #450 going through a blob source that bypasses FileBackedBlob.stream()'s guards? e.g. a v4-era blob, a non-file-backed/raw stream, or a path where getBlobReadTimeout doesn't apply. If so, this PR is covering a real gap and we should keep it; if it's all the guarded file-backed path, I think we can close #451 as redundant.

(Separately — your receiver-side harper#1444 looks like it may cover a distinct gap: a saveBlob.saving promise that never settles even after the source is destroyed, which blobsTimer's stream.destroy() doesn't fully resolve. That one feels worth keeping regardless; happy to dig in if useful.)

Really nice instinct filing these from the soak cluster either way — just want to make sure we're not double-guarding the same path.

— Claude (Opus 4.8), from the #453 investigation

Comment thread replication/replicationConnection.ts Outdated
Comment thread replication/replicationConnection.ts Outdated
@kriszyp
kriszyp merged commit 7cec4ea into main Jun 23, 2026
31 checks passed
@kriszyp
kriszyp deleted the fix/blob-sender-chunk-timeout branch June 23, 2026 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Blob send loop hangs on a stuck iterator when the local blob file is missing or corrupt

2 participants