Preserve the blob codec across replication (harper#2443) - #795
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements replication blob-codec preservation (harper#2443), allowing raw stored deflate-compressed blobs to be streamed directly to peers that advertise support, avoiding unnecessary inflation and recompression. It introduces capability negotiation, safe repair inflation with size limits to prevent compression bombs, and updates replication connection handling. Feedback on these changes highlights a potential resource leak if ws.send throws synchronously during the blob announcement before the main try-catch block is entered, and suggests logging teardown errors in the integration tests rather than silently swallowing them.
| let storedBody: ReturnType<typeof openStoredBlobBody>; | ||
| if (peerAcceptedBlobCodecs.has('deflate') && (blob as Blob & { storedCodec?: string }).storedCodec === 'deflate') { | ||
| storedBody = openStoredBlobBody(blob); | ||
| if (storedBody) { | ||
| ws.send( | ||
| encode([ | ||
| BLOB_CHUNK, | ||
| { fileId: id, transferId, size: storedBody.size, codec: storedBody.codec }, | ||
| Buffer.alloc(0), | ||
| ]) | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
If ws.send throws an error synchronously (for example, if the WebSocket connection is closed or in an invalid state), storedBody has already been opened and releaseBlobHold has been acquired, but neither will be closed or released because the error propagates before entering the main try block at line 6345. This can lead to a file descriptor and resource leak under network instability or high load. Wrapping the synchronous announcement in a try-catch block to clean up resources on failure prevents this leak.
| let storedBody: ReturnType<typeof openStoredBlobBody>; | |
| if (peerAcceptedBlobCodecs.has('deflate') && (blob as Blob & { storedCodec?: string }).storedCodec === 'deflate') { | |
| storedBody = openStoredBlobBody(blob); | |
| if (storedBody) { | |
| ws.send( | |
| encode([ | |
| BLOB_CHUNK, | |
| { fileId: id, transferId, size: storedBody.size, codec: storedBody.codec }, | |
| Buffer.alloc(0), | |
| ]) | |
| ); | |
| } | |
| } | |
| let storedBody: ReturnType<typeof openStoredBlobBody>; | |
| if (peerAcceptedBlobCodecs.has('deflate') && (blob as Blob & { storedCodec?: string }).storedCodec === 'deflate') { | |
| storedBody = openStoredBlobBody(blob); | |
| if (storedBody) { | |
| try { | |
| ws.send( | |
| encode([ | |
| BLOB_CHUNK, | |
| { fileId: id, transferId, size: storedBody.size, codec: storedBody.codec }, | |
| Buffer.alloc(0), | |
| ]) | |
| ); | |
| } catch (error) { | |
| storedBody.close(); | |
| releaseBlobHold?.(); | |
| blobsBeingSent.delete(blobSendKey); | |
| throw error; | |
| } | |
| } | |
| } |
| after(async () => { | ||
| await Promise.all( | ||
| [ctx.nodeA, ctx.nodeB, ctx.nodeC].map((node) => node && teardownHarper({ harper: node }).catch(() => null)) | ||
| ); | ||
| }); |
There was a problem hiding this comment.
According to the general rules, test cleanup hooks should log any failures with identifying information (such as hostnames) for easier diagnosis rather than silently swallowing them with .catch(() => null). Logging the error with the node's hostname helps troubleshoot teardown failures in CI/CD environments.
after(async () => {
await Promise.all(
[ctx.nodeA, ctx.nodeB, ctx.nodeC].map((node) =>
node && teardownHarper({ harper: node }).catch((error) => {
console.error(`Failed to teardown Harper node ${node.hostname || 'unknown'}:`, error);
})
)
);
});References
- In test cleanup hooks, wrap individual process termination or cleanup steps in separate try-catch blocks to ensure they are attempted independently, and log any failures with identifying information (such as hostnames) for easier diagnosis.
2e73bef to
3fca85a
Compare
|
Reviewed; no blockers found. |
…e record, verified at both ends Implements the harper-pro half of harper#2443. A deflate-stored blob is streamed as its raw stored body — no inflate on send, no recompress on receive — but only to a peer that advertised acceptBlobCodecs in its NODE_NAME capabilities object (HARPER_REPLICATION_ACCEPT_BLOB_CODECS=0 is the receiver-side kill switch; an old or opted-out peer gets today's inflated stream). Because records travel as raw stored audit bytes and the receiver stamps the blob file header at record-decode time, the sender announces the codec with a zero-length BLOB_CHUNK sent in sendBlobs' synchronous prefix, guaranteed to precede the owning record frame; the receiver binds the codec immutably to the transfer and rejects violations. Both ends verify the body by concurrent inflate, and the in-place repair path inflates locally, keeping its uncompressed contract. Co-Authored-By: Claude Fable <noreply@anthropic.com>
blobFileMissingOrIncompleteAsync only length-checked, which a deflate body cannot satisfy (its header records the uncompressed size), so a body torn by an unclean mid-write shutdown read as healthy and the harper-pro#699 identity-tie repair declined it. It now runs core's streamed inflatesToExactly for DEFLATE bodies, matching the locked sync recheck in core (harper#2443). Bumps core to the commit that makes the sync gate deflate-aware. Co-Authored-By: Claude Fable <noreply@anthropic.com>
…nded inflater Blob repair on the receiving side now runs a compressed repair body through `createRepairInflater`, a pipeline of the receive stream, `createInflate`, and a transform that fails as soon as the inflated output exceeds the advertised size, so a declined repair destroys the whole chain quietly and a body that lies about its length cannot write unbounded output. The durable identity tie and the repair prefilter both use core's `blobFileMissingOrIncompleteAsync` instead of a header-only check, so a compressed blob that inflates short no longer counts as durable, and the locked `repairBlobFile` recheck confirms the file the probe classified by identity. `sendBlobs` releases its stored body hold on every parked early return; the codec preservation cluster test carries multi-chunk incompressible payloads and asserts the origin file is actually deflated before checking the receiver. Core pointer bumped for the async classifier and bounded `inflatesToExactly`. Co-Authored-By: Claude Fable <noreply@anthropic.com>
…ored body is replaced mid-send Review round 2 follow-ups for codec-preserving blob replication: - The receive ladder refuses a codec announcement that carries no size: the inflated length is the only bound on a raw body, so the repair inflater now takes a mandatory expected size. - A stored body replaced between the sync sniff and the stream open (core now reports it as a transient 503) is forwarded as such, so the receiver holds the gap and re-decides on reconnect instead of advancing its cursor past a blob the sender still has. - The blob-ref hint is read as storedCodec (core rename); the unused getFilePathForBlob import is dropped. - blobCodecPreservation settles each node on its files decoding to exactly the written payloads instead of two unchanged sweeps, and compares content rather than lengths. Bumps core to f27fee029. Refs HarperFast/harper#2443 Co-Authored-By: Claude Fable <noreply@anthropic.com>
…b bound From the round-3 pre-push review (converged; one actionable finding). The receive-ladder guard that requires a size alongside an announced codec used `typeof size !== 'number'`, which NaN and Infinity both satisfy. A peer announcing `codec: 'deflate'` with a non-finite size would still pin `stream.codec` and set `stream.expectedSize` to that value; createRepairInflater's `inflatedLength > expectedSize` is then never true, so a compression bomb expands unbounded into the repair temp file until the volume fills. The guard is now `Number.isSafeInteger(size) && size >= 0`, and createRepairInflater refuses a non-finite/negative size outright as a second line of defense (the output cap is the only bound on a raw peer body). Bumps the core submodule pointer to the companion harper commit. Refs #2443 Co-Authored-By: Claude Fable <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CD4TfEBHJ3zcbWBTB4xfZn
…at codec binding Round-4 pre-push review follow-up. The receiver stored `stream.expectedSize = size` for every sized chunk before the codec-binding guard ran, so a later chunk carrying a non-finite/negative size and no codec could overwrite the good expectedSize a bound codec relied on. On a repair-candidate transfer that fed `createRepairInflater`, the now-invalid size tripped its validation throw, which rode the record-decode path into a connection close and reconnect loop — a peer-supplied value turned into a link problem. Validate the size at the assignment (`Number.isSafeInteger(size) && size >= 0`) so a malformed later chunk cannot re-poison the transfer. Bumps the core submodule pointer to the companion harper commit. Refs #2443 Co-Authored-By: Claude Fable <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CD4TfEBHJ3zcbWBTB4xfZn
Round-5 pre-push review follow-up. The prior fix validated the per-chunk expectedSize assignment but the stream-creation branch still stored the first chunk's size unconditionally, so an invalid first size (NaN/Infinity/negative) was written before the validated assignment could skip it. Drop the creation-branch assignment and let the single validated assignment below set expectedSize for the first chunk too — an invalid size is never stored, and a codec is then refused without a valid size rather than binding to a bad output bound. Bumps the core submodule pointer to the companion harper commit (async repair-swap identity check). Refs #2443 Co-Authored-By: Claude Fable <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CD4TfEBHJ3zcbWBTB4xfZn
Cross-model (Gemini) review of the draft PR. The codec-announcement BLOB_CHUNK is built and sent synchronously before the send's main try block, after `openStoredBlobBody` and `holdBlobFile` have already claimed the file's stored-body hold, the blob hold, and the `blobsBeingSent` entry. A synchronous throw there — an `encode` failure or an invalid-argument error out of `ws.send` — propagated out before any of that was released, stranding both holds and, worse, leaving the `blobsBeingSent` claim set, which blocks every later send of that blob for the life of the process. Wrap the announcement so a throw releases all three and returns, deferring to the peer's reconnect re-request like the wsClosed early-returns do. (No live descriptor leaks: openStoredBlobBody closes its sniff fd before returning.) The 595-test replication unit suite still passes; the send-path error branches have no isolated harness (sendBlobs is internal to the connection), noted as a follow-up in the dispatch findings. Refs HarperFast/harper#2443 Co-Authored-By: Claude Fable <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CD4TfEBHJ3zcbWBTB4xfZn
3fca85a to
46f3cd0
Compare
| } catch (announceError) { | ||
| // A synchronous throw while building or sending the announcement (an encode failure, or | ||
| // an invalid-argument error out of ws.send) would otherwise propagate before any cleanup | ||
| // runs. openStoredBlobBody already closed its sniff fd, so close() here releases the | ||
| // stored-body hold (not a live descriptor); leaking would also strand the blob hold and | ||
| // the blobsBeingSent claim — the last of which blocks every later send of this blob. | ||
| // Release all three and stop; the peer re-requests this blob on reconnect, the same | ||
| // recovery the wsClosed early-returns below rely on. | ||
| storedBody.close(); | ||
| releaseBlobHold?.(); | ||
| blobsBeingSent.delete(blobSendKey); | ||
| logger.debug?.( | ||
| `Blob ${id} codec announcement send failed; releasing and deferring to reconnect`, | ||
| announceError | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
Suggestion (non-blocking): this catch branch (cleaning up storedBody/releaseBlobHold/blobsBeingSent when the codec announcement's ws.send throws synchronously) has no test exercising it — the commit message itself notes sendBlobs is internal to the connection with no isolated harness. Worth a regression test that drives replicateOverWS with a mock ws whose send throws on the announcement frame, then asserts the hold/stored-body/blobsBeingSent entry are released rather than stranded, so a future refactor of this cleanup can't silently reintroduce the leak this PR fixes.
Preserve the blob codec across replication (harper#2443)
Rebased onto
mainat72f1a604without changing the advertised, backward-compatible deflate stored-body transfer: the capability negotiation and receiver-side inflater preserve a compressed body only for an opting-in peer. The merged core companion is already inmain, so this PR has no core gitlink diff or outstanding companion dependency.Where to look hardest: the bounded repair inflater and its codec-preserved receive path are the durability boundary; a malformed announced size must never permit an unbounded inflate.
For the human reviewer
storage.blobs.compressionsetting is off; reversing that choice would discard the feature's no-recompression benefit.3fca85ahead and do not cover46f3cd0.Verification
npm run test:unitpassed: 855 tests, 0 failures.npm run buildcould not complete after clean installs because this worktree's TypeScript resolution reports errors inanalytics/profile.ts,core/server/threads/socketRouter.ts, and existingWebSocket/Errortypings inreplication/replicationConnection.ts; the corresponding GitHub Build Harper Pro jobs passed on Node 22, 24, and 26.5.Refs HarperFast/harper#2443
🤖 Generated with GPT-5 Codex
Review-Coverage: authored=claude; ran=gemini,codex; adjudicated=domain; declined=cursor-grok,cursor-composer; rounds=9 @ 3fca85a
Human-Review-Need: 4 (decisions: codec-crosses-node-policy, kill-switch-env-only, stored-body-verify-failure-permanent, tie-inflate-cost, announcement-in-sync-prefix, no-in-place-retry-for-stored-body) @ 3fca85a