Skip to content

worker: fire 'error' for the VM log only when the worker fails - #45203

Open
robobun wants to merge 2 commits into
mainfrom
robobun/c8acf265/worker-error-only-on-failure
Open

robobun wants to merge 2 commits into
mainfrom
robobun/c8acf265/worker-error-only-on-failure

Conversation

@robobun

@robobun robobun commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

  • A worker that runs fires error twice for each message in its VM log, such as BuildMessage: Non-relative path "sub/lib.ts" is not allowed when "baseUrl" is not set from a tsconfig.json that tsc accepts. A node:worker_threads parent with no error listener exits 1.
  • spin() (src/jsc/web_worker.rs:782) calls flush_logs after the entry loads and again before shutdown(). No user reported this.

Fix

  • spin() calls flush_logs only on the five paths that end the worker. Each returns through shutdown(), so a message is reported at most once.
  • What start-up logged is dropped before the entry resolves, so it cannot join the report of a later failure.
  • Verified: test/js/web/workers/worker-entry-point.test.ts and worker_threads.test.ts (15 new tests fail on main). Also worker.test.ts, worker_blob.test.ts, 110 test-worker* Node tests.
  • Self-reviewed: 26 concerns raised, 23 addressed. The other 3 are sibling bugs, named in Notes.

Background

  • Each VM owns a bun_ast::Log. flush_logs turns a worker's log into the error event of its parent.
  • The first thread that reads a directory logs its tsconfig.json and package.json messages. Each worker logs an invalid --define again.
  • Considered only emptying the log in flush_logs: one error stays and ends a parent with no listener. Node fires error only when a worker fails.

Downsides

  • These messages no longer reach worker.onerror or self.onerror: since 1.2.13 for a config beside the entry, since 1.0.0 for a define.
  • Nothing prints them. The main thread prints some (cases and a question in Notes).
  • No speed or size cost: a clean worker start runs 37 fewer instructions, release .text is unchanged.
Notes

Repro (main, linux x64, 3 of 3 runs)

mkdir -p w/sub && cd w
echo '{"compilerOptions":{"baseUrl":"."}}' > tsconfig.base.json
echo '{"extends":"../tsconfig.base.json","compilerOptions":{"paths":{"lib":["sub/lib.ts"]}}}' > sub/tsconfig.json
echo 'export const x = 1;' > sub/lib.ts
printf 'import { parentPort } from "node:worker_threads";\nparentPort.postMessage("hi");\n' > sub/worker.ts
cat > main.mjs <<'JS'
import { Worker } from "node:worker_threads";
const worker = new Worker("./sub/worker.ts");
worker.on("message", data => console.log("message:", data));
worker.on("exit", code => console.log("exit", code));
JS
bun main.mjs

Main prints message: hi, then an uncaught BuildMessage: Non-relative path "sub/lib.ts" is not allowed when "baseUrl" is not set (did you forget a leading "./"?), and exits 1. This PR prints message: hi, exit 0. tsc 5.9.3 accepts this extends and paths layout (exit 0). With an error listener, main fires the event two times and the worker exits 0.

The worker thread must be the first thread that reads sub/. The resolver caches each directory for the process.

What puts a message in the log of a worker that runs

Each row fires error two times on main and no event with this PR (release builds of the merge base and of this PR).

  • A tsconfig.json or package.json beside the entry that does not parse or has a warning. Tested for 8 forms of the entry: a file, an absolute path, no extension, a directory with main, a directory with index.ts, a package in node_modules, an imports alias, a blob: URL of the file.
  • A package that the entry imports through a symlink (the layout of a workspace), with a warning in its tsconfig.json.
  • A --define or bunfig [define] key that is not valid. Each worker applies the defines again. The main thread still prints error: define key "a-b" must be a valid identifier one time.
  • A warning in the file of --tsconfig-override. Each worker reads that file when its VM starts.
  • A later import() of a Bun.file blob: URL from a directory with such a tsconfig.json: one event when the worker exits, now none.

Other results:

  • An error listener on self in the worker: 2 calls, now 0.
  • A worker that throws later: 2 events (the message, then the exception), now 1 (the exception).
  • A missing entry when a define is not valid: AggregateError: Error in worker, now BuildMessage: ModuleNotFound resolving "..." (entry point). On node:worker_threads the error.code is MODULE_NOT_FOUND again.
  • Not changed, byte for byte: a missing entry in a clean directory, a missing entry beside a broken tsconfig.json (AggregateError: Error in worker), an alias of a builtin, blob:foo, a blob: URL that is missing.

