FileReader: release the io-ref in on_cancel when close() doesn't reach on_reader_done

[robobun]

What

test/js/node/test/parallel/test-stdin-from-file-spawn.js panicked on the linux x64-asan lane (build 83189) with:

panic: assertion failed: !(self.done.get() && self.waiting_for_on_reader_done.get())

in FileReader::finalize_detach. The assertion says a FileReader cannot reach its JS finalizer with both done and waiting_for_on_reader_done set: waiting means the io-ref taken by from_pipe/on_start is still held (which upgrades the JS wrapper to Strong, so it should not be collectable), and done means on_cancel has run and nothing will ever call on_reader_done to release that ref.

Change

FileReader::on_cancel now releases the io-ref itself after close() returns. on_cancel is the only place that sets done = true (other than finalize_detach), so once it runs no more io will fire and the io-ref is its responsibility. Today close() always reaches on_reader_done for every in-tree from_pipe producer (subprocess pipes have a valid fd and CLOSE_HANDLE set, traced below), so this block is a no-op on current main; the release is idempotent with on_reader_done's own check-and-clear. waiting == true implies a prior increment_count, so ref_count >= 2 and the decrement cannot free the box mid-&self.

Why

This is defensive hardening, not a root-cause fix. I traced every producer of waiting_for_on_reader_done = true and every transition to an invalid reader fd:

• from_pipe (ReadableStream.rs:420) sets waiting after PosixBufferedReader::from() transferred a handle whose fd came from SubprocessPipeReader::start() → reader.start(fd, true), which stores handle = PollOrFd::Fd(fd).
• on_start's lazy path (FileReader.rs:417) sets waiting only when pollable with a debug_assert!(opened.fd.is_valid()) guard; on error it clears waiting before returning.
• on_start's non-lazy path (FileReader.rs:438) sets waiting only when POLLABLE, which is inserted after start() stored a real fd.
• PollOrFd::close_impl skips its callback only when get_fd() == INVALID, which for a live Poll never holds (the fd is set at construction and invalidated only inside close_impl itself); every path that reaches handle.close(cb) with waiting == true has a valid fd and fires done() → on_reader_done().
• CLOSE_HANDLE defaults on and is transferred wholesale by from(); the only removers are FileResponseStream and the shell IOReader, neither of which go through from_pipe.

So no in-tree path today produces (waiting = true, fd invalid) at on_cancel entry. The CI panic is a single sighting; 800+ local debug+asan runs under BUN_GARBAGE_COLLECTOR_LEVEL=1 and 4-way parallel stress did not reproduce it, and the release-asan artifact is not fetchable from this environment to symbolize the stack. Build 83189's branch is 3 commits behind main (merge-base 6c12afd) and the reporter's diff is unrelated (fs.cp non-UTF8 names).

close() is not contract-bound to dispatch on_reader_done for every reader state, and relying on it for the io-ref release means a future reader change (or a state I have not found) can strand the ref silently. Having on_cancel own the release makes the done && waiting state structurally impossible through on_cancel, which is the only producer of done = true.

Verification

test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts gains a post-cancel invariant check: after cancelling a from_pipe FileReader that never received data, no FileInternalReadableStreamSource remains Strong-protected and the wrappers are collectable. On current main this is satisfied by the existing close() → on_reader_done path, so the test is a guard against regressing that invariant rather than a reproduction of the CI panic. The three existing lifetime-invariant tests in that file, spawn-unread-stdout-gc.test.ts, native-source-onclose-leak.test.ts, process-stdin.test.ts, and spawn-streaming-stdout.test.ts all pass under bun bd.

---

no test proof · iteration 0 · Platform-specific test(s) that do not run on this machine. Deferring to CI, which covers all platforms: test/js/node/test/parallel/test-stdin-from-file-spawn.js test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 22 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: f6b9a28c-dcd5-4532-be57-1c3e287d4024

📥 Commits

Reviewing files that changed from the base of the PR and between 5c7f61b and 8a260d7.

📒 Files selected for processing (1)

• test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts

Walkthrough

Changes

