Skip to content

Reject Bun.file(fd).delete() and unlink() instead of resolving with the TypeError - #48560

Open
robobun wants to merge 2 commits into
mainfrom
robobun/abb69d55/reject-fd-unlink
Open

robobun wants to merge 2 commits into
mainfrom
robobun/abb69d55/reject-fd-unlink

Conversation

@robobun

@robobun robobun commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • Bun.file(fd).delete() and .unlink() resolve, and the resolved value is the error: TypeError [ERR_INVALID_ARG_TYPE]: Is not possible to unlink a file descriptor. An await inside try/catch sees no error.
  • The cause is the Fd arm of FileExt::unlink (src/runtime/webcore/blob/Store.rs:233). It builds the error and passes it to JSPromise::resolved_promise_value.

Fix

  • That arm now returns JSPromise::rejected_promise(...). The error, its code and its message do not change.
  • Correct because the path form already rejects (ENOENT), and fs.promises.unlink(fd) rejects with the same code.
  • Verified: test/js/web/fetch/blob-write.test.ts, two new cases. Without the fix both fail with Expected promise that rejects, Received promise that resolved. Also ran test/js/bun/util/bun-file.test.ts.

Background

  • A BunFile store holds a path or a file descriptor. A descriptor has no path, so delete() has nothing to unlink and reports an argument error.
  • Of the 41 callers of resolved_promise_value in src/, only this one passes an error.
  • Considered a synchronous throw, as a bytes-backed Blob does for delete(). This arm already returned a promise, and the path form reports its errors through the promise.

Downsides

  • Code that calls Bun.file(fd).delete() and does not handle the result got a resolved promise before. It now gets a rejection. With no handler, the process prints the error and exits with code 1, as the path form does for a missing file.
  • Code that read the resolved value to find the error (if (result instanceof Error)) no longer reaches that branch.
Notes

Reproduction, before the fix prints the try saw nothing, after it prints the try caught ERR_INVALID_ARG_TYPE:

try {
  await Bun.file(987654).delete();
  console.log("the try saw nothing");
} catch (e) {
  console.log("the try caught " + e.code);
}

The descriptor does not have to be invalid. An open descriptor and Bun.stdin.delete() take the same arm. The file is not deleted in any of these cases, before or after the fix.

The line dates from #15740, which added unlink(). In the same hunk the error arm of the path form used rejectedPromiseValue, and the fd arm used resolvedPromiseValue. The behaviour was never correct, so the test is in the existing file for this API and not under test/regression/.

Not changed: new Blob(["x"]).delete() throws synchronously (Cannot write to a Blob backed by bytes, which are always read-only). Two existing tests in blob-write.test.ts pin that.

Cost for callers that never hit this arm: none. The change replaces one call in an arm that runs only for delete() or unlink() on a descriptor-backed file.

The Fd arm of FileExt::unlink built ERR_INVALID_ARG_TYPE and passed it to
JSPromise::resolved_promise_value, so delete() and unlink() on a file made
from a descriptor resolved with the error as their value. The arm now
returns a rejected promise with the same error.
@robobun

robobun commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 1:05 AM PT - Nov 11th, 2026

❌ @robobun, your commit de58ed4 has some failures in Build #140072 (All Failures)


🧪   To try this PR locally:

bunx bun-pr 48560

That installs a local version of the PR into your bun-48560 executable, so you can run:

bun-48560 --bun

@robobun

robobun commented Nov 11, 2026 •

Copy link
Copy Markdown
Collaborator Author

Status: ready for review. The diff is green. The red CI status is not a test failure.

Reproduced on 1.4.3-canary (367d939) and confirmed in the source of main at 89452d2. This script prints the try saw nothing before the fix and the try caught ERR_INVALID_ARG_TYPE with this branch:

try {
  await Bun.file(987654).delete();
  console.log("the try saw nothing");
} catch (e) {
  console.log("the try caught " + e.code);
}