History

  • A config file beside the entry: Bun 1.0.0 to 1.2.12 fire no event (checked 1.0.0, 1.1.0, 1.2.0, 1.2.10, 1.2.11, 1.2.12). WebWorker.create() resolved the entry on the parent thread with a temporary log. node:worker_threads: improve error messages, support environmentData, emit worker event #18768 (1.2.13) moved the resolve to the worker thread and its VM log. Releases from 1.2.13 fire two events (checked 1.2.13, 1.2.15, 1.2.17, 1.2.20, 1.3.0, 1.3.5, 1.3.10, 1.3.13, 1.3.14).
  • A define that is not valid fires two events on 1.0.0, 1.2.12 and 1.2.13. A later import fires one event at exit on 1.2.12 and 1.2.13. For these two, this PR is a change of behaviour and not a restoration. The two flushLogs calls date from Implement Workers #3645.
  • Node v26.3.0 prints message hi, exit 0 for a worker beside a tsconfig.json that does not parse. Node does not read tsconfig.json or defines. For a package.json that does not parse, Node fails the worker with ERR_INVALID_PACKAGE_CONFIG. Bun runs the entry (with two error events before this PR, with none now).

Question for a maintainer: drop or print?

This PR reports nothing from the log of a worker that runs. The main thread prints what reaches its log and leaves the exit code alone (run_command.rs:1529-1532 after the entry loads, :1630-1633 at exit). With sub/tsconfig.json that holds {:

Input Main thread Worker with this PR
bun sub/index.ts nothing on stderr, exit 0 no event, nothing on stderr
bun ./sub, bun ./sub/index prints error: Expected "}" but found end of file with the path, exit 0 no event, nothing on stderr
a later import() of a Bun.file blob: URL prints the same at exit, exit 0 no event, nothing on stderr

If a print is wanted for workers, the place is spin(): after start-up and at the end of the loop. Each worker would then print an invalid define again, after the main thread. #37908 makes the level of the worker log follow the process.

Open PRs near these lines

Sibling bugs that this PR does not fix

  1. A missing entry beside a config message still reports AggregateError: Error in worker, and node:worker_threads loses MODULE_NOT_FOUND. The fix is a scratch resolver log inside resolve_entry_point_specifier, for the entry and for preloads. It fits after Worker: a preload that does not resolve throws the resolver's message again, not TypeError: undefined #45067.
  2. new Bun.FileSystemRouter() throws on the first construct for the same first-reader messages (filesystem_router.rs:285).
  3. A Bun.serve handler that throws inside a worker fires error and stops the worker (on_unhandled_rejection). The main thread prints and keeps the server.

Measurements

Release builds (LTO) of commit 281698a without and with this change, linux x64.

  • Events: a worker that runs with a message in its VM log fires error 0 times (was 2). A node:worker_threads parent with no listener exits 0 (was 1). 4 fixtures, 3 of 3 runs each.
  • gdb hit counts on a worker beside a tsconfig.json that holds {: WebWorker__dispatchError / BuildMessage__create / postErrorToWorkerObject = 0 / 0 / 0 (was 2 / 2 / 2). A clean worker calls flush_logs 0 times per life (was 2).
  • Instructions that a clean file-path worker executes in the changed parts of spin() (gdb stepi on the worker thread, 2 of 2 runs): 48, base 85. The check after start-up adds 6. The flush after the entry loads (22) and the flush before shutdown() (21) are gone.
  • Allocator entries and syscalls between the online post and the entry load: 5 and 6 on both builds. Writes to fd 2: 0 on both builds.
  • Binary: .text is 105,679,289 B on both builds. The thread closure that inlines spin() is 4,933 B (was 4,911). flush_logs is 437 B on both.
  • Wall clock, 200 clean workers one after another, two series of 20 interleaved runs: base 1823.2 and 2162.3 ms, this PR 1877.4 and 2078.9 ms (medians). The A/A spread is 111.4 and 138.4 ms, so the two builds do not differ.

Tests

  • Release builds, base and this PR: worker-entry-point.test.ts 2 pass and 11 fail on base, 13 pass here. worker_threads.test.ts 152 pass and 4 fail on base, 156 pass here. worker.test.ts 42 pass and worker_blob.test.ts 6 pass on both. The 110 test/js/node/test/parallel/test-worker* files exit with the same code on both.
  • Debug build with ASAN: worker-entry-point.test.ts 13 pass, worker_threads.test.ts 156 pass.
  • The LeakSanitizer test in worker-entry-point.test.ts now sets BUN_DESTRUCT_VM_ON_EXIT=1 for its child. Without it the child leaves a suppressed leak, and LeakSanitizer starts llvm-symbolizer on the whole binary: 8 s on a debug build, over the 5 s default of bun bd test. With it the child takes 1.5 s. The test still fails when the fix it guards (resolver: free the exports/imports scratch buffers when a thread exits #39947) is taken out: Direct leak of 12288 byte(s) in module_bufs.
  • No issue exists for this bug, so the tests are in the existing files.

Design notes

  • An earlier draft also emptied the log inside flush_logs (Honor preventDefault() on Worker error event #29060 had that line). With every flush_logs call followed by shutdown(), that line has no reader, so it is not in this PR.
  • An earlier revision also dropped the log after the entry loads. Its only reader there is the development error page of a Bun.serve inside the worker, which lists what the entry resolve logged (as on main). A test for that needs the whole 50 KB page from a worker that stops when its handler throws, and the page is cut off when it does not fit one socket write. So that part is not in this PR.

@robobun

robobun commented Oct 14, 2026 •

Copy link
Copy Markdown
Collaborator Author
Updated 12:49 PM PT - Oct 16th, 2026

✅ @robobun, your commit c237859b39acfbc711c541b505460c1b0dac3572 passed in Build #126786! 🎉


🧪   To try this PR locally:

bunx bun-pr 45203

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

bun-45203 --bun

@robobun

robobun commented Oct 14, 2026

Copy link
Copy Markdown
Collaborator Author

Reproduced on main (linux x64, release build, 3 of 3 runs).

  • A node:worker_threads worker beside a tsconfig.json whose paths use a baseUrl from extends posts its message. Then the parent, which has no error listener, exits 1 with an uncaught BuildMessage: Non-relative path "sub/lib.ts" is not allowed when "baseUrl" is not set.
  • With an error listener, the event fires two times and the worker exits 0.
  • With this PR the parent prints message: hi, exit 0 and exits 0.

The repro script is in the Notes of the description.

@coderabbitai

coderabbitai Bot commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: oven-sh/bun/.coderabbit.yaml
  • Review profile: ASSERTIVE
  • Plan: Advanced
  • Run ID: 76b7899c-afef-4ee0-b51e-65e6aa1cd6a8
📥 Commits

Reviewing files that changed from the base of the PR and between 6c60634 and a46bac5.

📒 Files selected for processing: 2
  • src/jsc/web_worker.rs
  • test/js/web/workers/worker-entry-point.test.ts

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

Walkthrough

WebWorker::spin resets accumulated VM logs after startup succeeds and removes later log flushes. New Node and web worker tests cover resolver and configuration diagnostics, worker execution, and runtime errors.

Changes

Worker diagnostic handling

Layer / File(s) Summary
Worker VM log lifecycle
src/jsc/web_worker.rs
WebWorker::spin resets accumulated VM logs after the startup termination check. It no longer flushes logs after entry-promise setup or event-loop processing.
Worker diagnostic and error tests
test/js/node/worker_threads/worker_threads.test.ts, test/js/web/workers/worker-entry-point.test.ts
Tests cover resolver and configuration diagnostics, invalid defines, successful worker execution, and runtime errors. The web worker leak-checking fixture sets BUN_DESTRUCT_VM_ON_EXIT=1.

Suggested reviewers: jarred-sumner

Priority: ➖ Normal

Merge Risk: ⚪ Minimal · up to a46ba

The worker diagnostic change is mergeable after normal checks; terminating worker failures still report errors.

Pre-merge checks: Passed 4
Passed Passed checks: 4 passed
Check name Status Explanation
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.
Title check Passed The title clearly and concisely describes the main change: workers emit VM log errors only when the worker fails.
Description check Passed The description explains the problem, fix, behavior changes, tradeoffs, testing, and known limitations. It does not use the exact template headings, but it provides the required change summary and ver…
  • 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

Comment @coderabbitai help to get the list of available commands.

@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.

Comment thread src/jsc/web_worker.rs
Comment thread test/js/web/workers/worker-entry-point.test.ts Outdated
A worker that runs fired its 'error' event two times for each message in
its VM log: after the entry loaded and again before shutdown. The
messages come from a tsconfig.json or a package.json that the worker
thread reads first, from a define or a --tsconfig-override file that each
worker applies again, or from a later import. A node:worker_threads
parent with no 'error' listener exited with code 1 for a worker that ran.

spin() now reports the log only on the paths that end the worker. Each of
them returns through shutdown(), so a message is reported at most once.
What start-up logged is dropped before the entry resolves, so that it
cannot join the report of a later failure.
@robobun
robobun force-pushed the robobun/c8acf265/worker-error-only-on-failure branch from 6c60634 to a46bac5 Compare October 14, 2026 05:08

@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 completed

Nothing new to post: everything this review found is already covered by existing comments on this pull request or didn't merit a separate one.

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.

1 participant