FileReader cancellation cleanup

Layer / File(s)	Summary
Cancellation cleanup and lifetime validation src/runtime/webcore/FileReader.rs, test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts	FileReader::on_cancel releases the pending Source reference when on_reader_done is skipped. A POSIX-only test checks garbage-collection state after cancelling lazy subprocess stdout before data arrives.

Suggested reviewers: jarred-sumner

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title is clear, specific, and accurately summarizes the main FileReader lifecycle fix.
Description check	✅ Passed	The description covers what changed and how it was verified, though it uses non-template headings.

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 2

🤖 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-stdout-filereader-gc-uaf.test.ts`:
- Around line 473-476: Remove the duplicate declarations of the result variable
in the surrounding test block, retaining a single let result binding with the
existing { iters, protectedAfterCancel, aliveAfterCancel } type before the try
statement.
- Around line 467-486: Add an explicit, non-strict assertion that the captured
stderr in the spawn test is acceptable before the existing exitCode assertion,
while allowing expected debug/ASAN/--smol noise. Keep stdout parsing and the
existing result assertions unchanged, and place the stderr validation in the
flow around proc.exited.

🪄 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: 2855fa7e-23b5-43dd-9322-e35e866dfe12

📥 Commits

Reviewing files that changed from the base of the PR and between 4eb6f99 and 068086a.

📒 Files selected for processing (2)

• src/runtime/webcore/FileReader.rs
• test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts

[claude]

Beyond the inline finding, I also checked whether the new on_cancel decrement can race a later on_reader_done and double-decrement — both sites gate on and clear waiting_for_on_reader_done, so they are mutually exclusive. The Windows deferred-done() path (DEFER_DONE_CALLBACK) was also examined for a post-decrement UAF: it only applies to Source::File/SyncFile, not the from_pipe subprocess path, and the later on_reader_done sees waiting == false and skips its own decrement.

Extended reasoning...

This PR touches unsafe refcount lifecycle code in FileReader (Source::decrement_count under unsafe), which per the repo's review guidelines is the most-blocked category and warrants human sign-off. The fix itself reads correctly — waiting_for_on_reader_done == true implies a prior increment_count, so ref_count >= 2 and the decrement cannot free the box mid-&self; and both on_reader_done and the new guard check-and-clear the same flag, so at most one decrement fires. The inline finding about the new test likely passing on a pre-fix build is the actionable item; the PR description already acknowledges the actual interleaving was not reproducible locally.

[robobun]

^{Updated 4:41 AM PT - Jul 27th, 2026}

✅ @robobun, your commit 8a260d7b8c8b4b979503ee89e7cbbbd2cd0be26a passed in Build #83280! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 36076

That installs a local version of the PR into your bun-36076 executable, so you can run:

bun-36076 --bun

[coderabbitai]

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-stdout-filereader-gc-uaf.test.ts`:
- Around line 396-403: Remove the implementation-history and future-change
rationale comment above the post-cancel invariant in the test; retain only a
tracked issue URL if one exists, following the repository’s regression-test
comment convention.

🪄 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: 86646784-e9f3-4569-b956-565928d99e90

📥 Commits

Reviewing files that changed from the base of the PR and between 068086a and 5c7f61b.

📒 Files selected for processing (2)

• src/runtime/webcore/FileReader.rs
• test/js/bun/spawn/spawn-stdout-filereader-gc-uaf.test.ts

[robobun]

Status: CI running (build 83280). The diff is ready for review.

The panic this targets is a single sighting and not reproducible locally (800+ debug+asan runs under GC=1 and 4-way parallel stress; details in the PR body). A code trace of every waiting_for_on_reader_done = true producer and every handle-invalidation path found no in-tree interleaving that reaches on_cancel with (waiting = true, fd invalid), so the new test is a guard for the post-cancel invariant rather than a reproduction and passes on a pre-fix build. The source change makes on_cancel own the io-ref release instead of relying on close() to dispatch on_reader_done; it is idempotent with the existing path and cannot double-decrement.

Needs a maintainer to judge whether the analysis warrants merging without a fail-before reproduction.