Conversation
Bun.ArrayBufferSink.write() after end() was appending to the internal Vec and returning the written byte count, but end()/flush() read from the post-end reset state and returned an empty buffer. A caller doing 'written += sink.write(chunk)' saw success accounting for data that was retained in RSS and never surfaced. JSSink::js_write now checks JsSinkType::done() before dispatching to the sink's write path and throws ERR_STREAM_WRITE_AFTER_END, matching the documented 'Once .end() is called, no more data can be written' contract and Node's ERR_STREAM_WRITE_AFTER_END. This also replaces FileSink's odd boolean 'true' return on write-after-end with the same error.
WalkthroughChangesThe sink write path now throws Sink write lifecycle
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@test/js/bun/util/arraybuffersink.test.ts`:
- Around line 145-150: Update the catch block in the RSS fixture’s sink write
loop to assert that the caught exception has the expected error code for writing
after end, and only then increment threw; rethrow or otherwise fail on
unexpected errors so the test proves the intended contract.
🪄 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: 29e1930b-a2fe-49e4-b71f-ab23fc3c46c4
📒 Files selected for processing (3)
src/runtime/webcore/Sink.rstest/js/bun/util/arraybuffersink.test.tstest/js/bun/util/filesink.test.ts
…ild exit The child in bad-fixture.js throws on startup, so by the time the test writes to stdin the pipe is closed and the FileSink is done. write() now throws instead of silently returning true; the test still asserts child.exited resolves to 1 (the original 'TODO' rejection bug) and nothing crashes under GC.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
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 `@test/js/bun/spawn/spawn-stdin-destroy.test.ts`:
- Around line 20-26: Update the test’s stdin operations so the expected
ERR_STREAM_WRITE_AFTER_END error is captured and asserted specifically around
the write() call that should fail, rather than sharing one try/catch with
flush(). Handle flush() in a separate check only if its behavior also requires
coverage, ensuring the test proves the write operation is the source of the
failure.
🪄 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: 3855260e-b834-4963-bb29-42ae37272801
📒 Files selected for processing (2)
test/js/bun/spawn/spawn-stdin-destroy.test.tstest/js/bun/util/arraybuffersink.test.ts
…onst The generic done() check in js_write applied to all JsSinkType implementors, but done() does not mean 'user called end()' for every sink: HTTPServerWritable sets it on client abort, and FileSink is wrapped by node:fs's writeFast fast-path which expects stream-state errors to reach the callback rather than throw synchronously. Gate the throw on a new THROW_ON_WRITE_AFTER_END associated const, default false, set true only for ArrayBufferSink (whose done bit is set exclusively by end_from_js and cleared by start). FileSink, HTTPServerWritable, and NetworkSink behavior is unchanged. Also: RSS threshold branches on isASAN || isDebug per convention.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
robobun
changed the title
There was a problem hiding this comment.
LGTM — the fix is now scoped to ArrayBufferSink via the opt-in THROW_ON_WRITE_AFTER_END trait const, so FileSink/HTTPServerWritable/NetworkSink behavior is unchanged, addressing the concerns I raised on the earlier revision.
What was reviewed:
- Confirmed
ArrayBufferSink::done()is set only byend_from_jsand cleared bystart(), so the guard fires exactly on user-calledend();start()reset is covered by the first test. - Verified no
src/js/internal callers ofArrayBufferSinkexist, so the new synchronous throw can't break a builtin the way the earlier FileSink change would have viawriteFast. ErrorCode::ERR_STREAM_WRITE_AFTER_END.throw(global, format_args!(...))matches the existing pattern at Sink.rs:414 and the code is already registered inErrorCode.ts.- RSS test: subprocess-isolated, drains pipes concurrently, asserts the specific error code in the catch, threshold branches on
isASAN || isDebug(well below the ~200 MiB unfixed leak).
Extended reasoning...
Overview
Adds a write-after-end guard to JSSink::js_write gated on a new JsSinkType::THROW_ON_WRITE_AFTER_END associated const (default false). Only ArrayBufferSink opts in. 1 line in ArrayBufferSink.rs, 7 lines in Sink.rs, and two new tests in arraybuffersink.test.ts. Earlier revisions applied the guard unconditionally to all sink types; after my prior review flagged that this broke FileSink (via the writeFast fast-path in child.stdin) and gave misleading errors for HTTPServerWritable on client abort, the author scoped it via the opt-in const and reverted the filesink.test.ts/spawn-stdin-destroy.test.ts changes.
Security risks
None. This adds a guard that throws earlier on a misuse pattern; no new input parsing, no auth/crypto/permissions surface. The guard runs before any argument coercion, so no user JS is entered before the check.
Level of scrutiny
Moderate — this is a user-facing behavior change to Bun.ArrayBufferSink.write(), but it replaces clearly-broken behavior (silent data loss + unbounded RSS growth reporting success) with the standard Node error code for exactly this situation. The compile-time const gate means the other three JsSinkType implementors are provably unaffected (the branch is dead code for them). The done() predicate for ArrayBufferSink is trivially self.done, set only in end_from_js and cleared in start(), so there is no ambiguity about what state triggers the throw.
Other factors
- All prior review feedback resolved: the CodeRabbit error-code assertion in the RSS fixture catch block, my
isASAN || isDebugthreshold nit, and the comment-cop doc-comment length flags were all applied. - The error uses the centralized
bun_jsc::ErrorCodemachinery and matches the invocation pattern already used two lines up inget_this(Sink.rs:414). - Tests follow harness conventions:
await usingfor the spawned proc,Promise.allon stdout/stderr/exited,Buffer.alloc(n, fill)instead of.repeat(), exit code asserted last, specific error code asserted (not baretoThrow()), and thestart()-resets-the-sink contract is explicitly covered. - Verified against
USE_SYSTEM_BUN=1(fails) andbun bd(passes) per the PR body's evidence block.
This branch has not been deployed
2 participants
Bun.ArrayBufferSink.write()called afterend()reported success (the full byte count), appended the data to its internalVec, and grew RSS, but the nextend()/flush()returned an empty buffer. A caller doingwritten += sink.write(chunk)would account bytes that were retained in memory and never recoverable, even after a subsequentstart().Reproduction
Cause
ArrayBufferSink::write/write_latin1/write_utf16unconditionally append toself.bytesand returnWritable::Owned(len).end_from_jssetsself.done = trueand takesself.bytes, leaving an emptyVecthat subsequent writes grow again, while every laterend_from_jsshort-circuits onself.doneand returns an empty buffer.The generic
JSSink::js_writehost-fn body already checksget_pending_error()before dispatching but had no ended-state guard.Fix
JsSinkTypegains an opt-inTHROW_ON_WRITE_AFTER_ENDassociated const (defaultfalse).JSSink::js_writethrowsERR_STREAM_WRITE_AFTER_END("write after end") when that const is set anddone()is true.Only
ArrayBufferSinkopts in: itsdonebit is set exclusively byend_from_jsand cleared bystart, sodone()there is exactly "user called end()".FileSink(wrapped bynode:fs'swriteFastfast-path, which routes stream-state errors through callbacks),HTTPServerWritable(whosedoneis also set on client abort), andNetworkSinkkeep their existing write-after-done behavior unchanged.start()still resets the sink for reuse.Verification
[review] gate passed · iteration 2 · 3 files touched
fails on main (without fix)
passes on PR (with fix)
diff hotspot
gate history · 1 passed · 1 rejected · iteration 2
evidence per changed file