Skip to content

Typecheck the built-in modules (src/js) in CI - #43649

Merged
dylan-conway merged 18 commits into
mainfrom
claude/typecheck-builtins
Sep 21, 2026
Merged

dylan-conway merged 18 commits into
mainfrom
claude/typecheck-builtins

Conversation

@dylan-conway

@dylan-conway dylan-conway commented Sep 20, 2026 •

Copy link
Copy Markdown
Member

What does this PR do?

tsc --noEmit -p src/js/tsconfig.json now passes and runs in lint.yml. Nothing typechecked the built-in modules before, and tsc reported about 1,800 errors.

Most of the diff only adds types: interfaces for internal state objects, declare class fields (a plain field would emit name; and change own-key order), optional parameters where the body already handles undefined, and corrected shared declarations. For most changed files under src/js, stripping the types from the old and the new version gives identical JavaScript. The behaviour changes are the ones listed below.

Fixes #20857
Fixes #31767
Fixes #8487

Runtime bugs the typecheck found

Each has a test in the module's existing test file that fails without the fix.

  • node:wasi poll_oneoff threw TypeError: Invalid mix of BigInt and other type in subtraction on any clock wait with a positive timeout, so a WASI sleep() failed. The elapsed time is now subtracted as a BigInt. The wait still blocks the thread, so JS timers do not run during it (Bug in wasi.poll_oneoff #20857 also asks about that). test/js/bun/wasm/wasi.test.js
  • node:wasi fd_pread reported twice the bytes read whenever a read filled its buffer, and read the next iov from that many bytes too far into the file: each chunk was added to the total inside the loop and the iov total again after it. (Found in review, not by tsc.) test/js/bun/wasm/wasi.test.js
  • http2 session.originSet threw after destroy() on a TLS session that had never read it. It returns undefined, as in Node. test/js/node/http2/node-http2.test.js
  • req.socket.setKeepAlive() and resetAndDestroy() on the node:http server socket returned undefined, so chaining threw. They return the socket. resetAndDestroy() is still a no-op on these sockets, as before; it does not reset the connection. test/js/node/http/node-http.test.ts
  • node:dns validators were called without the argument name, so errors read The "undefined" argument must be of type boolean. They now name options.all, options.verbatim, ipv4, ipv6, options.flags. test/js/node/dns/node-dns.test.js
  • AsyncLocalStorage.bind: same defect, now names fn. test/js/node/async_hooks/AsyncLocalStorage.test.ts
  • bun:sqlite Database.deserialize() threw for an ArrayBuffer, which its signature and docs accept. One from another realm (a vm context) is accepted too. test/js/bun/sqlite/sqlite.test.js
  • node-fetch new Response(stream) threw for an old-style Stream that is not a Readable; fetch() in the same file already handled it. This is what node-fetch-cache hit (Runtime TypeError with node-fetch-cache #8487): with 4.1.0 against a local server, Bun 1.4.2 throws and this branch returns the body and serves the second request from the cache, like Node. test/js/node/http/node-fetch.test.js
  • require("cluster") threw cluster._setupWorker is not a function when NODE_UNIQUE_ID was set after node:net had loaded. test/js/node/cluster.test.ts
  • console.write with several arguments returned the string "[object Promise][object Promise]" when stdout was backed up: a backed-up sink returns a Promise of the byte count, and the results were added with +. It now returns a Promise of the total, as the one-argument call already did, so awaiting it still waits for the drain and still receives a write error such as EPIPE. test/js/bun/console/console-write.test.ts
  • FileSink write() (Bun.stdout.writer(), and console.write through it): a write() to a backed-up sink returns the sink's one outstanding Promise. When the reader had hung up by the next write(), that write failed on the spot and came back as a second, already rejected Promise, so a script awaiting the first caught EPIPE and still died of an unhandled rejection. end() already delivered such an error to the outstanding Promise and returned it; write() now does the same. (Found while fixing console.write, not by tsc.) test/js/bun/util/filesink.test.ts
  • Dev server (bun:bake): a framework render() that did not return a Response gave ReferenceError: $ERR_SSR_RESPONSE_EXPECTED is not defined. hmr-runtime-server.ts threw through a $ERR_* name, which only src/js resolves; it now throws an Error with that code and the intended message, and the ErrorCode.ts entry, which nothing else produced, is removed. test/bake/dev/bundle.test.ts

Behaviour changed to match Node (existing test expectations updated)

  • dns.lookupService (callback and promises): anything that is not an IP throws ERR_INVALID_ARG_VALUE, The argument 'address' is invalid. Received ..., checked before the port and the callback. This replaces two Bun-specific messages the tests pinned.
  • child_process.spawnSync of a process that never ran (the command cannot be spawned) returns what Node returns: status: null, pid: 0, output: null, and stdout / stderr undefined. status and pid used to be undefined, and the test spawnSync(does-not-exist) pinned output: [null, null, null]. Bun.spawnSync also throws when the process ran to completion and reading its output failed, and Node reports that one with its pid and an output array, so that error now carries pid, exitCode and signalCode (set in js_bun_spawn_bindings.rs, where it is thrown) and child_process tells the two apart by them. test/js/bun/spawn/spawn-stdio-syscall-error.test.ts pins both sides.

Other runtime changes, none observable

  • node:dns Resolver#setLocalAddress calls its own native resolver. It used to go through the fallback to Bun.dns, which has no setLocalAddress; a call detached from a resolver throws a TypeError either way, as in Node. setServers parses the port when there is one, where it used to call parseInt on the default port number.
  • internal/html passes its routes to Bun.serve as routes, the current name; the native config reads routes and static the same way.
  • internal/quic: a placeholder key on QuicEndpoint's private state object was spelled stat; it is state, the key the code assigns.
  • A few expressions gained a numeric coercion or a typeof check that tsc needs and that does not change the result: net (res < 0 on a boolean | number, port | 0), zlib brotli params, punycode.

Removed

  • WASI#getState() / setState(): both have thrown ReferenceError: bindings is not defined since they were added, and Node's WASI has neither.
  • Code that could not run: a sentTrailers check on an http2 session (only streams have it); hideFromStack([...]) in http2.ts, which passed one array to a rest-parameter function; the writev branch of net.Socket#bytesWritten (_pendingData is never an array here); a branch reading ErrorEvent#stack, which does not exist.
  • internal/quic: local ERR_* functions that only forwarded to the $ERR_* intrinsics and were called with new.
  • node/wasi.ts was bundler output of the wasi-js package. Its three __commonJS wrappers are now plain top-level code; they already ran at module load, and everything that crossed them was untyped.

Declarations corrected

  • src/js/builtins.d.ts: Socket.$write takes (data, encoding) and returns boolean | number, $end takes (data, encoding); $isArray narrowed an unknown argument to never; $isObject / $isCallable narrowed any to a type with no properties; several $ERR_* that take no message.
  • Code generators: a class property with both a getter and a setter was typed readonly; NativeFilenameCPP now accepts the path form the resolver accepts.
  • bun-types: the PerformanceObserver constructor takes its callback and has supportedEntryTypes; process.binding("uv") has getErrorMap(); console.write() is declared number | Promise<number>: it returns the Promise when stdout is backed up.

Also typechecked now: the rest of src, and the dev server

  • src/tsconfig.json: its exclude list named directories that have since moved, so the project pulled in the bun init / bun create templates (users' projects, whose dependencies are not installed here), the bake project and the libuv stub generator, and reported their errors. With the paths corrected it checks the code generators, the *.classes.ts definitions and the other build-time TypeScript, and passes with no source change.
  • src/runtime/bake/tsconfig.json stopped at Cannot find type definition file for 'react/experimental': @types/react and @types/react-dom were not installed (added as root devDependencies, 18.x to match react), and with types set to that one entry neither bun-types nor the Node types were loaded. Behind that were the dev-server bug above and declaration fixes: Bake.RouteMetadata declares the request the runtime passes for mode = "ssr" pages, route params, the stream's bytes(), the abort reason, Bun's array-valued headers.
  • src/runtime/bake/generated.ts (the dev server's message enums, which the bake sources import) was an undeclared side output of the bake codegen edge, like the built-in modules' declarations were. It is a declared output now and part of the same generated-types target, so bun run build:types produces everything the typechecks need on a fresh checkout.
  • Both include bun-types by path, as src/js does, and run in lint.yml.
  • Not covered: test/ (its own project, about 7,500 errors today) and the bun init / bun create templates, which would need their dependencies installed.

Build system

  • The type declarations src/js is checked against are made from source alone and are byte-identical between the debug and release profiles. They were undeclared side outputs written into build/debug/codegen, which put a profile name in src/js/builtins.d.ts and let them go stale unnoticed. They are now declared outputs of the same edges, in build/types/, with a generated-types target.
  • --mode=codegen configures the code generators without looking for a compiler, linker, cmake or cargo, in a build directory of its own (build/debug-codegen), so it never replaces a profile's build.ninja or the compile_commands.json clangd reads from build/debug. codegen.ts takes a Pick of Config, so a generator that starts reading a native tool does not compile, and resolveBase() decides the build type, the paths, the JavaScript tools and the inputs of build_options.rs for both modes. bun run build:types is --mode=codegen --target=generated-types.
  • The ErrorCode edge lists src/js/builtins.d.ts, which its generator reads, as an input.
  • undici-types is a root devDependency, pinned to the version already in the lockfile. bun-types imports it for its fetch types when the DOM lib is not loaded, and the isolated install puts it nowhere packages/bun-types can resolve it, so the src/js typecheck lost Headers, Request and Response members. packages/bun-types/package.json is unchanged: whether the published package should declare it is [@types/bun]: Either make unidici-types a real dependency, or remove dependency on it entirely in code #22805.
  • src/js/tsconfig.json: bun-types is referenced by path (a types entry is resolved relative to node_modules/@types, which an isolated install does not create at the root); **/*.js is included; composite is off, because built-ins never emit declarations.

Known differences from Node left as they are

  • validatePort's message text (the shared native validator).
  • With NODE_UNIQUE_ID set after node:net loaded, Node reports cluster.isPrimary === false; Bun decided when node:net first loaded.

How did you verify your code works?

  • bun run build:types && bun x tsc --noEmit -p src/js/tsconfig.json exits 0, including with no pinned clang on PATH, where a full configure fails. A second build:types is "no work to do", a deleted declaration file is regenerated, and touching builtins.d.ts re-runs the error-code edge. build:types leaves build/debug's build.ninja and compile_commands.json byte-identical, and the full build's build.ninja is byte-identical with and without the resolveConfig() refactor.
  • For 76 of the 104 changed files under src/js, the JavaScript left after stripping types is byte-identical to main's. The other 28 were read line by line; everything in them is listed above.
  • Every new or changed test fails with Bun 1.4.2 (USE_SYSTEM_BUN=1 bun test) and passes on a debug build of this branch. Each bug was first reproduced on 1.4.2 and compared with Node v26.7.0.
  • test/integration/bun-types/bun-types.test.ts passes, as do tsc -p scripts/build/tsconfig.json and tsc -p scripts/tsconfig.json.
  • Smoke run on the debug build against Node: a net echo with a string port and bytesWritten, an http2 request and server.close(), brotli with a boolean parameter, a worker error, and loading node:quic.
  • Not verified: the GitHub runner itself (that it has perl and can fetch the pinned ninja), and the wider test suites, which CI runs.

`tsc --noEmit -p src/js/tsconfig.json` now passes and runs in lint.yml.
Nothing checked it before, and it reported about 1,800 errors.

Most of the change adds types: interfaces for internal state objects,
`declare` class fields, optional parameters where the body already
handles `undefined`, and corrected shared declarations. The emitted
JavaScript of most files is unchanged.

The check also found runtime bugs, fixed here with tests:
- node:wasi poll_oneoff threw on any clock wait (BigInt minus number)
- http2 session.originSet threw after destroy()
- http server socket setKeepAlive()/resetAndDestroy() returned undefined
- node:dns and AsyncLocalStorage.bind errors named the argument "undefined"
- dns.lookupService validates the address like node
- child_process.spawnSync of a missing command returns node's result shape
- bun:sqlite Database.deserialize() rejected an ArrayBuffer
- node-fetch Response rejected an old-style Stream body
- require("cluster") threw when NODE_UNIQUE_ID was set after node:net loaded
- console.write returned a Promise when stdout was backed up

Build system: the type declarations src/js is checked against are now
declared ninja outputs in build/types/, which no profile owns, and
`--mode=codegen` configures the code generators without looking for a
compiler, linker, cmake or cargo. `bun run build:types` runs both.
@dylan-conway
dylan-conway requested a review from alii as a code owner September 20, 2026 23:23
- node:events: the members typed for EventEmitter move into its
  `declare class`, instead of an interface merged with that class.
- node:dns: the ambient declarations of Bun.dns's private members and of
  the function-object caches are exported, so no-unused-vars does not
  count them as unused variables.
- node:wasi: remove three WASI constants that nothing reads.

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

Beyond the inline findings, I also checked two of the deletions the description lists as dead code: the writev branch removed from Socket#bytesWritten in src/js/node/net.ts is unreachable because Socket.prototype._writev concatenates its chunks into one Buffer before calling _write, so _pendingData is never an array; and the node:cluster change to require the primary module when NODE_UNIQUE_ID is set after node:net loaded leaves isPrimary decided at first load, which the description already lists as a known difference from Node.

Extended reasoning...

Six confirmed findings are being posted inline, so this body only records what else was examined. The bytesWritten writev deletion was verified against the current src/js/node/net.ts: every _pendingData assignment stores the single chunk passed to _write, and _writev (line ~2887) does Buffer.concat and delegates to _write, so the removed $isArray(data) branch could not execute. The cluster change was checked against the ruled-out list and the new comment in src/js/node/cluster.ts; the remaining isPrimary divergence is documented by the author as intentional. This is a 138-file PR with several behaviour changes, so the inline findings plus a human read of the runtime fixes are still warranted; nothing here is a guarantee of correctness.

Findings marked 🟡 are optional suggestions and need no follow-up push.

Comment thread src/js/node/child_process.ts Resolved
Comment thread src/js/builtins/ConsoleObject.ts Resolved
Comment thread scripts/build/codegen.ts Resolved
Comment thread scripts/build/configure.ts Resolved
Comment thread src/js/bun/sqlite.ts Resolved
Comment thread scripts/build/config.ts Resolved
@robobun

robobun commented Sep 20, 2026 •

Copy link
Copy Markdown
Collaborator
Updated 3:18 AM PT - Sep 21st, 2026

@dylan-conway, your commit 660d94a is building: #119178

@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 new issues

No new issues were found in this update; 6 findings from earlier reviews are still open above.

Still open from earlier reviews (6):

  • 🔴 src/js/builtins/ConsoleObject.ts:140 — Scripts that await console.write(big) and catch the EPIPE from a reader that hung up now die with an unhandled rejectio…
  • 🔴 src/js/node/child_process.ts:686 — Callers of spawnSync for a command that cannot be spawned now get stdout/stderr of undefined, where the base returned n…
  • Also unresolved: 4 minor or pre-existing.

If you have decided not to act on one of these findings, resolve its thread (a reply alone leaves it open) and the next review stops counting it. To review this commit again now, use Re-run on its "Claude Code Review" check.

Build system
- mode=codegen gets a build directory of its own (build/debug-codegen).
  Configuring it in a profile's directory replaced that directory's
  build.ninja and emptied its compile_commands.json, which clangd reads
  from build/debug.
- resolveBase() also resolves the JavaScript tools, so resolveConfig()
  and resolveCodegenConfig() no longer each copy them. CodegenFields
  lists only what the generators read.

Types that stated something false
- duplexpair: #otherSide is `DuplexSide | null`; null is a real state.
- node:events: EventEmitter's members are declared as they are, without
  a Required<> `this` type that claimed _events is always set.
- FakeSocket's socket data is optional; nothing assigns it there.
- http2: the session's socket proxy is typed as a socket, and five
  fields assigned on one branch are `number | undefined`.
- node:dns setLocalAddress calls its own resolver instead of casting
  the Bun.dns fallback, which has no such method.
- The global parseInt is no longer widened for one call site.

Conventions and simplification
- The Bun.spawn, Bun.dns and process._eval augmentations move to
  private.d.ts with the other private declarations.
- internal/html passes `routes`, so the private `static` option
  declaration is not needed; the native config reads both the same way.
- Remove declarations and annotations that tsc does not need.

Tests
- node-http: resetAndDestroy() resets the connection in node, so the
  test no longer requires the request to succeed.
- wasi: poll_oneoff now reaches the sleep path, with an injected sleep.
- bun-types fixtures use PerformanceObserver and getErrorMap().
A backed-up FileSink returns one Promise, the same for every write made
while it is backed up, of the bytes those writes added. console.write
returns it (plus any counts already returned) so that awaiting it waits
for the drain and receives a write error such as EPIPE. With several
arguments the Promises are no longer added together with `+`.

Also:
- Database.deserialize() detects an ArrayBuffer with isAnyArrayBuffer,
  which also accepts one from another realm.
- writeIfNotChanged() replaces the file through a rename: two build
  directories write the same declarations in build/types, and a reader
  must not see half a file.
- The CodegenOutputs literal in build-codegen-declared-outputs.test.ts
  sets builtinTypes.

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

Beyond the inline finding, I checked the follow-up fixes in the two newest commits: the reworked console.write sums numeric counts with the single pending Promise a backed-up FileSink returns (FileSink.rs accumulates consumed into the same promise, so pending.then(n => wrote + n) yields the right total and keeps EPIPE observable), Database.deserialize now uses isAnyArrayBuffer, writeIfNotChanged renames a per-pid temp file into place, and --mode=codegen now configures build/debug-codegen rather than the profile directory — none of these raised new problems.

Extended reasoning...

This run's new commits (83af22c, 9b01970) addressed most of the earlier inline findings; I re-read the console.write rework against FileSink.rs's pending-promise semantics (the sink returns one shared pending promise whose resolved value is the accumulated consumed count, so summing numeric results with that promise is correct and the multi-argument path no longer string-concatenates Promises), the sqlite isAnyArrayBuffer change, the atomic rename in writeIfNotChanged, and the separate codegen build directory. The spawnSync stdout/stderr undefined-vs-null thread is unchanged in the code and remains open as an inline thread, so it is not restated here. Approval is not appropriate because a verified finding is being posted and another verified finding was dropped from the post.

Comment thread src/js/node/_http_server.ts Resolved
- child_process.spawnSync: keep the result it has always returned when
  Bun.spawnSync throws. Node's "could not be spawned" result (output
  null, pid 0, status null) does not apply to every throw: Bun.spawnSync
  also throws when the process ran and reading its output failed
  (spawn-stdio-syscall-error.test.ts), and nothing tells the two apart
  here.
- src/js/eval/fuzzilli-reprl.ts is evaluated as plain JavaScript by
  fuzzilli-reprl-execve.fixture.ts, so it cannot contain type syntax.
  resetCoverage() is declared in private.d.ts instead.
@coderabbitai

coderabbitai Bot commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack


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.

Beyond the inline finding, I re-checked the spawnSync unspawnable-command path after the latest push: src/js/node/child_process.ts:615-620 now sets exitCode/pid to undefined and output stays [null, null, null], so the result shape matches the base and the unchanged spawnSync(does-not-exist) test — no regression there. The PR description's "Behaviour changed to match Node" entry for spawnSync (status: null, pid: 0, output: null) and the "Fixes #31767" line no longer describe the code, so merging as-is would close that issue without changing the behaviour it reports.

Extended reasoning...

Since the previous review, one commit landed. The spawnSync catch block in src/js/node/child_process.ts was reverted to the base result shape (status/pid undefined, output [null, null, null], stdout/stderr null), and test/js/node/child_process/child_process.test.ts is no longer in the diff, so the earlier null-vs-undefined concern is moot. The remaining discrepancy is purely between the PR text and the code: the description still lists the spawnSync change and a "Fixes #31767" reference. The http resetAndDestroy() stubs in _http_server.ts and FakeSocket.ts are unchanged by the latest push, which is already covered by an open inline thread and is not repeated here.

Comment thread src/js/node/wasi.ts Resolved
fd_pread added every chunk to the running total inside the read loop and
then added the iov's total again after it, so a read that filled its
buffer reported twice the bytes, and the next iov was read from that
many bytes too far into the file. fd_read already counts once.
…type

child_process.spawnSync returns what node returns when the process never
ran (status: null, pid: 0, output: null, stdout/stderr undefined). It
cannot treat every error Bun.spawnSync throws that way: Bun.spawnSync
also throws when the process ran to completion and reading its output
failed. That error now carries `pid` and `exitCode`, set where it is
thrown, and child_process reports such a process with its pid and an
output array, as node does.

Readable::take_read_error() takes the stored read error out; spawnSync
builds the JS error from it, so to_buffered_value() no longer throws.

bun-types: console.write() is declared `number | Promise<number>`, which
is what it returns when stdout is backed up.
write() on a backed-up sink returns the sink's one outstanding promise.
When the reader had hung up by the next write(), that write failed on
the spot and came back as a second, already rejected promise: a script
awaiting the first caught the error and still died of an unhandled
rejection. end() already delivers such an error to the outstanding
promise and returns it; to_result() now does the same for write().

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

A child whose output was lost often dies of SIGPIPE, so its exitCode is
null. With only `pid` and `exitCode` on the error, child_process reported
`status: null` and `signal: null` together, which node never returns for
a process that ran. The error now has `signalCode` too, as the result
does, and the test pins that one of the two is set.
src/tsconfig.json: its exclude list named directories that have since
moved, so the project pulled in the `bun init` / `bun create` templates
(users' projects, whose dependencies are not installed), the bake
project and the libuv stub generator, and reported their errors. With
the paths corrected it checks the code generators, the class definitions
and the other build-time TypeScript, and passes.

src/runtime/bake: the project stopped at "Cannot find type definition
file for 'react/experimental'": @types/react was not installed, and with
`types` set to that one entry neither bun-types nor the node types were
loaded. Behind that were the errors fixed here:

- hmr-runtime-server.ts threw `$ERR_SSR_RESPONSE_EXPECTED(...)`, a name
  only src/js resolves. A framework `render()` that did not return a
  Response gave "ReferenceError: $ERR_SSR_RESPONSE_EXPECTED is not
  defined". It throws an Error with that code.
- Bake.RouteMetadata declares `request`, which the runtime passes for
  `mode = "ssr"` pages, and the framework's third `render()` argument is
  typed where it is passed.
- The rest are declarations: route params, the stream's bytes(), the
  abort reason, Bun's array-valued headers.

Both projects include bun-types by path, as src/js does, and run in
lint.yml.
…uiltins

# Conflicts:
#	scripts/build.ts
#	scripts/build/configure.ts
src/runtime/bake/generated.ts (the dev server's message enums, which the
bake sources import) was an undeclared side output of the bake codegen
edge, so ninja did not know it existed and `build:types` did not produce
it: the bake typecheck failed on a fresh checkout with "Cannot find
module './generated'". The edge declares it, and it is part of the
target the typechecks build first, which is now named `generated-types`
since it is no longer only the built-in modules' declarations.

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

Additional findings (outside the current diff — GitHub can't attach inline comments there):

  • 🟣 src/runtime/webcore/FileSink.rs — pre-existing: a script that does await console.write(chunk, "\n") in a try/catch still dies of an unhandled EPIPE rejection, and the await resolves before the bytes drain, when the backpressured chunk's leftover plus the next argument is under 4096 bytes. PipeWriter.rs:913 coalesces that small argument and reports it as Drained; FileSink.rs:401-417 takes that for the pending write's completion and resolves its promise. The flush(true) at ConsoleObject.ts:145 then opens a second promise nobody holds, which is where EPIPE lands. Fix: settle pending in on_write only once the writer's buffer is empty (!has_pending_data), so one promise covers the coalesced tail for console.write and direct sink.write; add that shape to the tests.

    Extended reasoning...

    The PR's console.write fix and the new globals.d.ts doc promise that awaiting the returned Promise waits for the drain and receives EPIPE. That holds for the tested shape (8 MB then "tail") but not when the tail is coalesced into the writer's buffer. Trigger: stdout is a pipe with a slower reader, so the kernel takes only part of a chunk; the leftover plus the next argument is under PipeWriter::CHUNK_SIZE (4096 on Linux, PipeWriter.rs:665). Example: 62 KB free in the pipe, console.write(json65KB, "\n"). First write: PipeWriter.rs:930 try_write returns Pending(62K), the 3 KB rest is buffered, parent_on_write(Pending) returns early at FileSink.rs:397, to_result opens promise P (FileSink.rs:1552-1556) with consumed = 65K. Second write "\n": should_buffer at PipeWriter.rs:904 is true (3 KB + 1 < 4096), so PipeWriter.rs:913 calls parent_on_write(1, Drained) and returns Wrote(1). FileSink::on_write: status is Drained so the early return at :397 is skipped, was_pending at :401 is true, :412-414 sets Owned(consumed) and :417 run_pending resolves P with 65K although 3 KB + 1 are…

    Verification: pre-existing. Trigger: stdout is a pipe whose reader lags so a console.write(a, b) (or sink.write(a); sink.write(b)) leaves a leftover from a that, plus b, is under PosixStreamingWriter::CHUNK_SIZE (4096 on Linux, /home/claude/bun/src/io/PipeWriter.rs:665-669). Mechanism traced in code (no debug build was available, so this is by reading, not by running): 1. First write of a (bigger…

Comment thread test/js/bun/util/filesink.test.ts Resolved
The child now blocks in a synchronous read of stdin between its two
writes, and the parent releases it only after closing its end of stdout.
No event-loop turn happens in between, so the second write() is the one
that finds the hang-up, with the first still pending: the case the
FileSink change is for, instead of whichever way a race fell. Without
that change it reports a second promise and an unhandled rejection.

@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/runtime/bake/hmr-runtime-server.ts Resolved
Its one user was the dev server's `$ERR_SSR_RESPONSE_EXPECTED(...)`,
which never resolved there and now throws a plain Error with that code.
Nothing else produces it, so the generators were emitting a C++ enum
case, a Rust entry and a TypeScript declaration for a code no code
throws.

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

…ency

bun-types imports undici-types for its fetch types when the DOM lib is
not loaded, and the isolated install puts it nowhere packages/bun-types
can resolve it, so the src/js typecheck lost `Headers`, `Request` and
`Response` members. Declaring it in bun-types is a change to the
published package and is left for its own discussion; the root
package.json, whose typechecks are what need it here, declares it
instead, pinned to the version already in the lockfile.

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

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

@robobun

robobun commented Sep 26, 2026

Copy link
Copy Markdown
Collaborator

The case an awaited console.write rejects with EPIPE when the reader has hung up from this PR fails on macOS with caught ENOTCONN. #44072 makes the test accept that code there.

Jarred-Sumner pushed a commit that referenced this pull request Sep 27, 2026
#44072)

### Problem
- `test/js/bun/console/console-write.test.ts:134` fails on macOS with `-
"caught EPIPE` / `+ "caught ENOTCONN`.
- The case closes the reader while the child writes 8 MB. The macOS
kernel fails the write that the close lands in with `ENOTCONN`, not
`EPIPE`.
- #43649 added the case and expects `EPIPE` only. Builds 121009 and
120123 were red: each attempt failed with this or with the unhandled
rejection that #43679 fixes.

### Fix
- On macOS the case accepts `caught EPIPE` or `caught ENOTCONN`. Other
platforms accept `caught EPIPE` only.
- The case still requires a rejected promise, no other stderr output,
and exit code 0.
- Verified on a CI Mac (macOS 26.6.1, M2), `main` canary, 150 runs of
the file. `caught ENOTCONN` fails 7 runs of the old file and 0 runs of
the new file.
- `bun bd test test/js/bun/console/console-write.test.ts` passes on
Linux.

### Background
- `Bun.spawn` gives a child with `stdout: "pipe"` one end of an AF_UNIX
socketpair.
- XNU `sosend()` tests for `EPIPE`, then unlocks the socket to copy the
bytes. `uipc_send()` then tests "connected" first and returns
`ENOTCONN`. Each later write returns `EPIPE`.
- Considered a retry in `try_write_with_write_fn`
(`src/io/PipeWriter.rs:84`) so that the writer reports `EPIPE`. #40935
decided that Bun passes the kernel errno through, as Node does. Node
gave `ENOTCONN` in 65 of 150 runs of this scenario.
- The unhandled rejection still fails 67 of those 150 runs. With #43679
and this PR the file passes 100 of 100 runs.

<details><summary>Notes</summary>

**This PR changes one test.** No file under `src/` changes.

**Kernel path** (xnu-12377, `bsd/kern`):
- `sosendcheck()` (`uipc_socket.c`) tests `SS_CANTSENDMORE` first and
returns `EPIPE`.
- `sosend()` calls `socket_unlock()` before the `uiomove` copy and
`socket_lock()` after it. It calls `pru_send` with no second state test.
- `uipc_send()` (`uipc_usrreq.c`, `SOCK_STREAM` branch) tests
`SS_ISCONNECTED` first (`ENOTCONN`) and `SS_CANTSENDMORE` second
(`EPIPE`).
- `soisdisconnected()` (`uipc_socket2.c`) clears `SS_ISCONNECTED` and
sets `SS_CANTSENDMORE` in one step.
- `write(2)` on a socket takes the same path (`soo_write()` calls
`sosend()`).

**C probe, no Bun and no Node.** An 8 MB write on an AF_UNIX socketpair
with 512 KB buffers. The peer reads one chunk and closes. 500 runs each.

| host | macOS | non-blocking `send()` | blocking `write()` |
| --- | --- | --- | --- |
| M2 mini | 26.6.1 arm64 | `ENOTCONN` 141 | `ENOTCONN` 396 |
| M2 Ultra | 15.7.9 arm64 | `ENOTCONN` 442 | `ENOTCONN` 392 |
| Intel i7 | 14.8.9 x64 | `ENOTCONN` 1 | `ENOTCONN` 287 |
| Linux x64 | | `ENOTCONN` 0 of 300 | `ENOTCONN` 0 of 200 |

All other runs gave `EPIPE`. So the errno is not specific to macOS 26 or
to one machine. With the default socket buffer size the M2 mini gave
`EPIPE` in 500 of 500 runs.

One more write directly after the `ENOTCONN` returned `EPIPE` in 1659 of
1659 runs. A socket that was never connected returns `ENOTCONN` on each
write.

**The scenario of the test**, on the M2 mini (macOS 26.6.1), Bun
`1.4.3-canary.1+37da174d5`, Node v26.3.0:

| parent | child | `EPIPE` | `ENOTCONN` |
| --- | --- | --- | --- |
| bun | bun, `await console.write(big)` | 63 | 137 |
| bun | bun, `process.stdout.write(big, cb)` | 66 | 84 |
| bun | node, `process.stdout.write(big, cb)` | 145 | 55 |
| node | node, `process.stdout.write(big, cb)` | 85 | 65 |

On macOS 14.8.9 x64, node parent and node child: `ENOTCONN` in 18 of 200
runs.

**Runs of the test file** on the M2 mini:

| binary | test file | runs | failed | failures |
| --- | --- | --- | --- | --- |
| `main` canary | `main` | 150 | 76 | 7 `caught ENOTCONN` (one
argument), 69 unhandled rejection (several arguments) |
| `main` canary | this PR | 150 | 67 | 67 unhandled rejection (several
arguments) |
| #43679 (4133870) | #43679 | 100 | 3 | 3 `caught ENOTCONN` (one
argument) |
| #43679 (4133870) | #43679 merged with this PR | 100 | 0 | |

The test on `main` stays flaky on macOS until #43679 merges.

**The decision in #40935.** An earlier revision of #40935 folded
`ENOTCONN` into `EPIPE` in `write_to_socket`. The merged commit
(118fdd2) dropped the fold: "The kernel errno is passed through
unchanged." Node accepts both codes on macOS in its own test
(`test/js/node/test/parallel/test-cluster-concurrent-disconnect.js:27-33`).
At that time the race was not reproduced (1100 runs, all `EPIPE`). The
probes above reproduce it. This PR keeps the decision.

**`process.platform` and not `isMacOS`.** #43679 changes the `harness`
import line of this file. The test reads `process.platform` so that the
two PRs merge in either order with no conflict (checked with `git
merge-tree`).

**The same errno in other tests.** In the annotations of the last 60
finished builds, `ENOTCONN` appears in the output of three test files:
this one, `test/js/bun/util/filesink.test.ts` (#43794 removes the race
there) and `test/js/node/process/process-stdin.test.ts` (#42260 ignores
the code on macOS).

</details>

<!-- robobun:evidence:begin -->

---

**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/bun/console/console-write.test.ts

<!-- robobun:evidence:end -->
dylan-conway added a commit that referenced this pull request Oct 1, 2026
### What does this PR do?

Puts `console.write` back to how it behaved before #43649, and marks the
tests that depend on the new behaviour as todo.

#43649 turned on typechecking for `src/js`. `wrote += writer.write(...)`
does not typecheck, because a backed-up `FileSink` returns a Promise
instead of a byte count. That PR rewrote the loop to track Promises and
added tests for it. One of them, `an awaited console.write rejects with
EPIPE when the reader has hung up (several arguments)`, is flaky on
`main`: it prints `caught EPIPE` and then dies on an unhandled `EPIPE`.

- The old function body is back. Two `as number` casts and the type
annotations on `this` are the only differences, and they have no effect
at runtime.
- Only `console.write` is reverted. The rest of #43649 stays.
- The two several-arguments test cases are now `test.todo`. They keep
their original assertions, and each has a comment with the failure it
shows. The one-argument byte count and `EPIPE` tests stay as they were,
because the old code passes them.

#### Known bugs this brings back

Bun 1.4.2 has both, and the two todo tests cover them:

- `await console.write(big, new Uint8Array(3))` gives the string
`"[object Promise][object Promise]"` when stdout is backed up.
- `await console.write(big, "tail")` to a reader that hung up dies on an
unhandled `EPIPE`, even inside `try`/`catch`.

#### Not changed

`packages/bun-types/globals.d.ts` still declares `number |
Promise<number>`. That is correct for one argument.

### How did you verify your code works?

- `bun bd test test/js/bun/console/console-write.test.ts`: 4 pass, 2
todo on a Linux debug build.
- With `--todo`, both todo tests fail in the way their comments
describe.
- Before the todo tests were added, the same file ran 200 times, 8 at a
time, on that build: 200 passes.
- The 4 tests that run also pass on Bun 1.4.2, which has the old
implementation.
- `bun x tsc --noEmit -p src/js/tsconfig.json` passes.
- Not run on macOS or Windows.
robobun added a commit that referenced this pull request Oct 3, 2026
…ests stricter

- `$putByIdDirectPrivate(this, "isNodeWorkerThreadsWorker", true)` needs the
  field in the type of `this` for the src/js typecheck (#43649). The
  declaration emits no JavaScript.
- The single-delivery test registers its global-scope listener through
  `EventTarget.prototype.addEventListener`, the one way left in a node worker,
  so the assertion can fail again.
- The spawned test has no per-test timeout. Its spawn timeout is shorter than
  the test timeout of the file.
dylan-conway added a commit that referenced this pull request Oct 13, 2026
### What does this PR do?

Removes two races from the test `a write() that fails while another is
pending rejects the pending promise once` in `filesink.test.ts`. It was
the flaky test seen in the most CI builds: 181 of 569 finished builds
from Oct 8 to Oct 13 on the Linux lanes, and on macOS and ASAN too. No
runtime code changes.

The test needs three things in order: a first `write()` that is pending,
the reader hanging up, and a second `write()` that finds out. The old
test used a child process whose stdout the parent read and then
cancelled. Both edges of that were timing.

**The hang-up could come too early (the flake).** The parent waited for
one chunk of stdout. Under load that chunk arrives while the child is
still inside its first `write()`, so that write is the one that gets
`EPIPE`. Nothing is left in the buffer, the 4-byte second write is
buffered, and it returns `4` instead of the pending promise.

**The hang-up could come too late (a missed bug).** `cancel()` closes
the read end on a pool thread (`src/io/pipes.rs`). If the second
`write()` runs first, it gets `EAGAIN` and the pending promise, and the
test passes without taking the path it is for.

**Fix.** The test owns both ends of a `createSocketPair()`, as its
neighbours in the file do. The first write, `fs.closeSync(readFd)` and
the second write run in one tick, so there is nothing to wait for. It
asserts that the first promise is pending before it hangs up.

The sink is now `Bun.file(fd).writer()` and not `Bun.stdout.writer()`,
which is where the bug was found (#43649). The test only runs on POSIX,
where both are set up the same way: `open_for_writing` duplicates the fd
it is given, nothing in `FileSink.rs`, `PipeWriter.rs` or
`openForWriting.rs` looks at stdio, and the fix is in
`FileSink::to_result`, which every sink shares.

### How did you verify your code works?

Linux x64, 96 copies of the test at once on 32 cores. "main" is the
release build of 110efa0. Bun 1.4.2 predates the sink fix (#43649), so
it must fail.

| | main, must pass | 1.4.2, must fail |
|---|---|---|
| old test | 145 of 384 failed | not measured |
| this PR | 0 of 1152 failed | 0 of 1152 passed |

- In a copy of the old test that printed more, all 66 failures of 384
runs had the first promise already rejected when `write()` returned. All
318 passes had it pending.
- The first commit of this PR fixed only the early hang-up. 1.4.2 passed
it in 24 of 576 runs, which is the late hang-up.
- The whole file passes: 77 tests, 3 runs.
- Not run locally on macOS.
robobun added a commit that referenced this pull request Oct 27, 2026
tsc reports TS2339 for `e.name` and `e.message` in this catch block. A catch
variable is `unknown` under `strict`, and `typeof e === "object"` narrows it
to `object`. CI type-checks src/js since #43649.

Cast to `Partial<Error>` for the two reads. kWriteToConsole in
ConsoleObject.ts does the same for the same check. The bundled JS is
byte-identical.
robobun added a commit that referenced this pull request Oct 27, 2026
The src/js type check (`tsc --noEmit -p src/js/tsconfig.json`) runs in
CI since #43649. That change typed the `appendFile` options without
`signal`, and this branch reads `options.signal`.

Add `signal?: AbortSignal` to the options type, as on
`FileHandle#writeFile`.

Types only. The bundled JS is byte-identical.
robobun added a commit that referenced this pull request Oct 27, 2026
The src/js type check (`tsc --noEmit -p src/js/tsconfig.json`) runs in
CI since #43649. This branch was last checked before that.

- `FSStream.path` accepts a `Uint8Array`. The streams keep a Buffer
  path as it is now, so the old `string` type was wrong.
- The fast-path `Bun.file(this.path)` call casts to `string`. Only
  internal callers reach it, and they pass a string.
- The `rm` and `rmSync` lstat catch blocks read `err.code`, so the
  catch variable is `any`, like the other catch blocks in fs.ts.
- The three new tests in fs.test.ts no longer add type errors to
  `test/`.

Types only. The bundled JS is byte-identical.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants