zlib: brotli + zstd dictionaries, Node-compatible reset() — +4 node v26.3.0 tests, zlib suite 57/62 → 61/62 (98.4%) - #34427
Conversation
node:zlib accepted `options.dictionary` for brotli but silently dropped it:
NativeBrotli.init() took no dictionary argument, so createBrotliCompress({
dictionary }) produced output identical to compressing with no dictionary, and
a stream could not interoperate with a peer using a shared dictionary.
Thread the dictionary through to brotli, mirroring Node's
BrotliEncoderContext::Init / BrotliDecoderContext::Init: prepare it with
BrotliEncoderPrepareDictionary + BrotliEncoderAttachPreparedDictionary on the
encode side, BrotliDecoderAttachDictionary on the decode side, and keep an
owned copy alive for as long as the borrower (brotli does not copy the data).
init() now frees any previous instance and dictionary first, as Node's
state_.reset(...) does. reset() clears the dictionary, matching Node's
BrotliContext::ResetStream(), which reaches Init() with an empty dictionary --
verified against node v26.3.0 rather than inferred.
The zlib/iter brotli path validated options.dictionary and then discarded it;
it now passes it to init() like Node's lib/internal/streams/iter/transform.js.
reset() during an in-flight write now throws "Cannot reset zlib stream while a
write is in progress" instead of deferring the reset until the write lands.
Node throws here, and throwing still leaves the encoder state untouched, so the
use-after-free the deferral was added to prevent remains impossible; the
pending_reset machinery is therefore removed. zlib-reset-race.test.ts is
updated to assert the throw -- its fixtures now pass unmodified on node v26.3.0
as well as on bun.
ERR_INVALID_ARG_TYPE for options.dictionary passed a pre-joined string, which
renders "must be of type"; Node passes an array and renders "must be an
instance of". Pass the array on both the brotli and deflate paths.
Adds test-zlib-brotli-dictionary.js, test-zlib-reset-during-write.js and
test-zlib-zstd-dictionary.js verbatim from node v26.3.0. All 60 vendored
test-zlib-*.js now pass (was 57 files, 58/62 with these added), and the zlib,
web-streams compression and zlib/iter suites are unaffected (448 + 12 passing).
|
Updated 12:18 AM PT - Jul 17th, 2026
❌ @robobun, your commit b711ed1 has 1 failures in
🧪 To try this PR locally: bunx bun-pr 34427That installs a local version of the PR into your bun-34427 --bun |
|
Found 2 issues this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughChangesBrotli and Zstd dictionaries are forwarded from JavaScript into native contexts and cleared on reset. Compression stream resets now throw during active writes, with updated race tests. DecompressionStream trailing-data handling gains deflate and gzip coverage. Node zlib behavior
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/runtime/node/zlib/NativeBrotli.rs`:
- Around line 30-35: Update NativeBrotli’s memory accounting so estimated_size()
includes the owned dictionary and prepared_dictionary allocations, with
thread-safe external-memory updates when dictionaries are attached and
deinit_dictionary releases them. Ensure accounting is balanced across
initialization, replacement, and cleanup, or enforce a bounded dictionary size
and account for that bound consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 036ea9e2-8c8e-419c-8b63-f4cff26156af
📒 Files selected for processing (11)
.claude/skills/verify/SKILL.mdsrc/js/internal/streams/iter/transform.tssrc/js/node/zlib.tssrc/runtime/node/node_zlib_binding.rssrc/runtime/node/zlib/NativeBrotli.rssrc/runtime/node/zlib/NativeZlib.rssrc/runtime/node/zlib/NativeZstd.rstest/js/node/test/parallel/test-zlib-brotli-dictionary.jstest/js/node/test/parallel/test-zlib-reset-during-write.jstest/js/node/test/parallel/test-zlib-zstd-dictionary.jstest/js/node/zlib/zlib-reset-race.test.ts
💤 Files with no reviewable changes (2)
- src/runtime/node/zlib/NativeZstd.rs
- src/runtime/node/zlib/NativeZlib.rs
The file uses node:test, which scripts/runner.node.mjs routes through `bun test` rather than `bun run` (runner.node.mjs:750). It passes as-is -- DecompressionStream already rejects trailing data with a TypeError on all eight cases the test covers. Brings the vendored zlib suite to 61 of node v26.3.0's 62 test-zlib-*.js.
zstd had the same gap brotli did: NativeZstd.init() took no dictionary
argument and the Zstd class never read options.dictionary, so
zstdCompressSync(input, { dictionary }) produced a plain frame. Against
node v26.3.0 on the same input: node compressed to 18 bytes and refused to
decode the frame without the dictionary, bun compressed to 64 bytes (the
no-dictionary size) and decoded it without one.
Load the dictionary with ZSTD_CCtx_loadDictionary / ZSTD_DCtx_loadDictionary,
mirroring node's ZstdCompressContext::Init -- dictionary first, then
setPledgedSrcSize. zstd copies the dictionary into the context (zstd.h:
"`dict` content will be copied internally"), so unlike brotli no owned copy
is needed. reset() drops the dictionary, matching node's ResetStream(),
which reaches Init() with an empty one.
Node does not validate options.dictionary for zstd the way it does for
zlib/brotli -- a non-ArrayBufferView is silently ignored -- so the JS side
matches that rather than throwing.
This also makes the vendored test-zlib-zstd-dictionary.js meaningful. It
passed before only because it asserts a round-trip and a loose size bound,
neither of which is dictionary-sensitive (it passes with the dictionary
removed on node too), so its green status had certified a feature bun did
not have.
Neither reproduces. A src/js/**/*.ts edit followed by a plain `bun bd` does regenerate the bundle and reach the binary (verified with a probe that throws from the built module), and a clean relink is signed correctly by the existing -Wl,-adhoc_codesign + macho-postlink shim -- no manual codesign needed. Both notes came from one session where the source was edited while a build was already past its bundle step, and from a link that had been interrupted; they described those artifacts as build bugs.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/runtime/node/zlib/NativeZstd.rs (1)
362-372: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winTrack the loaded dictionary in GC accounting.
ZSTD_CCtx_loadDictionary/ZSTD_DCtx_loadDictionarycopy the dictionary into the context, butestimated_external_sizestays at the construction-time baseline. Add the dictionary bytes to the estimate and clear that contribution on init failure, reset, and close.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/runtime/node/zlib/NativeZstd.rs` around lines 362 - 372, Update the dictionary-loading paths using ZSTD_CCtx_loadDictionary and ZSTD_DCtx_loadDictionary to add the loaded dictionary byte count to estimated_external_size after successful loading. Ensure this contribution is removed when initialization fails, and reset it during context reset and close alongside the existing native-state cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/runtime/node/zlib/NativeZstd.rs`:
- Around line 362-372: Update the dictionary-loading paths using
ZSTD_CCtx_loadDictionary and ZSTD_DCtx_loadDictionary to add the loaded
dictionary byte count to estimated_external_size after successful loading.
Ensure this contribution is removed when initialization fails, and reset it
during context reset and close alongside the existing native-state cleanup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 483f1f2d-db34-4209-8291-31a67bc97f30
📒 Files selected for processing (1)
src/runtime/node/zlib/NativeZstd.rs
Context::init overwrote self.state without freeing it, leaking the previous ZSTD_CCtx/DCtx when init() is called twice on one handle. Brotli already guards this; zstd was missed. Mirrors node's state_.reset(ZSTD_createCCtx()). reset() was unaffected -- it calls deinit_state() first.
|
@robobun adopt it |
|
✅ Adopted and green on everything this PR touches. #74395: 281 passed, zero zlib/brotli/zstd failures; the one hard red is |
Context::init() now frees the previous context first (1c97d56), so the guard in reset() immediately before calling init() is dead. Brotli's reset() was already trimmed to just self.init(None) for the same reason; this brings zstd to the same shape.
…ctionary-node-v26
There was a problem hiding this comment.
No new issues found — all prior review feedback has been addressed (zstd dictionary support landed, zstd re-init leak guarded, redundant reset() guard dropped). Deferring to a human for sign-off given the scope: ~350 lines of native FFI across brotli/zstd with manual dictionary-lifetime management, plus removal of the pending_reset deferral machinery in the shared CompressionStream mixin.
Checked: brotli dictionary ownership — owned Vec<u8> copy outlives the prepared dictionary and encoder/decoder; deinit_dictionary destroys the prepared dict before clearing the Vec, and close() frees state before the dictionary it borrows.
Checked: zstd init() now frees the prior context on re-entry (matches brotli); dictionary-load failure paths free the just-created CCtx/DCtx and null self.state.
Checked: reset() throw path leaves write_in_progress and encoder state untouched, so the UAF the old pending_reset deferral guarded remains impossible.
Checked: JS-side Zstd triple-read of opts?.dictionary intentionally mirrors node's lib/zlib.js:920 for parity (verified against a real node binary per the thread).
Extended reasoning...
Overview
This PR wires options.dictionary through to the native brotli and zstd contexts (previously validated but silently dropped), changes reset() during an in-flight write from defer-until-complete to throw (matching Node), fixes the ERR_INVALID_ARG_TYPE argument shape for options.dictionary, and vendors 4 Node v26.3.0 tests. It touches 12 files: 5 native Rust (node_zlib_binding.rs, Native{Brotli,Zlib,Zstd}.rs, src/zstd/lib.rs), 2 built-in JS modules, 4 new vendored tests, and 1 updated regression test.
Security risks
None identified. The dictionary bytes come from user JS but are only handed to brotli/zstd as opaque compression seeds; there's no path/command construction. The main risk class is memory safety (borrowed pointers into the dictionary Vec outliving it, or double-free on re-init), which the diff handles carefully — the brotli path keeps an owned copy and destroys borrowers before releasing it; zstd's loadDictionary copies internally so no owned copy is needed. The bomb/OOM concern (unbounded dictionary allocation not counted in GC pressure) was raised by CodeRabbit and deliberately deferred with maintainer agreement as matching Node's behavior.
Level of scrutiny
High. This is native FFI code in a Node-compat hot path with explicit unsafe blocks whose SAFETY comments hinge on lifetime ordering (deinit_dictionary after deinit_state, prepared-dict destroy before Vec clear). It also removes a concurrency guard (pending_reset) from the shared mixin used by all three stream types — the replacement (throw instead of defer) is simpler and matches Node, but changing UAF-adjacent machinery warrants a maintainer's eyes. Per CLAUDE.md's landing guidance, memory-safety changes of this shape are the most-blocked category.
Other factors
The PR has been through three rounds of bot review with fixes landed for each (zstd dictionaries were initially missing, zstd re-init leak, redundant reset() guard). A maintainer (cirospaciari) drove the fix commits and asked robobun to adopt; Jarred-Sumner's one concern (verify-skill gotchas) was addressed by removing them. Verification is thorough — an 11-scenario driver diffed byte-identical against node v26.3.0, and the vendored tests are unmodified upstream files. CI build #74327 was green on zlib. Given all that, this is close to approvable, but the combination of size, unsafe FFI, and concurrency-mechanism removal puts it past what I'd shadow-approve without a human look.
…imit (#305) Gotcha 14 said `node:zlib`'s brotli has NO dictionary parameter, listed the nine params as the whole list, and rested part of the dcz-over-dcb case on dcb needing the `brotli` CLI back in the build path. Node 26 takes a `dictionary` option (nodejs/node#61763, merged 2026-02-13) and `.node-version` is already 26, so CI has it too. Measured on v26.7.0, q11 over a 21KB target: 70 bytes with no dictionary against 18 with one. The CONCLUSION is untouched, because dcz won on decode and neither encoder's availability was ever the axis. What changed is one of the stated reasons. Four copies of the claim moved together, per gotcha 18's own lesson about grepping the tree when a note lands: CLAUDE.md twice, the build.mjs comment at the delta emitter, the roll-shell-dictionary.mjs header, and /garage/compression, which carried it in prose AND in an understanding-check explanation. While in that page, its dcz-vs-dcb table still held the pre-correction figures (79 vs 80 bytes, "about twice as fast"). The 2026-07-28 re-measure against real dictionaries never reached it: dcb is 245 bytes ahead across all 12 pairs (6.8%, winning 11 of 12) and dcz decodes 8.3x faster, not 2x. Both inputs to the original argument were wrong, in opposite directions, and the call survived on the structural point instead: decode scales with the RECONSTRUCTION, not the delta, so a smaller dcb delta never shrinks the decode gap. The quiz's correct answer restated the old figures verbatim and now states that. Adds one general rule, since three runtimes disagree on this single option: probe dictionary support, never infer it. node 26 honours it, workerd accepts it for zstd and silently ignores it, and bun does the same through 1.3.14 with the fix landing in oven-sh/bun#34427 (unreleased; verified on 1.4.0-canary.1). The failure is silent every time, because a frame compressed WITHOUT the dictionary still decodes fine WITH it, so the only signal is a byte count that never shrank. Co-authored-by: Aadharsh Pannirselvam <19518661+oddharsh@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…d 2x faster (#307) * feat(build): a control for bun, which builds this byte-identically and 2x faster Bun 1.4 is worth a real answer rather than a guess, so `npm run bun:check` measures one instead. Same idiom as `kitesurf:check`: three questions, run when a bun release lands, not in CI. Measured 2026-08-10, node v26.7.0 against bun 1.4.0-canary.1+827475e21: ok zstd honours `dictionary` — 73 none / 24 good / 73 wrong ok build output is byte-identical — 1975 files, node 14.4s vs bun 7.1s ok contract suite passes under bun — 206 pass, 0 fail BYTE-IDENTICAL is the bar, and it is a lot higher than "the build succeeds". /a/ and /i/ are content-addressed, so one differing byte mints a new URL, orphans every a-dict snapshot naming the old hash, and moves the CSP hashes the documents are served under. Not adopting it, for three reasons and only the first is about bun. The newest STABLE bun is 1.3.14, which predates the dictionary fix (oven-sh/bun#34427) and silently ignores `zstdCompressSync`'s `dictionary`. wrangler, miniflare and workerd are the deploy path and the route oracle, node-pinned. And the win is seconds on a step CI already spends longer on in dry-runs. The failure would be LOUD, which the check now says out loud: build.mjs already feature-detects the same collapse and throws, so 1.3.14 kills the build rather than shipping no-op deltas. The engine is silent; this build is not. Verified in both directions — canary all green, 1.3.14 fails at question 1 before spending two builds, and the tree comes back clean either way. Also fixes a real defect bun found in our own suite. `withSecurityHeaders` rebuilds every response as `new Response(response.body, …)`, which per Fetch LOCKS the body, and one contract test pushed the same four case objects through it twice. Bun throws `Body object should not be disturbed or locked`; node's undici allows it. The assertions are about headers, so the leniency was never load-bearing — it just made the suite depend on which runtime ran it. Cases are built fresh per pass now, and the test passes on both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs: gotcha 27 gains its cleanest control, a prose-only PR failing it Two more instances on 2026-08-10 (#305, #307), so four across four unrelated PRs, every one alongside a CodeQL pass. #305 is what makes this worth writing down rather than just recounting. It changed documentation, four code COMMENTS and one quiz string, and failed the check identically to a build-script PR, with `title: null` and a lone annotation at `.github:211`. A diff with no executable change cannot carry a security finding, so a check that reddens on it is reporting on itself. That is a sharper discriminator than the API tell when the API tell is ambiguous. Also records where to confirm the stakes: the ruleset, not the check list. `validate` is the only required context, so this has been red on four PRs while gating none of them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Aadharsh Pannirselvam <19518661+oddharsh@users.noreply.github.com> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
What
node:zlibacceptedoptions.dictionaryfor brotli but silently dropped it —NativeBrotli.init()took no dictionary argument, socreateBrotliCompress({ dictionary })produced output byte-identical to compressing with no dictionary and could not interoperate with a peer using a shared dictionary.This wires dictionaries through to both brotli and zstd and fixes three adjacent divergences from Node v26.3.0.
+4 tests vendored from node v26.3.0. Vendored zlib suite: 57 → 61 files, all passing — 61 of node v26.3.0's 62
test-zlib-*.js(91.9% → 98.4%).Changes
Brotli dictionary support — mirrors Node's
BrotliEncoderContext::Init/BrotliDecoderContext::Init:BrotliEncoderPrepareDictionary+BrotliEncoderAttachPreparedDictionaryBrotliDecoderAttachDictionarydecode.h: "Data provided to this method should be kept accessible until decoding is finished";compound_dictionary.cstores a raw pointer). Node keeps astd::vector<uint8_t> dictionary_for the same reason.init()frees any previous instance and dictionary first, as Node'sstate_.reset(...)does. It destroys the state before the dictionary it borrows (Node does the reverse, briefly leaving the old encoder pointing at freed bytes).reset()clears the dictionary, matchingBrotliContext::ResetStream()→Init()with an empty dictionary. This differs from the zlib path, which deliberately re-arms viaSetDictionary()— verified against a real node v26.3.0 binary rather than inferred.Zstd dictionary support — zstd had the identical gap, found in review. Against node v26.3.0 on the same input, node compressed to 18 bytes and refused to decode the frame without the dictionary; bun compressed to 64 (the no-dictionary size) and decoded it happily. Now loaded via
ZSTD_CCtx_loadDictionary/ZSTD_DCtx_loadDictionary, mirroring node'sZstdCompressContext::Init(dictionary first, thensetPledgedSrcSize). zstd copies the dictionary into the context (zstd.h: "dictcontent will be copied internally"), so unlike brotli no owned copy is needed.reset()drops it, matching node'sResetStream(). Node does not validateoptions.dictionaryfor zstd the way it does for zlib/brotli — a non-view is silently ignored — so the JS side matches that rather than throwing.zlib/iter—createBrotliHandleandcreateZstdHandlevalidatedoptions.dictionarythen discarded it (their comments said bun'sinit()had no dictionary parameter, which this PR fixes). They now pass it through, matching Node'slib/internal/streams/iter/transform.js.reset()during an in-flight write now throwsCannot reset zlib stream while a write is in progress, as Node does, instead of deferring the reset until the write lands. Throwing leaves the encoder state untouched, so the use-after-free the deferral guarded against remains impossible — thepending_resetmachinery is removed as dead.zlib-reset-race.test.tsis updated to assert the throw; its fixtures now pass unmodified on node v26.3.0 as well as on bun, where previously they asserted behaviour Node does not have.ERR_INVALID_ARG_TYPEforoptions.dictionarypassed a pre-joined string (rendersmust be of type); Node passes an array (rendersmust be an instance of). Fixed on the brotli and deflate paths.Tests
4 tests added, verbatim from node v26.3.0 (byte-identical,
diff-clean):test-zlib-brotli-dictionary.jstest-zlib-reset-during-write.jstest-zlib-zstd-dictionary.jstest-zlib-type-error.jsAll 61 vendored
test-zlib-*.jspass. Also green and unaffected:test/js/node/zlib/,test/js/web/streams/compression.test.ts,test/regression/issue/18413-all-compressions.test.ts(448 passing) and the 12zlib/itertests.Verification
An 11-scenario driver run through the public API on this build and on a real node v26.3.0 binary produces byte-identical output on every line — round-trip, ratio (
12 < 53bytes, proving the dictionary is applied), wrong/missing dictionary errors, ArrayBuffer/TypedArray/DataView forms, all fiveERR_INVALID_ARG_TYPEmessages verbatim, zero-length and 1MB dictionaries, and the reset semantics. Double-init()was exercised under ASAN with no abort.The one remaining test
test-zlib-unused-weak.js(62nd) is not vendored. It is not a zlib defect:process.memoryUsage().externalin bun reflects only what was counted during the last GC's marking (reportExtraMemoryVisitedrebuildsm_extraMemorySize), whereas V8's counter updates at allocation. zlib does report ~21.4 KB/handle (node: ~15.7 KB), but only once a GC runs, so the delta the test samples immediately after creation is 0.Reporting at allocation (JSC's
deprecatedReportExtraMemory) fixes that half — the creation delta goes 0 → 350,900 — but the test still fails, because the residual it measures is first-run warmup, not retained handles:~135 KB of one-time external allocation lands between the test's two samples; by the third loop the handles are fully collected. Node passes because its streams are already warm from bootstrap. Passing it on the cold loop would mean inflating the reported per-handle size to ≥27 KB, so that experiment was dropped rather than shipped.
Known limitation
The dictionary copy and prepared dictionary are not counted in
estimated_size, so they apply no GC pressure. Bun's brotli external accounting is already a fixed approximation (ENCODER_STATE_SIZE = 5143vs the hundreds of MB a quality-11 encoder can allocate), and Node likewise does not apply GC pressure for itsdictionary_vector — it only tracks it inMemoryInfo.Review follow-ups
zstd dictionaries were ignored (
claude[bot]): correct —test-zlib-zstd-dictionary.jsonly asserts a round-trip and a loose size bound, neither dictionary-sensitive, so it passed with the dictionary silently dropped (it passes with the dictionary removed on node too). Fixed by landing zstd dictionary support rather than dropping the test.Account dictionary allocations in GC pressure (
coderabbitai): left as-is; see Known limitation. Node does not apply GC pressure for itsdictionary_vector either. zstd needs nothing here —ZSTD_CCtx_loadDictionarycopies into the context.Two build gotchas added to the verify skill (
Jarred-Sumner: "if true this is a bug to fix not a thing to work around"): neither reproduced — removed. Asrc/js/**/*.tsedit does reach the binary on a plainbun bd, and a clean relink is signed correctly by the existing-Wl,-adhoc_codesign+macho-postlinkshim. Both notes were artifacts of editing mid-build and of a link I had interrupted.zstd
Context::initleaked the previous context on re-init (claude[bot]): correct — brotli got the guard in this PR and zstd didn't. Fixed.Cache the triple
opts?.dictionaryread inZstd(claude[bot]): tried, reverted — verified on a real node binary that caching diverges. Node has the same triple read (lib/zlib.js:920) and throwsERR_INVALID_ARG_TYPEfor a mutating getter; the cached version silently accepts. The within-diff inconsistency is node's own (it caches inZlib/Brotli, not inZstd), so mirroring it per-class is what keeps all three at parity.Related
node:zlibfor brotli and zstd).dictionaryoption for BrotliCompress/BrotliDecompress #33747, which does the brotli JS half only; this PR also covers the native attach, zstd,zlib/iter, thereset()semantics, and vendors the upstream tests.[review] gate passed · iteration 1 · 12 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 1
evidence per changed file