Repository navigation
sys(windows): stat/futime HANDLE-backed fds directly instead of leaking a CRT fd - #33713
Conversation
|
Updated 9:38 PM PT - Jul 7th, 2026
⏳ @robobun, your commit ddc62e1 is still building in |
|
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)
WalkthroughWindows ChangesWindows HANDLE stat and time updates
Possibly related PRs: Suggested labels: windows, regression-test, filesystem Suggested reviewers: None identified. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
|
This PR may be a duplicate of:
🤖 Generated with Claude Code |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/js/bun/sys/fstat-windows-crt-fd-leak.test.ts (1)
10-56: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider an explicit per-test timeout for the 9000-iteration loop.
bun:test's default timeout is 5000ms. This test spawns a subprocess that performs 9000 failingBun.Image(...).metadata()calls (each doing anopenat+fstat+decode-failure round trip) plus 10 morefsopen/fstat/close calls, on Windows where debug/ASAN builds run materially slower. Since the iteration count is dictated by the CRT fd table size (8192) and can't be meaningfully shrunk without weakening the regression, this is the same class of heavy subprocess-iteration test where the repo's precedent (e.g.,cc.test.ts,glob/leak.test.ts) adds an explicit generous per-test timeout to avoid spurious CI failures on slow lanes.🕒 Suggested fix
-test.skipIf(!isWindows)("fstat on HANDLE-backed fd does not leak CRT fd slots", async () => { +test.skipIf(!isWindows)("fstat on HANDLE-backed fd does not leak CRT fd slots", async () => { ... -}); +}, 60_000);Based on learnings, explicit per-test timeouts are accepted for RSS/heavy subprocess-iteration regression tests (e.g., "do not treat explicit per-test timeouts as a guideline violation when the test is an RSS-leak regression that spawns a subprocess and performs many iterations"), which is analogous to this CRT-fd-exhaustion regression test.
🤖 Prompt for 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. In `@test/js/bun/sys/fstat-windows-crt-fd-leak.test.ts` around lines 10 - 56, The Windows CRT fd leak regression test in test/js/bun/sys/fstat-windows-crt-fd-leak.test.ts can exceed bun:test’s default 5000ms timeout because it runs a slow subprocess loop with 9000 Bun.Image(...).metadata() iterations plus fs fstat checks. Add an explicit generous per-test timeout on the test.skipIf(!isWindows) block for the fstat-on-HANDLE-backed-fd case, matching the existing heavy regression-test pattern used elsewhere, so the test remains stable on slower CI lanes without changing the iteration count.Source: Learnings
🤖 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.
Outside diff comments:
In `@test/js/bun/sys/fstat-windows-crt-fd-leak.test.ts`:
- Around line 10-56: The Windows CRT fd leak regression test in
test/js/bun/sys/fstat-windows-crt-fd-leak.test.ts can exceed bun:test’s default
5000ms timeout because it runs a slow subprocess loop with 9000
Bun.Image(...).metadata() iterations plus fs fstat checks. Add an explicit
generous per-test timeout on the test.skipIf(!isWindows) block for the
fstat-on-HANDLE-backed-fd case, matching the existing heavy regression-test
pattern used elsewhere, so the test remains stable on slower CI lanes without
changing the iteration count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fabc9ba7-2568-4ea0-b705-499e8028af37
📒 Files selected for processing (1)
test/js/bun/sys/fstat-windows-crt-fd-leak.test.ts
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡
src/sys/lib.rs:3786-3790— On the error path,sys_uv::fstat(uvfd)attachesuvfd— the temporary CRT fd — to the returnedError, andfstatthen closes that fd and returns the error unchanged, so the caller sees anError.fdreferencing a now-closed CRT slot.futimensin this same PR handles this correctly witherr.with_fd(fd)(line 4242); mirroring that here — e.g.let result = sys_uv::fstat(uvfd).map_err(|e| e.with_fd(fd));— would keep the two consistent. Diagnostic quality only.Extended reasoning...
What the bug is
windows_impl::fstatnow callsborrow_as_uv_fdto obtain a temporary CRT fd (uvfd) for HANDLE-backed inputs, passes it tosys_uv::fstat, then closes the temp fd and returns the result. Butsys_uv::fstatbuilds its error asError::new(errno, Tag::fstat).with_fd(fd)wherefdis its argument (src/sys/sys_uv.rs:492) — i.e. the temporaryuvfd, not the caller's originalFd.windows_impl::fstatreturns that error unchanged after closing the temp fd, so the error's.fdfield references a CRT fd number that no longer exists (and may already be reused).Code path
For a HANDLE-backed
fd(FdKind::System):borrow_as_uv_fd(fd, Tag::fstat)duplicates the HANDLE, wraps the dup in a CRT fd, and returns(uvfd, Some(uvfd))— whereuvfdis a fresh CRT fd number wrapping the duplicated HANDLE.sys_uv::fstat(uvfd)runsuv_fs_fstat. Suppose it fails with, say,EBADF. It returnsErr(Error { errno: EBADF, syscall: Tag::fstat, fd: uvfd, .. }).to_closeisSome(uvfd), soclose(uvfd)runs — the CRT slot and duplicated HANDLE are released.resultis returned as-is. The caller receives an error whose.fdis the numeric value of the now-closeduvfd.
The pass-through case (
FdKind::Uv,to_close == None) is unaffected because thereuvfd == fdand nothing is closed.Why existing code doesn't prevent it
sys_uv::fstathas no way to know its argument is a temporary; it always attaches whateverFdit was called with. The wrapper is the only layer that knowsuvfdis ephemeral and that the caller's real handle isfd.futimensin this same diff already handles this: at src/sys/lib.rs:4242 it doesreturn Err(err.with_fd(fd)), overriding whatever fd libuv-layer error construction attached.fstatsimply doesn't do the equivalent.Before this PR the attached fd was also the temp
uvfd, but that fd was leaked (still open) and wrapped the caller's own HANDLE (viamake_libuv_ownedon the original), so it at least pointed at the right kernel object. After this PR the attached fd is a closed dup — strictly less useful for diagnostics, though still not a correctness issue.Impact
Diagnostic quality only.
Error.fdflows intoSystemErrorformatting and the JS-visible.fdproperty on the thrown error; a stale CRT fd number there is misleading but doesn't affect program behavior. No memory-safety or correctness impact.Step-by-step example
- Caller holds a HANDLE-backed
fd(say theopenatresult insideBun.Image(path).metadata()). borrow_as_uv_fdreturns(uvfd = Fd(42, Uv), Some(Fd(42, Uv)))— 42 is a fresh CRT slot wrapping a duplicated HANDLE.uv_fs_fstatfails;sys_uv::fstatreturnsErr(Error { fd: Fd(42, Uv), .. }).close(Fd(42, Uv))runs — CRT slot 42 is released and may be immediately reused by the next_open_osfhandle.- The error propagates to JS with
.fd == 42, which now refers to nothing (or to an unrelated file).
Fix
Mirror
futimens:let result = sys_uv::fstat(uvfd).map_err(|e| e.with_fd(fd));
One line, keeps the two call sites consistent.
-
🟣
src/sys/lib.rs:3771-3783— Nice —borrow_as_uv_fdis exactly the shape the neighboringfsync/fdatasync/fchmod/fchown(lines 3810-3814, 3851-3855) also need: they still callsys_uv::*→fd.uv()directly and panic onFdKind::Systemfds (theftruncatecomment at line 3819 already calls this out for that site). This is pre-existing and not what this PR is fixing, so no need to block on it — but since the helper now sits right next to them it'd be an easy follow-up, or worth a line in the description on why they're intentionally left for later.Extended reasoning...
What this is. The PR introduces
borrow_as_uv_fdat src/sys/lib.rs:3771 whose doc comment states the problem plainly: "sys_uv::*helpers dofd.uv(), which PANICS for HANDLE-backed (FdKind::System) fds such asopenat()results." The helper is applied tofstatandfutimens. But four sibling wrappers in the samewindows_implmodule still delegate straight tosys_uv::*with a rawFd:fchmod(line 3810) →sys_uv::fchmod→fd.uv()(sys_uv.rs:196)fchown(line 3813) →sys_uv::fchown→fd.uv()(sys_uv.rs:295)fsync(line 3851) →sys_uv::fsync→fd.uv()(sys_uv.rs:514)fdatasync(line 3854) →sys_uv::fdatasync→fd.uv()(sys_uv.rs:500)
Per
bun_core::Fd::uv()(bun_core/util.rs:1034-1044), on Windows a non-stdioFdKind::Systemfd hits theunreachable!/panic branch. The adjacentftruncateat line 3816 already carries a comment acknowledging exactly this ("sys_uv::ftruncate requires a CRT fd viafd.uv(), which fails for HANDLE-backedFds") and works around it viaNtSetInformationFile— so the codebase already recognizes this as a real hazard for these sibling functions.Concrete path.
File::openat(...)on Windows routes throughopenat_windows_a→NtCreateFile, returning a HANDLE-backedFdwithkind() == FdKind::System(see line 3859-3866). If any caller then doesbun_sys::fsync(that_fd)(orfdatasync/fchmod/fchown),sys_uv::fsynccallsfd.uv()on it and panics. Nothing in the call chain converts the fd first — unlikefstat/futimens, these four never had themake_libuv_owned()workaround.Why this is pre-existing, not introduced. The bug this PR fixes is specifically the CRT-fd leak in the two sites that had already been converted to
make_libuv_owned()(the old comment said "acknowledged CRT-fd leak — a leak is strictly better than a guaranteed panic").fsync/fdatasync/fchmod/fchownnever received that conversion in the first place; they panicked on HANDLE-backed fds before this PR and still do after it. The PR neither adds callers to them nor changes their behavior — it's a strict improvement that happens to leave a pre-existing adjacent defect in place.Impact. Any code path that obtains a HANDLE-backed fd (via
openat) and then calls one of these four functions crashes the process on Windows. Whether such a path is currently reachable from user JS wasn't established here (it may well not be, which would explain why these were left as-is), but the hazard is identical to the one the helper's doc comment describes.Suggested follow-up. Route the four sites through the new helper the same way
fstatnow does, e.g.:pub fn fsync(fd: Fd) -> Maybe<()> { let (uvfd, to_close) = borrow_as_uv_fd(fd, Tag::fsync)?; let result = sys_uv::fsync(uvfd); if let Some(tmp) = to_close { let _ = close(tmp); } result }
Alternatively, if these are intentionally excluded (e.g. no caller ever passes a HANDLE-backed fd to them today, or
fchownis a no-op on Windows anyway), a note in the PR description satisfies the "if a site is intentionally excluded, say so" convention from CLAUDE.md's Fix the whole class guidance.
eaa39c3 to
741c726
Compare
|
Reworked per review: dropped the On the earlier bot findings:
Verified on Windows x64: test fails with canary bun ( |
…ng a CRT fd On Windows, bun_sys::fstat and bun_sys::futimens on a HANDLE-backed fd (FdKind::System, what openat/NtCreateFile return) converted the HANDLE to a CRT fd via _open_osfhandle so uv_fs_fstat/uv_fs_futime could consume it, then never released that CRT slot. It cannot be released: _close on a CRT fd also closes the underlying HANDLE, which the caller still owns. The CRT fd table is capped at 8192, so long-running processes that repeatedly hit this path (e.g. Bun.Image(path)) drift into EMFILE on unrelated file operations. libuv's uv_fs_fstat/uv_fs_futime immediately convert the CRT fd back to a HANDLE and do all their work on it; the CRT layer is API-shape friction. Port the HANDLE-based stat logic (NtQueryInformationFile / NtQueryVolumeInformationFile, matching libuv's fs__stat_handle) and SetFileTime into bun_sys and call them directly for HANDLE-backed fds. No CRT fd is allocated and no extra syscalls are added. Uv-kind fds continue through sys_uv::fstat unchanged.
741c726 to
8980bab
Compare
|
Reworked again per review: dropped the libuv patch. Both helpers are now implemented in Rust inside
Verified on Windows x64 at |
|
CI status: the diff itself is green. The remaining red on builds 70244 and 70272 is None of these touch this change: the diff is entirely The new test ( |
Line-by-line verificationStruct layouts (verified on Windows against real SDK headers)Compiled
Constants (verified against SDK headers)
Arithmetic (verified against libuv source at oven-sh/libuv@4dcfac47)
|
| Branch | libuv | This PR | Match |
|---|---|---|---|
FdKind::Uv |
n/a (public uv_fs_fstat) |
early return to sys_uv::fstat |
no change from before |
| Pipe | _S_IFIFO, nlink=1, rdev=FILE_DEVICE_NAMED_PIPE<<16, ino=handle |
same | yes |
| Console | _S_IFCHR, nlink=1, rdev=FILE_DEVICE_CONSOLE<<16, ino=handle |
same (gated on GetConsoleMode success) |
yes |
| Non-console char | falls through to fs__stat_handle |
falls through to NT-query path | yes |
FILE_DEVICE_NULL |
fs__stat_assign_statbuf_null: _S_IFCHR|rw-rw-rw-, nlink=1, blksize=4096, rdev=NULL<<16 |
same | yes |
| Directory | _S_IFDIR, size=0 |
same | yes |
| Regular | _S_IFREG, size=EndOfFile |
same | yes |
| Readonly | mode += r--r--r-- |
same | yes |
| Writable | mode += rw-rw-rw- |
same | yes |
| Timestamps | 4× uv__filetime_to_timespec |
4× filetime_to_timespec |
yes |
st_dev |
VolumeSerialNumber.LowPart |
VolumeSerialNumber as u64 (u32 widened) |
yes |
st_ino/nlink/blocks/blksize |
IndexNumber / NumberOfLinks / Alloc>>9 / 4096 | same | yes |
| Reparse point (do_lstat=0) | falls through to DIR/REG | falls through (no IFLNK arm) | yes |
Error handling: all three NT queries use NT_ERROR (excludes warnings like STATUS_BUFFER_OVERFLOW), matching libuv. The NOT_IMPLEMENTED check reads rc instead of io_status.Status; these are documented-identical for synchronous NtQuery* calls.
futimens
SetFileTime(handle, NULL, &a, &m) with TimeLike → FILETIME. Matches libuv's fs__utime_handle → SetFileTime path.
Callers and blast radius
windows_impl::fstat with a FdKind::System fd is reached from exactly 6 sites: Bun.Image(path) pipeline, bun pm pack bundledDependencies, bun create Dir::copy_file, bundler get_hashed_filename, ModKey::generate (no live callers), MiniEventLoop::stdout(). All other fstat callers pass FdKind::Uv (early return, unchanged) or are #[cfg(unix)].
windows_impl::futimens has zero callers (node:fs futimes on Windows calls uv_fs_futime directly).
Non-regressions
futimensdoes not handleUTIME_NOW/UTIME_OMITsentinels. Neither did the previous code (it computed0.0 + (-1.0/1e9)which is not inf/nan, so libuv's sentinel handling never fired). Zero callers.- Pre-existing:
fsync/fdatasync/fchmod/fchownin the same module still panic onFdKind::Systemfds. Separate issue (panic, not leak), no callers pass one today.
Thread safety
All buffers are stack-local. NtQuery*/GetFileType/GetConsoleMode/SetFileTime are stateless syscalls. Bun.Image already ran sys_uv::fstat from a worker thread before this change; same thread context now.
Perf
Old (FdKind::System): _open_osfhandle (CRT lock) + uv_fs_fstat (= uv__get_osfhandle + GetFileType + 3× NT query). New: GetFileType + 3× NT query. Two fewer calls, no CRT lock contention. FdKind::Uv: unchanged.
End-to-end Windows verification at ddc62e1a4d
- Leak test: fails with canary (
EMFILE at iteration 8189 syscall=uv_open_osfhandle), passes with this change (9000 iterations, 2.6s debug). image.test.ts: 94/94bun-file.test.ts: 6/6fs.test.ts -t stat: 16/16- Stat correctness probe:
Bun.Image(png).metadata()→ 2×2; directory →ENODEV.
- externs.rs: main and PR both added FILE_ALL_INFORMATION / FS_INFORMATION_CLASS / FILE_FS_* types independently; auto-merge kept both. Removed main's copies (PR's have quirk tags and are what winfs consumes). - sys/windows/mod.rs: removed main's filetime_to_timespec / timespec_to_filetime (added for #33713's stat path; superseded by winfs's own definitions and now unused after the merge). - node_fs.rs mkdtemp: Windows arm now returns encode_path_result(out, encoding) to match the StringOrBuffer return type main introduced.
Problem
On Windows,
bun_sys::fstatandbun_sys::futimenson a HANDLE-backedFd(FdKind::System, whatopenat/NtCreateFilereturn) converted the HANDLE to a CRT fd via_open_osfhandlesouv_fs_fstat/uv_fs_futimecould consume it, then never released that CRT slot. The in-code comment said as much: "acknowledged CRT-fd leak, a leak is strictly better than a guaranteed panic".The slot cannot be released:
_closeon a CRT fd also closes the underlying HANDLE, which the caller still owns. The CRT fd table is capped at 8192 (UCRT default), so long-running processes that repeatedly hit this path drift intoEMFILEon unrelated file operations.bun_sys::fstatis bun's cross-platform wrapper; on WindowsFdis intentionally two-kind (CRT int fromuv_fs_open, or raw HANDLE fromNtCreateFile) becauseopenathas no CRT/libuv equivalent and must go throughNtCreateFilewithObjectAttributes.RootDirectory. Call sites that pass a HANDLE-backed fd today:Bun.Image(path)pipeline,bun pm packbundled deps,bun create(WindowsDir::copy_file), bundlerget_hashed_filename, andMiniEventLoop::stdout().Repro
Before this change, on Windows:
Fix
libuv's
uv_fs_fstat/uv_fs_futimeimmediately convert the CRT fd back to a HANDLE and do all their work on it; the CRT layer is API-shape friction. This change ports the HANDLE-based logic intobun_sysand calls it directly for HANDLE-backed fds; uv-kind fds continue throughsys_uv::fstatunchanged.fstat:fstat_handle()mirrors libuv'sfs__fstat_handle+fs__stat_handle+fs__stat_assign_statbuf:GetFileTypedispatch (pipe/console get a synthetic stat), thenNtQueryVolumeInformationFile(FileFsDeviceInformation)+NtQueryInformationFile(FileAllInformation)+NtQueryVolumeInformationFile(FileFsVolumeInformation)to filluv_stat_twith the same field semantics (mode bits, timestamps, ino, dev, nlink, blocks).futimens:SetFileTimeon the HANDLE, with aTimeLike→FILETIMEconversion.Supporting additions:
FILE_ALL_INFORMATION,FILE_FS_DEVICE_INFORMATION,FILE_FS_VOLUME_INFORMATION,FS_INFORMATION_CLASS,NtQueryVolumeInformationFile,SetFileTime,NT_ERROR, andfiletime_to_timespec/timespec_to_filetimehelpers.fsync/fdatasync/fchmod/fchownin the same module still delegate tosys_uv::*and would panic on a HANDLE-backed fd, but no caller passes one today; that is a separate pre-existing issue (panic, not leak) and not what this change fixes.Verification
Windows (x64), fail-before with canary bun:
With this change (
bun bd test):Also verified on Windows x64 with the change applied:
test/js/bun/image/image.test.ts: 94 pass (exercises the HANDLE-path fstat end-to-end)test/js/bun/util/bun-file.test.ts: 6 passtest/js/node/fs/fs.test.ts -t stat: 16 passBun.Image(png).metadata()returns correct dimensions (st_size correct) and a directory is rejected withENODEV(ISREG/ISDIR correct)The test is Windows-only (
test.skipIf(!isWindows)); Linux has no CRT fd layer and thewindows_implmodule is#[cfg(windows)], so the gate's linux fail-before check will see a skip both ways.