Repository navigation
Conversation
terminate() waited for the native 'close' event with a listener that it added on each call. The parent-side exit handler is itself the 'close' listener, and it runs user code before it stores the exit state: the 'message' listeners for what the worker posted before it ended, the 'readable' listeners at the stdio EOF, and parked stdin write callbacks. A terminate() from one of them added its listener during the only dispatch of 'close', so its promise never settled. The exit handler now settles the pending terminate() after it emits 'exit', and terminate() adds no listener. The handler runs in the order of node's kOnExit: deliver the worker's last messages, store the exit state, end stdio, emit 'exit'. A 'message' or 'exit' listener that throws is reported as an uncaught exception and the handler continues. A stdio listener that throws ends the handler, as before, and terminate() still settles. Co-authored-by: Alistair Smith <hi@alistair.sh>
|
Status: ready for review. How I reproduced it
|
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: oven-sh/bun/.coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review. WalkthroughWorker termination now shares a pending promise across calls and retains the thread ID for exit-time operations. Close handling drains queued messages before closing the port and emitting ChangesWorker termination lifecycle
Suggested reviewers: Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The change addresses termination settlement during worker exit handling. No actionable merge-blocking issue remains in the supplied evidence; merge after normal checks pass. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
The exit handler does not run for a Worker whose Bun.ModuleGraph was disposed, so the copy of the thread id stayed live after the thread ended. threadId now reads the copy only while the exit handler runs, before it stores the exit state. At every other time it reads the native id, as before.
|
Updated 4:29 PM PT - Sep 29th, 2026
✅ @robobun, your commit 225acf1da5d7ed30254092228d9e6aa9a762a748 passed in 🧪 To try this PR locally: bunx bun-pr 44252That installs a local version of the PR into your bun-44252 --bun |
There was a problem hiding this comment.
I re-reviewed after the follow-up commit and found no new bugs; the second commit addresses the disposed-graph threadId concern (the getter now reads the native id outside the exit handler, with a test for the disposed-graph case). Because this reworks the exit/terminate lifecycle where user listeners re-enter mid-handler, a human look is still worthwhile.
What was reviewed:
#onClose/#exitordering and thefinallysettle:#exited, terminate callbacks and the pending promise are settled even when a stdio listener throws out of#exit.terminate()called while the exit handler runs (#exiting, not yet#exited): the extra#worker.ref()is a no-op there sinceWorkerMessagingProxy::setKeepAlivereturns early once keep-alive is released, so it cannot pin the event loop.getHeapSnapshot/startHeapProfileswitching to#worker.threadId === -1keepsERR_WORKER_NOT_RUNNINGfor both the exited and mid-exit cases.
Extended reasoning...
The change touches src/js/node/worker_threads.ts (Worker exit handler, terminate(), threadId getter) and exports reportUncaughtException from src/js/internal/shared.ts, with ~330 lines of new tests. It touches no auth, crypto or injection surface; the sensitive area is re-entrancy of user JS during the exit handler and node-compat ordering of message/exit/stdio events. The follow-up commit resolved the one non-pre-existing issue I raised earlier and added a regression test for it. I did not approve because the lifecycle rework changes observable ordering versus main in several cells (messages before stdio EOF, threadId/postMessage semantics inside late message listeners), which is a judgment call for a maintainer rather than a mechanical change.
Still open from earlier reviews (1):
- Unresolved: 1 minor or pre-existing.
Problem
await worker.terminate()never continues when code that the Worker's exit handler runs calls it first: a 'message' listener, a stdio 'readable' listener at EOF, or a stdin write callback.terminate()added a native 'close' listener per call (src/js/node/worker_threads.ts:1074). The exit handler#onCloseis the 'close' listener and stored the exit state (:1209) after that user code. A listener added during a dispatch does not run.Fix
#onClosesettles the pendingterminate()after 'exit'.terminate()adds no listener.#onClosefollows node'skOnExitorder: last messages, exit state, stdio EOF, 'exit'. Aterminate()from a 'message' listener resolves the exit code, one from a stdio EOF resolvesundefined, as in node v26.3.0.test/js/node/worker_threads/worker_threads.test.ts(18 fail on main), that whole file, vendoredtest-worker-*files.Background
kOnExitdelivers the worker's last messages, then disposes the handle:threadIdbecomes -1 andterminate()resolvesundefined.terminate()for a close in progress (keeps a listener per call), node'sonce('exit')(removeAllListeners()loses the promise).Downsides
new Worker()+4,threadIdread 15 (main 7). Builtin source +586 bytes, binary text +613 bytes.threadId,threadName,postMessage()). Node does both.Notes
Repro with the public API only, on each run
exit 0,terminate() resolved undefinedexit 0exit 0,terminate() resolved undefinedbun 1.3.x has no
worker.stdout, so this form starts with 1.4.0.The reported form
The exit handler delivers a 'message' only when the worker posted and ended before the parent started its port. The parent starts the port at the end of
new Worker(), after it started the thread. Once the port is started, the port itself delivers what is left when the worker's side closes (MessagePort::peerClosed), before the exit handler runs. The script holds the parent inside the constructor to reach that state on each run.exit 0and stops there, 3 of 3. Under top-level await the process then never ends on a release build (the spin is the separate matter in Detect unsettled top-level await and exit like Node (exit 13 + warning) #33283).exit 0, thenterminated, 3 of 3.terminate()from the listener of message 1,500. On main the promise stayed pending in 2 of 190 runs. With this PR the exit handler delivered that message in 2 of 190 runs, and the promise settled in both. With one message: 0 of 400 on main, 0 of 400 on this PR.Value per place and form
mainleaves the promise pending in all 12 cells.terminate()fromterminate(callback)[Symbol.asyncDispose]()callback(null, code)after 'exit', then the promiseundefinedundefinedundefined, callback not calledundefinedworker.stdin.write()callbackundefinedundefined, callback not calledundefinednode v26.3.0 gives the same values in the first two rows, except that it ignores the callback. It does not run the stdin callback at all, because its exit handler does not destroy stdin.
A listener that throws inside the exit handler
terminate()callbackguardCallbackdoes for fs and dns callbacks. For a thrown value that is not an Error, the report then has no code frame. A native report needs a change tojsFunctionReportUncaughtExceptionor a second native listener. I did not take either.Other behaviour changes
threadId,threadName,postMessage()(it throws for a value that cannot be cloned), andworker.stdinis not destroyed. main reads -1 and null, andpostMessage()returns.threadIdis the native value, as on main. So a Worker of a disposedBun.ModuleGraphreads -1 after its thread ended. A listener for a 'message' from the worker's globalpostMessage()that the native close task delivers reads -1 too.terminate()before the exit, then a second one from a stdio EOF listener: the second resolvesundefined(main: the exit code). Node resolvesundefined.startHeapProfile()now reads the native state, so it rejectsERR_WORKER_NOT_RUNNINGin each of these places, as before.Bun.ModuleGraph: when a graph is disposed from inside the exit handler of its Worker, a pendingterminate()now resolves. On main it stays pending, because the native dispatch skips the listeners of a disposed graph.test/js/bun/module-graph/module-graph-workers.test.ts:706accepts both.Differences from node that stay as on main
terminate(callback)is honoured, with DEP0132. Node v26 ignores the callback.terminate()adds no 'exit' listener, soremoveAllListeners()and an 'exit' listener that throws do not lose the promise.worker.stdinis destroyed at exit.Measurements
Release builds of ad60a9b with and without this change, unless the line says another build.
new Worker()11 (main 11),terminate()0 (main 1),terminate(callback)0 more (main 1 more).listenerCount('exit')does not change in either.terminate()(heapStats, 20 Workers, 2 runs): first call 4 (main 8), repeated call 0 (main 2), call after exit 1 (main 3). Retained while pending: 4 (main 5).terminate()(gdb hit counters on debug builds, 20 and 40 Workers):addEventListener0 (main 1),JSEventListener0 (main 1), Weak handles 0 (main 2),fastMallocabout 3 (main about 6).BUN_JSC_dumpGeneratedBytecodes):terminate()first, cached, after exit: 35, 17, 14 (main 48, 21, 22). One Worker exit: 121 and 10 branches (main 99 and 9), no allocation.new Worker(): 4 more, for one more private field.threadIdon a Worker that runs: 15 (main 7).#onMessageis unchanged.size): 80,667,862 to 80,668,475.node:worker_threadsbinding keeps 17 entries.perfandvalgrindare not available in my environment, so there is no instruction count forbench/postMessage.Tests
terminate()on a running Worker in the three forms, the stdio listener that throws, and thethreadIdof a Worker whoseBun.ModuleGraphwas disposed.EventTarget.prototype.addEventListenerduringnew Worker()so that the public port never starts. Then only the exit handler can deliver what the worker posted.finally, each guard, the copy of the thread id, the place where the exit state is stored). Each change fails at least one new test.worker_threads.test.ts(167 pass),worker_destruction,worker-async-dispose,worker-transfer-terminate-stress,worker-top-level-await,worker-transfer-list,worker-shutdown-post-leak,15787,message-channel. Vendoredtest-worker-*: 107 of 108 pass (debug build at ef8e000, release build at 225acf1).test-worker-arraybuffer-zerofill.jsfails on main in the same way (it callsdescribefromnode:testoutside the test runner).module-graph-workers.test.ts, rows with a node Worker: 253 pass. On a machine under heavy load one row at times reportsfdsAboveBaseline: 2. It did so in 2 of 12 runs on main and in 2 of 12 runs on this PR.Not in this PR
process.exit()is lost when the worker ends before the parent starts its stdio ports. Node'skOnExitdrains that port too. This is on main and this PR does not change it.MessagePort.close(callback)waits withthis.once('close', callback). It has the same shape as the oldterminate(). Its output is the same on main and here.terminate(), the native Worker drops the messages that the worker posted through the globalpostMessage()(m_wasTerminatedinWorker::dispatchEvent). So aterminate()from the listener of the first of 3,000 such messages leaves 1 message received, on main and here. Messages throughparentPort.postMessage()do not pass that gate: main, this PR and node v26.3.0 deliver all 3,000.Prior work
#19940 had
#onCloseresolve a deferred that the Worker owns. This PR keeps that idea. Here the firstterminate()makes the deferred, and#onCloseresolves it after 'exit'.