FileSink: resolve the pending write()'s promise when end()'s flush drains it - #35397
FileSink: resolve the pending write()'s promise when end()'s flush drains it#35397robobun wants to merge 1 commit into
Conversation
…ains it When a backpressured write() leaves its promise outstanding and the caller then drains the read side and calls sink.end() before any await, end_from_js's own flush() can push the buffered remainder out synchronously and land in the Done/Wrote arm. Those arms called writer.end() and returned the drained byte count without touching the pending slot; IOWriter::flush() doesn't route through on_write, so nothing else ever settled the write()'s promise and it hung forever. Mirror the Pending/Err arms: when a write is pending, hand back that promise and schedule run_pending to resolve it with the Owned(consumed) that to_result already latched. The no-pending path is unchanged.
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
WalkthroughFileSink now preserves an outstanding backpressured write promise when ChangesFileSink pending write handoff
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| const reader = (async () => { | ||
| while (drained < total) { | ||
| drainSync(); | ||
| if (drained >= total) break; | ||
| await new Promise<void>(r => setImmediate(r)); | ||
| } | ||
| })(); | ||
|
|
||
| expect(await writePromise).toBe(chunkSize); | ||
| await reader; |
There was a problem hiding this comment.
🟡 The reader IIFE is only awaited inside the try body (line 345), not in finally — if expect(await writePromise).toBe(chunkSize) at line 344 throws, finally closes readFd, and on the next tick drainSync() hits EBADF → caught → break inner loop with drained unchanged → outer while (drained < total) re-awaits setImmediate forever, leaking a spinning loop into later tests. The sibling tests in this file put await reader in finally for exactly this reason; add a done flag the loop checks, set it in finally before closing the fds, and await reader there.
Extended reasoning...
What the bug is
The new test creates a background reader IIFE at lines 336-342 that loops while (drained < total), calling drainSync() and then await new Promise(r => setImmediate(r)) each iteration. The only place this reader is awaited is line 345, inside the try body — the finally block closes writeFd and readFd but never signals or awaits reader.
The specific code path that triggers it
If line 344 — expect(await writePromise).toBe(chunkSize) — throws (either because writePromise rejects, or because it resolves to something other than chunkSize, i.e. exactly the regression this test guards against), control jumps straight to finally without ever reaching await reader. The finally block then does fs.closeSync(readFd).
Step-by-step proof
- Line 344 throws → jump to
finally. finallyrunsfs.closeSync(readFd). The reader IIFE is still suspended onsetImmediate.- Next tick: reader resumes, calls
drainSync()→fs.readSync(readFd, buf)on the closed fd → throwsEBADF. - The
catch { break }breaks only the innerfor(;;)loop.drainedwas not incremented. - Back at the outer loop:
drained < totalis still true (nothing changed), so it does not exit. await new Promise(r => setImmediate(r))→ step 3 repeats forever.
The setImmediate handle keeps the event loop referenced each tick, so this leaks a hot-spinning loop into every subsequent test in the file. Worse, closed fd numbers are recycled — a later test that opens a socket may get the same fd number, and this loop will silently readSync from that unrelated fd.
Why existing code doesn't prevent it
drainSync()'s catch handler was written for the happy-path EAGAIN case (nonblocking socket has no more data right now), where breaking the inner loop and re-polling via setImmediate is correct. It doesn't distinguish EBADF, and nothing else terminates the outer while. The finally block has no reference to reader at all.
Impact
This only bites when the test is already failing — the endResult === writePromise assertion at line 333 (the primary regression guard) fires before the reader is created, so the leak window is exactly line 344. But line 344 is the test's core resolution-value assertion; a future regression in pending.consumed accounting would make it fail and then poison the rest of the suite on persistent CI runners. This is precisely what REVIEW.md's rule targets: "Release every resource via using/await using or try/finally registered BEFORE the assertions (cleanup after expectations leaks on the first failure and poisons later tests on persistent CI runners)".
How to fix
Match the sibling tests in this file (e.g. "a backpressured write() resolves to the chunk's byte count"), which put await reader inside finally. Since this reader has no natural EOF, add a termination flag:
let readerDone = false;
const reader = (async () => {
while (!readerDone && drained < total) {
drainSync();
if (drained >= total) break;
await new Promise<void>(r => setImmediate(r));
}
})();and in finally, before closing the fds:
readerDone = true;
await reader;(or equivalently, bound the loop with an iteration cap).
1 participant
Sibling of #35344 (the Err arm) for the Done/Wrote arms of
FileSink::end_from_js().Repro
Cause
FileSink::end_from_js()'sflush()call drains the buffered remainder synchronously and returnsDone/Wrote. Those arms calledwriter.end()and returnedjs_number(written)without touchingself.pending.IOWriter::flush()updates its buffer head but doesn't route throughon_write, andwriter.end()only fireson_closewhich never touches the pending slot either. Nothing ever schedulesrun_pending, so the backpressuredwrite()'s promise is orphaned.The deferred auto-flush path already handles this (its Done/Wrote arms call
run_pending_later()); the bug is only reachable whenend()runs before the first microtask checkpoint after the backpressured write, soend_from_jsdoes the flush itself.#35344 fixed the Err arm of
end_from_js()with the same shape; this completes the Done/Wrote arms. #35365 covers the Err arm ofFileSink::end()(sink.close()), whose Done/Wrote arms have the same gap and are left for that PR's scope.Fix
When a write is pending, Done/Wrote now behave like the Pending/Err arms already do: grab the outstanding promise, tear the writer down, schedule
run_pending_later(), and return that promise.pending.resultalready holdsOwned(consumed)fromto_result, sorun_pendingresolves it with the write's chunk size. The no-pending path is unchanged.Verification
Also green:
spawn.test.ts -t EPIPE,spawn-streaming-stdin.test.ts,spawn-stdin-readable-stream.test.ts,shell/epipe.test.ts,rust:check-all(10/10).