test: close the /proc FileHandle in test-fs-promises-file-handle-readFile - #34283
Conversation
…File Syncs validateReadFileProc() with upstream nodejs/node@2eeb65fa81. Since #33693, a FileHandle collected without close() throws ERR_INVALID_STATE from a FinalizationRegistry. This test opened /proc/sys/kernel/hostname and never closed it, so whenever GC fired during the later doReadAndCancel() section the process crashed with 'A FileHandle object was closed during garbage collection'. It has been flaking on ~35% of Linux CI builds (140/400 sampled).
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
Updated 6:52 PM PT - Jul 15th, 2026
❌ @robobun, your commit 075fb10 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34283That installs a local version of the PR into your bun-34283 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — one-line test-only sync with upstream Node to close a leaked FileHandle.
What was reviewed:
- Confirmed
FileHandleimplements[Symbol.asyncDispose]→close()insrc/js/node/fs.promises.ts:726, soawait usingdisposes correctly. - Verified this was the only unclosed
open()in the file; the other handles already have explicitclose()/ try-finally. - Change matches upstream nodejs/node@2eeb65fa81 and the pattern applied to sibling tests in #33693.
Extended reasoning...
Overview
Single-line change in a vendored Node.js parallel test: const fileHandle = await open(...) → await using fileHandle = await open(...) in validateReadFileProc(). This ensures the /proc/sys/kernel/hostname FileHandle is closed when the function returns, instead of being leaked and later triggering the DEP0137 end-of-life ERR_INVALID_STATE from the FinalizationRegistry added in #33693.
Security risks
None. Test-only change; no production code paths, no new inputs, no external I/O beyond what the test already did.
Level of scrutiny
Low. This is a mechanical de-flake of a vendored upstream test, mirroring the exact fix Node.js itself made (nodejs/node@2eeb65fa81). I verified in src/js/node/fs.promises.ts that Bun's FileHandle defines [Symbol.asyncDispose] delegating to close(), so await using behaves as intended. I also scanned the rest of the test file: every other await open(...) is paired with an explicit await fileHandle.close() (one inside try/finally), so this was the only remaining leak — consistent with the CI error message naming /proc/sys/kernel/hostname.
Other factors
The PR description documents 140/400 flaky annotations on Linux lanes and explains why a deterministic local repro isn't available (GC-timing-dependent, Linux-only). No outstanding reviewer comments; no prior reviews from me. The change is self-contained, follows an established pattern already applied to three sibling tests in #33693, and cannot affect anything outside this single test file.
|
CI red on build 73457 is the two most frequent hard-red flakes in the pipeline right now, neither related to this diff:
This diff touches only |
2 participants
What
Sync
validateReadFileProc()with upstream nodejs/node@2eeb65fa81 (consttoawait using) so the/proc/sys/kernel/hostnameFileHandle is disposed.Why
#33693 added a FinalizationRegistry that throws
ERR_INVALID_STATEwhen aFileHandleis collected withoutclose()(DEP0137 end-of-life). This test opened the hostname fd and never closed it; whenever GC happened to run during the laterdoReadAndCancel()section, the registry fired and the process exited 1 with:It has been in the
flakyannotation on 140 of the last 400 Buildkite builds (Linux lanes only, heaviest onx64-asanwhere GC pressure is highest).#33693 already fixed three other vendored tests the same way; this one was missed because it is Linux-only and GC-timing-dependent. Upstream Node made the same change in nodejs/node@2eeb65fa81.
Verification
The failure is GC-timing-dependent so a deterministic fail-before is not available locally; the evidence is the CI annotation count plus the fact that the unclosed handle is the only one in the file whose path matches the error's
(/proc/sys/kernel/hostname).no test proof · iteration 0 · docs-only change; test-proof not applicable