Repository navigation
sys(windows): map unknown NTSTATUS through RtlNtStatusToDosError for fs.rm - #32538
Conversation
…fs.rm
On Windows, fs.rm({recursive: true}) deletes each entry via NtCreateFile +
NtSetInformationFile. Filter drivers, AV hooks and cloud-sync placeholder
providers (OneDrive, Dropbox) return NTSTATUS codes from those calls that
were not in the explicit mapping table. The shipped Zig implementation hit
an unreachable and crashed; the Rust port returned E::UNKNOWN which the
fs.rm error table then surfaced as the misleading EFAULT.
Route the fallthrough through RtlNtStatusToDosError and the existing
libuv-derived Win32->errno table so these statuses produce the same errno
Node.js would (STATUS_CANNOT_DELETE -> ERROR_ACCESS_DENIED -> EPERM,
STATUS_DISK_FULL -> ENOSPC, etc). Also:
- add an explicit STATUS_CANNOT_DELETE -> EPERM entry
- treat STATUS_DELETE_PENDING/STATUS_FILE_DELETED from NtSetInformationFile
as success, matching the NtCreateFile handling
- add PermissionDenied -> EPERM in the rm/rmdir recursive error tables so
STATUS_ACCESS_DENIED reaches JS as EPERM instead of EFAULT
- expose translateNtStatusToE via bun:internal-for-testing so the mapping
can be asserted directly on Windows CI
|
Updated 11:36 AM PT - Jun 20th, 2026
❌ @robobun, your commit 098a84e has 1 failures in 🧪 To try this PR locally: bunx bun-pr 32538That installs a local version of the PR into your bun-32538 --bun |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds ChangesWindows fs.rm NTSTATUS error mapping
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
With bun -e, process.argv is [bunExe, arg1, ...] so argv[2] was undefined and fs.rmSync threw ERR_INVALID_ARG_TYPE instead of exercising the DeleteFileBun path.
…heck Windows grants DELETE on a file if the parent directory grants FILE_DELETE_CHILD, so denying DELETE on the file alone does not make NtCreateFile(DELETE) fail. Deny DC on the parent as well, matching the approach in test-fs-mkdir-recursive-eaccess.js. Also move the DELETE_PENDING/FILE_DELETED -> success short-circuit to after both NtSetInformationFile calls so it covers the legacy FileDispositionInformation fallback as well as the Ex path.
…lthrough RtlNtStatusToDosError maps STATUS_NOT_IMPLEMENTED, STATUS_INVALID_DEVICE_REQUEST and STATUS_ILLEGAL_FUNCTION all to ERROR_INVALID_FUNCTION, which the libuv Win32 table maps to EISDIR (for the DeleteFileW-on-a-directory case). At the NTSTATUS layer the is-a-directory case has its own status (STATUS_FILE_IS_A_DIRECTORY, handled explicitly); anything else that funnels into ERROR_INVALID_FUNCTION means the driver did not implement the request. Returning ISDIR from the fallthrough would make recursive fs.rm flip treat_as_dir forever when a filter driver returns one of these, so override it to NOTSUP.
|
CI status at 098a84e (build 63660): 281 jobs passed, 1 failed. The new The only error-level failure is All eight review threads have been addressed and resolved. Ready for a maintainer to merge. |
2 participants
What
Sentry BUN-2V67 reports a steady stream of
Panic: reached unreachable codecrashes from asyncfs.rm(..., { recursive: true })on Windows (1057 lifetime events, 304 on 1.3.14 alone, 100% Windows). The stack bottoms out in:Cause
Recursive
fs.rmon Windows deletes each entry viaNtCreateFile+NtSetInformationFile(FileDispositionInformation[Ex]). Real-world Windows filesystems (and especially filter drivers, AV hooks, and cloud-sync placeholder providers like OneDrive/Dropbox) return NTSTATUS codes from those calls that were not enumerated in the status mapping:STATUS_CANNOT_DELETE(readonly attribute / memory-mapped section),STATUS_IO_REPARSE_TAG_NOT_HANDLED, various network-redirector statuses, and so on.The shipped Zig implementation called
std.fs.Dir.deleteFilewhose NTSTATUS switch ends in anunreachablefor any status it does not recognise, crashing the worker-pool thread and the whole process. On main the Rust port routes the same path throughDeleteFileBun/translate_ntstatus_to_errno, which no longer panics, but its fallthrough arm returnedE::UNKNOWN. Thefs.rmerror-name table has no entry for that and surfaces it to JS asEFAULT, so a "this file is readonly" or "AV is holding this file" condition is reported as a bad-pointer error.Separately,
STATUS_ACCESS_DENIEDwas already mapped toE::PERM, butdt_err(EPERM)produces the tag"PermissionDenied"which neither rm/rmdir recursive error table handles, so a plain access-denied during a recursive rm also surfaced asEFAULTon Windows.Fix
translate_ntstatus_to_errno: for any NTSTATUS not in the explicit table, callRtlNtStatusToDosErrorand run the resulting Win32 error through the existing libuv-derived Win32->errno table (the same mapping Node.js uses). This turnsSTATUS_CANNOT_DELETEintoEPERM,STATUS_DISK_FULLintoENOSPC,STATUS_MEDIA_WRITE_PROTECTEDintoEROFS, and so on, instead ofUNKNOWN. Codes thatRtlNtStatusToDosErrorcannot map still fall back toUNKNOWN, never a panic.STATUS_CANNOT_DELETE -> E::PERMarm so the intent is visible without reading the ntdll mapping.DeleteFileBun: treatSTATUS_DELETE_PENDING/STATUS_FILE_DELETEDfromNtSetInformationFileas success, matching the existingNtCreateFilehandling (another handle already marked the file for deletion; the caller's intent is satisfied).map_anyerror_to_errno/map_anyerror_to_errno_rm_tree: add"PermissionDenied" -> EPERMsoSTATUS_ACCESS_DENIEDandSTATUS_CANNOT_DELETEreach JS asEPERMrather than falling through toEFAULT.Verification
There is no portable way to force
NtSetInformationFileto return an arbitrary NTSTATUS from userspace, so this follows the precedent oftranslateUVErrorToE/readdir-windows-ntstatus.test.ts:translateNtStatusToEis exposed throughbun:internal-for-testingandtest/js/node/fs/rm-windows-ntstatus.test.tspins the mapping directly on Windows CI:STATUS_CANNOT_DELETE(0xC0000121) ->PERM(wasUNKNOWN)STATUS_DISK_FULL,STATUS_NO_SUCH_FILE,STATUS_TOO_MANY_OPENED_FILES,STATUS_NOT_SUPPORTED,STATUS_MEDIA_WRITE_PROTECTED-> their libuv errnos viaRtlNtStatusToDosError(all wereUNKNOWN)UNKNOWN, no crashAn additional Windows-only integration test denies DELETE access on a file via
icaclsand asserts that bothfs.rmSyncandfs.promises.rmwith{ recursive: true }throw a catchableEPERM/EACCES/EBUSY(neverEFAULT) and exit 0, covering the fullzig_delete_tree -> unlinkat -> DeleteFileBun -> errno_syspath and both rm error tables.On the unfixed build the test file fails at import (
translateNtStatusToEis not exported). With this change the non-Windows sanity test passes and the Windows assertions exercise the fix.cargo check -p bun_runtimeis clean on host,x86_64-pc-windows-msvc, andaarch64-pc-windows-msvc. Existingfs.test.ts -t rmandtest-fs-rm*.jspass unchanged.