Conversation
Several tests in bun-write.test.js built their paths as tmpbase + "name". That was correct when tmpbase was tmpdir() + path.sep, but since the tests moved to per-test `using tmpbase = tempDir(...)` (which has no trailing separator) the concatenation names a sibling of the temp dir, so every run left the scratch files behind in os.tmpdir() after the dir was disposed. Use join(tmpbase, name) everywhere so the files land inside the disposed directory, and drop the try/unlinkSync blocks in the "large file" tests: unlinkSync was never imported there, so they only ever swallowed a ReferenceError, and disposal now covers the cleanup.
|
Warning Review limit reached
Next review available in: 26 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 |
|
Status: test-only change, ready for review. Reproduced on main with the release binary: CI (build 95995): 177 of 179 jobs passed, including every Linux and Windows test lane. The remaining two are the |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — mechanical test-hermeticity cleanup, no runtime changes.
What was reviewed:
- Each converted call site:
tmpbase + "name"→join(tmpbase, name)matches the pattern already used elsewhere in this file, andtempDirreturns aDisposableString(extendsString) thatjoin()already accepts at the untouched sites. - Confirmed
unlinkSyncis not imported in this file (import fs, { mkdirSync } from "fs"), so the removedtry { unlinkSync(...) } catch {}blocks were indeed swallowingReferenceErrorand doing nothing. - Removed
fs.unlinkSyncpre-cleanup andDate.now()suffixes are redundant now that each test has a fresh per-test temp dir; the two droppedgcTick()calls in "Bun.file -> Bun.file" were tied to the deleted unlinks. - All assertions in every touched test are byte-identical to before.
Extended reasoning...
Overview
This PR touches a single test file, test/js/bun/io/bun-write.test.js, fixing seven tests that were leaking ~1.9 MB of scratch files into os.tmpdir() on every CI run. The root cause was tmpbase + "name" concatenation where tmpbase is a using tempDir(...) handle with no trailing separator, producing sibling paths that survive the directory's disposal. The fix replaces every such concatenation with join(tmpbase, name), removes dead try { unlinkSync } catch {} blocks (where unlinkSync was never imported and the catch swallowed the ReferenceError), removes now-redundant pre-cleanup fs.unlinkSync calls on fresh temp dirs, and drops Date.now() filename suffixes made unnecessary by per-test isolation.
Security risks
None. Test-only change; no runtime code, no new inputs, no external I/O beyond what the tests already did.
Level of scrutiny
Low. This is a mechanical refactor of test scratch-path construction with no assertion changes. I verified against test/harness.ts that tempDir returns a DisposableString extends String, which path.join already handles at the untouched join(tmpbase, ...) sites in the same file (e.g. the first four lines of the "Bun.write blob" test), so the new sites follow an established, working local convention. I confirmed from the file's import list that bare unlinkSync is genuinely undefined, validating the PR description's claim that those blocks were dead. Every assertion in the diff is unchanged; only the path variable feeding it moved from a leaking sibling path to a path inside the disposed directory.
Other factors
The PR description documents both a before/after ls /tmp verification and a full bun bd test run (49 pass, one pre-existing timeout unchanged from main and tracked separately). The change aligns directly with the repo's review guidance that tests must be hermetic and leave nothing behind on persistent CI runners. The two removed await gcTick() calls in "Bun.file -> Bun.file" were interleaved with the deleted no-op unlinks and had no bearing on the assertions that follow. No CODEOWNERS or security-sensitive paths are involved.
This branch has not been deployed
1 participant
Problem
test/js/bun/io/bun-write.test.js(bun testorbun bd teston a dev machine) leaves 10 scratch files (about 1.9 MB) behind inos.tmpdir(), for example/tmp/bun-write-output-html_mfwTfcoutput.htmland three 630 KBlarge-file-*_XXXXXXbun-test-large-file-<ts>.txt*files. CI does not accumulate them only becausescripts/runner.node.mjshands each test process a privateTMPDIRand removes it afterwards.tmpbase + "name".tmpbaseis ausing tmpbase = tempDir(...)directory with no trailing separator, so the concatenation names a sibling of the temp dir, not a file inside it, and disposing the dir does not remove it.const tmpbase = tmpdir() + path.sep; abb82a6 moved the tests to per-testtempDir(...)and converted some call sites tojoin()but not these.try { unlinkSync(...) } catch {}blocks call a bareunlinkSyncthat is never imported in this file, so each block swallows aReferenceErrorand nothing is unlinked.Fix
join(tmpbase, name), which the other tests in this file already do (including untouched lines in the same test bodies), so the files are created inside the directory thatusingdisposes.unlinkSyncblocks in the "large file" tests; the per-test directory is fresh, and disposal now covers the cleanup. TheDate.now()suffixes are also dropped since the directory already makes the names unique.ls /tmpbefore and afterUSE_SYSTEM_BUN=1 bun test test/js/bun/io/bun-write.test.json main shows the 10 stray files listed below; the same before/after with this branch underbun bd testshows none.bun bd test test/js/bun/io/bun-write.test.js: 49 pass. The one failure,should work when copyFileRange is not available > on large filestiming out at 5 s under the debug build, is unchanged from main (reproduced with the unmodified file) and is being addressed separately in test: stop the Bun.write copy_file_range fallback test from starving its concurrent siblings #37792.Stray files left by one run on main