Repository navigation
fix(replication): re-request in-flight blobs on connection close instead of waiting out the idle timeout - #527
Merged
Conversation
…ead of waiting out the idle timeout A component deploy triggers `Restarting http_workers`, which tears down the worker mid-stream while it is sending a replication blob. The receiver was leaving the half-written blob receive streams in `blobsInFlight` for core's source-idle watchdog to reap up to `blobTimeout` (REPLICATION_BLOBTIMEOUT, now 900s) later, only then stamping a PENDING stub — so a diverged deploy-payload blob lingered for the whole timeout before the reconnect could re-request it (seen live as a week-old PENDING stub). On `ws` close, abort each still-receiving blob immediately (new `abortInFlightBlobsOnClose`): the plain Error is classified transient by `receiveBlobs`, which clamps the resume cursor so the reconnect re-streams the blob promptly. Completed-but-unconnected streams (`writableEnded` — chunks that arrived ahead of their record) are preserved so an in-flight handler can still attach and save them. The abort error is marked `replicationConnectionClosed` so `receiveBlobs` treats it as a routine, self-healing interruption — clamp + re-request — without logging an error or bumping the cluster_status divergence metric on every deploy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![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 mechanism to immediately abort in-flight blob receives when a replication connection closes, avoiding long timeouts and ensuring prompt re-requests upon reconnection. It includes the helper functions abortInFlightBlobsOnClose and isReplicationConnectionClosedError, integrates them into the WebSocket teardown and error handling paths, and adds comprehensive unit tests. The review feedback suggests improving the robustness of the error message when remoteNodeName is undefined, and adding connectionId to the debug log for better traceability.
This comment has been minimized.
This comment has been minimized.
- fall back to 'unknown' in the abort error when remoteNodeName is unset (pre-handshake close) - include connectionId in the connection-close debug log for correlation Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
Patch cherry-pick: mergedCherry-picked onto |
cb1kenobi
reviewed
Jul 8, 2026
…ableEnded blob streams on close A completed-but-unconnected blob stream (writableEnded, chunks outran its record) is intentionally preserved in blobsInFlight on connection close so an in-flight handler can still attach and save it. But its core-level registerBlobReceiveInFlight marker was never released if the record never arrived: receiveBlobs's .finally (the normal release site) never runs for a stream it never touched, and blobsTimer is already cleared. That pinned isBlobReceiveInFlight true for the process lifetime, permanently 503-ing reads of that blob. abortInFlightBlobsOnClose now invokes onAbort for the preserved stream too (without destroying it or removing it from the map), trading a brief window where an in-flight handler on the closing connection could still attach after the marker is released for closing a leak that otherwise never heals. unregisterBlobReceiveInFlight is idempotent, so a later legitimate release for the same stream is a safe no-op. Addresses cb1kenobi's review comment on PR #527. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 8, 2026
…ableEnded blob streams on close A completed-but-unconnected blob stream (writableEnded, chunks outran its record) is intentionally preserved in blobsInFlight on connection close so an in-flight handler can still attach and save it. But its core-level registerBlobReceiveInFlight marker was never released if the record never arrived: receiveBlobs's .finally (the normal release site) never runs for a stream it never touched, and blobsTimer is already cleared. That pinned isBlobReceiveInFlight true for the process lifetime, permanently 503-ing reads of that blob. abortInFlightBlobsOnClose now invokes onAbort for the preserved stream too (without destroying it or removing it from the map), trading a brief window where an in-flight handler on the closing connection could still attach after the marker is released for closing a leak that otherwise never heals. unregisterBlobReceiveInFlight is idempotent, so a later legitimate release for the same stream is a safe no-op. Addresses cb1kenobi's review comment on PR #527. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
github-actions Bot
pushed a commit
that referenced
this pull request
Jul 8, 2026
…overy fix(replication): re-request in-flight blobs on connection close instead of waiting out the idle timeout
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On a replication WS close, abort each still-receiving blob immediately instead of leaving it in
blobsInFlightfor core's source-idle watchdog to reap up toblobTimeout(REPLICATION_BLOBTIMEOUT, now 900s) later. The abort is classified transient byreceiveBlobs, so it clamps the resume cursor and the reconnect re-streams the blob promptly.Why
Live Fabric finding (harper-pro 5.1.14): a deploy-payload blob was diverged on one node as a week-old 62-byte PENDING stub. Root cause, confirmed by a deterministic correlation (every
Blob source stream idle for 120000msreceive failure preceded ~120s earlier by aRestarting http_workerson the peer):deploy_componenttriggersRestarting http_workers; the rolling restart tears down the worker mid-stream while it is sending a blob to a peer.Raising the timeout (#520) widens the window but doesn't stop the interruption or resume the killed transfer. This makes the receiver react to the close immediately instead of waiting it out.
What to look at
receiveBlobs's.catchclassification interplay: the abort error is markedreplicationConnectionClosedso it takes a quiet transient path — clamp + re-request, but no error log and nocluster_status.blobReplicationFailuresbump (this is routine on every deploy, not a divergence).writableEndedpreservation: a completed-but-unconnected stream (chunks that arrived ahead of their record) is intentionally not aborted, so an in-flight handler can still attach and save it.Scope / follow-up
Receiver-side only (harper-pro). A complementary sender-side hardening — graceful bounded drain of in-flight blob sends before the worker restart — is a planned follow-up (coordinated core + harper-pro). The
DeployLifecyclelistener-leak warning seen alongside this is separately addressed by harper#1465.Reviewed cross-model (Codex + Gemini); their findings (preserve completed streams; avoid log/metric spam on routine closes) are already folded into the diff.
🤖 Generated with Claude Code (model: Claude Opus 4.8)