Repository navigation
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Callback handling and overload tests src/js/node/fs.ts, test/js/node/fs/fs.test.ts |
The wrapper passes a callable callback or a no-op to fs.symlinkCb. Tests check string and null callbacks after successful and failed operations, and verify that explicit undefined throws ERR_INVALID_ARG_TYPE without creating the link. |
Suggested reviewers: cirospaciari
Priority:
Merge Risk: 
7376d
-
No actionable merge-blocking risk remains for this change.
Pre-merge checks: 
-
Check name Status Explanation Title check The title clearly describes the main change: preventing errors from non-callable callbacks in the four-argument fs.symlink overload. Description check The description explains the problem, root cause, fix, behavior differences, testing, verification results, and known limitations. It does not use the exact template headings, but it provides the requ… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Full details: Description check
-
Explanation
The description explains the problem, root cause, fix, behavior differences, testing, verification results, and known limitations. It does not use the exact template headings, but it provides the required information in detail.
-
- 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.
Comment @coderabbitai help to get the list of available commands.
|
Updated 9:31 PM PT - Oct 27th, 2026
❌ @autofix-ci[bot], your commit 7376d56 has 4 failures in
🧪 To try this PR locally: bunx bun-pr 38086That installs a local version of the PR into your bun-38086 --bun |
|
Status: rebased onto main 8d5672c, waiting on CI. Reproduced on a debug build of main 8d5672c with The rebase had one conflict, in #40916 edits the same line of |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, targeted Node compat fix.
What was reviewed:
- The
elsebranch installs a shared no-op so a rejectedfs.symlinkpromise has a real handler; the 3-arg overload and callable path are unchanged. - Checked that
callback === nullhits the new branch (not the=== undefinedone), matching the addednulltest case. - Test replaces the old
setTimeout(50)sleep withprocess.on("exit")+ explicitunhandledRejection/uncaughtExceptionlisteners, so the failure case would printREJECTED:ENOENTon the unfixed build — verifiedlstatSync/existsSync/joinare already in scope.
Extended reasoning...
Overview
This PR fixes a Node.js compatibility divergence in fs.symlink(target, path, type, callback). When the 4-argument overload receives a non-callable callback, Node stores it on the request as-is and skips it on completion (both success and failure). Bun previously left the non-callable value as the .then handler, which Promise.prototype.then treats as absent — fine on success, but on failure the rejection propagated as an unhandled rejection and killed the process. The fix adds an explicit else branch that swaps in a shared module-level no-op (discardResult), so both fulfillment and rejection are handled and dropped. The 3-argument overload (which validates via ensureCallback) and the callable branch are unchanged.
The test is upgraded from a single sleep-based case to a small describe that spawns a child, wires unhandledRejection/uncaughtException to observable output, and asserts stdout is exactly "exited" (from process.on("exit")) — no timer, and any regression would surface as REJECTED:ENOENT before exited. It covers the success half (with a new lstatSync().isSymbolicLink() assertion) and two failure cases ("notafunc" and null callbacks) into a missing directory.
Security risks
None. This is a narrow behavioral change to how a non-callable callback argument is handled in one node:fs wrapper — no new I/O paths, no user-controlled data reaching new sinks.
Level of scrutiny
Low. The source change is 7 lines in a JS wrapper, the mechanism (.then treating non-callables as absent → unhandled rejection) is well-understood and clearly explained, and the fix is the obvious minimal one. discardResult is a named module-level function matching the file's existing pattern (nullcallback, FunctionPrototypeBind) and avoids allocating a closure per call.
Other factors
- The old test's
setTimeout(..., 50)is gone, which addresses the REVIEW.md guidance against sleep-based waits — the pending native symlink op keeps the child alive until completion, thenexitfires deterministically. - Verified the
nullcase takes the newelse(not thecallback === undefinedbranch), and thatfs.symlink(t, p, "file")(3-arg with string type) still throwsERR_INVALID_ARG_TYPEviaensureCallback(type)— unchanged. - The PR description notes cross-verification against Node v26.3.0 and Node's own
test-fs-symlink*suite, and flags the one-line rebase interaction with #38065.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — small, correct Node-compat fix.
What was reviewed:
- The
elsebranch installsdiscardResultso.then(cb, cb)has a real rejection handler;undefinedstill routes toensureCallback(type)andnull/strings/numbers correctly land in the new branch. - Tests: the
exit-event print replaces the oldsetTimeout(50)(the pending op keeps the child alive), the failure cases assert exact stdout so a leakedREJECTED:line would fail, and the in-processundefinedcase pins the 3-arg overload's throw. - The comment-cop bot flag on the 3-line comment is a heuristic hit — the comment documents Node's
MakeCallbackskip, not a workaround, and matches other Node-compat notes in this file.
Extended reasoning...
Overview
Two files touched. In src/js/node/fs.ts, the symlink wrapper's non-callable-4th-arg branch now assigns a shared no-op discardResult instead of leaving the non-callable in place, so .then(callback, callback) always has a real rejection handler and a failing operation no longer surfaces as an unhandled rejection. In test/js/node/fs/fs.test.ts, the previous single test is replaced by a describe.concurrent covering the success path (link created, no output), two failure-path callback values ("notafunc", null), and an in-process check that an explicit undefined 4th arg still selects the 3-arg overload and throws ERR_INVALID_ARG_TYPE.
Security risks
None. This only changes how a non-callable callback is handled in fs.symlink's promise-adapter layer; no path handling, permissions, or data flow is affected.
Level of scrutiny
Low-to-medium. The runtime change is ~5 lines in a well-understood adapter pattern and the mechanism (.then treats non-callable handlers as absent → derived promise rejects unhandled) is exactly what the PR description states. I traced the three overload paths: callback === undefined → ensureCallback(type) (unchanged, still throws for the 3-arg form); $isCallable(callback) → wrapFsCallback (unchanged); everything else (null, strings, numbers, objects) → discardResult. The success path already worked because a fulfilled promise with a non-callable onFulfilled just adopts the value; only the rejection side needed a real handler.
Other factors
- The tests are well-shaped: subprocess-based, no sleeps (the pending symlink op keeps the child alive until completion, then
process.on('exit')prints), exact-stdout assertions that would catch a strayREJECTED:ENOENT, andlstatSync/existsSyncpost-conditions. All referenced identifiers (fs,lstatSync,existsSync,join,runScript,dir,file) are in scope. - The only outstanding thread is the automated
comment-copbot flagging the 3-line explanatory comment. That comment documents why this branch diverges from the file-wideensureCallbackpattern (Node'sFSReqCallback/MakeCallbackskip), not a workaround; it's the same style as other Node-compat notes already in this file (e.g. thermdirSyncrecursive note, thewriteSyncerror-context block). It could be trimmed to one line but isn't a merge blocker. - PR description notes the textual overlap with #38065 on the same
.thenline and that whichever lands second is a one-line rebase; nothing to act on here.
There was a problem hiding this comment.
LGTM — small, targeted Node-compat fix for fs.symlink's 4-argument overload with a non-callable callback.
What was reviewed:
- Verified the
discardResultno-op absorbs both fulfillment and rejection so the derived promise never goes unhandled; the callable and 3-argument (callback === undefined) branches are unchanged. - Checked the new
describe.concurrenttests: distinct link paths per case so concurrent runs don't collide; the failure cases share a path but neither creates it (ENOENT), so no race. Subprocess awaits the pending op via theexitevent — no sleeps. - The comment-cop bot flag looks stale/false-positive: the in-tree comment is a single line documenting the Node quirk, not a paragraph justifying a workaround.
Extended reasoning...
Overview
This PR fixes a Node.js compatibility gap in fs.symlink(target, path, type, callback): when the 4th argument is present but not callable (e.g. null, 123, "notafunc"), Bun previously passed it straight to .then(callback, callback). Per spec, .then ignores non-callable handlers, so a failed symlink operation produced an unhandled rejection that killed the process. Node instead stores the value on the FSReqCallback and silently skips a non-function oncomplete at completion time — the operation runs and its outcome (success or error) is dropped. The fix adds a module-level no-op discardResult and installs it as both handlers in the non-callable branch, so the promise chain is always handled. Three new subprocess tests cover the success path (link is created), the failure path (ENOENT is dropped, tested with two callback shapes), and the undefined-callback case (still selects the validated 3-argument overload and throws ERR_INVALID_ARG_TYPE).
Security risks
None. This is a callback-dispatch quirk in the node:fs compat layer; no input parsing, path traversal, permission, or crypto surface is touched. The change only affects how an already-completed internal promise is observed.
Level of scrutiny
Low-to-medium. The runtime change is +5/-2 in built-in JS: one named empty function at module scope (per the "named functions over inline closures" guidance for src/js/) plus an else branch that assigns it. The 3-argument overload and the callable 4-argument path are byte-identical to before. The test additions replace a single 50ms-timeout test with a properly event-driven describe.concurrent block that asserts exact stdout ("exited") and pins the undefined overload boundary.
Other factors
- The github-actions comment-cop flagged "paragraph-long comment" on this file, but commit 7e6422f already shortened the comment to one line and that's what's in the diff — the flag appears stale or a false positive. The one-liner explains why the branch exists (Node doesn't validate this overload's callback), which is exactly the kind of non-obvious context a comment should carry.
- The PR description verified the failure cases print
REJECTED:ENOENTon the released binary and pass on this build, satisfying the "fails for the right reason" requirement. - Concurrent-test safety: the two
it.eachfailure cases sharejoin(dir, "missing-dir", "lnc"), but sincemissing-dirdoesn't exist, neither can create the link — the sharedexistsSyncassertion is race-free. - Noted overlap with #38065 on the same
.thenline is textual only; whichever lands second is a trivial rebase.
…is not callable fs.symlink(target, path, type, callback) passed a non-callable callback straight into .then(callback, callback). Promise machinery ignores non-callable handlers, so when the operation failed the error surfaced as an unhandled rejection and the process exited with 1. Node stores that callback on the request without validating it and skips a non-function one on completion, on failure as well as on success, so the operation's outcome is dropped either way. Attach a no-op in that branch instead.
…ed-callback overload
Since #43728 a callable callback of fs.symlink() goes to the native callback arm (fs.symlinkCb), and only a non-callable one stayed on the promise arm. This branch already replaces a non-callable callback with a no-op, so the value is callable on every path and the promise arm is unreachable. Remove it: fs.symlink() now creates no promise at all. The no-op is an arrow in the branch that needs it, so loading node:fs allocates no new function.
7e6422f to
d7efffc
Compare
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline nit, I also checked the two things that could make this fix wrong: the silent drop of a failed symlink does match Node, whose symlink only runs makeCallback when callback === undefined and whose AsyncWrap::MakeCallback returns without calling a non-function oncomplete; and no other node:fs callback overload in src/js/node/fs.ts still routes a non-callable callback through a .then(callback, callback) promise arm, so this is the only site of the pattern.
Extended reasoning...
The PR rewrites six lines of the 4-argument fs.symlink callback overload in src/js/node/fs.ts so a non-callable callback becomes a no-op and every call uses the native symlinkCb arm, and replaces one subprocess test with a three-case describe.concurrent block in test/js/node/fs/fs.test.ts. It touches no security-sensitive surface. The error-silencing is a deliberate Node-parity tradeoff on a hot-path built-in module, and an inline nit remains, so a human sign-off is still worthwhile rather than an automated approve.
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked: the it.each control callback now proves the dropped error is a real ENOENT on the same path (addresses the vacuous-assertion nit from last round); removing the .then arm leaves fs.symlink (promise form) still live via fs.promises, so no dead binding; and a 4th argument of undefined still reaches ensureCallback, so that overload keeps its sync ERR_INVALID_ARG_TYPE. The one thing I could not settle from this checkout is the premise itself — whether Node's completion path (AsyncWrap::MakeCallback on a non-function oncomplete) silently skips or raises "Callback must be a function" as an uncaught error; a quick node -e with a non-function 4th argument would confirm the no-op arm matches.
Extended reasoning...
The change replaces the promise fallback in the fs.symlink 4-argument overload at src/js/node/fs.ts:428-440 with a no-op callback for non-callable values and rewrites the matching subprocess tests in test/js/node/fs/fs.test.ts:7575-7620. It touches no security-sensitive surface. The decision rests on Node-compat behavior that cannot be verified from the checkout (no node binary, no network), and an inline finding is already posted, so a human should weigh the premise rather than this run approving.
|
On the premise: node v26.3.0 skips a non-function 4th argument without an error, on failure too. The same script on main prints |
|
CI status: the lanes this change touches are green. The red lanes are the aot perf suites (test/aot/perf/*, also red on main build 134092) and tests that passed on retry or alone. The one fs.test.ts failure is the pre-existing fs.stat vs setImmediate order test, which passed on retry and does not call symlink. |
Problem
fs.symlink(target, path, type, callback)with a non-callablecallback(123,null) turns a failed link into an unhandled rejection:ENOENT: no such file or directory, symlink 't' -> '/missing-dir/x', exit code 1. Node v26.3.0 stays silent..then(callback, callback)atsrc/js/node/fs.ts:439.thenignores a non-callable handler, so nothing handles the rejection.Fix
symlinkreplaces a non-callable callback with a no-op. Every call then uses the native callback arm,fs.symlinkCb, andsymlinkno longer creates a promise.ERR_INVALID_ARG_TYPE.test/js/node/fs/fs.test.ts(ignores a non-callable 4-argument symlink callback). A callable callback on the same path is the control and printsCONTROL:ENOENT. With main'ssrc/, 2 of its 4 tests fail with an extraREJECTED:ENOENT. Also 43 of node's fs test files.Background
node:fsoperation has two native arms. The callback arm,xCb(callback, ...args), calls the callback from the native completion. The promise arm servesfs.promises.Downsides
Notes
History of this PR
The first version (August, before #43728) kept the promise and put a no-op on both
thenhandlers. #43728 then moved a callable callback to the native callback arm and left only a non-callable callback on the promise arm. After the rebase onto 8d5672c, the no-op makes that promise arm unreachable, so the last commit removes it.Repro
Node v26.3.0 prints
aliveand exits with 0.Node's code
symlinkinlib/fs.jscallsmakeCallback(type)only whencallback === undefined. The 4-argument overload doesreq.oncomplete = callbackwith no check.MakeCallbackreturns without a call whenoncompleteis not a function. Node validates the callback of every othernode:fsfunction, so this overload is the only one that accepts such a value.Probes, on debug builds of main 8d5672c and of this branch
123,"notafunc",null,{},false,0n) and 3 controls: main givesunhandledRejection ENOENTfor 6 of 6. This branch and node v26.3.0 give it for none.type, the 3-argument overload, a badtype,targetandpath,EEXIST,BufferandURLpaths, each for a link that is made and a link that fails. 40 lines change with this branch, each to node's answer.undefinedon success, where node passesnull. Main does the same. node:fs: apply node's callback argument rule in the native completion, and make access give undefined #40916 changes it.ENAMETOOLONG): silent with a non-callable callback, as in node.process.nextTickand a microtask iscb0 tick0 mt0 cb1 tick1 mt1, as in node.Allocations
JS heap objects that one pending call roots. The script starts 1000 calls in one synchronous turn and reads
heapStats()in the same turn, so the count is exact. Three runs of each build give the same numbers.PromisemainPromisePRFunctionmainFunctionPRAfter
node:fsloads, both builds have 793Functionobjects. The generatednode:fsmodule of the debug build is 32,176 bytes on main and 32,113 bytes with this PR.Tests
src/, 4 pass with this branch.test/js/node/fs/fs.test.tsunder the ASAN debug build: two runs with this branch have 7 and 4 failures. One run with main'ssrc/has 7, besides the 2 that this PR fixes. All of them are timeouts inreaddir ... x 100,fs/promises > rmdirandcreateReadStream on a large file, or an fd count that a timed-out test leaves behind. None of these tests callsfs.symlink, and the set changes between runs.test-fs-*andtest-trace-events-fs*files intest/parallelthat mentionsymlink, plustest-fs-make-callback.js,test-fs-null-bytes.js,test-fs-link.jsandtest-fs-readlink-type-check.js: 43 files, all pass.Related
#40916 (open) edits the same line of
symlink: it passes the callback tofs.symlinkCbwithout the adapter. The two changes are independent. The PR that lands second needs a one-line merge.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/node/fs/fs.test.ts