An open descriptor, Bun.stdin.delete(), Bun.stdout.delete() and Bun.stderr.delete() behave the same way. Verified with bun bd test test/js/web/fetch/blob-write.test.ts (17 pass, the two new cases fail under USE_SYSTEM_BUN=1).

CI at de58ed4 (build 140072): all 184 jobs that ran passed. That includes the Windows 2019 x64 and Windows 11 aarch64 test lanes, and both ran blob-write.test.ts. The build shows as failing because the four darwin x64 - test-bun jobs expired in the queue after 73 minutes with no free agent. Buildkite retried them and they are queued again. The tests under the flaky annotations do not touch this change and passed when they ran again.

@coderabbitai

coderabbitai Bot commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

File-descriptor inputs to FileExt.unlink now produce a rejected promise with the existing invalid-argument message. Tests cover Bun.file(fd).unlink() and .delete(), and verify the file remains present.

Changes

File descriptor unlink behavior

Layer / File(s) Summary
Reject descriptor unlink operations
src/runtime/webcore/blob/Store.rs, test/js/web/fetch/blob-write.test.ts
The descriptor case now returns a rejected promise with the existing error. Tests cover unlink and delete, check the error, and verify the file remains present.

Possibly related PRs

  • oven-sh/bun#46789: Changes the Blob.file binding to parse paths through PathOrFileDescriptor, connecting to the descriptor case handled here.

Suggested reviewers: jarred-sumner

Merge Risk: Minimal · up to de58e

Bun.file(fd).delete() and .unlink() will now reject, so try/catch around await catches the error as it does for the path form. Code that inspected the resolved error value will no longer reach that branch, which the PR description discloses. No merge-blocking risk remains.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title clearly and concisely describes the main change: descriptor-based delete and unlink operations now reject instead of resolve with an error.
Description check Passed The description explains the problem, fix, behavior, rationale, verification steps, tests, and compatibility impact. It does not use the exact template headings, but it provides the required informati…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Description check

Explanation

The description explains the problem, fix, behavior, rationale, verification steps, tests, and compatibility impact. It does not use the exact template headings, but it provides the required information in equivalent sections.

  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between 677a2c2 and de58ed4.

Files selected for processing: 1

  • test/js/web/fetch/blob-write.test.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.

LGTM — one-line fix that swaps the fd arm of FileExt::unlink from a resolved promise to the documented rejected_promise(...).to_js() idiom, with a test covering both the unlink and delete aliases; the inline nit about pinning the error class is non-blocking.

I did not run the new test; that it fails on the base branch follows from reading (the old arm resolved, so .rejects cannot pass there).

Extended reasoning...

The diff touches src/runtime/webcore/blob/Store.rs (one match arm in FileExt::unlink for PathOrFileDescriptor::Fd) and adds a test.each case to test/js/web/fetch/blob-write.test.ts; it touches no security-sensitive surface. JSPromise::rejected_promise returns a GC-managed &mut JSPromise whose doc comment names .to_js() as the conversion, and the C++ JSC__JSPromise__rejectedPromise already handles an empty value from a terminated error build, so no exception check is skipped relative to the old path. delete is declared in Blob.bind.ts as an alias with rustName: "unlink", so both user-facing entry points go through the fixed arm, and a repo-wide grep found no existing test or doc asserting the old resolve-with-error behavior. The only behavioral downside (callers who never await now get an unhandled rejection) is the intended correction of a promise that resolved with an error object, which the PR description states. Neither file is covered by .github/CODEOWNERS, the change is small and self-contained, and the single inline finding is a minor test-strength nit.

Comment thread test/js/web/fetch/blob-write.test.ts Resolved
@robobun

robobun commented Nov 11, 2026

Copy link
Copy Markdown
Collaborator Author

Applied the optional review suggestion in de58ed4: the new test also pins the error class with rejects.toBeInstanceOf(TypeError). No change to src/. The two cases still fail without the fix and pass with it (17 pass in blob-write.test.ts on the debug build).

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review found no issues

No high-confidence issues detected in this change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants