Opt-in deflate compression for file-backed blobs (harper#2443) - #2460
Conversation
…nfig, stored-codec transfer support Part of #2443: the read path for DEFLATE blobs becomes a bounded streaming inflate (the old path buffered the entire inflated content as one chunk); storage.blobs.compression adds the opt-in per-content-type policy resolved in saveBlob (the choke point every local blob write funnels through, including HTTP uploads); createBlobFromStoredBody/openStoredBlobBody let replication preserve a peer's stored deflate body verbatim, with concurrent-inflate verification on both ends so a torn or forged body can never publish or ship. Co-Authored-By: Claude Fable <noreply@anthropic.com>
…he declared size A deflate header records the uncompressed length, so a body torn by an unclean mid-write shutdown carries a finalized header and passes a length-only gate as healthy. With compression opt-in that is the common crash shape, so blobFileMissingOrIncomplete (the locked recheck for the harper-pro#699 in-place repair) now inflates a DEFLATE body, capped at the declared size, and inflatesToExactly is exported for harper-pro's async prefilter to classify the same way. bytes() on a compressed blob is now held to the same output ceiling as the streaming path (a lying header is refused, not allocated), maps zlib failures to the same 500 corruption error, and handles the empty-blob cap that zlib rejects at 0. Also: configValidator coverage for storage.blobs.compression, a codec hint round-trip test, and comment/schema-doc fixes from review. Co-Authored-By: Claude Fable <noreply@anthropic.com>
…entity The locked in-place repair recheck inflated a deflate body synchronously on the event loop, with an allocation the size of the file. Move the inflating classification into `blobFileMissingOrIncompleteAsync` (streamed, output bounded at the declared size, previously duplicated in harper-pro), which records the damaged file's length and header; the sync gate under the writer lock now answers `true` for a deflate body only while the file still matches that observation, and declines otherwise. Also: `inflatesToExactly` abandons an inflate as soon as the output passes the expected size; a ranged read past the end of a blob clamps instead of failing as incomplete (the header size was being widened to the slice end). Co-Authored-By: Claude Fable <noreply@anthropic.com>
…iptor for inflate Review round 3 follow-ups for the blob compression read path: - openStoredBlobBody.stream() opens its own descriptor and re-reads the header; a mismatch with the sniffed header (a repair rename landed in between) is a transient 503, not a corrupt-body 500. close() marks the body consumed. - readCompressedBlob reuses the first 256 KiB read's descriptor and the body bytes it already pulled instead of reopening and re-reading. - blobFileMissingOrIncompleteAsync clears probedDamage at the start of every probe so an early exit cannot leave a stale verdict behind. - The stored blob-ref hint is namespaced as storedCodec; a user blob property named codec is no longer clobbered. - Tests: the bomb test's inflate accounting now actually observes zlib (its exports are non-writable, so the patch is a defineProperty); unreferenced stored-body saves are removed so the orphan-sweep test is independent of file ordering. Refs #2443 Co-Authored-By: Claude Fable <noreply@anthropic.com>
…d after a repair swap Three correctness fixes surfaced by the round-4 pre-push review, all root-caused and covered by tests that hang or misclassify against the prior code and pass now. - Dropped zlib write callback (both verify paths). A mid-stream Z_DATA_ERROR (a corrupt, not merely truncated, deflate body) fires the inflater's error/close events but never invokes the pending inflater.write callback. createStoredDeflateVerifier (receive) and openStoredBlobBody.stream (send) waited only on that callback, so a corrupt body hung the write pipeline (and its :blob lock) or suspended the send generator with its descriptor and file hold leaked. Both now settle from error/close. - Compressed-read vs in-place-repair race. readCompressedBlob re-read the header from the descriptor it opened before the writer-lock wait; a repair renames a fresh uncompressed file over the path while holding that lock, so the read inflated the orphaned torn inode and returned a permanent 500 for a now-healthy blob. waitForBlobWriteCompletion now reports whether it waited; on a contended wait the read reopens the path, and a repaired (uncompressed) header is reported retryable 503, mirroring openStoredBlobBody. The timeout is also armed only on contention. - Config schema: the storage.blobs.compression entry object gains .unknown(false) so a nested typo (e.g. treshold) is rejected instead of silently taking the default, matching config-root.schema.json. Also: correct the classifyBlobFileForCapture comment (compression is operator-configurable now, not "unused inside Harper"), and make the two typestrip instrumentation tests call syncBuiltinESMExports so their CJS namespace patches reach blob.ts's ESM link-time bindings. Refs #2443 Co-Authored-By: Claude Fable <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CD4TfEBHJ3zcbWBTB4xfZn
…clared size Round-5 pre-push review follow-ups (double-close fixed; the rest downgraded to minor/nit). - The repair-swap reopen left the closed descriptor number in `fd` across the `await open(...)`, so a concurrent cancel() or a rejected reopen (blob unlinked during the wait) would close that number a second time after the OS may have reassigned it — the exact harper#1457 double-close hazard. Null `fd` before closing the stale descriptor. - The reopen keyed on whether we waited for the writer lock, which misses a repair that lands in the gap between the first read and the lock probe (an uncontended read then keeps the orphaned descriptor and returns a permanent 500 for a just-healed blob). Detect the swap by file identity instead — `fstat` on the descriptor vs `stat` on the path — and reopen whenever the inode changed. `waitForBlobWriteCompletion` reverts to returning void. - A known-size compressed write stamps the declared size into the header up front and never counted the deflated bytes, so a source ending cleanly at a different length committed a file that inflates to the wrong size — a permanent 500 no reader can serve, where the uncompressed path reads as retryable-incomplete. `finished()` now fails the save when the deflated content length disagrees with the declared size (matching the storedCodec verifier and the repair check). - Config: `.unknown(false)` on the `storage.blobs` object (and `additionalProperties:false` in the JSON schema) so a `compresion:` typo one level up is rejected too, not just inside an entry. Tests: an auto-compressed size-mismatch write fails the save (red against base); a storage.blobs typo is rejected. blob + blobCompression 142 passing; configValidator compression schema green. Refs #2443 Co-Authored-By: Claude Fable <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CD4TfEBHJ3zcbWBTB4xfZn
Round-6 pre-push review follow-up. The identity check added last round used fstatSync + statSync on every compressed read, blocking the worker event loop on a general read path with no ordering or descriptor-ownership reason to be synchronous. Switch to async fstat + stat (run together), with the same post-await cancellation recheck the surrounding reopen already uses. Behavior is unchanged; only the blocking is removed. blob + blobCompression 142 passing. Refs #2443 Co-Authored-By: Claude Fable <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01CD4TfEBHJ3zcbWBTB4xfZn
There was a problem hiding this comment.
Code Review
This pull request implements opt-in deflate compression for file-backed blobs, including configuration validation, streaming compression/decompression, and verification of compressed bodies during replication. The review feedback recommends replacing loose equality assertions in the new test file with strict assertions to align with the repository style guide. Additionally, it identifies bugs in resources/blob.ts where asynchronous close() is called without a callback, and points out a potential resource leak if an error occurs during stream setup before entering the main try-finally block.
| @@ -0,0 +1,632 @@ | |||
| require('../testUtils'); | |||
| const assert = require('assert'); | |||
There was a problem hiding this comment.
According to the repository style guide and general rules, tests should explicitly use strict assertions like assert.strictEqual and assert.deepStrictEqual instead of loose equality assertions (e.g., assert.equal, assert.deepEqual) to prevent type-coercion bugs from being masked. Please update the assertions throughout this file to use strict equality.
References
- Tests use plain node:assert. Use assert.strictEqual/assert.deepStrictEqual explicitly where strict semantics are needed. (link)
- Use strict assertions like
assert.strictEqualandassert.deepStrictEqualinstead of loose equality assertions (e.g.,assert.equal,assert.deepEqual) in tests to prevent type-coercion bugs (such as string-vs-number typing issues) from being masked.
| // have reassigned it to an unrelated file or socket (#1457). | ||
| const staleFd = fd; | ||
| fd = null; | ||
| close(staleFd); |
There was a problem hiding this comment.
The asynchronous fs.close() function requires a callback argument in Node.js. Calling it without one (e.g., close(staleFd)) is deprecated and can throw a TypeError depending on the Node.js version and environment. Since closeSync is already imported and safe to use synchronously here, please use closeSync(staleFd) instead.
closeSync(staleFd);| `Blob ${fileId} deflated ${compressedStream.bytesWritten} bytes but its header declares ${(blob as { size?: number }).size}` | ||
| ) | ||
| ); | ||
| close(fd); |
There was a problem hiding this comment.
The asynchronous fs.close() function requires a callback argument in Node.js. Calling it without one (e.g., close(fd)) is deprecated and can throw a TypeError depending on the Node.js version and environment. Since closeSync is already imported and safe to use synchronously here, please use closeSync(fd) instead.
closeSync(fd);| } catch (error) { | ||
| if (streamFd !== undefined) closeSync(streamFd); | ||
| releaseHoldOnce(); | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
If an error is thrown during the setup of the verifier or the registration of its event listeners (lines 3638–3663), the outer try-catch block (line 3621) has already exited, and the inner try-finally block (line 3664) has not yet been entered. This could result in source and streamFd being leaked, and the file hold not being released. Consider wrapping the entire setup and consumption of the stream in a single try-finally block to ensure robust cleanup.
Opt-in deflate compression for file-backed blobs (harper#2443)
Adds opt-in, per-content-type deflate compression for Harper's file-backed blobs, and
converts the compressed-blob read path to a bounded streaming inflate so a large blob no
longer buffers its whole inflated content in memory. Companion to harper-pro#795, which preserves the codec across replication.
Compression is off unless configured. An operator opts in per content type through a new
storage.blobs.compressionmap; absent config means no blob is ever compressed. The on-diskformat is unchanged: type byte 1 (
DEFLATE_TYPE) is the same one every v5.2.x reader alreadyinflates, and the 8-byte header still records the uncompressed size for both forms, so no
version fence is needed and every existing completeness check keeps working.
What changed
previously called
blob.bytes()and buffered the entire inflated content as one chunk.stream()now inflates incrementally with a hard output ceiling at the declared size, so alarge compressed blob streams with bounded heap and a header that understates its size is
refused before it can emit past the bound.
storage.blobs.compressionis a map of content type →{ codec, threshold }or
false, with exact-type-beats-type/*-beats-defaultprecedence and a size threshold.Already-compressed types (
application/gzip,application/zip,application/zstd,image/*,video/*,audio/*) ship asfalseso an ordinary upload of those is never re-compressed.saveBlob, the single funnel every local write passesthrough (including an ordinary HTTP upload, which never sees creation options), so an uploaded
body of a configured type is compressed on disk. An explicit
compressoption or apre-compressed source still takes precedence. Unknown-size streamed writes never compress (a
threshold cannot be evaluated without a size).
openStoredBlobBody(sender) andcreateBlobFromStoredBody(receiver) let replication move a peer's raw deflate body verbatim, each verifying the actual
bytes by concurrent inflate so a torn or forged body can neither ship nor publish. Used by the
harper-pro companion PR.
isBlobComplete, the backup capture classifier, andthe in-place repair gates inflate to classify a compressed body, so a torn deflate body (which a
length check cannot see, since the header records the uncompressed size) is still healed.
For the human reviewer
Framing note (a plan-review disagreement, decided by the task owner). The step-6 planning
review preferred temp-file + atomic rename for new compressed writes. The owner ruled to keep
in-place publication — it is what lets a read stream a blob while it is still being
write-streamed, the contract
bytes()already relies on — and to fence off the PENDING/ERRORmarker semantics (harper-pro#481) and the reclamation/orphan sweep. This PR implements that
ruling; completeness of an in-place compressed body is established through the writer lock, not by
publication atomicity.
Look hardest at the two stored-body verifiers (
createStoredDeflateVerifierandopenStoredBlobBody.stream), which must settle on the inflater's error/close because a mid-streamZ_DATA_ERRORdrops zlib's write callback, and atreadCompressedBlob's post-wait file-identity check against an in-place repair swap.Gemini bot review — dispositions. The strict-assertion suggestion is left: the compression and
blob.test.jssuites usenode:assert'sassert.equalthroughout, so the new assertions match the file's convention. Theclose(fd)"requires a callback" flags are declined: callback-lessfs.closedoes not throw on the supported Node versions (verified on 26) and is the file's established fire-and-forget close (acloseSyncwould block the event loop). TheopenStoredBlobBody.streamsetup-leak concern is on an unreachable path —createInflateand.on()do not throw in practice — so it is left rather than restructure a reviewed function.Decisions worth a second look, none of them accidental:
streams, so the workload arguably most worth compressing gets nothing today. Reversible as a
follow-up, but operators will measure "compression on" against it.
default. Cached response bodies carry no blob type, so the shippedimage/*/application/gzipexclusions cannot fire for them; adefaultentry compresses them.storedCodecdecodes onto the public blob (record.blob.storedCodec). It is a deliberate,namespaced transfer hint (a user
codecproperty is proven to survive), but it is de-factopublic surface on every compressed blob; removing it later is a visible change.
crash-torn body indistinguishable by length from a complete one, so an unclean shutdown yields
permanent-classified blobs that only the repair sweep heals — the same taxonomy torn blobs use
today, and the repair sweep is deflate-aware.
start. A high-offset range on a compressed blobcosts an O(start) prefix inflate. Block-framed deflate with an offset index was explicitly out
of scope; this is policy-reversible (exclude a type, or set a size cap) without a format change.
falsebuilt-in beats an operator'stype/*wildcard. An operator who setsapplication/*: {codec: deflate}will not getapplication/gzip/zip/zstdcompressed, becausethe exact-key built-in exclusions win over a broader wildcard. Documented in the schema and
tested; a reviewer could reasonably want operator entries to displace built-ins.
Verification
unitTests/resources/blobCompression.test.js(compression suite) andblob.test.jsruntogether: 142 passing. Full
test:unit:resources: 1915 passing / 23 pending / 0 failing.streaming test fails on the
bytes()-buffering implementation, and each correctness fix has atest that hangs or misclassifies against base and passes with the fix — the two deflate-verifier
hangs (receive and send), the compressed-read vs in-place-repair race, and the auto-compressed
size-mismatch write.
configValidatorcompression-schema tests pass, including a nested-typo rejection case. (Onepre-existing
domainSocketpath-length env failure in that suite is unrelated — the deepworktree path exceeds the 107-byte Unix socket limit.)
blobCodecPreservation.test.mjs) passes end to end.Format Check(prettier --check .) is red onunitTests/resources/query-array-scoping.test.js, an unformatted file merged to main by Pin element-scoping semantics of queries over array-valued properties #2437 — not touched by this PR (this branch is behind that commit). Per the task's format-only-touched-files rule it is left alone; a rebase onto a fixed main clears it.Refs #2443
🤖 Generated with Claude Code
Review-Coverage: authored=claude; ran=codex; blocked=gemini(auth); declined=cursor-grok,cursor-composer,domain; rounds=7 @ 4a5a5fb
Human-Review-Need: 4 @ 4a5a5fb