fix(blob): decompress (inflate) compressed blobs on read instead of re-deflating - #1393
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements decompression support for file-backed blobs, replacing the incorrect use of deflate with inflate when reading compressed blobs. It introduces robust handling for concurrent writes of compressed blobs using lock verification and updates the streaming read path to handle compressed content by delegating to the buffered inflate path. Unit tests have been added to verify round-tripping and ranged slicing of compressed blobs. The reviewer suggested wrapping controller.enqueue and controller.close in try-catch blocks to prevent runtime crashes if the stream is cancelled or closed while blob.bytes() is resolving.
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.
|
Reviewed; no blockers found. |
Ethan-Arrowood
left a comment
There was a problem hiding this comment.
Code looks good — clear correctness fix (the double-deflate → inflate bug, plus the stream() TODO), and I traced the waitForCompletion lock-probe logic that does the heavy lifting for compressed-completeness; it holds up and terminates cleanly. Two non-blocking notes for the record: compressed reads are now lock-dependent for completeness (a writer dying while holding the :blob lock would stall the read), and stream() buffers the whole compressed body in memory rather than a true streaming inflate — both fine given compress is unused/off by default today.
One thing before merge: the branch is currently conflicting with main (needs a rebase, almost certainly against the recent blob.ts changes). Approving on the assumption the rebase is mechanical — if it touches anything substantive in bytes()/stream(), give me a ping for a quick re-look; otherwise good to merge at your discretion once it's rebased and CI is green.
sent with Claude Opus 4.8
614b7a0 to
1177604
Compare
1177604 to
174dc92
Compare
FileBackedBlob.bytes() called deflate() on the already-compressed on-disk body for DEFLATE_TYPE blobs, double-compressing instead of decompressing, and inflate was never imported. The streaming read path had a "TODO: Implement support for decompression" and likewise returned the raw compressed bytes. Reading any DEFLATE-compressed blob therefore returned corrupt data. Latent today because blob compression (compress?: boolean) is exposed but unused/off by default; this makes it correct before it is enabled. - bytes(): inflate the body for DEFLATE_TYPE; inflate-then-slice so start/end range over the uncompressed content. Completeness for a compressed blob can't be judged from the (uncompressed) header size vs the compressed body length, and the header size is finalized up front when the size is known, so completeness is verified via the writer's existing fileId+":blob" lock (new mustVerifyViaLock probe) before inflating. Uncompressed path behavior is preserved (exact size-vs-body comparison). - stream(): on detecting DEFLATE_TYPE in the header, delegate to the buffered inflate path and emit a single chunk. Correct-or-safe fallback: a true streaming inflate was left out as too risky given the position-seeking / watcher framing (uncompressed-offset seeking into a deflate stream isn't possible). - Tests: compressed round-trip via bytes() and stream(), plus a ranged read. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
174dc92 to
bb25dea
Compare
Summary
FileBackedBlob.bytes()read DEFLATE-compressed blobs incorrectly: forDEFLATE_TYPEit calleddeflate()on the already-compressed on-disk body (double-compressing) instead of decompressing, andinflatewasn't even imported. The streaming read path (stream()) had a// TODO: Implement support for decompressionand likewise returned the raw compressed bytes. Net effect: reading any DEFLATE-compressed blob returned corrupt data.Why it was latent
Blob compression is exposed (
compress?: booleanonBlobCreationOptions) but off by default and unused — nothing in harper/harper-pro setscompress: truefor blobs. So this never fired in practice. This is a correctness fix that must be in place before anyone enablescompress: true.The fix
Buffered path (
bytes()):DEFLATE_TYPE(wasdeflate).start/endranges index into the uncompressed content, so we inflate first, then slice.size(which is the uncompressed length) vs the compressed on-disk body length, and the header size is written up front when the size is known — so neither bytes nor header size is a reliable in-flight signal. Completeness is instead verified via the writer's existingfileId + ":blob"lock (newmustVerifyViaLockprobe inwaitForCompletion): if the writer still holds the lock we wait for release and re-read; otherwise we re-read once and inflate. TheERROR_TYPE,UNKNOWN_SIZE, and lock-wait semantics are preserved. The uncompressed path keeps its exact size-vs-body completeness check (refactored for clarity, behavior unchanged).Streaming path (
stream()):DEFLATE_TYPEin the header (first read), delegate to the buffered inflate path and emit the decompressed result as a single chunk. This is the correct-or-safe choice the task sanctioned: it never streams compressed garbage. A true streaming inflate would require reworking the position-seeking / watcher framing (you can't seek into a deflate stream by uncompressed offset), which I judged too risky for a correctness fix.Tests
Added to
unitTests/resources/blob.test.js:bytes()andstream()— asserts decompressed output equals the original.slice(300, 400)) over a compressed blob, via bothbytes()andstream().Both fail before the fix (the double-compress produces a body shorter than the uncompressed size → "Incomplete blob" / inflate error) and pass after.
blob.test.js(31) and the fulltest:unit:resourcessuite (843) are green; lint and TypeStrip (node --experimental-strip-types --check) clean.Where to look
mustVerifyViaLocklock-probe logic inwaitForCompletion— this is the load-bearing correctness piece. The cross-model reviews (Codex, Gemini) found no deadlock / incorrect-throw issues here, but it's the part most worth a careful read.stream()buffered-fallback decision: deliberate tradeoff (correctness over true streaming inflate). Open for discussion if a streaming inflate is wanted later — flagged as a known limitation, not a regression.Note: this PR touches
resources/blob.tsbut onlyFileBackedBlob.bytes()andstream()— disjoint from PR #1387 (isBlobFileComplete/findIncompleteBlobRefs), so they should coexist cleanly.🤖 Generated by Claude Code (model: claude-opus-4-8). Cross-reviewed by Codex and Gemini; no open blockers.