chore: sync upstream main through 7a503a7899 - #87
Merged
Merged
Conversation
…ses it and whether or not it ever opened (oven-sh#44327) ### What does this PR do? Fixes a use-after-free that crashes `bun test --isolate` / `--parallel`, and the class of bug behind it. #### The crash Crash reports on 1.4.0 through canary, always under `bun test`: ``` us_internal_ssl_close / us_internal_socket_close_raw PostgresSQLConnection::ref_and_close PostgresSQLConnection::fail_with_js_value PostgresSQLConnection::on_connection_timeout __bun_fire_timer ``` 1. At the end of a file, the `--isolate` swap calls `close_all` on every socket group. 2. Closing the pool's socket rejects the pending query. User code queries again (any polling loop), so the pool dials a new socket into the same group, from inside `close_all`. 3. The new socket links in at the head, behind the walk. `close_all`'s force-drain loop then closed it with `us_internal_socket_close_raw`, which dispatched **nothing** for a socket whose connect had not completed. 4. The new connection stays `Connecting`, with its timeout timer armed and a pointer that is freed at the end of the tick. When the timer fires it closes freed memory. TLS is not involved: `us_socket_close` takes the SSL path because the freed memory reads `s->ssl != 0`. The repro has TLS off. #### The class Closing a `us_socket_t` that never opened was silent, so every owner and every bulk closer had to special-case it. Whether a connect gets a `us_socket_t` or a `us_connecting_socket_t` depends on IP literal vs hostname and on the DNS cache, which owners cannot see, and closing a `us_connecting_socket_t` already dispatches `on_connecting_error`. libuv and Node also complete a pending connect request when its handle is closed. Places that did not special-case it: | Where | Symptom | |---|---| | `close_all` force-drain, Postgres | the crash above | | `close_all` force-drain, MySQL and `RedisClient` | same use-after-free from their connection timeouts (ASAN) | | `close_all` force-drain, `Bun.connect` | use-after-free in `NewSocket::finalize` (ASAN), promise never settles | | Postgres `connectionTimeout` against a host that never answers | the query rejects, but the process never exits: `ref_and_close` takes an event loop ref that is released "on socket close" | #### The fix **`packages/bun-usockets/src/socket.c`, the `else if (!s->connect_state)` branch:** whoever holds a socket hears that it is gone exactly once, whoever closed it. `on_close` if it opened, `on_connect_error` if it never did. A happy-eyeballs candidate is held through its `us_connecting_socket_t`, which is told instead. The contract is written down at the vtable in `libusockets.h`. In usockets: - `us_internal_socket_after_open` closes a failed connect through the same function, so the three Rust trampolines no longer each "close first, then notify". The fd is still closed before the handler runs, which the libuv backend needs. - `close_all` is one walk: close gracefully, force it if TLS deferred. The `SEMI_SOCKET` branch and the force-drain loop are gone. What a handler opens is behind the walk and is not chased: with closes that notify, a force-drain never ends for a handler that dials again from `connectError`. No caller that frees its group next can gain a socket during the walk (`Listener`, the uWS `App`: only through listeners, closed first; `HTTPContext`: a client holds a ref on it, so none is attached when it drops), and `us_socket_group_deinit` asserts it. - A connect that was closed is reported as `ECANCELED` on both arms (`LIBUS_ECANCELED`; a `us_connecting_socket_t` used to say `ECONNABORTED`). `connect(2)` cannot fail with it, so a holder can tell "closed" from "failed", and it is what `uv_close` completes a pending request with. - `us_socket_shutdown` does nothing to a socket that is still connecting. It used to overwrite the poll type with `SOCKET_SHUT_DOWN`, which passes for a socket that opened (and stops polling for the connect's completion). In the `--isolate` swap: - It sweeps sockets exactly twice, as it already does for every other kind of handle: once while the finished file's script can hear about it, and once after the realm is retired, for what its close handlers dialed. Nothing can dial after that, and a `debug_assert!` says no socket of the file is left. - Each sweep walks the loop's groups through `loop->data.iterator`, which `us_internal_loop_unlink_group` advances past a group it unlinks, as the timeout sweep does. It used to save `next` and read `next->linked` after close handlers had run, by when the group's owner may have freed it. - That argument needs "nothing enters a retired realm's script again" (`ZigGlobalObject.h`) to be true, and it was only enforced for microtasks and in `EventLoop::run_callback*`. A direct `JSValue::call` (every `Bun.connect` handler) still ran the finished file's script; the new assertion caught it on its first run. `run_callback`'s check moved down into `JSValue::call`, still behind `test_isolation_enabled`, and `AsyncContextFrame::call` and `ScriptExecutionContext::isJSExecutionForbidden` gained it, next to their "VM is stopping" and "graph was disposed" checks. In the owners: - `NewSocket::on_connect_error` always releases the ref `connect_finish` took (as `on_close` does) instead of inferring it from the detached state, and does not enter JS from a finalizer (as `on_close` does not). It keeps `ECANCELED` and a real `ECONNABORTED` instead of folding them into `ECONNREFUSED`. - `node:net` passes `ECANCELED` on to the request as it is. It does so synchronously, where libuv completes a cancelled request on a later loop turn: deferred, it lands on a `connect()` made right after `destroy()` and fails that one, which works on main. - A Postgres connection that failed sends its close_notify and FIN (`shutdown()`), then closes without waiting for the peer's. A graceful TLS close waits for it, and the event loop ref with it, so against a peer that has gone silent the process never exited; `close()` hid that with the manual `unref` this PR removes. The close_notify is still sent because idle and max-lifetime timeouts take this path with a healthy peer, and PostgreSQL logs a TLS connection that ends without one as an error. - MySQL's `clean_queue_and_close` moved from `MySQLConnection` (`&mut self`) to `JSMySQLConnection` (`&self`), so the event a close now raises for a connecting socket does not run under a `connection_mut()` borrow. Hand-written compensation for the silent close, deleted: - `NewSocket::close` and `NewSocket::terminate` (`is_semi_connect`) - `ValkeyClient::close` (ran `on_close` by hand) - `PostgresSQLConnection::close` (manual `poll_ref.unref`) - `WebSocketUpgradeClient::cancel` (took the ext owner back) - the `SEMI_SOCKET` branch of the `close_all` walk Also deleted: `thunk::ext_owner` and `ExtSlot::get`, which only the trampolines used, and `us_socket_detach`, which had no callers and queued a socket for freeing without telling anyone. Kept, with the outdated reasoning removed from the comment: MySQL `do_close` and Postgres `close` still fail with "Connection closed" first, because the socket event would otherwise report a failed connect; the HTTP/2 parser still leaves connecting sockets to the connect-error path. fetch's HTTP client is unaffected: it marks a socket dead before every close. #### Not changed, and the same on main - `sql.end()` against a connected TLS peer that has gone silent never resolves (Postgres also keeps the process alive), and a MySQL connection that fails or idles out against one keeps its fd open: both still close gracefully. - `RareData::close_all_socket_groups` still bounds its rounds at 8, and `us_loop_close_all_groups` still saves `next` across a close. Script is forbidden there. - `MySQLConnection::read_and_process_data(&mut self)` still calls into JS under its borrow. - A leaked `RedisClient` with the default `autoReconnect` still dials again from its retry timer, inside the next file. The swap's second sweep is about what close handlers dial. None of the finished file's script runs for it. - What can still reach a retired realm is what oven-sh#41831 listed and no general boundary covers: an FFI `JSCallback`, a napi threadsafe function, functions of a `node:vm` context or `ShadowRealm` the file created. None is called from a socket's close path. ### How did you verify your code works? New tests. No step that has to succeed is timed. | Test | 1.4.2 (`USE_SYSTEM_BUN=1`) | main, debug+ASAN | this branch, debug and release | |---|---|---|---| | `isolation.test.ts`, "what a leaked $client's close handler dials is gone before next file": Postgres | fail (5/5 runs) | fail | pass | | MySQL | fail (5/5) | fail | pass | | `RedisClient` | fail (5/5) | fail | pass | | `Bun.connect` to a name (its `open` handler runs in the next file) | fail (5/5) | fail | pass | | `Bun.connect` to an address (use-after-free in the finalizer, needs ASAN) | pass | fail | pass | | `sql-close-pending-connection.test.ts`, "the process exits after the connection timeout of a dial that never completes": Postgres | fail | fail | pass | | MySQL (pins current behaviour) | pass | pass | pass | | "the process exits after the server's refusal of a TLS connection whose peer reads nothing more": Postgres | fail | fail | pass | | after `close()`: Postgres (guards the removed `unref`) | pass | pass | pass | | both, MySQL (pins current behaviour) | pass | pass | pass | | `tls-sql.test.ts`, "postgres sends a close_notify when idleTimeout / maxLifetime closes a TLS connection" (guards the non-waiting close) | | pass | pass | | `connect-autoselectfamily-stale-timer.test.ts`: `destroy()` during an attempt reports `ECANCELED` | fail | fail | pass | | `node-net.test.ts`, "closing a handle that was shut down while connecting fails its attempt" | fail | fail | pass | | "a socket destroyed while connecting to an address / several addresses can connect again at once" (guards the synchronous completion: both fail with it deferred) | | pass / fail (no `connectionAttemptFailed`) | pass | The blackhole listener fixture moved from `valkey-gc.test.ts` to `harness.ts` so three files share it. Standalone repro of the reported crash (a fake Postgres server, a leaked polling query, `connectionTimeout: 1`, a second file that outlives it): | Build | `--isolate` | without | |---|---|---| | 1.4.2, Linux x64 | 5/5 crash | 0/5 | | canary bf42a52, Linux x64 | 5/5 crash | 0/5 | | 1.4.0, macOS arm64 | 3/3 crash | 0/3 | | this branch, debug and release | 0/5 | | The gdb backtrace on canary matches the reported stack frame for frame, and an ASAN build plus tracing in `close_all` shows the sequence above (walk closes the first socket, connect, force-drain closes the second with `semi=1`, use-after-free on the second). `node:net` against Node 26.7, with a dial to a port that never answers: | | Node | main | this branch | |---|---|---|---| | `destroy()` during an `autoSelectFamily` attempt | `connectionAttemptFailed` `ECANCELED`, `close` | `close` | as Node | | `destroy(err)` | `error`, `connectionAttemptFailed` `ECANCELED`, `close` | `error`, `close` | `connectionAttemptFailed` `ECANCELED`, `error`, `close` | | the kernel aborts the first attempt (`ss -K` in a network namespace, `SO_ERROR` = `ECONNABORTED`) | `ECONNABORTED`, connects to the next address | `ECONNREFUSED`, connects to the next address | as Node | The last row has no automated test: producing that error needs `CAP_NET_ADMIN`. A single connect that is destroyed, ended, or written to and destroyed, and an attempt that times out, give the same events on all three. PostgreSQL's own log, on the `postgres_tls` container, when an idle or expired connection is recycled: nothing, as on main. Closing without the close_notify logged `could not receive data from client: Connection reset by peer` every time. Every dialer, under ASAN and LeakSanitizer on a debug build, against a port that never answers, by address and by hostname (the six callers of `SocketGroup::connect*` are `NewSocket`, the WebSocket upgrade client, Postgres, MySQL, Valkey and fetch's HTTP client; nothing in C++ dials): | | main | this branch | |---|---|---| | the owner closes its pending dial: `net` `destroy()` and `_handle.terminate()`, `tls` `destroy()`, `WebSocket` `close()` at once and later, `wss`, `terminate()`, an aborted `fetch`, a `Bun.connect` left to process exit (18 cases) | clean | clean | | a worker is terminated, or exits, with pending dials whose failure handlers dial again, for `Bun.connect`, `net`, `ws`, `wss`, Postgres, MySQL, Redis, `fetch` (32 cases) | clean | clean | | the same eight under `bun test --isolate`, pending at the swap (16 cases) | heap-use-after-free in 4: `Bun.connect`, Postgres, MySQL and Redis by address | clean | As a control for the first row, not releasing the ref in `NewSocket::on_connect_error` and in the upgrade client's `handle_connect_error` makes LeakSanitizer report all 16 of their cases. Existing tests, on a debug+ASAN build, Linux x64: 566 files: `test/js/bun/net`, `node/net`, `node/tls`, `node/http`, Node's `test-net-*` and `test-tls-*`, `web/websocket`, `first_party/ws`, workers, `module-graph`, `test/cli/test/{isolation,parallel}`, and `sql` and `valkey` against Postgres, MySQL and Redis in Docker. Every file that failed was rerun alone on this branch and on a debug build of main: nothing fails only on this branch. What fails on both is 5 s timeouts of heavy tests on a debug build, tests that need `host.docker.internal`, and tests that flake on both. The usockets change on its own was also run against 1171 of Node's `test-{net,tls,http,https,http2,socket,worker}-*`, `web/fetch`, `bun/http`, `node/http2`, DNS, hot reload, the inspector and third-party clients. Cost of the retired-realm check in `JSValue::call`, outside `--isolate`. `perf stat -e instructions:u`, three runs each, the same tree with and without the condition, 2,000,000 `HTMLRewriter` element handler calls (nothing but callbacks): | | without | with | |---|---|---| | release, no LTO | 6.272 to 6.274 billion | 6.277 to 6.285 billion | | release, LTO | 5.981 to 5.982 billion | 5.988 to 5.990 billion | `is_from_retired_test_isolation_realm` is `#[cold]` and `#[inline(never)]` because inlined it keeps `call` itself from being inlined: that measured 6.339 to 6.341 billion without LTO.
…coders (oven-sh#33711) ### Problem - `fetch()` fails a valid `Content-Encoding: deflate` body with `ZlibError` in two cases. The zlib window is under 32K, or the first read holds one byte. - `Decompressor::init` (`src/http/Decompressor.rs:41`) picks zlib or raw deflate with `first_chunk.len() > 1 && first_chunk[0] == 120`. - The libdeflate fast path (`src/http/InternalState.rs:361`) tries each whole body as raw deflate first. So one body can give two contents, whole and split. ### Fix - `has_zlib_header` (from oven-sh#31520) applies the RFC 1950 check to two bytes. Both places use it. - With one byte and more to come, `decompress_chunk` consumes nothing. The caller keeps the byte (oven-sh#43123). - The libdeflate path leaves a zlib-headed body to zlib-ng, so whole and split bodies agree. - Verified: `test/js/web/fetch/fetch-gzip.test.ts` (9 new cases, 8 fail on main), one HTTP/2 case, and `test/regression/issue/18413*`. ### Background - `Content-Encoding: deflate` means a zlib stream (RFC 9110 section 8.4.1.2). Some servers send raw deflate. - A zlib stream starts with two bytes. They hold the method (8), the window size and a check: the pair is a multiple of 31. - libdeflate inflates a complete body in one call. zlib-ng is the streaming decoder. - Considered Node's rule (low nibble of the first byte is 8). It needs no wait, but fails raw bodies that Bun decodes. ### Downsides - Raw deflate that starts with a valid zlib header (66 of 65,536 prefixes) now always fails. It decoded whole before. Three deflaters emitted none in 16,984 streams. - The first read of a zlib-wrapped body runs 13 more instructions (165 to 178). Release `.text` grows 256 bytes. <details><summary>Notes</summary> **Earlier work.** oven-sh#31520 has the same header check for the small-window case. This PR takes its helper (co-author on the commit) and adds the one-byte case and the libdeflate path. The first version of this PR stored the lone byte in a new `Decompressor::PendingDeflate` variant and copied the next chunk. Since oven-sh#43123 `decompress_chunk` returns the count of input bytes it consumed and the caller presents the rest again, so that variant is not necessary. **Two contents from one body.** The same bytes can be a valid zlib stream and a valid raw deflate stream, with different content. The test builds such a body of 2,765 bytes (`twoReadings`) and checks both readings with `node:zlib`. On 1.4.3-canary 367d939, `fetch()` returns the raw deflate content when the body arrives whole (libdeflate) or when the first read holds one byte. It returns the zlib content when the first read holds two bytes. No error is raised. With this PR every delivery returns the zlib content. Node v26.3.0 also returns the zlib content in every delivery. **Reference table.** Content-Length framing. Each cell lists the result when the first read holds the whole body, one byte, two bytes. `error` and `ZlibError` are a rejected `fetch()` body. The body names are the rows of the new test. | body | 1.4.3-canary 367d939 | this PR | Node v26.3.0 | curl 8.14.1, whole | | --- | --- | --- | --- | --- | | `raw` | ok, ok, ok | ok, ok, ok | ok, ok, ok | ok | | `raw78` (starts `78 e8`) | ok, ok, ZlibError | ok, ok, ok | error, error, error | ok | | `wb9` | ZlibError, ZlibError, ZlibError | ok, ok, ok | ok, ok, ok | ok | | `wb15` | ok, ZlibError, ok | ok, ok, ok | ok, ok, ok | ok | | `twoReadings` | raw content, raw content, zlib content | zlib content in all | zlib content in all | zlib content | | `rawPassing1950` (starts `78 01`) | ok, ok, ZlibError | ZlibError in all | error in all | ok | | `lyingWindow` | ZlibError in all | ZlibError in all | ok in all | ok | | `oneByte` | ZlibError | ZlibError | empty body | empty body | The test runs these bodies with chunked and close-delimited framing too, with libdeflate on and off. A wider probe of 48 cells (three framings, four bodies, a first read of 0, 1, 2 or 6 body bytes) fails 15 cells on the canary, 0 with this PR and 12 on Node. In each probe the server sends the rest of the body only after the client's `fetch()` promise resolved, so no timer is involved. **What moves.** Raw deflate can start with a valid zlib header only as a stored block with padding bits set, for example `78 01`. On main that body decodes when libdeflate takes it whole or when the first read holds one byte, and fails in the other deliveries. It now fails in all of them, unless the bytes are also a valid zlib stream (the paragraph above). Node's zlib, zlib-ng and libdeflate emitted no such stream: levels 0 to 9 (libdeflate 0 to 12), strategies 0 to 4, windows 9 to 15, 8 inputs. Raw bodies that start `78 e8` or `08 e8` (first byte of a zlib stream, header check fails) now decode in every delivery. Node rejects both. **Unchanged.** A body that ends after one byte is `ZlibError` (Node resolves with an empty body). A zlib stream whose header declares a smaller window than its data uses is `ZlibError` in every delivery (Node decodes it). The cause is not in `fetch()`: Bun builds zlib-ng with `INFLATE_STRICT`, and `zlib.inflateSync` rejects the same stream. A whole raw deflate body that libdeflate accepts and zlib-ng rejects (a block with HLIT 287) still decodes whole and fails split. **The one-byte wait.** `decompress_chunk` returns 0 only when the body has not ended. The single-packet callers always pass a complete body, so they never see 0. `decompress_output_pending` stays false for a held byte, so `pump_held_body` does not loop. A body that ends during the wait gets the final call with `is_done`, and zlib reports the truncated stream. Late consumer, slow reader and byte-by-byte delivery, probed by hand: 60 of 70 cells decode. The other 10 are bodies cut after one byte (9) and an unmet Content-Length (1). **Cost, merge base bf42a52 against the same tree with this change, release builds.** - Instructions per `InternalState::decompress_bytes` call (gdb `nexti`, calls stepped over). First streamed read: gzip 160 to 156, brotli 167 to 168, zstd 173 to 173, zlib-wrapped deflate 165 to 178, raw deflate 165 to 166. Later reads are equal (67, 70, 73, 67). Whole body in one read: gzip 108 to 105, brotli 160 to 161, zstd 167 to 167, zlib-wrapped deflate 204 to 200, raw deflate 107 to 111. - gzip, brotli and zstd run no added compare or branch. Branch, call and compare instructions per call: gzip 41 to 40 and 45 to 44, brotli 51 and 52 on both, zstd 48 and 50 on both. The other differences are register moves. - Whole zlib-wrapped body, per response: raw libdeflate calls 1 to 0, `inflateInit2_` 1 to 1, zlib-ng heap blocks 2 to 2 (112 B and 42,176 B). - One-byte first read: 2 decode calls per response on both builds. Allocator calls for a 120 KB body: 9 with a one-byte first read, 9 with a two-byte first read. - `size bun`: `.text` 80,676,555 to 80,676,811. `InternalState::decompress_bytes` 2287 B to 2394 B. `Decompressor` is 24 B and `InternalState` 400 B on both. **Suites.** On the debug build at 4b02e10 plus this change: `fetch-gzip.test.ts` 89 of 89, `fetch-http2-client.test.ts` 76 of 76, `test/regression/issue/18413*.test.ts` 29 of 29. On 1.4.3-canary 367d939 the new `describe` block fails 8 of 9 and the HTTP/2 case fails with `ZlibError`. In `fetch.stream.test.ts` and `fetch-backpressure.test.ts`, the tests that move 2 to 16 MB time out at the 5 s default on my machine, on main too. With a 60 s timeout they pass on both builds, and this PR is not slower. **Self-review.** A review of this diff has not finished yet. It produced two findings so far. Both are in this push: the `twoReadings` test row, and code comments cut to one line each. **Not in this PR: a raw retry.** curl, Chromium and Firefox do not look at the header. They inflate the body as zlib, and when that fails early they start again as raw deflate (curl `lib/content_encoding.c`, Chromium `net/filter/gzip_source_stream.cc`, Firefox `netwerk/streamconv/converters/nsHTTPCompressConv.cpp`). That decodes `rawPassing1950`. curl retries only inside its first write call. Chromium keeps the input until the first output byte, up to 1000 bytes. In Bun the result must not depend on the read boundaries, so a retry needs the Chromium shape: new decoder state that holds the input until the first output byte. No deflater that I tested emits such a body, so this PR does not add it. A review thread on `src/http/InternalState.rs` asks for it. **Not in this PR: the libdeflate zlib entry.** libdeflate has a zlib entry. It would save the zlib-ng stream for a whole zlib-wrapped body that inflates to 512 KiB or less. Above that it discards a pass, and libdeflate accepts streams that zlib-ng rejects, so the result would depend on the delivery again. That change needs its own numbers. </details> Co-authored-by: Alistair Smith <hi@alistair.sh>
…n-sh#44370) Behaviour change: none One error path differs. See Downsides. ### Problem - `bun build --dump-environment-variables` does nothing. `BUILD_ONLY_PARAMS` declares the flag (`src/runtime/cli/Arguments.rs:538`), but no code reads it. - `DebugOptions.dump_environment_variables` is always `false`, so the branch at `src/runtime/cli/build_command.rs:604` and `Transpiler::dump_environment_variables` cannot run. - The completions still offer the flag, with `--dump-limits` and `--disable-bun-js`, whose fields oven-sh#36184 deleted. ### Fix - Delete the param, the field, the branch and the function. Remove the three names from the completions. - Correct on current main: the parser skips an unknown long flag in silence (`src/clap/streaming.rs:157`). The bare flag gives the same bundle as before. - Verified: `bun bd`, `test/bundler/cli.test.ts`, `bundler_env.test.ts`, `test/cli/bun.test.ts`, `test/cli/run/env.test.ts`. No new test: `REVIEW.md` says "Do not add tests to check dead code stays dead". - Self-reviewed: 14 concerns raised, 13 addressed. Rejected: to drop the first line. It stays because the PR adds no test. ### Background - The flag printed the environment map as JSON and stopped. `bun build` printed valid JSON up to bun 0.8.0, a malformed stream up to 1.0.11, and nothing since 1.0.12 (oven-sh#6395). - oven-sh#43745, oven-sh#44102 and oven-sh#44343 ask to connect the flag or remove it. Close this PR if a maintainer wants it connected. - Considered connecting it: it prints every process variable, and no `.env` value without `--env`. The docs give `bun --print process.env`. ### Downsides - `bun build --dump-environment-variables=value` exited 1 with "does not take a value". It is now skipped. - The bare flag still builds with no message. With oven-sh#40560 (open) it exits 1, like every unknown flag. <details><summary>Notes</summary> #### Deleted, one line each - `parse_param!("--dump-environment-variables")` in `BUILD_ONLY_PARAMS` (`src/runtime/cli/Arguments.rs`). - `DebugOptions.dump_environment_variables` and its default (`src/options_types/context.rs`). - The `if ctx.debug.dump_environment_variables { ... }` branch in `BuildCommand::exec`, and the stale comment above it (`src/runtime/cli/build_command.rs`). - `Transpiler::dump_environment_variables` (`src/bundler/transpiler.rs`). `write_json_string` and the iterator of `bun_dotenv::Map` have other callers. - `completions/bun.zsh`: the `--dump-environment-variables` and `--dump-limits` specs in `_bun_run_completion`. - `completions/bun.bash`: `--dump-environment-variables --dump-limits --disable-bun-js` in `GLOBAL_OPTIONS`. #### Behaviour, measured Released bun 1.4.3 against a debug build of this branch. "Same bundle" means the same bytes on stdout as `bun build x.js`. | Input | Before | After | |---|---|---| | `bun build --dump-environment-variables x.js` | exit 0, same bundle | exit 0, same bundle | | `bun build --dump-environment-variables=1 x.js` | exit 1, "The argument '--dump-environment-variables' does not take a value." | exit 0, same bundle | | `bun build --dump-environment-variables= x.js` | exit 1, same error | exit 0, same bundle | | `BUN_OPTIONS=--dump-environment-variables=1 bun build x.js` | exit 1, same error | exit 0, same bundle | | `bun build --no-such-flag=1 x.js` | exit 0, same bundle | exit 0, same bundle | `bun build --help` never listed the flag. `WARN_ON_UNRECOGNIZED_FLAG` (`src/clap/streaming.rs:10`) is never set to true, so the skip prints nothing. This PR does not touch it. oven-sh#40560 removes it. #### History - `38f83c50c4` and `5691bf385b` (October 2021) added the flag for the old `bun bun` and `bun dev` commands. - Runs of release binaries with `bun build --dump-environment-variables x.js`: 0.8.0 prints valid JSON. 1.0.1 and 1.0.11 print one JSON string per fragment (`"{",` then `"\n ",` then `"PATH",`), which is not valid JSON. The cause is `19aa9d93de` (oven-sh#4233, first in 0.8.1): `Map.jsonStringify` changed from `writer.writeAll` to `writer.write` on the JSON stream. - Up to 1.0.11 the flag was declared one time, with help text, in `debug_params`, and `src/cli.zig:593` read it for every command. oven-sh#6395 deleted that list and added a new declaration, with no help text, to `build_only_params` only. - In the branch of oven-sh#6395, `5bf8ad8fb9` commented out the line that reads the flag, under a source comment that says the two dump flags were "only used in bun dev". `fbbb5e7e38` ("Remove comments") deleted the commented lines. The merged commit does not contain that comment. `bun build` had its own consumer (`src/cli/build_command.zig:220`), and it stayed. - oven-sh#5712 is the only issue that names the flag. It asked for `bun run` on bun 1.0.2, and it ended with "This flag appears to have been removed". - No maintainer has ruled on the flag. oven-sh#36184 deleted `DebugOptions.dump_limits` and `DebugOptions.fallback_only`, the fields of the other two debug flags. oven-sh#41699 removed the dead Solid JSX runtime in the same way. #### If the flag is wanted back The one-line reader is not enough. `bun build --app` returns into `bake::production::build_command` before the branch. The branch runs after the compile target lookup, so it must move to directly after `configure_defines()`. `--watch` needs a decision. The flag needs help text and docs. #### Completions: what stays - The three names are the completion entries of the three old `DebugOptions` debug flags. After this PR none of their fields exists. - `completions/bun.bash` `GLOBAL_OPTIONS` still has nine names that no param table declares: `--use --bunfile --server-bunfile --disable-react-fast-refresh --disable-hmr --jsx-production --platform --public-dir --inject`. oven-sh#35443 generates that file from `bun-cli.json`. oven-sh#42275 rewrites the same line. - `_bun_run_completion` in `completions/bun.zsh` still has eleven specs that `bun run` does not declare: `--no-summary --version -v --revision --external --packages --minify --minify-syntax --minify-whitespace --minify-identifiers --target`. Other commands declare them. #### `DebugOptions` fields with no reader that stay - `output_file`: oven-sh#44050 removes it. - `editor`: oven-sh#38059 connects it. - `package_bundle_map`: `[bundle.packages]` in `bunfig.toml` fills it (`src/bunfig/bunfig.rs:910`) and nothing reads it. No open PR covers it. #### Other params with no reader 207 long flags are declared in `Arguments.rs`. 13 have no `b"--name"` reader in `src/**/*.rs`: - `--tls-min-v1.0`, `--tls-min-v1.1`, `--tls-min-v1.2`, `--tls-max-v1.3`: `src/js/node/tls.ts` reads them from `process.execArgv`. - `--trace-events-enabled` and six more `--trace-*` flags: declared on purpose, see the comment above them. - `--grep`: an alias of `--test-name-pattern`. - `--dump-environment-variables`: the only one with no reader and no stated reason. #### Checks - `test/bundler/cli.test.ts`: 38 pass, 0 fail. `test/bundler/bundler_env.test.ts`: 7 pass, 0 fail. `test/cli/bun.test.ts`: 39 pass, 0 fail. `test/cli/run/env.test.ts`: 107 pass, 0 fail. All with `--timeout 120000`, because the machine was under heavy load. - `bash -n completions/bun.bash` passes. zsh was not available, so `completions/bun.zsh` was checked by reading. The new last line of the `_arguments` call ends with `' &&`, like the other calls in the file. - The debug binary has no `dump-environment-variables` string. The completion scripts are embedded with `include_bytes!`, so `bun completions` installs the edited scripts. - Release binary size in CI against `main`: no target grows. Four of the twelve targets are 2 to 4 KB smaller (FreeBSD x64 and aarch64, Windows x64 and aarch64). - Substitutes, run on bun 1.4.3: `bun --print process.env` prints the process variables and the `.env` values. `logLevel = "debug"` in `bunfig.toml` makes `bun build --env inline` print the `.env` files that it loads. </details> <!-- robobun:evidence:begin --> --- **no test proof** · iteration 1 · the description declares no behaviour change, so there is no failing test to prove; the existing suite in CI is the check <!-- robobun:evidence:end -->
…ailed transfer detaches nothing (oven-sh#37966) ### Problem - `port.postMessage(msg, [ab, port])` throws `DataCloneError` when a getter closes `port`, but `ab` is already detached. So do `worker.postMessage` and `new Worker`. - A `markAsUntransferable(ab)` made by a getter is ignored. `postMessage` on a closed port detaches `ab` and leaves a listed port usable. - Cause: `SerializedScriptValue::create` validates the transfer list, runs user code (`src/jsc/bindings/webcore/SerializedScriptValue.cpp:4760`), then detaches the buffers. ### Fix - `create()` checks the list after serialization: untransferable mark, detached buffer, closed port. A failure throws `DataCloneError` and detaches nothing. An empty list skips the block. - `MessagePort::postMessage` disentangles listed ports before its closed-sender return. Correct: all entry points use `create()`, and no user code runs between check and detach. - Verified: `test/js/node/worker_threads/worker_threads.test.ts` and two more files, 12 new tests that fail on main. - Self-reviewed: 6 concerns raised, 4 addressed. Rejected: checking only after serialization (Node's error order), splitting out `postMessage` (same bug). ### Background - A transfer detaches listed `ArrayBuffer`s and disentangles listed `MessagePort`s. Serialization runs getters mid-call. - WebKit checks only after serialization (https://commits.webkit.org/310996@main). That needs no second pass, but a getter's error then beats a bad list, and Node reports the list first. A check per caller is too late. ### Downsides - Now throws, returned before: a buffer or port that a getter marks untransferable. Also `structuredClone` when a getter closed a listed port. Node delivers the marked port. - With a transfer list a call pays +71 instructions, about +32 per extra entry. Without one: +0. `.text` grows 512 bytes. - Still open: `structuredClone` leaves a listed port attached (oven-sh#34725). A `new Worker` that throws later consumes its list. <details><summary>Notes</summary> Found by fuzzing. There is no user report. #### Measurements Unit: the instructions that the main thread executes inside one call of the host function (`jsFunctionStructuredClone` or `MessagePort::postMessage`, callees included), after 2,000 warm-up calls. Builds: `bun run build:release` (ThinLTO) of main 4b02e10 with and without this diff. The two source files are the same on the current base. The count is a single-step of the call in gdb. `perf` and `valgrind` are not available in my environment (`perf_event_open` returns EPERM, valgrind is not installed), so there is no whole-process count. | call | main | this PR | delta | | --- | --- | --- | --- | | `structuredClone(i)` | 1917 | 1917 | +0 | | `structuredClone(i, { transfer: [] })` | 3559 | 3559 | +0 | | `port.postMessage(i)` | 1213 | 1214 | +1 | | `structuredClone(ab, { transfer: [ab] })`, 16-byte `ab` | 6139 | 6210 | +71 | | `port.postMessage(ab, [ab])` | 2401 | 2473 | +72 | | `port.postMessage(p, [p])`, `p` a new port | 2685 | 2764 | +79 (one sample, see below) | | `structuredClone(list, { transfer: list })`, 16 buffers | 49657 | 50061 | +404 (one sample, see below) | - The +71 is +36 in `create()` (the three loops), +18 in `JSObject::getDirect` (the mark lookup) and +17 in an out-of-line `std::expected` destructor for the result of `transferArrayBuffers`. Each extra entry adds one `getDirect` call and one loop iteration. - The +1 in `port.postMessage(i)` is one `xorps` that the compiler emits again after the reorder. - The last two rows vary between calls, and each is one aligned sample per build. Both are low. A whole-process `perf stat` count over many calls (measured independently, `perf` does not run in my environment) gives +93.6 for the port row and +546 for the 16 buffers row. So an extra entry costs about +32 instructions, not the +22 that the one sample suggests. The same count agrees on the other rows: +0.00 for `structuredClone(i)`, +71.4 and +72.3 for the two rows with one buffer, +1.05 for `port.postMessage(i)`. - Sizes: `.text` 80,705,111 to 80,705,623 bytes (+512). `create()` 19,814 to 20,207 bytes. `MessagePort::postMessage` 2,131 to 2,162 bytes. #### Why the guard also wraps the buffer transfer `structuredClone(i)` is 1917 instructions on main. The shapes I measured: - No guard (the earlier head of this PR): 1937 (+20). - `if (!transferList.isEmpty())` around the re-check only: 1928 (+11). The guard is 2 instructions. The rest is spills, because `transferList` and `messagePorts` stay live across the serialize call, and a split merge of the `ExceptionOr` that `transferArrayBuffers` returns. - The same with the re-check kept out of line: 1934 (+17). - One guard around the re-check and `transferArrayBuffers`, with the result in a plain `std::unique_ptr`: 1917 (+0). This is the shape in the PR. #### Behaviour A getter does the action while the message is serialized. "Intact" means that the listed buffers keep their `byteLength`. | action of the getter | main | this PR | Node v26.3.0 | | --- | --- | --- | --- | | closes a listed port that is also in the message (`port.postMessage`, `worker.postMessage`, `new Worker`) | `DataCloneError`, buffers detached | `DataCloneError`, intact | `DataCloneError`, intact | | the same with `structuredClone` | returns, buffers detached | `DataCloneError`, intact | `DataCloneError`, intact | | closes a listed port that is not in the message, `structuredClone` | returns, buffers detached | `DataCloneError`, intact | returns, buffers detached | | calls `markAsUntransferable(ab)` on a listed buffer | returns, `ab` detached | `DataCloneError`, intact | `TypeError`, `ab` intact | | calls `markAsUntransferable(port)` on a listed port | returns, port delivered | `DataCloneError`, intact | returns, port delivered | | detaches a listed buffer | `TypeError`, earlier buffers detached | `DataCloneError`, other buffers intact | returns, no error | | transfers a listed port elsewhere | `DataCloneError`, buffers detached | `DataCloneError`, intact | aborts (`CHECK(data_)`) | - A closed sending port with `[ab, otherPort]`: main detaches `ab` and leaves `otherPort` usable. This PR consumes both, and the peer of `otherPort` gets `close`. Node does the same. - The mark on a port is the one row where this PR leaves both main and Node. The rule here is one rule for every entry: a mark that exists when the transfer is applied stops it. To match Node on that row, limit the mark loop to `JSArrayBuffer` entries. #### References - HTML StructuredSerializeWithTransfer checks for a detached entry after serialization (step 5), and it checks and detaches entry by entry. A late failure leaves the earlier entries detached. - WebKit validates the whole list after serialization and then detaches (https://commits.webkit.org/310996@main, "Validate transferable states after serialization per spec"). It has no check before serialization any more. So when the list is already bad and a getter throws, WebKit reports the getter's error (the WPT `transfer-errors` order). - Node checks the list before serialization. It reports `DataCloneError` for a bad list before a getter runs. `MessagePort::postMessage` in bun documents the same order. This PR keeps those checks and adds the check after serialization. #### Self-review Six points came back. Four are addressed: cite the WebKit change, say where the bug came from, name the cases that stay open, and say that a port marked by a getter now throws where Node delivers. Two are rejected: - Do the check only after serialization, as WebKit does. It changes which error wins for a list that is already bad, and the current order is Node's. That is a separate decision. - Split the `MessagePort::postMessage` change into its own PR. It is the second half of the same report, and oven-sh#38066 carries the same reorder inside a larger change. The second of the two to merge drops its copy. #### Related open PRs - oven-sh#38066: the same `MessagePort::postMessage` reorder inside a larger change. - oven-sh#34725: makes `structuredClone` disentangle the listed ports. It composes with this PR. - oven-sh#34326: buffers pinned by native I/O. It touches the same function for a different rule. - A `new Worker` that throws after its transfer list was applied (invalid file URL, a preload that does not resolve) is reported separately. This PR does not change `JSWorker.cpp`. #### Suites run On the debug + ASAN build: `worker_threads.test.ts`, `message-channel.test.ts`, `worker-transfer-list.test.ts`, `message-event.test.ts`, `message-port-closed-leak.test.ts`, `structuredClone-classes.test.ts` (241 pass), `worker-postmessage-transfer.test.ts` (7 pass), the `transferables` block of `structured-clone.test.ts` (11 pass). The upstream tests `test-worker-message-port-transfer-{closed,target,self,duplicate,terminate}.js`, `test-worker-message-port-terminate-transfer-list.js`, `test-worker-message-transfer-port-mark-as-untransferable.js`, `test-worker-message-mark-as-uncloneable.js`, `test-worker-message-port-arraybuffer.js`, `test-worker-message-port-message-port-transferring.js`, `test-worker-workerdata-messageport.js`, `test-worker-message-port.js` and `test-buffer-pool-untransferable.js` pass with `BUN_JSC_validateExceptionChecks=1`. On release builds the whole of `structured-clone.test.ts` passes with this diff (244 tests), and main fails only the new test. On the debug build, the "under 2GiB clones without crashing" tests of that file can time out at 5 s when the machine is loaded. They use no transfer list. #### Reproduction ```js import { MessageChannel, Worker, markAsUntransferable } from "node:worker_threads"; const { port1, port2 } = new MessageChannel(); const ab1 = new ArrayBuffer(8), ab2 = new ArrayBuffer(8); const v = { ab1, get g() { port2.close(); return 1; }, p: port2, ab2 }; let err = null; try { new MessageChannel().port1.postMessage(v, [ab1, port2, ab2]); } catch (e) { err = e.name; } console.log("(a) port.postMessage:", err, "ab1", ab1.byteLength, "ab2", ab2.byteLength); const w = new Worker("1", { eval: true }); const ab3 = new ArrayBuffer(8); const { port2: p3 } = new MessageChannel(); err = null; try { w.postMessage({ ab3, get g() { p3.close(); return 1; }, p3 }, [ab3, p3]); } catch (e) { err = e.name; } console.log("(a) worker.postMessage:", err, "ab3", ab3.byteLength); const ab4 = new ArrayBuffer(16); let r; try { r = structuredClone({ get g() { markAsUntransferable(ab4); return 1; }, ab4 }, { transfer: [ab4] }); r = "returned"; } catch (e) { r = e.name; } console.log("(b) mark during serialization:", r, "ab4", ab4.byteLength); await w.terminate(); port1.close(); ``` ``` main: (a) DataCloneError ab1 0 ab2 0 | (a) DataCloneError ab3 0 | (b) returned ab4 0 this PR: (a) DataCloneError ab1 8 ab2 8 | (a) DataCloneError ab3 8 | (b) DataCloneError ab4 16 node 26.3: (a) DataCloneError ab1 8 ab2 8 | (a) DataCloneError ab3 8 | (b) TypeError ab4 16 ``` #### Not changed here - On main, `CloneSerializer::fillTransferMap` runs out of line twice per call, also when both inputs are empty. That is 38 instructions per `structuredClone` or `postMessage` call. </details> <!-- robobun:evidence:begin --> --- **[review]** gate passed · iteration 1 · 5 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 12 FAILED $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/worker_threads/worker_threads.test.ts test/js/web/workers/structured-clone.test.ts test/js/web/workers/worker-postmessage-transfer.test.ts bun test v1.4.1 (861e9ae) test/js/web/workers/worker-postmessage-transfer.test.ts: (pass) self.postMessage transfer list > postMessage(msg, [ArrayBuffer]) transfers and detaches the buffer [190.14ms] (pass) self.postMessage transfer list > postMessage(msg, { transfer: [ArrayBuffer] }) transfers and detaches the buffer [181.60ms] (pass) self.postMessage transfer list > postMessage(msg, [ArrayBuffer, null]) throws TypeError and detaches nothing [190.26ms] (pass) self.postMessage transfer list > postMessage(msg, invalidOptions) throws TypeError [139.17ms] 125 | } 126 | self.postMessage({ name, byteLength: buf.byteLength }); 127 | `, 128 | 1, 129 | ); 130 | expect(results[0]).toEqual({ name: "DataCloneError", byteLength: 16 }); ^ error: expect(received).toEqual(expected) { - "byteLength": 16, + "byteLength": 0, " ... (truncated) release without fix: 12 FAILED bun test v1.4.1-canary.1 (861e9ae) test/js/web/workers/worker-postmessage-transfer.test.ts: (pass) self.postMessage transfer list > postMessage(msg, [ArrayBuffer]) transfers and detaches the buffer [3.46ms] (pass) self.postMessage transfer list > postMessage(msg, { transfer: [ArrayBuffer] }) transfers and detaches the buffer [2.08ms] (pass) self.postMessage transfer list > postMessage(msg, [ArrayBuffer, null]) throws TypeError and detaches nothing [1.53ms] (pass) self.postMessage transfer list > postMessage(msg, invalidOptions) throws TypeError [1.52ms] 125 | } 126 | self.postMessage({ name, byteLength: buf.byteLength }); 127 | `, 128 | 1, 129 | ); 130 | expect(results[0]).toEqual({ name: "DataCloneError", byteLength: 16 }); ^ error: expect(received).toEqual(expected) { - "byteLength": 16, + "byteLength": 0, "name": "DataCloneError", } - Expected - 1 + Received + 1 at <anonymous> (/workspace/bun/test/js/web/workers/worker-postmessage-transfer.test.ts:130:24) (fail) self.postMessage transfer list > postMessage with a listed port closed during serialization detaches nothing [2.1 ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: all passed $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/mechgate.xml" test/js/node/worker_threads/worker_threads.test.ts test/js/web/workers/structured-clone.test.ts test/js/web/workers/worker-postmessage-transfer.test.ts bun test v1.4.1 (861e9ae) test/js/web/workers/worker-postmessage-transfer.test.ts: (pass) self.postMessage transfer list > postMessage(msg, [ArrayBuffer]) transfers and detaches the buffer [188.62ms] (pass) self.postMessage transfer list > postMessage(msg, { transfer: [ArrayBuffer] }) transfers and detaches the buffer [197.72ms] (pass) self.postMessage transfer list > postMessage(msg, [ArrayBuffer, null]) throws TypeError and detaches nothing [193.39ms] (pass) self.postMessage transfer list > postMessage(msg, invalidOptions) throws TypeError [202.08ms] (pass) self.postMessage transfer list > postMessage with a listed port closed during serialization detaches nothing [195.34ms] (pass) self.postMessage transfer list > postMessage(msg, [MessagePort]) transfers the port [135.38ms] (pass) Worker#postMessage transfer list > a listed port closed during serialization fails the transfer witho ... (truncated) release with fix: all passed $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 896ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/53] cxx obj/unified/UnifiedSource-src_jsc_bindings-3.cpp.o [2/53] cxx obj/unified/UnifiedSource-src_jsc_bindings-1.cpp.o [3/53] cxx obj/unified/UnifiedSource-src_jsc_bindings_node_http-0.cpp.o [4/53] cxx obj/unified/UnifiedSource-src_jsc_bindings_node_crypto-0.cpp.o [5/53] cxx obj/unified/UnifiedSource-src_jsc_bindings_node_crypto-1.cpp.o [6/53] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcore-1.cpp.o [7/53] cxx obj/unified/UnifiedSource-src_jsc_bindings-2.cpp.o [8/53] cxx obj/unified/UnifiedSource-src_jsc_bindings-5.cpp.o [9/53] cxx obj/unified/UnifiedSource-src_jsc_bindings-0.cpp.o [10/53] cxx obj/unified/UnifiedSource-src_jsc_bindings-4.cpp.o [11/53] cxx obj/unified/UnifiedSource-src_runtime_webview-0.cpp.o [12/53] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcrypto-0.cpp.o [13/53] cxx obj/unified/UnifiedSource-src_jsc_modules-0.cpp.o [14/53] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcore-2.cpp.o [15/53] cxx obj/unified/UnifiedSource-src_jsc_bindings_webcore-3.cpp.o [16/53] cxx obj ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/jsc/bindings/webcore/MessagePort.cpp | 37 +++-- src/jsc/bindings/webcore/SerializedScriptValue.cpp | 23 +++ test/js/node/worker_threads/worker_threads.test.ts | 158 +++++++++++++++++++++ test/js/web/workers/structured-clone.test.ts | 29 ++++ .../workers/worker-postmessage-transfer.test.ts | 63 ++++++++ 5 files changed, 290 insertions(+), 20 deletions(-) ``` </details> **gate history** · 3 passed · 0 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/jsc/bindings/webcore/MessagePort.cpp 3 7 0 src/jsc/bindings/webcore/SerializedScriptValue.cpp 6 11 0 test/js/node/worker_threads/worker_threads.test.ts 4 3 0 test/js/web/workers/structured-clone.test.ts 2 1 0 test/js/web/workers/worker-postmessage-transfer.test.ts 1 3 0 ``` </details> <!-- robobun:evidence:end -->
…names (oven-sh#41985) ### Problem - The glob matcher (`src/glob/matcher.rs`) steps `?`, `*` and `[...]` over path bytes by the length the UTF-8 lead byte promises and never checks the following bytes. A Linux file name need not be valid UTF-8. - So `?` and `*` swallow the next byte, a `/` included, or step past the end. `bun pm pack` packs `caf\xe9.pem` and `secret/key\xc3` that `.npmignore` (`*.pem`, `secret/*`) excludes. `new Bun.Glob("t*.js")` misses `t\xe4\xb8.js`. ### Fix - `bun_core::strings::utf8_codepoint_with_fffd` exposes the strict decoder that turns these names into JS strings. The matcher steps by its `len`: one U+FFFD per ill-formed piece, never past the end. - So `scan(p)` equals `scan("*")` filtered by `Glob.match(p)` for wildcards and classes. The test asserts it. - Verified: `test/js/bun/glob/scan.test.ts` (11 new cases, stock bun fails 9), `match.test.ts`, the `Bun.Archive` glob tests, the `bun pm pack` ignore tests. ### Background - A Linux file name is any bytes without `/` or NUL. Latin-1 archives, Windows zips and Samba shares produce non-UTF-8 names. - Considered a private decode table in the glob crate: it duplicated the shared decoder and disagreed with it on `ED A0 80`. ### Downsides - A valid non-ASCII name pays a decoder call per non-ASCII character under `?` or `*`: +26.8% instructions per `Glob.match()` over six non-ASCII pairs (+8% for `report-café-résumé.txt`, +51% for a long Japanese name). ASCII is tested first and never calls: -6.4%. - A literal U+FFFD in a pattern still compares bytes: `t�*` misses an ill-formed name that `t[�]*` matches. - A raw `ED A0 80` in a name is three characters for `?` now, one before. <details><summary>Notes</summary> **Repro for the pack and archive cases** (Linux, bun 1.4.3-canary.1+367d939d9 against this branch): ```sh mkdir secret; echo '{"name":"p","version":"1.0.0"}' > package.json; printf '*.pem\nsecret/*\n' > .npmignore python3 -c " for n in (b'index.js', b'ok.pem', b'caf\xe9.pem', b'secret/key', b'secret/key\xc3', b'report-caf\xe9.txt', b'report-ok.txt'): open(n, 'w').close()" bun pm pack --dry-run ``` Stock bun lists `caf\xe9.pem` and `secret/key\xc3` as packed. This branch packs neither. `new Bun.Glob("report-*.txt").scanSync(".")` finds 1 of 2 on stock bun, 2 of 2 here. With a tar that holds `pub/ok.txt`, `pub/x\xc3/deep.txt`, `secret/key` and `secret/key\xc3`, `new Bun.Archive(tar).files("pub/*")` also returns `pub/x\xc3/deep.txt` on stock bun, and `files(["**", "!secret/*"])` keeps `secret/key\xc3`. Here `pub/*` returns only `pub/ok.txt` and no `secret/` member passes. **Instruction counts.** Release builds (`bun run build:release`) of this branch, and of this branch with `src/glob/matcher.rs` from main, so the matcher is the only difference. `perf` and `valgrind` are not available in the build container and `perf_event_open` is not permitted. A ptrace tracer single-steps the main thread between two marker signals instead. Each pair runs 24 and then 48 `Glob.match()` calls with `BUN_JSC_useJIT=0`. The table shows (count(48) - count(24)) / 24, instructions per call. Two runs of each binary give identical counts in all 24 regions. | pattern | string | main | this branch | | |---|---|---|---|---| | `**/*.test.ts` | `test/js/bun/glob/scan.test.ts` | 3292 | 3165 | -3.9% | | `report-*.txt` | `report-quarterly-2024.txt` | 2130 | 2072 | -2.7% | | `src/**/*.{ts,tsx}` | `src/runtime/server/ServerWebSocket.ts` | 4034 | 3851 | -4.5% | | `????-??-??.log` | `2024-09-08.log` | 1000 | 1001 | +0.1% | | `*[0-9].md` | `changelog-v12.md` | 2999 | 2412 | -19.6% | | `*.js` | `a-rather-long-file-name-that-is-not-javascript.css` | 4581 | 4379 | -4.4% | | ASCII sum | | 18036 | 16880 | -6.4% | | `report-*.txt` | `report-café-résumé.txt` | 2528 | 2731 | +8.0% | | `*文*.txt` | `中文字中文字中文字.txt` | 2183 | 2945 | +34.9% | | `**/*.md` | `ドキュメント/設計/概要.md` | 2941 | 3992 | +35.7% | | `???.txt` | `日本語.txt` | 1240 | 1508 | +21.6% | | `*[é]*.pem` | `naïve-café-déjà-vu.pem` | 4439 | 4702 | +5.9% | | `*.js` | `ファイル名がとても長いけれどジャバスクリプトではない.css` | 4238 | 6404 | +51.1% | | non-ASCII sum | | 17569 | 22282 | +26.8% | `.text` is 1,280 bytes smaller than with main's matcher (80,677,713 against 80,678,993). The non-ASCII cost can be recovered: a length-only step that recognises a well-formed sequence inline and asks the decoder for anything else measures +5.3% on the non-ASCII sum and -5.5% on the ASCII sum in the same setup. It is not in this PR. **Limits.** A literal U+FFFD in a pattern compares bytes (`EF BF BD`), so it does not match an ill-formed piece on disk. A component without a wildcard never reaches the matcher (the walker opens it by its bytes), so a change to the literal arm alone would make `t�*` match a name that `t�x` still misses. **Self-review.** The first revision carried a private Unicode Table 3-7 transcription in the glob crate that kept WTF-8 surrogates (`ED A0..BF`) as one 3-byte unit. The review raised that `bun_core` already has the strict decoder and that the surrogate row disagreed with how the returned names decode. Both addressed: the helper is in `bun_core::strings` over the existing decoder, and the test pins the `ED A0 80` row. **Tests.** The `#[test]` table for `utf8_codepoint_with_fffd` is compile-checked only: `bun_core` is not in the crate list of the miri job, and `cargo test -p bun_core` does not link standalone. Its rows are exercised through the scan test. Six `glob.match > ... recursive search` cases in `scan.test.ts` scan the whole repository with a hardcoded 30 s timeout. They time out in this container under debug+ASAN with and without this diff. Counts after the fix for a directory that holds `t\xc3x`, `t\xe4\xb8.js`, `t\xc3\xa9\xc3`, `t😀js` and `tx\x80`: `*` 5, `t*` 5, `t?` 0, `t??` 3, `t???` 1 (`t😀js`), `t*.js` 1, `t*s` 2. `fs.globSync` differs only on `t???` (0) because its `?` matches one UTF-16 code unit and the emoji is two. </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/glob/scan.test.ts <!-- robobun:evidence:end -->
… an override throws (oven-sh#44352) ### What does this PR do? Fixes a segfault when a preload sets `Module.runMain` to something that is not a function, and shows the error when an override throws. ```js // preload.cjs require("module").runMain = {}; ``` | `Module.runMain =` | 1.4.2, canary `7fe13e1b9` | this PR | Node 26.7 | |---|---|---|---| | `{}`, `[]`, `"a string"`, `Symbol("s")`, `10n` | segfault | `TypeError: Object is not a function` (`Array`, `"a string"`, `Symbol(s)`, `10`), exit 1 | `TypeError: require(...).Module.runMain is not a function`, exit 1 | | `() => { throw new RangeError("x") }` | `Error occurred loading entry point: JSError`, and nothing else | `RangeError: x` with its source line and stack, exit 1 | the same | | `class A {}` | `Error occurred loading entry point: JSError` | `TypeError: Cannot call a class constructor A without \|new\|` | `TypeError: Class constructor A cannot be invoked without 'new'` | **Cause** The setter stores any cell. `NodeModuleModule__callOverriddenRunMain` cast it to `JSObject` unchecked and called it with the `CallData` of a value that cannot be called. An exception from the call made `reload_entry_point` return `JSError`, which `entry_point_load_failed` prints by name. The exception itself was never shown. **Fix** `NodeModuleModule__callOverriddenRunMain` throws JSC's own error for a call of what cannot be called, `createNotAFunctionError`. There is no JavaScript at the call to quote, so it describes the value and does not name `Module.runMain`. `reload_entry_point` matches on the `JsResult` of the call and turns what it threw into a rejected promise (`rejected_promise_with_caught_exception`; a termination still unwinds) instead of returning `JSError`. The block propagates with `?`: `From<JsError> for CrateError` keeps a termination and an out-of-memory apart from a throw, which `.map_err(|_| CrateError::JSError)` did not. `process.exit()` in an override, on the main thread and in a Worker, and `terminate()` of a Worker that is in one, give what canary gives. - If the override did not call the original, that promise is the entry point's. It is reported the way an exception from the entry point itself is, and it is marked handled, like the loader's: whoever loads the entry point reports it, and a Worker relies on nothing else doing so. - If the override had called the original, the promise that stored stays the entry point's, as on main. Nobody else looks at it, so replacing it loses what the main file throws. What the override threw besides is an unhandled rejection like any other. - A promise the override **returns** is treated as on main. **All of it, measured** Six overrides, with no handler, `uncaughtException`, `unhandledRejection` or both, and a main file that prints or one that throws: 48 cases, comparing stdout, the number of errors on stderr and the exit code, and leaving the wording of the `TypeError` aside. Linux x64, canary `7fe13e1b9`, Node 26.7. | `Module.runMain =` | cases | the same as Node | the same as canary | |---|---|---|---| | `() => { throw }` | 8 | 8 | 0: `JSError`, exit 1 | | `{}` | 8 | 8 | 0: segfault | | `async () => { throw }` | 8 | 4 | 8 | | `async () => { await; throw }` | 8 | 0 | 8 | | `async (...a) => { await; original(...a) }` | 8 | 4 | 8 | | `(...a) => { original(...a); throw }` | 8 | 1 | 0: `JSError`, exit 1, the main file does not run | In a Worker's preload, with both handlers: `() => { throw }` closes and says nothing on canary, `{}` segfaults, and both are one `uncaughtException` with this PR. `async () => { throw }` is `uncaughtException` and `unhandledRejection` on both. In the last row of the table everything that is thrown is reported, the main file's error too. It differs from Node for these reasons, none changed here: - Bun's `runMain` starts the load and returns, where Node's runs a CommonJS main file. So what a wrapper throws after the original is a rejection beside the entry point's promise: it comes as `unhandledRejection`, where Node gives `uncaughtException`. - With an `uncaughtException` handler only, an unhandled rejection is printed and ends the process with 1, where Node calls the handler. That is so for any unhandled rejection, on canary too: in the main file, in a preload, from a timer. Also different from Node and not changed here: - The `origin` the handler is given is `"unhandledRejection"`, as for every entry point that is not CommonJS. Node gives `"uncaughtException"` for a throw. - An override that calls the original only after an `await`: what the main file throws is never reported, and the process ends with 0. The same on canary. - An `async` override that rejects later is reported twice. The same on canary. - A primitive that is not a cell (`null`, `5`, `undefined`) is ignored by the setter, where Node throws the `TypeError`. That did not crash. ### How did you verify your code works? Nine tests in `node-module-module.test.js`: each value that is not a function, with all of stderr; an override that throws, with a snapshot of the source lines, the caret and the stack; a wrapper that calls the original and throws, with and without handlers, and with a main file that throws too; and one that is not a function and one that throws, each reported once on the main thread and once in a Worker. | | the nine tests | the file and `hot.test.ts` | |---|---|---| | 1.4.2 | 9 fail | | | this PR, debug | 9 pass | 75 pass, 0 fail | `BUN_JSC_validateExceptionChecks=1` with five of the six overrides, both handlers and a main file that throws: no report.
…alue (oven-sh#44353) ### What does this PR do? Fixes a stack overflow in `bun test` when it prints a deeply nested value. The process ends with SIGSEGV and prints nothing: no panic, no test name, no diff. ```js let value = 1; for (let i = 0; i < 20_000; i++) value = i % 2 ? [value] : { a: value }; expect(value).toMatchInlineSnapshot(`"x"`); // exit code 139 ``` `toEqual` has it too. `Bun__deepEquals` checks the stack and throws a `RangeError`, but its frames are smaller than the printer's, so there is a range of depths that compare fine and then overflow in the message. **Cause** `console.log`'s formatter asks `StackCheck::is_safe_to_recurse()` before it goes into a value. The one in `pretty_format.rs`, which prints values for matcher messages, diffs and snapshots, has no stack check and no depth limit. **Fix** The same check at the top of `print_as`, for every value. It throws the `RangeError` that `toEqual` throws for a deeper value, and that Jest ends with. It is not tied to `can_have_circular_references()`: JSX elements and events are not in that set and print what they hold all the same. | `toMatchInlineSnapshot`, 100,000 deep | canary `7fe13e1b9` | this PR | |---|---|---| | arrays and objects, instances of a class, JSX children, JSX props, the `data` of `MessageEvent`s, `objectContaining` | SIGSEGV | `RangeError` | | `Map`, `Set` | `RangeError` | `RangeError` | | the `cause` of errors, promises, `toJSON`, `arrayContaining` | the snapshot does not match | the same | **A writer as deep as the matchers** `print_asymmetric_matcher` is generic over the writer, but called back into the formatter with `&mut dyn bun_io::Write`, which `Formatter` wrapped in an `AsFmt` and then in a `FmtAdapter` to get back to its own kind of writer. So each matcher inside a matcher added two adapters, and every write went through all of them: a recursion as deep as the nesting, inside the write, where nothing checks the stack. The check leaves a fixed reserve, and the chain outgrows it once there is enough stack to nest that far. `amf_print_as` is now generic over the writer too, and `Formatter` passes it on as it is. The mapping of tags it used the bridge for is `impl From<FormatTag> for Tag`. | `objectContaining`, 100,000 deep, `toMatchInlineSnapshot` | with the check only | with this | |---|---|---| | Windows x64, CI's build | `panic: Stack overflow`, at 10,000 deep too | left to CI | | Linux x64, debug, `ulimit -s` 8 MB | `RangeError` | `RangeError` | | 64 MB, 256 MB | SIGSEGV | `RangeError` | | 1 GB | not done after 300 s | `RangeError` | `console.log` goes through the same function with its own formatter, which is not changed: `Bun.inspect(value, { depth: Infinity })` of the same value on Windows throws the `RangeError`. | release builds, Linux x64 | 1.4.2, canary `7fe13e1b9` | |---|---| | `toMatchInlineSnapshot`, alternating `[v]` and `{ a: v }`, 20,000 deep | SIGSEGV | | `toEqual`, the same, 8,000 deep | prints the diff | | `toEqual`, the same, 10,000 to 15,000 deep | SIGSEGV | | `toEqual`, the same, 20,000 deep | `RangeError` from `Bun__deepEquals` | | `toEqual`, objects of 51 properties (the shape in `pretty-format-overflow.test.ts`), 4,000 deep | prints the diff | | the same, 6,000 deep | SIGSEGV | ### How did you verify your code works? Six new tests in `bun-test.test.ts`, which run `bun test` on a value 100,000 deep, one for each kind in the first row above. | | new tests | |---|---| | 1.4.2 | 6 fail: the child is killed by a signal | | this PR, debug | 6 pass | With this PR, debug, every depth from 500 to 20,000 that I tried ends in the diff or in a `RangeError`. `expect.test.js`, the snapshot tests, `pretty-format-*.test.ts` and `diffexample.test.ts`, debug: 490 pass, 2 fail. Both fail on a debug build of main too: - `pretty-format-overflow.test.ts` prints an object 500 deep, which is more than an unoptimized ASAN build has stack for. On main that build crashes from 500 on (139); with this PR it throws the `RangeError`. Both print the diff at 450. Release builds print it at 4,000. - `error snapshots` in `snapshot.test.ts` differs in ANSI codes only.
…ement that is not there (oven-sh#44348) ### What does this PR do? Fixes a segfault at address `0x5` in `bun test`, when an asymmetric matcher is compared with an array element that is not there. ```js expect({ a: [["x"]] }).toMatchObject({ a: expect.arrayContaining([["x", expect.stringContaining("y")]]), }); ``` This is an assertion that should fail and print a diff. It ends the test runner instead. So does `expect([,]).toEqual([expect.any(String)])`. **Cause** The loop over two arrays in `Bun__deepEquals` runs to the length of the first and reads the same index of the second with `getIndexWithoutAccessors`, which returns the empty `JSValue` for a hole, for an index past the end, and for an index that is a getter. `Bun__deepEquals` takes empty values, but its first step, the dispatch to asymmetric matchers, checks only that the matcher is not empty and hands the other value on. The empty value passes `isCell()`, so `isString()`, `isObject()`, `cell->type()` and the rest read the type byte of a null cell. `arrayContaining` passes the expected value first, so there an expected array that is longer than the actual one is enough. **Fix** When one element came back empty and the other may be a matcher, that loop reads the element the ordinary way, with `getIndex`: `undefined` for a hole or past the end, the value of a getter. That is what `a[i]` gives a matcher in Jest. `toStrictEqual` has returned by then and is not changed. | other value is a hole, matcher is | 1.4.2, canary `7fe13e1b9` | this PR | Jest `expect` 29.7.0 | |---|---|---|---| | `stringContaining`, `stringMatching`, `objectContaining`, one from `expect.extend` | segfault | fails | fails | | `any(String)`, `any(Symbol)`, `any(BigInt)`, `any(Array)`, `any(Object)`, `any(Promise)`, `any(Date)` | segfault | fails | fails | | `not.stringContaining` | segfault | passes | passes | | `anything` | **passes** | fails | fails | | `arrayContaining`, `closeTo` | fails | fails | fails | | the `toMatchObject` above | segfault | fails | fails | | index 1 is a getter that returns `"x"`, matcher is | 1.4.2, canary `7fe13e1b9` | this PR | Jest `expect` 29.7.0 | |---|---|---|---| | `anything` | passes | passes | passes | | `stringContaining("x")`, `any(String)` | segfault | passes | passes | Not changed, the same as on main, and different from Jest: a getter at an index compared with a plain value (`"x"`) does not match, and `expect([1]).toEqual([1, expect.not.stringContaining("a")])` fails, because the loop over the rest of a longer expected array does not ask the matcher. ### How did you verify your code works? A new test in `expect.test.js`, in "deepEquals with asymmetric matchers": sixteen matchers against a hole, against a hole under `arrayContaining`, and against the end of a shorter array under `toMatchObject`, and a matcher from `expect.extend` that records what it is given. Also one for a getter at an index, and the crashing inputs in a child process with `BUN_JSC_validateExceptionChecks=1`. | | the three new tests | `expect.test.js` | |---|---|---| | 1.4.2 | `Segmentation fault at address 0x5`; the child ends with SIGSEGV | | | this PR, debug | pass | 422 pass, 0 fail | That file is meant to run in Jest too: the assertions of the first new test hold with Jest's `expect` 29.7.0 under Node, and so do the cases in the tables.
… Blobs were collected (oven-sh#38685) ### Problem - A slice read through a stream returns the whole parent Blob once the `Blob` objects are collected. `await new Response(parent.slice(7, 13).stream()).text()` gives `"SECRET-public-SECRET"`, not `"public"`. - `Any::to_internal_blob_if_possible` (`src/runtime/webcore/Blob.rs:6034`) moves the whole buffer out of a store with one reference. It ignores the Blob's `offset` and `size`. - oven-sh#42116 widened it on main: a body keeps a Blob-backed stream, and `text()` reads it this way. `Bun.readableStreamToText(slice.stream())` was already wrong in 1.4.0. ### Fix - `to_internal_blob_if_possible` also requires `offset == 0 && size >= store length`. A windowed Blob stays an `Any::Blob`. - Correct because the `Any::Blob` arms of `to_action_value` apply the window. The same calls take that path while the parent is alive. - Verified: `test/js/web/fetch/blob.test.ts` (`consuming a slice's stream`). 31 of 37 cases fail on the unfixed 1.4.3 canary, all pass here. Notes list the other suites. ### Background - A Blob's bytes live in a refcounted `Store`. `slice()` shares it and narrows `offset` and `size`. Each `Blob` object holds one reference until it is finalized. - `blob::Any` is what a buffered stream consumer reads. `Any::Blob` views a shared store. `Any::InternalBlob` owns a `Vec<u8>` that JS adopts without a copy. - Considered a `to_blob_if_possible()` call in `text()`, as the other body getters have. It repairs the three body cases only. ### Downsides - A slice whose stream holds the last reference is now copied (bytes) or read in place (text). Before, JS adopted the whole buffer. - None found for an unsliced Blob: two integer compares before the take. Checked both callers of `Any::to_promise`. <details><summary>Notes</summary> **What reads wrong on main (4b02e10), after the parent and the slice are collected** | Read | main | 1.4.0 to 1.4.2 | | --- | --- | --- | | `Bun.readableStreamToText / ToBytes / ToArrayBuffer / ToJSON(slice.stream())` | whole parent | whole parent | | `slice.stream().text() / .json() / .bytes()` | whole parent | whole parent | | `res = new Response(slice); res.body; await res.text()` | whole parent | whole parent | | `await new Response(slice.stream()).text()` | whole parent | slice | | `await new Response(new Response(slice).body).text()` | whole parent | slice | | `await new Request(url, { method: "POST", body: res.body }).text()` | whole parent | slice | | `json()`, `bytes()`, `arrayBuffer()`, `blob()` of those bodies | slice | slice | | `Bun.readableStreamToBlob(slice.stream())` | slice | slice | The main column is from runs of a debug build of main and of the 1.4.3 canary (367d939). The release column is from the report and from the first revision of this PR (17 of 21 cases failed on 1.4.0). **Why only `text()` of a body** `BodyMixin::get_text` (`src/runtime/webcore/Body.rs:1755`) passes a `Locked` body's stream to `readableStreamToText`. The other getters call `to_blob_if_possible()` first, which lifts the windowed Blob back out of the stream and reads it through the Blob paths. Before oven-sh#42116, `Body::Value::from_js` lifted the Blob out of a Blob-backed stream when the body was made, so `text()` never saw the stream. **The path** `ByteBlobLoader::to_any_blob` (`src/runtime/webcore/ByteBlobLoader.rs:146`) checks the window before its own whole-buffer shortcut (`Store::to_any_blob`). For a slice it returns a windowed `Any::Blob` on purpose. `ByteBlobLoader::to_buffered_value` passes it to `Any::to_promise`, then `to_action_value`, which ran the unguarded conversion for each action except `blob()`. The store's other references are the parent and the slice objects. After the GC finalizes them, `has_one_ref()` is true and the window was lost. `>=` and not `==`: `shared_view` clamps a `size` that is larger than the store, so such a Blob views all of it too. **Tests** `describe("consuming a slice's stream after the Blobs were collected")`, 37 cases: 3 sources (`slice.stream()`, `new Response(slice).body`, `new Blob([slice]).stream()`) x 8 consumers, 4 bodies read by `text()`, 7 windows, and two unsliced controls. The windows are a prefix, a prefix of a prefix, a suffix, a slice of a slice, an empty slice at the start, an empty slice in the middle, and `slice(0)`. A prefix has offset 0 and a suffix ends where the store ends, so each clause of the guard has a case that depends on it. The test waits on a `FinalizationRegistry` for each Blob, because the finalizer is what releases the store reference. Unfixed builds: 31 of 37 fail on the 1.4.3 canary (367d939, `USE_SYSTEM_BUN=1`), and 27 of the first 32 failed on a debug build of main 4b02e10. The 6 that pass are the 3 `readableStreamToBlob` cases (that action skips the conversion), `slice(0)`, and the 2 unsliced controls. **Suites run on the debug build of this branch** `blob.test.ts` (147 pass), `body.test.ts` (774 pass, 4 skip), `body-clone.test.ts` (85), `body-mixin-errors.test.ts` (13), `streams.test.js` (624), `readable-stream-blob-consumed.test.ts`, `blob-cow.test.ts`, `blob-array-fast-path.test.ts`, `readablestreamtoarraybuffer.test.ts`, `sync-pull-fast-path.test.ts`. All pass. The suites other than `blob.test.ts` ran before the guard got its name in the last commit, which does not change what the guard does. </details> --------- Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
…r end() (oven-sh#44290) Regression from oven-sh#42181 (in no release). Merge before 1.4.3 is tagged, and before oven-sh#43957 and oven-sh#43962. ### Problem - After `end()` during its handshake, a node:tls client accepts a trusted certificate for another name: no error, and it reads the server's data. Node and Bun 1.4.2 report `ERR_TLS_CERT_ALTNAME_INVALID`. - `on_handshake` (`src/runtime/socket/socket_body.rs`) passed its name verdict to the handler as the success flag. After `end()`, `onClientHandshake` (`src/js/node/net.ts:496`) reads a failure with no error as the client's own close and skips `checkServerIdentity`. ### Fix - `on_handshake` no longer runs the native name check for a node:tls socket. The handler's flag is the handshake result. - `Bun.connect` and `upgradeTLS` sockets do not change. - Verified: `node-tls-duplex-end-verify.test.ts`, 19 new tests. `main` fails 18. Node v26.3.0 passes 49, skips 1. - Self-reviewed: 10 concerns raised, 10 addressed. One is oven-sh#44422 (`onClientHandshake` reads some failed handshakes as established). ### Background - node:tls checks the name in JS (`checkServerIdentity`), on sockets that `DEFERS_SERVER_IDENTITY` marks. The native check ran for them: another name gave `(false, null)`, like a handshake that fails after this side's FIN. - Considered a `getAuthorizationError()` test in the guard, or a second flag variable: both keep an unused native check. ### Downsides - Needs a maintainer's yes: the internal `socket._handle.authorized` and `getAuthorizationError()` of a node:tls client drop the name check (no reader in `src/js` or `test`). The public values do not change. - Still open (oven-sh#43957): only the first such connection of a process reports if its server keeps its side open. <details><summary>Notes</summary> **The regression** (`tls.connect({ socket })` on a connected socket, then `end()`. Trusted chain, certificate for another name, server in the same process, 3 runs each) | Runtime | Output | |---|---| | Node v26.3.0 | `finish, error ERR_TLS_CERT_ALTNAME_INVALID, close` | | Bun 1.4.2 (744846f) | `error ERR_TLS_CERT_ALTNAME_INVALID, close` | | Bun 1.3.13 | `end, finish, error ECONNRESET, close` | | canary 1.4.3-canary.1 (367d939, has oven-sh#42181) | `finish, end, close`, no error | | this PR | `finish, error ERR_TLS_CERT_ALTNAME_INVALID, close` | - oven-sh#42181 (44e51b7) is on `main` and is not an ancestor of the tag `bun-v1.4.2`. The client handler at that tag has no such guard. **Merge order, and why** - This PR first. Both later PRs let more handshakes complete after `end()`, and on `main` each of those accepts a certificate for another name. - oven-sh#43957 removes the engine defect that today keeps the second and later connections of a process from completing such a handshake. Merged before this PR, it turns one silently accepted connection into all of them. Measured with five connections in one process (`tls.connect({ socket })`, another name, server keeps its side open): on `main` 1 of 5 handshakes completes, and the client reports no error for it. On `main` with the source change of oven-sh#43957, 5 of 5 complete with no error. - oven-sh#43962 sends a client's FIN after its ClientHello. It contains this PR's commits. - With oven-sh#43957 only the test file conflicts: both PRs insert a block. Keep both. **Where the native check came from** - oven-sh#31339 added the native name check for all clients. - oven-sh#32359 proposed to remove it and was closed for oven-sh#33755. oven-sh#33755 added `DEFERS_SERVER_IDENTITY`: the check stayed enforced for `Bun.connect`, and for node:tls it was computed but not enforced. It took node:tls sockets out of `reject_unauthorized` and out of the error argument. - oven-sh#42181 then added the guard for "failed after our own FIN", which is the same pair as "computed, another name". - oven-sh#43863 says "Each connection has one server name check", and lists node:tls as "checked after the handshake, in JS". - This PR is the node:tls half of oven-sh#32359. It does not touch the check for `Bun.connect`. - oven-sh#43865, oven-sh#43924 and oven-sh#43947 are the fixes of the chain check in the same test file. **What the client handler gets from the fd engine** (`packages/bun-usockets/src/crypto/openssl.c`) `on_handshake` calls the handler with `(socket, flag, error)`. For a node:tls client: | Case | Before | Now | |---|---|---| | Handshake completed, chain and name good | `true, null` | `true, null` | | Handshake completed, chain good, another name | `false, null` | `true, null` | | Handshake completed, chain bad | `true, X509 error` | `true, X509 error` | | Chain refused inside the handshake | `false, X509 error` | `false, X509 error` | | Handshake failed after this side's FIN | `false, null` | `false, null` | | Handshake failed with a TLS reason | `false, EPROTO` | `false, EPROTO` | | The peer closed before the handshake finished | `false, ECONNRESET` | `false, ECONNRESET` | - Row 2 and row 5 were the same pair. The guard that oven-sh#42181 added for row 5 also caught row 2 once the client had ended. - With no `end()`, row 2 already reached `checkServerIdentity`, because the guard tests `writableFinished`. So a client that does not end behaves as before. - `Bun.connect` and `upgradeTLS` sockets do not carry `DEFERS_SERVER_IDENTITY`. Their flag is `authorized`, as documented. **Rows that this PR does not change** The engine for a Duplex, a named pipe and TLS inside TLS (`SSLWrapper`, `src/uws/lib.rs`) reports no TLS reason. A failed handshake arrives as `(false, null)` after a verified chain and as `(false, X509 code)` otherwise. `onClientHandshake` reads both as an established session unless the client rejects the code. Measured on this PR with a client over a Duplex that does not call `end()`: | The peer answers the ClientHello with | `rejectUnauthorized` | `main`, Bun 1.4.2 and this PR | Node v26.3.0 | |---|---|---|---| | a fatal `handshake_failure` alert | `true` | `error UNABLE_TO_GET_ISSUER_CERT, close` | `error ERR_SSL_SSL/TLS_ALERT_HANDSHAKE_FAILURE, close` | | a fatal `handshake_failure` alert | `false` | `secureConnect authorized=false authError=UNABLE_TO_GET_ISSUER_CERT`, `end` | `error ERR_SSL_SSL/TLS_ALERT_HANDSHAKE_FAILURE, close` | | bytes that are not TLS | `false` | `secureConnect authorized=false authError=UNABLE_TO_GET_ISSUER_CERT`, `end` | `error ERR_SSL_WRONG_VERSION_NUMBER, close` | | a `close_notify` alert | either | no event | `end, error ECONNRESET, close` | - These rows are older than this PR: Bun 1.4.2 prints them too. The native socket is marked unusable after a failed handshake (`transport_unusable` in `on_handshake`). - Owners: oven-sh#32929 (the engine reports the fatal alert over a Duplex), oven-sh#44223 and oven-sh#44021 (a refused renegotiation). - oven-sh#44422 makes a handshake that did not complete terminal in `onClientHandshake`, as it is in the server handler. This PR is the step before it: while `(false, null)` could be a completed handshake, that arm could not go. It stays out of this PR so that this PR can land, or be reverted, alone. **What the native handle of a node:tls client reports** (trusted chain, another name, `rejectUnauthorized: false`, measured) | Value | `main` | This PR | |---|---|---| | `socket.authorized` | `false` | `false` | | `socket.authorizationError` | `ERR_TLS_CERT_ALTNAME_INVALID` | `ERR_TLS_CERT_ALTNAME_INVALID` | | `socket._handle.authorized` | `false` | `true` | | `socket._handle.getAuthorizationError()` | `ERR_TLS_CERT_ALTNAME_INVALID` | `null` | - The native check for these sockets had three effects on `main`: the flag in row 2 above, and the two internal values. - The in-handshake check (`server_identity`) already left node:tls sockets out. Both sites now agree. - `Flags::HOSTNAME_MISMATCH` has no reader on `main`. This PR does not remove it. - The hunk that stops the native check came from an optional finding of an automated review. No person has reviewed it yet. **Measured** (Linux x64, debug builds, Node v26.3.0, server in its own Node process, 40 s watchdog) `tls.connect({ socket })` on a connected socket, then `end()`. The chain is trusted, the certificate is for another name, `rejectUnauthorized: true`: | Server | TLS | Node | `main` (d115f54) | This PR | |---|---|---|---|---| | keeps its side open | 1.3 | `finish, error ERR_TLS_CERT_ALTNAME_INVALID, close` | `finish`, never exits | same as Node | | ordinary | 1.3 | `finish, error ERR_TLS_CERT_ALTNAME_INVALID, close` | `finish, end, close`, no error | same as Node | | keeps its side open | 1.2 | `finish`, never exits | `finish`, never exits | same as Node | | ordinary | 1.2 | `finish, end, error ECONNRESET, close` | same as Node | same as Node | - `end()` in the same tick and `end()` one `setImmediate` later give the same lines. - In the new tests the handshake completes after the client's `end()` under TLS 1.2 and over a Duplex as well. There `main` delivers `secret-banner` to a client with `rejectUnauthorized: true`. - A client that passes its own `checkServerIdentity` is asked: one test asserts the name that it gets, and `authorized=true` when it accepts. **Still open, the engine** - Each line of the table above is the first connection of its process. Five such connections, one after the other in one process, against a server that keeps its side open: with this PR the first reports and the other four print `finish` only. `main` does the same when the name is good. The flight that the first socket seals after its FIN takes the event loop's one spill slot and never leaves it. oven-sh#43957 is open for that. - `tls.connect(port)` followed at once by `end()` still sends its FIN before its ClientHello, so no handshake runs there. oven-sh#43962 changes that order. - oven-sh#43957 is open for the spill slot. Measured with its source change on this PR: all five connections report, for another name and for a good name. - A handshake that fails after `end()` with no certificate still reports nothing. The tests of oven-sh#42181 and oven-sh#43947 for that case pass. **Tests** - 19 new tests against `main`: the name check after `end()` over a Duplex, over TCP behind a record proxy (TLS 1.2 and 1.3) and on a connected `net.Socket` (3 each), `end()` and `end("")` in the turn of `tls.connect()` over a socket that is still connecting and over a Duplex (6), and a resumed session (1, Bun only). - 7 of them are the commit of another branch (`robobun/baba0385/tls-early-end-hostname-check`), taken as it is. The resumed-session test now asserts `isSessionReused()`. - `main` (4b02e10) fails 18 of the 19. The one that passes is `end("")` over a Duplex. **Suites** (this machine ran at a load average of 200 to 800) - On this head: `node-tls-duplex-end-verify` (50 of 50), `node-tls-connect` (121), `tls-reject-before-client-cert` (121), `node-https-agent-checkserveridentity-reuse` (30), `renegotiation` (21), `node-tls-wrapped-socket-close` (15), `node-tls-connect-hostname-verification` (11), `node-tls-upgrade` (5), `node-tls-raw-end` (4), `node-https-checkServerIdentity` (4). - On the head before the rebase: 14 vendored Node tests that use `checkServerIdentity` or the name error, `fetch.tls` (61). `socket.test.ts`, `node-tls-cert` and `node-tls-server` had only failures that `main` has on this machine too (timeouts of child processes, and one test that needs `www.example.com`). - Not run on this machine: macOS, Windows. </details>
…t wraps (oven-sh#37664) ### Problem - `new tls.TLSSocket(socket)` on the client side (STARTTLS) does nothing on main. A write throws `TypeError: socket.@Write is not a function`. `_start()` throws `ERR_MISSING_ARGS`. - Since oven-sh#42181 (not released), `end()` throws an uncaught `TypeError: socket.shutdown is not a function` at `endNT (node:net)`. - Cause: the constructor stored the wrapped stream as `_handle` (`src/js/node/tls.ts`). Nothing replaced it with a TLS handle. ### Fix - The constructor runs the upgrade of `tls.connect({ socket })` (`kUpgradeClientTLS`, `src/js/node/net.ts`). `_handle` is never the stream. `_start()` is a no-op. - The wrap completes like node's `_finishInit`: `'secure'` and `ssl.verifyError()`. It gets no hostname check and no `'secureConnect'`, and `authorized` stays `false`. - Verified: `test/js/node/tls/node-tls-connect.test.ts`. 16 of its 21 new tests fail on main. Also `test/js/node/tls/` and 657 vendored node tests. ### Background - STARTTLS changes a plaintext connection to TLS in place. The `mysql` driver 2.18.1 does it with this constructor. No user filed an issue for it. - In `node:net`, `_write`, `_final` and `_destroy` call into `_handle` as a native handle. - Only `tls.connect()` adds node's `onConnectSecure` (hostname check, `authorized`, `'secureConnect'`). A wrap gets `_finishInit` only. - Considered a start on `_start()`, as in node. Each handle call then needs a guard. ### Downsides - An unused wrap now sends a ClientHello of 1450 bytes (main and node: 0). `setServername()` and `setSession()` after construction have no effect on that handshake. - Unlike node, a wrap rejects an untrusted certificate unless the caller passes `rejectUnauthorized: false`. An app that does its own check must pass `false`. <details><summary>Notes</summary> **Scope.** This head is the core only, as the review of 2026-09-24 asked. The same review decided that a wrap rejects an untrusted certificate by default. Two parts of the earlier head are gone, because other changes own them. oven-sh#42235 landed the forwarding of the `'error'` of a `Duplex`, and oven-sh#43791 owns it for a `net.Socket`. oven-sh#38028 owns the destroy of a wrapped socket that has not connected yet. The `UpgradedDuplex.rs` hunk landed with oven-sh#36909. The review of 2026-09-25 asked for three more changes: commits 6657bc3 and ae669bf, and this body. The review of 2026-09-30 asked for one more: commit a460a9d. **Changes since the earlier head that the review did not list.** - The `open` handler of an upgraded socket applies the `session` option on the native socket. See "Sessions" below. - A wrap gives the `NODE_TLS_REJECT_UNAUTHORIZED=0` warning of `tls.connect()`. - `authorized` and `authorizationError` keep their initial values on a wrap, as in node. The earlier head set `authorized = true` for a good chain. The wrap checks no host name, so that value accepted a certificate of any host. The verdict is `ssl.verifyError()`. - A wrap does not get `onConnectEnd`. A peer that closes during the handshake gives `'end'`, `'finish'`, `'close'` and no `ECONNRESET`, as in node. `tls.connect({ socket })` keeps its `ECONNRESET`. - `servername` is passed into the upgrade. Before, it reached the ClientHello only when the caller gave no `secureContext`. - `kStandaloneWrap` is initialised in the `Socket` constructor. **Differences from node v26.3.0 that stay.** The review kept the start of the handshake in the constructor. The `mysql` driver, the main user of this API, calls `_start()` right after the constructor, so it sees no difference. | shape | node | this PR | | --- | --- | --- | | wrap that is never used | sends nothing | sends a ClientHello (1450 bytes) | | `setServername()` after the constructor | applies, the handshake starts later | too late. Pass `servername` as an option. | | `setSession()` after the constructor | applies | no effect. Pass `session` as an option. | | untrusted certificate, `rejectUnauthorized` absent or `true` | `'secure'`, the caller must read `ssl.verifyError()` | destroy with the verify error, `'_tlsError'`, no `'secure'` | | untrusted certificate, `rejectUnauthorized: false` | `'secure'`, data flows | the same | | `new TLSSocket(raw)`, then `tls.connect({ socket: raw })` | `EALREADY` on the wrap | `Invalid socket` on the `tls.connect` client (main: works, because its wrap does nothing) | Node never rejects a certificate on a wrap. It leaves the check to the app, so an app that forgets the check accepts any certificate. Here the wrap uses the rule of `tls.connect()`: it rejects unless the caller passes `rejectUnauthorized: false`, or `NODE_TLS_REJECT_UNAUTHORIZED` is `0`. Before commit 4d8b9e5, only `rejectUnauthorized: true` rejected, and a wrap with default options accepted each certificate, as in node. The `mysql` driver listens to `'secure'` and to `'_tlsError'`. If the wrap emitted `'secure'` and then destroyed itself, the driver would report a bad chain two times. The replay test asserts one report. **Sessions.** BoringSSL's `SSL_set_session` calls `abort()` when the handshake has started, in release builds too. `TLSSocket.prototype.setSession()` calls the native function at once, on main and on this head. oven-sh#41671 puts the guard in the native function, for each caller: a late `setSession()` then throws `Already started.`. An earlier head of this PR made `setSession()` only store the session. The review asked to take that rule out, and commit ae669bf did. For a client-side wrap the review then asked for one narrow rule, in commit a460a9d: the `TLSSocket` constructor sets `kStandaloneWrap`, and `setSession()` returns at once when it is set. | shape | node | main | this PR | | --- | --- | --- | --- | | `new TLSSocket(raw)`, `setSession()`, `_start()` | resumes | throws `ERR_MISSING_ARGS` | no effect, full handshake | | `tls.connect({ socket })`, then `setSession()` | no effect | process aborted | process aborted | | `setSession()` inside `'secureConnect'` | no effect | process aborted | process aborted | | `tls.connect({ port })`, then `setSession()` before it connects | resumes | resumes | resumes | | `tls.connect({ port })`, then `setSession()` in the `'connect'` listener | full handshake | resumes | resumes | | `tls.connect({ socket, session })` | resumes | full handshake | resumes | | `new TLSSocket(raw, { session })` | resumes | throws `ERR_MISSING_ARGS` | resumes | In the first row, the wrap has sent its ClientHello when `setSession()` runs. Without the check in `setSession()`, that row aborts the process (exit 134). The rule also holds for a wrap over a socket that is still connecting: node resumes there, and this PR runs a full handshake. The last two rows come from one hunk that stays. `SocketHandlers2.open` applies the `session` option on the native socket. An fd upgrade assigns `_handle` after `open`, so `self.setSession()` dropped the option there. **Two bugs of main that this PR does not fix. An open PR owns each.** - The native `setSession()` has no check of the handshake state. `socket.setSession()` in the `handshake` callback of a `Bun.connect` socket aborts the process (exit 134, release build of main). oven-sh#41671 fixes it in `set_session` in `src/runtime/socket/tls_socket_functions.rs`, for each caller. It makes a late `setSession()` throw `Already started.`. - Over a `Duplex`, TLS inside TLS, or a named pipe, a handshake that fails is reported as success. A peer that answers the ClientHello with plaintext gives `'secureConnect'` for `tls.connect({ socket: duplex, rejectUnauthorized: false })` on main, and `'secure'` for a wrap with `rejectUnauthorized: false` here. Node gives `ERR_SSL_WRONG_VERSION_NUMBER`. Over a TCP socket the result is correct. The stream engine in `src/uws/lib.rs` reports no protocol error. oven-sh#32929 fixes it there. One more door of the same bug: with `rejectUnauthorized: true` and the `session` of an earlier verified connection, `tls.connect({ socket: duplex })` emits `'secureConnect'` with `authorized` true on main, and a wrap emits `'secure'` with `ssl.verifyError()` null here. A write after that fails with `ERR_SOCKET_CLOSED`, and no byte reaches the transport. Reproduction for the first one (needs a key and a certificate, for example `test/js/node/tls/fixtures/agent1-*.pem`): ```ts const server = Bun.listen({ hostname: "127.0.0.1", port: 0, tls: { key, cert }, socket: { data() {}, open() {}, error() {} } }); await Bun.connect({ hostname: "127.0.0.1", port: server.port, tls: { rejectUnauthorized: false }, socket: { data() {}, error() {}, handshake(socket) { socket.setSession(socket.getSession()); /* the process aborts here */ } }, }); ``` Reproduction for the second one: ```js const tls = require("tls"), { Duplex } = require("stream"); let answered = false; const raw = new Duplex({ read() {}, write(chunk, encoding, callback) { callback(); if (!answered) { answered = true; setImmediate(() => this.push(Buffer.from("HTTP/1.1 400 Bad Request\r\n\r\n"))); } }, }); const socket = tls.connect({ socket: raw, rejectUnauthorized: false }); socket.on("secureConnect", () => console.log("secureConnect")); // main prints this socket.on("error", error => console.log(error.code)); // node prints ERR_SSL_WRONG_VERSION_NUMBER ``` **Gaps that this PR does not close.** Each one also exists on main for `tls.connect({ socket })`, with the same result. | shape | node | this PR | owner | | --- | --- | --- | --- | | `end()` or `destroySoon()` before the socket connects | waits for `'connect'`, then sends the FIN | `'finish'` at once, no FIN | oven-sh#42339 | | refused connection under a wrap | `'_tlsError'`, `'close'` | uncaught `ECONNREFUSED`, the wrap stays open | oven-sh#38122 | | `end()` over a `Duplex` before the handshake completes | runs the `final()` of the `Duplex` | `'finish'`, no `final()` | oven-sh#42350 | A wrapped `Duplex` that fails when it is read was in this list. oven-sh#42235 landed, and the wrap now reports `'_tlsError'` and then `'close'`, as node does (measured on d31efd7). **Inherited options.** `kUpgradeClientTLS` passed a plain `{ socket, servername }` object to `Socket.prototype.connect`, and that function reads `rejectUnauthorized` through the prototype chain. With `Object.prototype.rejectUnauthorized = false`, the wrap accepted an untrusted certificate, also with an explicit `rejectUnauthorized: true`. Commit 6657bc3 passes the decision of the constructor as an own property, as `tls.connect()` does. Measured on this head under that pollution: a wrap with default options, with `{}` and with an explicit `true` rejects, and an own `false` accepts. `new TLSSocket(raw, { rejectUnauthorized: undefined })` with `NODE_TLS_REJECT_UNAUTHORIZED=0` rejects. **`'finish'`.** `new TLSSocket(new PassThrough()).end()` emits `'finish'` and `'close'` on this head. The shutdown cells do not assert `'finish'`. A separate check reports that `'finish'` is lost there when oven-sh#43962 is applied on top of this PR. This session did not build that combination. Cell by cell for the 21-cell matrix of oven-sh#42330 on the earlier head: oven-sh#37664 (comment) **Releases.** Earlier comments in this thread measured the same failures on Bun 1.4.0 and 1.4.3. This session measured main only. **User.** `Connection.prototype._startTLS` in mysql 2.18.1 (`lib/Connection.js`) is the known caller of the client-side constructor. The replay test matches that function line by line, and `lib/protocol/sequences/Handshake.js` sends the SSLRequest and starts TLS with no reply in between. The xmpp report in this thread is for `tls.connect({ socket })`, a different path. A search of the open and closed issues finds no report for the constructor. **Guard design.** oven-sh#42330 kept the stream as `_handle` and added a guard at 2 of the 6 places that call it as a native handle. **Signatures on main.** `destroy()` on the wrap fails with `handle.close is not a function`. `end()`, `end(cb)` and `destroySoon()` throw `socket.shutdown is not a function` from `process.nextTick`. **Tests.** All are in `test/js/node/tls/node-tls-connect.test.ts`, block `new tls.TLSSocket(socket) on the client side`. - Four reports come from `node-tls-client-wrap-fixture.mjs`. Bun calls its functions in the test process. Node runs the same file as a script. The expected report is the same for both. - `shutdown`: 16 cells. The methods are `end()`, `end(cb)`, `destroySoon()` and `destroy()`. The streams are a connected, a connecting and a never-connected `net.Socket`, and a `Duplex`. Each cell calls the method and then `destroy()`. It asserts no throw, no `'error'` and `'close'`. The cells run together. On main each cell fails with `socket.shutdown is not a function` or `handle.close is not a function`. - `mysql`: the calls of `Connection.prototype._startTLS` in mysql 2.18.1, in the driver's order and at its time. The driver writes the SSLRequest and starts TLS in the same turn, and the server sends no reply in between. Three configurations: `rejectUnauthorized: false`, the CA of the server, no CA. `onSecure` runs one time in each. - `peerCloses`: the peer closes when the ClientHello arrives. - `session`: the `session` option on the three paths, and `setSession()` before the socket connects. Each one resumes. - Both sides of `rejectUnauthorized` run in the test process only, because node accepts in each case. `unlike node, an untrusted certificate destroys the wrap with the verify error` has one case for default options and one for `true`. `with rejectUnauthorized: false, 'secure' fires for an untrusted certificate and a write goes out over TLS` is the other side. `NODE_TLS_REJECT_UNAUTHORIZED=0 turns the default off, as for tls.connect()` pins the environment variable. - `an inherited rejectUnauthorized cannot turn the check of a wrap off` runs in a child process with `Object.prototype.rejectUnauthorized = false`: default options and an own `true` reject, an own `false` accepts. It fails on d31efd7. ``an own `rejectUnauthorized: undefined` still rejects with NODE_TLS_REJECT_UNAUTHORIZED=0, as for tls.connect()`` pins that rule. - `setSession() on a wrap has no effect: it does not abort the process and does not throw` runs in a child process, because the failure is a process abort. Without the check it gets exit code 134. No test covers a late `setSession()` on a `tls.connect()` socket: it aborts the process until oven-sh#41671 lands. - On main (canary 367d939, release build), 16 of the 21 tests in the block fail. 8 fail at once, and 8 fail by the timeout, because main starts no handshake. The 5 that pass are the http2-wrapper guard and the 4 rows that run node. - The SNI test fails when only the `servername` argument is reverted (`Expected: "sni.example"`, `Received: undefined`). **Cost for callers that never wrap a socket,** from the diff: - per `net.Socket`: one more property store in the constructor. - per TLS `connect()`, per client handshake and per `setSession()`: one more property read and branch. - per `internalConnect` and `internalConnectMultiple`: two property reads and branches fewer. Each `[buntls]` options object has one property fewer. - `tls.connect({ socket })` puts the same bytes on the wire as on main (1452). - This session did not measure instructions, syscalls or binary size: `perf`, `valgrind`, `strace` and `bloaty` are not in the test container. A separate differential check of d31efd7 merged onto main reports equal instruction counts: 10,162,756 (main) and 10,159,922 (this PR) for each TLS connection, and 2,195 and 2,200 for `new net.Socket()`. **Suites run with a debug build of this head.** - On the head a460a9d: `test/js/node/tls/node-tls-connect.test.ts` gives 107 pass, 18 skip, 0 fail with a 30 s limit, in 2 of 2 runs. With the default 5 s limit, 4 to 8 tests reach the timeout in each run on this machine, and the set differs from run to run. Most of them came from main. The load average was 450 to 970 on 16 cores. - One of those tests from main (`server write() and end(data) from inside ALPNCallback`) takes the same time with the source of main and with this PR: 3.9 to 6.4 s and 3.8 to 6.0 s, 10 runs each. - `tsc --noEmit -p src/js/tsconfig.json` and `bun lint` pass on a460a9d. - The suites below ran on the head c81cee3, before the merge of main. - `test/js/node/tls/` (28 files): 2 failures in each run, and this change causes neither. `SNICallback runs even when the requested servername matches the bind hostname` fails on the release build of main too. `concurrent Workers all see the same CA certificate lists` fails 5 of 5 times with the `net.ts` and `tls.ts` of main on the same debug build. In the last run the machine was overloaded, and 2 more tests reached the 5 s timeout. Both pass alone in 3 of 3 runs, and neither builds a client-side wrap. - `test-tls-*`, `test-https-*`, `test-net-*`, `test-http2-*` in `test/js/node/test/parallel` (657 files): no failure from this change. 10 files fail on main too (`test-https-proxy-request*.mjs`, `test-https-request-proxy-post.mjs`, `test-tls-client-allow-partial-trust-chain.js`). `test-https-timeout.js` hangs on a debug build, with the source of main too. - `test/js/bun/net/socket.test.ts`: 94 pass, 1 fail. The failure needs DNS for `www.example.com` and fails on main too. </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 0 · 5 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 16 failed, 18 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/tls/node-tls-connect.test.ts bun test v1.4.3 (367d939) test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [3.32ms] (pass) should thow ECONNRESET if FIN is received before handshake [351.98ms] (pass) initializes authorizationError to null in the TLSSocket constructor [9.71ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [175.18ms] (pass) should be able to grab the JSStreamSocket constructor [18.49ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [114.28ms] (pass) tls.connect > should have peer certificate when using self asign certificate [269.78ms] (skip) tls.connect > should have peer certificate (skip) tls.connect > getCipher, getProtocol, getEphemeralKeyInfo, getSharedSigalgs, getSession, exportKeyingMaterial and isSessionReused should work (skip) tls.connect > should process options correctly when connect is called with only options (skip) tls.connect > should process port ... (truncated) release without fix: 34 failed, 18 skipped bun test v1.4.3-canary.1 (367d939) test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [20.95ms] (pass) should thow ECONNRESET if FIN is received before handshake [48.67ms] (pass) initializes authorizationError to null in the TLSSocket constructor [0.39ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [5.88ms] (pass) should be able to grab the JSStreamSocket constructor [0.30ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [5.44ms] (pass) tls.connect > should have peer certificate when using self asign certificate [24.26ms] (skip) tls.connect > should have peer certificate (skip) tls.connect > getCipher, getProtocol, getEphemeralKeyInfo, getSharedSigalgs, getSession, exportKeyingMaterial and isSessionReused should work (skip) tls.connect > should process options correctly when connect is called with only options (skip) tls.connect > should process port and host correctly (skip) tls.connect > should process port, host, and callback correctly (skip) tls.connect > should handle the absence of a callback gracefully (skip) tl ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: 18 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/tls/node-tls-connect.test.ts bun test v1.4.3 (367d939) test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [3.27ms] (pass) should thow ECONNRESET if FIN is received before handshake [394.13ms] (pass) initializes authorizationError to null in the TLSSocket constructor [8.43ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [196.56ms] (pass) should be able to grab the JSStreamSocket constructor [33.22ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [298.86ms] (pass) tls.connect > should have peer certificate when using self asign certificate [101.96ms] (skip) tls.connect > should have peer certificate (skip) tls.connect > getCipher, getProtocol, getEphemeralKeyInfo, getSharedSigalgs, getSession, exportKeyingMaterial and isSessionReused should work (skip) tls.connect > should process options correctly when connect is called with only options (skip) tls.connect > should process port ... (truncated) release with fix: 18 skipped $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) target linux-x64-gnu build type Release build dir ./build/release revision d31efd7 features lto, baseline 23 deps, 136 codegen, 1176 objects in 6018ms ninja: Entering directory `/workspace/bun/build/release' [1/4] fetch lolhtml [lolhtml] up to date [2/4] fetch rust-argon2 [rust-argon2] up to date [2/4] cargo plan → /workspace/bun/build/release/rust-target/plan.json 244 units: 172 lib, 16 proc-macro (host), 19 custom-build (host), 15 run custom-build, 17 lib (host), 4 run custom-build (host), 1 rlib [3/4] reconfigure [1/1499] mkdir stamps [2/1499] mkdir codegen [3/1499] install /workspace/bun bun install v1.4.3-canary.1 (367d939) Checked 26 installs across 65 packages (no changes) [231.00ms] [4/1499] rustc unicode_xid [5/1499] rustc heck [6/1499] rustc build_script_build [7/1499] rustc build_script_build [8/1499] rustc unicode_ident [9/1499] rustc build_script_build [10/1499] rustc build_script_build [11/1499] install /workspace/bun/packages/bun-error bun install v1.4.3-canary.1 (367d939) Checked 1 install across 2 packages (no changes ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/js/internal/net/symbols.ts | 2 + src/js/node/net.ts | 68 ++-- src/js/node/tls.ts | 47 +-- test/js/node/tls/node-tls-client-wrap-fixture.mjs | 387 ++++++++++++++++++++++ test/js/node/tls/node-tls-connect.test.ts | 356 +++++++++++++++++++- 5 files changed, 806 insertions(+), 54 deletions(-) ``` </details> **gate history** · 2 passed · 0 rejected · iteration 0 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/js/internal/net/symbols.ts 0 0 72 src/js/node/net.ts 8 0 73 src/js/node/tls.ts 1 0 74 test/js/node/tls/node-tls-client-wrap-fixture.mjs 2 3 77 test/js/node/tls/node-tls-connect.test.ts 0 0 69 ``` </details> <!-- robobun:evidence:end -->
…n-sh#44285) Fixes oven-sh#44275 ### Problem - `setTimeout[util.promisify.custom]` returns another timer's function once the property read is warm. `promisify(setTimeout)(1, "value")` then throws `TypeError: The "options" argument must be of type object`. - `createTimerFunction` (`src/jsc/bindings/node/NodeTimers.cpp:263`) gave each timer a different `CustomGetterSetter`. The three functions share one Structure, and JSC keys the inline cache for a `CustomAccessor` getter on the Structure. ### Fix - Store the property as a `GetterSetter` in each function's own slot (`putDirectAccessor`). The getter is a host `JSFunction` named `get`, as in Node's `lib/timers.js`. - Correct because the inline cache loads a `GetterSetter` from the object's slot, not from the Structure. The getter ignores the receiver, as Node's does. - A getter-only accessor rejects a strict-mode write and `Object.assign` with a `TypeError`, as in Node. This covers the native change in oven-sh#39935, so that PR can close. Its test hunk is adopted here. - Verified: `test/regression/issue/44275.test.ts` fails 3/3 on the unfixed debug build. Also `node-timers.test.ts`, `util-promisify.test.js`, and Node's promisified timer tests. ### Background - A `CustomGetterSetter` holds a raw C++ getter pointer, and JSC treats it as a property of the Structure. A `GetterSetter` lives in the object's property storage, so objects with one Structure can differ. - Considered one shared custom getter that dispatches on `thisValue`: it receives the receiver, not the slot base, so `Reflect.get(setTimeout, sym, other)` picks the wrong timer. Considered `CustomValue`: its descriptor is a data property, not Node's accessor. ### Downsides - Each global object (workers included) allocates 3 `JSFunction` (32 bytes) and 3 `NativeExecutable` (80 bytes) cells: 336 bytes, `sizeof` from the debug JSC headers. `GetterSetter` replaces `CustomGetterSetter` one for one. - A read of `fn[promisify.custom]` (one per `util.promisify(fn)` call) goes through a host function call frame instead of a direct C++ getter call. Same tree, debug ASAN, 100k reads: base 530 to 610 ms, this PR 537 to 551 ms, inside the noise. Release base is 61 ms. No release build of the PR side in this container. <details><summary>Notes</summary> - Regression since 1.4.1 (oven-sh#39919), which moved the property from `internal/promisify.ts` to a native custom accessor. 1.4.0 does not have that commit: the official 1.4.0 binary is right, and 1.4.1 and 1.4.2 are wrong. - Repro from the issue: `warm: setImmediate setImmediate` or `warm: setTimeout setTimeout`, varying per run. `BUN_JSC_useJIT=0` and bun 1.3.9 print `warm: setTimeout setImmediate`. - 200 loop iterations reproduce on release in every run with `BUN_JSC_useConcurrentJIT=0`. With concurrent JIT the tier-up point varies, so the test sets `BUN_JSC_useConcurrentJIT=0` and loops 300 times. - Reach: `util.promisify()` reads the property at one place for the whole program, so calls on any functions warm that read. After them, one `promisify(setImmediate)` and one `promisify(setTimeout)` anywhere in the program give a `sleep` that is the promise form of `setImmediate`: `await sleep(300)` resolves at once with the value 300 and no error. With `setInterval` first, it returns an async iterator and the process does not exit. The second test in `44275.test.ts` covers both orders (32b34c1). On the official 1.4.2 binary, 200 warm-up calls give the wrong function in 6 of 10 runs with the concurrent JIT, and in 10 of 10 with `BUN_JSC_useConcurrentJIT=0`. - The three timer globals are property callbacks in `ZigGlobalObject.lut.txt`, created the same way, so they follow the same Structure transitions. - Other `putDirectCustomAccessor` call sites in `src/jsc/bindings` use one getter per property name across all objects of a Structure (`JSCommonJSModule.cpp`, `NodeSqlite.cpp`, `JSEnvironmentVariableMap.cpp`), so they do not share this bug. - Self-reviewed: 3 concerns raised, 3 addressed (the oven-sh#39935 overlap and its write test, the comment on the setInterval accessor being a Bun extension, the test's iteration count, which is startup-bound and kept at 300). - Probed on the fixed build: `Reflect.get(setTimeout, sym, {})` is `setTimeout`, `Object.create(setInterval)[sym]` is `setInterval`, a Worker sees `setImmediate[sym]` as `setImmediate`. </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/node/timers/node-timers.test.ts <!-- robobun:evidence:end -->
…en-sh#43957) Follow-up to oven-sh#43947, which is merged. Rebased on main. ### Problem - After `shutdown()` or `end()` during a handshake the socket still seals handshake records. `us_socket_raw_write` sends nothing after a FIN, so `ssl_flush_write_batch` (`packages/bun-usockets/src/crypto/openssl.c`) parks them as a spill that can never drain. - `us_internal_ssl_close` waits for that spill, so the socket never closes. The spill holds the loop's one spill slot, so the next half-closed handshake never finishes. - No report is linked. 1.4.2 reaches it through `socket.shutdown()`, node:tls since oven-sh#42181 (not released). ### Fix - `ssl_flush_write_batch` and the unbatched path of `BIO_s_custom_write` drop records that the socket can never send. - One predicate, `us_internal_socket_can_raw_write`, gates the raw writes, the raw shutdown and both drops. - Correct because these records can never reach the peer. Node completes the same handshakes. - Verified: 12 new tests, 10 fail without this change when run alone. Node v26.3.0 passes 34 of 34. Self-reviewed: 16 concerns raised, 14 addressed. ### Background - A spill is the part of a batch that the kernel did not take. The loop has one spill slot. - While the slot is taken, every socket writes record by record. - Considered a close that ignores the spill, as oven-sh#43946 does for a forceful close. The slot stays taken until the close. ### Downsides - A slow reader still takes the spill slot. This PR removes only the owner that never lets go. - The peer never gets the dropped records. Nor does it on the base or on Node. - Release `.text`: 58154908 to 58153884 B. Each flush costs 1 more direct call. <details><summary>Notes</summary> **Cases, each test run alone, on the head of oven-sh#43947 before its merge (771b423) and with this change** Each socket calls `shutdown()` or `end()` while its handshake runs. The peer keeps the connection open. | Case | oven-sh#43947 | This PR | |---|---|---| | `Bun.listen`, `requestCert`, `rejectUnauthorized: true`, TLS 1.2, untrusted client | `handshake(true, authorized=false, error)`, the socket never closes | same report, then close | | Same, trusted client (control) | stays open until the client ends the connection, then both close | same | | Both rows, while another socket waits for a slow reader | the handshake never reports | same as the two rows above | | Two `Bun.connect` clients, TLS 1.3 | `close` never fires | both report the handshake, both close | | `Bun.connect` client, TLS 1.2, ended after the server's first flight | `close` never fires | `handshake(false)`, close | | `Bun.connect` client that writes to a second TLS socket from its handshake callback | `close` never fires | the write arrives, close | | Two `tls.connect` clients, the first (TLS 1.3 or TLS 1.2) stays open | the second never emits `secureConnect` | it emits it, as on Node v26.3.0 | | `tls.createServer`, TLS 1.2, `end()` in the handshake, untrusted client (control) | both sockets close | same | | Four `tls.connect` sockets after one such handshake in the same process | flight and first write leave in 2 segments | 1 segment | Run in one process, more tests fail on oven-sh#43947 than the table shows: a test that times out leaves the socket that holds the spill slot. That is also the only way in which `TLS 1.3 client sees 'end', not ECONNRESET, when the server tears down right after rejecting its certificate (oven-sh#40653)` failed. No CI run has shown the lost batching. The two-client probe gives the same result on the released 1.4.2 (`1.4.2+744846f84`): the second client never reports its handshake. **Mechanism** - With batching the handshake records go into the batch, and `ssl_flush_write_batch` sends them. After a FIN `us_socket_raw_write` returns 0. The flush parked all of it as a spill and took the spill slot. A spill drains from the writable event, and a socket that sent its FIN gets none. - `us_internal_ssl_close` defers a close with code 0 or 2 while the socket has a spill. - Batching needs a free slot (`hs_batching` in `us_internal_ssl_on_data`, `batching` in `us_internal_ssl_write`). With the slot taken, `BIO_s_custom_write` writes each record at once. After a FIN that write returns 0, the BIO asks for a retry, and `SSL_do_handshake` answers `SSL_ERROR_WANT_WRITE` until the socket closes. - Three places turn a short raw write into a wait for a writable event: the unbatched write, the flush, and `ssl_drain_spill`. The first two now drop. `ssl_drain_spill` has no guard on purpose: `us_internal_ssl_shutdown` defers the FIN behind a spill, and a close releases the spill, so a socket that sent its FIN has no spill to drain. **Why a drop is correct** - SSL counts a sealed record as written. It cannot be sent again, and after the FIN it cannot be sent at all. - The base already reports a completed handshake for the first half-closed socket on a loop: the batch takes the records and the report runs before the flush. The drop makes the second socket, and a socket next to a slow reader, behave like the first. - `us_internal_ssl_write` refuses application data after a FIN, and `ssl_handle_shutdown` sends no close_notify after one. So only handshake records and alerts reach the new branches. **Not in this PR** - A slow reader takes the spill slot like before. The slot is state of the loop, and a spill per connection is a different design. - A close on an open socket whose peer never reads still waits for its spill. That spill can drain. - A `send` that the kernel refuses with `ECONNRESET` or `EPIPE` still parks a spill. That needs the error of the raw write (oven-sh#42336, oven-sh#34510), not the state of the socket. - An `allowHalfOpen` socket that shut down during its handshake stays open after the peer's close_notify until the peer's FIN (since 1.4.1, oven-sh#40384). Main does the same, and it does not reach the new branches. - oven-sh#43946 changes what a forceful close does with a batch that is still held. It does not cover records that are sealed after a FIN. - `end()` from the `'connect'` listener sends the FIN before the ClientHello. Node sends the ClientHello first. **Self-review** Rejected, with the reason: - Move the tests that fail by a timeout into child processes. With this change no test times out. `afterAll` now releases what a timed-out test left, so the tests after the block are not affected. - A test for the case with no relay. The peer has to keep the connection open after our FIN, and a relay is how a test decides that. **Measurements** Call counts are gdb breakpoint hit counts on debug builds with the debug info stripped. Sizes are from release builds of both trees. - Healthy handshake, 100 in-process `Bun.listen` / `Bun.connect` connections: `SSL_do_handshake` 600 and 600, `BIO_s_custom_write` 700 and 700, `ssl_flush_write_batch` 300 and 300, `bsd_send` 600 and 600, `bsd_recv` 600 and 600, `us_internal_socket_close_raw` 200 and 200. - Both new branches are behind a write that the wire did not take in full. A socket that can write pays one more compare there, and nothing on a full write. - Release `.text`: 58154908 to 58153884 B (`size -A`). Stripped `bun`: 80844320 B both. `ssl_flush_write_batch` grows from 214 to 254 B and is no longer inlined into its callers: `BIO_s_custom_write` 683 to 512 B, `us_internal_ssl_close` 1195 to 999 B, `us_internal_ssl_on_data` 2651 to 2240 B, `us_internal_ssl_shutdown` 608 to 412 B. So each flush is one direct call more in a release build, 3 per healthy connection pair. - The shared predicate changes no size: `us_socket_raw_write` 178 B, `us_socket_raw_writev` 356 B and `us_internal_socket_raw_shutdown` 85 B before and after. - Allocations: the change adds none, and a dropped flight saves the `us_malloc` of its spill. **Tests** - `test/js/bun/net/socket.test.ts`: 8 new tests, 17 with "while the handshake runs" in the name. Run alone on oven-sh#43947, 7 of the 8 fail, and the trusted TLS 1.2 client is the control. - `test/js/node/tls/node-tls-duplex-end-verify.test.ts`: 3 new tests. Run alone on oven-sh#43947, 2 fail, and the TLS 1.2 server is the control. Node v26.3.0 passes 34 of 34 (`node --test`). - `test/js/node/tls/node-tls-connect.test.ts`: 1 new test, the one-segment flight. It fails on oven-sh#43947 with 3 chunks for each connection. - The block was run 5 times on this change: 5 passes. - `test/js/node/tls/` and `test/js/bun/net/` together (728 tests): no failure is only on this change. Failures on the base too: `SNICallback runs even when the requested servername matches the bind hostname`, `should not call drain before handshake`, and tests that wait for a garbage collection. - After the rebase on main (ecf3490, with oven-sh#43947 merged) the two directories have 802 tests, and 6 fail: 4 tests that wait for a garbage collection or for workers, a cluster test, and `should not call drain before handshake`. Each of them fails on main on the test machine too. Before that rebase, on e375701, the two directories had 765 tests. 6 fail, the same 6 as on oven-sh#43947 at that base. In one process, oven-sh#43947 at that base fails 8 of the 17 tests with "while the handshake runs" in the name, and the one-segment test. - The vendored `test-tls-*`, `test-https-*`, `test-http2-*` and `test-net-*` files: 660 pass, and the 3 that fail do so on oven-sh#43924 too. This is a check for regressions only: no vendored file changes its result with this PR. </details> --------- Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
…role bit (oven-sh#44447) ### Problem - A `node:http` server dies with `panic(main thread): Segmentation fault at address 0x30` when a `'request'` listener throws for a request that arrived behind a buffered raw write of the socket. 1.4.2 stays alive. Top frame: `NodeHTTPResponsePrototype__onDataGetCachedValue`. - Such a response is queued, and the connection has no current response. `mark_dispatch_threw_if_queued` (`src/runtime/server/NodeHTTPResponse.rs`, from oven-sh#43557) compared it with the socket's current-response slot. An empty slot meant "not queued", so the dispatch tail ended a response that has no connection. ### Fix - `mark_dispatch_threw_if_queued` reads `Flags::CURRENT` of the response itself. The constructor and `grant_connection` set that bit, and `take_back_connection` clears it. - The thrown dispatch follows the rule for a queued response: nothing of it goes out, and the connection closes at its turn. - Verified: `test/js/node/http/node-http.test.ts`, a new block of 20 tests. All fail on a debug ASAN build of main and pass with the change. The 11 tests of the block for pipelined requests also pass. Windows runs only the TLS half (Notes). ### Background - The server socket grants the connection to one response at a time. A response without the grant is queued, and its `res.socket` is `null`. - `Flags::DISPATCH_THREW_WHILE_QUEUED` tells `_http_server.ts` to close the connection when the queued response gets its turn. - Considered treating an empty slot as queued. The response's own bit already answers that question. ### Downsides - None found on the normal path. The one caller is the exception arm of the dispatch (`src/runtime/server/mod.rs:1485`), where one flag test replaces a call into C++. - Node v26.3.0 keeps such a connection open. Bun closes it, as it does for a pipelined request whose dispatch throws. <details><summary>Notes</summary> **Repro.** A `'connection'` listener writes 8 MB to the raw socket. The client does not read yet and sends its first request. The `'request'` listener throws, and an `'uncaughtException'` handler keeps the process alive. ```js const http = require("node:http"), net = require("node:net"); process.on("uncaughtException", e => console.log("uncaught:", e.message)); const server = http.createServer((req, res) => { res.on("error", () => {}); console.log("request", req.url, "| res.socket === null:", res.socket === null); throw new Error("the listener threw"); }); server.on("connection", s => { s.on("error", () => {}); s.write(Buffer.alloc(8 * 1024 * 1024, "x")); }); server.listen(0, "127.0.0.1", () => { const c = net.connect(server.address().port, "127.0.0.1"); c.pause(); c.on("error", () => {}); let n = 0; c.on("data", d => { n += d.length; }); c.on("connect", () => { c.write("GET /first HTTP/1.1\r\nHost: x\r\n\r\n"); setTimeout(() => c.resume(), 300); }); setTimeout(() => { console.log("the server is alive; the client got", n, "bytes"); process.exit(0); }, 2000); }); ``` Debug ASAN build of main bc7a813, 3 of 3 runs: `res.socket === null: true`, then the report below, exit 1. With the change, 3 of 3 runs: `the server is alive; the client got 8388608 bytes`. Without the raw write the server stays alive on both builds. An outside check on release builds, 3 of 3 runs each: main bc7a813 prints `panic(main thread): Segmentation fault at address 0x30` and exits with 139. **Where it started.** An outside check of the daily builds of main puts the first crash between `37da174d5` (clean) and `5d5f03ff4`, the step that has oven-sh#43557. Official 1.4.2, 1.4.1, 1.4.0 and 1.3.14 stay alive. In those builds the request is not queued (`res.socket === null` is `false`). **The ASAN report on main.** `runtime error: member call on null pointer of type 'JSC::JSCell'` (JSCast.h:182): `NodeHTTPResponsePrototype__onDataGetCachedValue` <- `on_data_get_cached` (NodeHTTPResponse.rs:423) <- `maybe_stop_reading_body` (:721) <- `on_node_http_request_with_upgrade_ctx` (server/mod.rs:1537). **Other differences from Node v26.3.0 in the new tests.** Node sends the `100 Continue` of the `expect-request` mode before the throw, and it answers the `body-to-come` mode with a 400. **Windows.** The Windows loopback takes the whole 8 MiB raw write of a plain TCP socket at once, so the first request is not queued there. Build 122859 showed it: `queued: false` in the ten plain TCP modes on both Windows lanes. The block skips plain TCP on Windows. Over TLS the write stays in the buffer, and the ten TLS modes passed on Windows. Linux and macOS run all 20 modes, and they passed on those lanes of build 122859. </details>
…oven-sh#44414) ### Problem - `bun update <dep> <peer>` exits 1 with `error: util@^1.0.0 failed to resolve` when `<peer>` is an auto-installed peer that nothing else depends on. Each name alone works. - `enqueue_peer_rows` (`src/install/update_transitive.rs:452`) calls `populate_manifest_cache` while dependencies wait for manifests. Its wait runs `run_tasks` in manifests-only mode, which skips their waiter lists (`runTasks.rs:662`, `:1070`). - The opposite mismatch aborts `bun install` over a yarn.lock with an unusable scope registry: `panic: infallible: task queued`. ### Fix - `run_tasks` takes the waiter list of a finished manifest request on every pass. A prefetch request has none. - `print_log` returns `InstallFailed` when the log it resets holds an error. `populate_manifest_cache` starts the progress bar before its wait (`panic: downloads_node active`). - Verified: `bun-update-transitive.test.ts` (18 new cases), `yarn-lock-migration.test.ts` (1). All 19 fail without the fix. Also the update, audit and migration suites. ### Background - A waiter is a dependency that waits for a manifest request. `task_queue` maps request ids to waiters. Only `run_tasks` retires requests. - `populate_manifest_cache` fetches manifests ahead of use (`bun outdated`, bare `bun update`, five more entrances). Its requests have no waiters. - Considered a reorder of the named update: it covers 1 of 7 entrances and keeps both skips and the abort. ### Downsides - A prefetch pass does one `task_queue` lookup per stored manifest: 15 more instructions, no insert, no allocation. - Queued dependencies now resolve inside the prefetch wait. Such a run can print no `Resolving dependencies` line. - After a failed download, `bun update --latest <name>` exits 1 and saves nothing. It exited 0 and saved bun.lock. <details><summary>Notes</summary> **Repro.** A loopback registry has `util@1.0.0` with `peerDependencies: { core: "^1.0.0" }` and `core@1.0.0`. The project depends on `util ^1.0.0`. `bun install`, then `bun update util core`: exit 1, `error: util@^1.0.0 failed to resolve`, nothing saved. The result is the same after `util@1.1.0` and `core@1.1.0` exist, and in either name order. The failure is in every release since 1.4.0 (oven-sh#38333 added `enqueue_peer_rows`). **Why 1.4.2 passed one shape.** When `core` also depends on `util`, 1.4.2 moved both packages. The re-resolved peer made a new `core@1.1.0`, its dependency started a tarball download of a new `util`, and the extract arm of `run_tasks` ran the waiter list of the `util` manifest. oven-sh#43122 removed that download. No revert is needed: the waiter was already dropped, and shapes without that download fail on 1.4.2 too. **Mechanism.** `bun update` does not read the manifest disk cache, so each queued dependency starts a manifest request and parks a `TaskCallbackContext::Dependency` in `task_queue` (`PackageManagerEnqueue.rs:1331`). `populate_manifest_cache` flushes and schedules every queued request and waits until the manager has no pending task. In that wait the old code stored the manifest and took `continue` before the `task_queue` take. `network_dedupe_map` keeps the request id, so nothing asks again. The dependency stays unresolved. **Forms that failed and now pass (each is a test).** Both name orders. A glob next to the peer and `*`. `--latest`. A transitive dependency named next to the peer. The peer named from a workspace member. The peer itself added to package.json. A dependency, an optional dependency, an override or a `file:` folder added to package.json before `bun update <peer>`. `--prefer-offline` with only the peer's manifest cached. A patched dependency named next to the peer. Two forms were silent on main. With an optional dependency added to package.json, `bun update <peer>` exits 0 and saves a bun.lock without it. `bun update <optional-dep> <peer>` exits 0, writes `"util": ""` to package.json and saves a bun.lock with no packages. **`print_log`.** It has two callers, `enqueue_peer_rows` and `plan_edges`. Both print and reset the log in the middle of an install, and `install_with_manager` reads `log.has_errors()` later to decide whether to save. With the waiters delivered inside the prefetch wait, an error of such a dependency reaches the first caller: a tarball 404 or an integrity failure for a dependency that the request does not name. The second caller has the same defect on main: `bun update --latest <name>` runs `plan_edges` after the first resolve wave, so a tarball 404 in that wave prints `error: GET ... - 404`, exits 0 and saves bun.lock and package.json, where `bun update <name>` exits 1 and saves nothing. Two tests pin both callers. Each fails without the guard. **Migration abort.** `Packages::All` returns at the first manifest request it cannot start, after it scheduled earlier ones. `fetch_necessary_package_metadata_after_yarn_or_pnpm_migration` drops that error. The install wait then completed a request with no `task_queue` entry and hit `.expect("infallible: task queued")`. Release build of the merge base: exit 134. This branch: exit 0, bun.lock written. **Progress bar.** `start_manifest_task` starts the bar only when it creates a request. When every name is cached or already requested by a queued dependency, the wait starts with no bar, and the first pass of `run_tasks` names the bar if a manifest download has completed by then. Real timing did not hit the window: 0 panics in 200 pty runs on each build. With the main thread paused for 400 ms before that first pass (gdb), a release build of main panics with `downloads_node active` in 5 of 5 runs, for the peer added to package.json and for `--prefer-offline` with only the peer's manifest cached. This branch: 0 of 25 runs. No test covers it, because it needs a pty and that pause. **Measurements (release builds of the merge base 4b02e10 and of this branch, unless noted).** - `run_tasks_erased`, pass of a normal install: parsed-manifest arm 35 -> 33 instructions from the manifest store to the `process_dependency_list_for_ctx` call, 304 arm 34 -> 33. The compare-and-branch on `manifests_only` is gone at both sites. Whole function: 6517 -> 6448 instructions. - `run_tasks_erased`: 33,102 -> 32,764 bytes. `populate_manifest_cache`: 3,724 -> 3,756 bytes. Release `.text`: 0 bytes delta (80,679,983 both). Stripped binary: 80,848,456 bytes both. These sizes are from the first push. The later `print_log` change adds one compare and one early return. - Prefetch pass with nothing queued (`bun outdated`, 30 dependencies, empty manifest cache): 30 `task_queue` lookups (0 on the merge base), 0 inserts, 0 waiter deliveries. After the manifest store the pass runs 20 instructions for each manifest, 12 of them in `HashMap::get_index`. The merge base runs 5. - Waiters (debug build, gdb): `bun update util core` 1 of 1 delivered once. With `core` depending on `util`: 2 of 2. `bun update '*'` over 40 direct dependencies and 1 peer: 40 of 40 delivered once, 41 manifest requests with no duplicate, 41 packages moved, `bun install --frozen-lockfile` passes. The take in the extract arm found 0 non-empty lists. - Requests of `bun update util core` with both packages newer: 1 `GET /util`, 1 `GET /core`, then the 2 tarballs. `bun update core nope`: 0 requests, same reject text. - stdout, stderr and requests of `bun update util` and of `bun update core`: 0 changed lines. - Event-loop waits entered (`AnyEventLoop::tick_raw`, debug builds, two runs each): `bun update util` 3 and 5 on both builds. `bun update core` 3 and 5 to 6 on the merge base, 3 and 6 on this branch. **Left for later.** - A waiter that is parked on a failed manifest request stays parked in both modes. - The prefetch pass still runs with `install_peer = true`. Only non-peer dependencies can be parked there today. - `MANIFESTS_ONLY` now only keeps the parsed-manifest arm from naming the progress bar. - oven-sh#40284 rewrites the same two hunks and keeps both early exits. oven-sh#43981 adds another `populate_manifest_cache` call inside an install, which this change makes safe in any call order. </details> --------- Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
… module, and require() with a plugin's namespace (oven-sh#44473) ### What does this PR do? A module specifier is resolved once. It was resolved up to three times, each time from the answer before, which is harmless only when the answer about an answer is the same answer. With a symlink, a path from `Module._resolveFilename` or a plugin's `onResolve`, it is not. **A segfault** ```js // esm.mjs: export const who = "esm"; // main.cjs: const Module = require("node:module"); Module._resolveFilename = () => __dirname + "/./esm.mjs"; // or a symlink, "//", "/sub/../", "./esm.mjs" console.log(require("anything").who); ``` ``` panic(main thread): Segmentation fault at address 0x18 ``` **An ordinary plugin that `require()` cannot use** ```js build.onResolve({ filter: /\.virtual$/ }, ({ path }) => ({ path: "from " + basename(path), namespace: "virtual" })); build.onLoad({ filter: /.*/, namespace: "virtual" }, ({ path }) => ({ contents: "export const from = " + JSON.stringify(path), loader: "js" })); ``` ```js require("./x.virtual"); // error: Cannot find package 'virtual:from x.virtual' import "./x.virtual"; // works ``` **The wrong module**: with an `onResolve` that sends `a` to `b` and `b` to `c`, `import("./a.mjs")` loads `d`, an import statement and `require()` load `c`. ### Who resolved again **1. JavaScriptCore's loader.** `loadModule(key)` fetches and registers a record under `key`, then goes through `hostLoadImportedModule`, which calls `resolve(key)`. `loadAndEvaluateModule(name)` and `requestImportModule(name)` each call `resolve` once before that. For its own hosts that is harmless: the `jsc` shell and WebCore turn an absolute path or URL into the same URL every time. The one thing of theirs that is not idempotent is the import map, and for that the hook has a parameter: `useImportMap` is `true` for an import statement and for `import()`, `false` for the name or key of a top-level load. WebCore, when it is `false`, parses the URL and returns. Bun's hook did not read it. When the answer about a key was another key, a second record was registered under that, and that one was linked and evaluated. `functionEsmLoadSync` then looked up the key it had asked for, found the first record, never linked, and asked it for its namespace. **2. `import()`.** `moduleLoaderImportModule` resolved the specifier and handed the loader the answer, and `requestImportModule` resolves what it is handed. WebCore's hands over the specifier and the referrer. **3. The transpiler.** `Linker::link` put every import record that is not a dynamic import through `onResolve` while the file was transpiled, and printed the answer into the code. When the code ran, the loader or `require()` resolved what was printed. Nothing else is resolved while transpiling at run time. Printing the namespace into the path (`PRINT_NAMESPACE_IN_PATH`, "used to prevent running resolve plugins multiple times for the same path"), with a shortcut in the resolve hook that returned any key in a namespace that has an `onLoad`, hid that for one case. `require()` has no such shortcut. ### Fix - `Zig::GlobalObject::moduleLoaderResolve` returns the key when `useImportMap` is `false`. - `JSC::loadAndEvaluateModule(name)` only ever asks with `false`, so it takes a key. That is how WebCore uses it: it parses `<script src>` into a URL itself (`document->encodingParseURL`), and what it hands JSC is called `moduleKey`. Bun handed it names that still had to be resolved. Of its six callers, three hold a key and go on as they were, through the same binding: the entry point (`"bun:main"`), a macro's entry (a generated `macro:` id) and a `bun build --app` config (what the resolver has just answered). Three hold a name: a test file, the entry point under `BUN_DISABLE_TRANSPILER`, and the argument of `Module.runMain`. Those call `JSModuleLoader::resolve` with `true` and then `JSC::loadAndEvaluateModule` with the key; in Rust that is `resolve_and_load_and_evaluate_module_ptr`, beside `load_and_evaluate_module_ptr`. What `BUN_JSC_dumpModuleLoadingState` logs is what it was for a script, a script with `import()`, and executables compiled plain, with `--splitting` and with `--bytecode`; a name costs one more line, for the question about the key that the hook now answers with the key. - Two callers passed `false` for what is a specifier, which did not matter while nobody read it: `Bun.ModuleGraph`'s `import()`, and the static imports of a compiled executable. They pass `true`. - `Bun.ModuleGraph`'s `import()` needs the key before the load starts, so it resolves itself. It handed the key to `requestImportModule`, which resolves what it is handed. It does what that function does after its resolve: `loadModule(key, { Evaluate, Dynamic })`, whose promise is fulfilled with the namespace, and JSC's `ImportModuleNamespace` reaction. - `import()` hands the loader the specifier and the referrer. - `onResolve` is asked when the code runs, and not before. Removed: the block in `Linker::link`; the `PluginResolver` trait and `Linker::plugin_runner` it went through; `PluginRunner::on_resolve`, a second copy of `plugin_runner_on_resolve_jsc` that leaked a buffer for each answer; `PRINT_NAMESPACE_IN_PATH` and what the printer did for it; the shortcut in the hook, with its `FIXME`; the lookup of virtual modules in `moduleLoaderImportModule`, which the hook does. With that block gone, so are `Linker::generate_import_path`, whose other caller passes constants and is a `match` of three arms in place, and `intern`; `externals` and the check of `had_resolve_errors` after the loop were dead before. `VirtualMachine::plugin_runner` was only ever asked whether it was there, which C++ knows: `Bun__hasPlugins` asks the two lists, and `Bun__onDidAppendPlugin`, the field, its reset between isolated test files and the export that C++ called back are gone. `Bun__runOnResolvePlugins` and `Bun__runOnLoadPlugins` took a `target` that C++ did not read and only `Linker::link` varied. `extract_namespace` and `could_be_plugin` were in the bundler's crate for `Linker::link`; they move to `bun_jsc`, where they are used, and `BunPluginTarget` to `JSBundler.rs`, which is all that uses it. What is printed changes, so the version of the transpiler cache goes from 33 to 34. - Asking again also did something that was needed, two things in fact. If the plugin's filter did not match its own answer, the answer went through the resolver: the example in the documentation answers `"./public/images/..."`. If it did, the answer came back and was the key as it stood: that is how a plugin with `filter: /\.virt$/` serves, from its `onLoad`, a path that is not on disk. So `resolve_maybe_needs_trailing_slash` is two functions. The outer one asks `onResolve`, and puts an answer through the inner one, which is what was there without `onResolve`: the limit on length, the builtins, the file resolver, from the same importer. If that finds nothing and an `onLoad` would be called for the answer, the answer is the key. - That a path in a namespace is a key is what the shortcut in the hook was good for. It is a rule of the inner function now, after `onResolve` and for every way to load, and it asks whether an `onLoad` would be called (`Bun__hasOnLoad`), filter included. What a plugin is asked about for a specifier, and whether it is asked at all, was worked out in three places; `plugin_namespace_and_path` is the one place now, so what is taken for a key is what loading will serve. - A test file that does not resolve was a fatal error before JavaScript starts, printed by nothing. It is a rejected promise, as when it does not load. ### Measured Linux x64, canary `7fe13e1b9`, Node 26.7. **Every way to load or resolve.** 13 ways, to an ES module, a CommonJS module and a path that `onResolve` moves into a namespace, with a literal and with a specifier the parser cannot know, with the plugin from a `--preload` and from the file itself: 138 cases. `onResolve` sends `a` to `b`, `b` to `c`, `c` to `d`. Right is `b`, with one call. | | canary | this PR | |---|---|---| | wrong, of 138 | 53 | 12 | The 12 are `import.meta.resolve()`, which does not ask `onResolve` at all. Not changed here. **What Bun loads itself**, under the same plugin: | | canary | this PR | |---|---|---| | a later `--preload`, a test file, `Module.runMain("./a.js")` | `c`, 2 calls | `b`, 1 call | | the entry point, a Worker's | `b`, 1 call | `b`, 1 call | | `Bun.ModuleGraph`'s `import()` | `d`, 3 calls | `b`, 1 call | **What `onResolve` answers**, without a namespace: an absolute path, a relative one, one without its extension (relative and absolute), a directory (both), a package's name, `../`, a file that is missing. Eight ways to load or resolve, 72 cases. | | canary | this PR | |---|---|---| | an import statement, `require("...")`, `require.resolve("...")` (27) | resolved from the importer | the same, all 27 | | `import()` | resolved from the working directory | from the importer | | `require(variable)`, `import.meta.require()` | taken as it is: a segfault for a relative path to an ES module, `ENOENT`, `EISDIR` | from the importer | | `Bun.resolveSync()` | returns the answer as it is | returns what it resolves to | An answer that is missing is `Cannot find module` from the importer, as on canary. Eight more kinds of answer, by an import statement, `require("...")`, `import()` and `require(variable)`: a `file:` URL, the name of a `build.module()` module, `"ns:thing"` where `ns` has an `onLoad`, a builtin with and without `node:`, `bun:sqlite`, a path with a query, a `data:` URL. All load. On canary three of the 32 do not: `require(variable)` with the URL and with `"path"`, `require("...")` with `"ns:thing"`. **An answer that is not on disk**, with `filter: /\.virt$/` on both `onResolve` and `onLoad`, by an import statement, `export * from`, `import()`, `require()` and `require.resolve()`: loads by all five, as on canary, with one call where canary makes two or three. **An answer of 9000 characters**, on Linux (on Windows, where a path can be longer, 200,000): | | canary | this PR | |---|---|---| | `import()` | rejects, `ENAMETOOLONG while resolving` | the same | | an import statement, `require()`, `require.resolve()` | `panic: range end index 9003 out of range for slice of length 4095` | that error | **`bun test a.test.ts b.test.ts`**, with an `onResolve` for `a.test.ts`: | | canary | this PR | |---|---|---| | it answers a path that is missing | an error under `a.test.ts`, `b.test.ts` runs, `1 pass 1 fail 1 error` | the same | | it throws | exits 1 and prints nothing | the error under `a.test.ts`, `b.test.ts` runs, `1 pass 1 fail 1 error` | **On Windows, a file written after Bun has read its directory**, with no plugin. Windows x64, `main` at `e29a7ca47`, each case in its own process: | the path given to `import()` or `require()` | main | this PR | |---|---|---| | `root + "/src/later.mjs"`, or the same with forward slashes only | `Cannot find module` | loads | | `import.meta.dir + "/later.mjs"`, `path.join(...)`, `"./later.mjs"` | loads | loads | `Bun.resolveSync()` answers all five on both: the resolver spells the path with backslashes, and it was the second question, about that spelling, that failed. If that file is a symlink, the resolver answers the path of the link for `root + "/src/link.mjs"`, and the real path for `path.join(...)` (the other spellings were not tried). `Bun.resolveSync()` shows that on `main` and here alike, and it is not changed. `import()` and `require()` load what the resolver answers, where they threw. **`Module._resolveFilename` returning...** | | canary | this PR | Node | |---|---|---|---| | `dir + "/./esm.mjs"`, `"/sub/../esm.mjs"`, `"//esm.mjs"` | segfault | loads | loads | | `"./esm.mjs"` | segfault | loads, with `import.meta.url` `file:///esm.mjs` | loads | | a symlink to the file, a path through a symlinked directory | segfault | loads, apart from the real path's module | the same | | `dir + "/./esm.mts"`, a module that imports another | segfault | loads | loads | | a file that is missing, a directory, a `file:` URL | throws | throws | throws | **The same as canary**: `Module.runMain()` with nine kinds of argument (absolute, relative, no extension, a directory, a symlink, a `.` segment, missing); `--preload`, `--import` and `--require` of eight names of builtins, 24 cases, and a Worker's `preload` of `node:sys`; an entry point through a symlink, through `sub/..` and without its extension, with and without `BUN_DISABLE_TRANSPILER`; a macro through each of those; `import()` of 24 kinds (relative, absolute, `file:` URL, query, hash, builtins, missing, JSON, text, CommonJS, from `eval`, `new Function`, a timer, `node:vm`, a `data:` URL), down to the name, message, `code`, `referrer` and `specifier` of the error; the number of calls of each hook that `BUN_JSC_dumpModuleLoadingState` logs. ### What else changes | | before | this PR | |---|---|---| | a `require("...")` in a branch or a function that does not run | `onResolve` is asked when the file loads | it is not asked | | `onResolve` throws, and the `require()` is in a `try` | the file does not load, nothing to catch | caught | | `onResolve` returns nothing, or a `path` or `namespace` that is not valid | asked twice | asked once, the same message | | `import("ns:thing")`, `require("ns:thing")` where an `onLoad` for `ns` matches and no `onResolve` claims it | `Cannot find package 'ns:thing'` | load, like `import "ns:thing"` | | `import "ns:thing"` where `ns` has an `onLoad` and its filter does not match | `ENOENT reading "ns:thing"` | `Cannot find package 'ns:thing'` | | an `onResolve` in the namespace `bun` that matches `main` | asked about `bun:main` twice when Bun starts | not asked: Bun loads it by its key | | a literal `require("...")` that runs three times | `onResolve` is asked once, when the file loads | three times, like `require(variable)` | | an answer that is missing, from a plugin whose filter matches its own answer and which has no `onLoad` for it | `ENOENT reading` when it loads; `require.resolve()` returns it; `import` of an extension with no loader gives the path | `Cannot find module`, as from a plugin whose filter does not match | One test used the first of those errors to see that an `onResolve` registered while a path is resolving does not apply to that path. It has the late one mark what it resolves instead. Another followed `a` to `d` to get a module registered under a key other than the one asked for; it follows `a` to `b`. **Where a plugin's filter matches its own answer and the resolver finds another file**, the answer was the key on canary. What the resolver finds is the key now, as when the filter does not match and as when the path is written by hand. With a symlink, that is a fix: there is one module, and the flag that is for this decides. | `onResolve` answers a path through a symlink | canary | this PR | |---|---|---| | `onLoad` and `import.meta` get | that path | the real path | | the file is also imported by its real path | `onLoad` is called twice, two modules | once, one module | | with `--preserve-symlinks` | that path | that path | With an extension that the resolver rewrites, it is only a difference: | `onResolve` answers | canary | this PR | |---|---|---| | `dir + "/gen.js"`, which is missing, next to a `gen.ts` | `onLoad` is called for `gen.js` | `gen.ts` is loaded | ### Different from Node, or wrong, and not changed here - `import.meta.resolve()` does not ask `onResolve`. - `onResolve` is not asked about a specifier with no dot and no colon (`"env"`), by any way to load. The same on canary. - On Windows, `extract_namespace` does not take `A:`, `Z:`, `a:` and `z:` for drives: its comparisons leave out both ends. The same on `main`; the function only moved. - `Module.runMain()` with no argument resolves `"undefined"`, where Node takes `process.argv[1]`. The same on canary. - With a relative path from `Module._resolveFilename`, the key is that path, and `import.meta` is made from the key: `url` is `file:///esm.mjs`, `dirname` is empty. Its own imports work. With `/./` and `/../`, `import.meta` is what Node gives. - With `/./`, `/../`, `//` or a relative path from `Module._resolveFilename`, the module is apart from the one of the normalized path, so the file is evaluated again if both are loaded. Node has one, because it keys ES modules by URL. On canary that is so for the spellings that did not crash, and for CommonJS in Node too. ### How did you verify your code works? In `plugins.test.ts`, 38 tests are new or changed, and 31 of them fail on 1.4.2: one call of `onResolve` for each of eight ways to load and for four things Bun loads itself; five ways into a namespace; three ways to load a path in a namespace with no `onResolve`; a `require()` that does not run; a `require()` in a `try`; nine kinds of answer, two of them not on disk, through each of five ways to load; an answer that is a symlink; an answer that only the filter of an `onLoad` matches, which would not be called for it; an answer that is too long; the importer of a module with a query; an empty specifier and a URL that is not one, with a `build.module()` registered; a test file about which `onResolve` throws or answers what is missing; `node:sys` with an `onLoad` in the namespace `node`. In `resolve.test.ts`: `import()` and `require()` of a file written after its directory was read, by two spellings with forward slashes. On Windows x64 both fail on `main` and pass with the binary CI built from this branch; elsewhere they pass on both. In `node-module-module.test.js`: six paths from `Module._resolveFilename`, which fail on 1.4.2, and six arguments of `Module.runMain`. In `preload-test.test.js`: three ways to preload `node:sys`. Those nine pass on 1.4.2: an earlier commit of this PR broke them and nothing noticed. `test/js/bun/plugin`, `node-module-module.test.js`, `preload-test.test.js`, `mock-module.test.ts`, debug: 185 pass, 0 fail. `isolation.test.ts` and `pre-port-identifiers.test.ts`: 63 pass, 0 fail. `module-graph.test.ts` with `isolation.test.ts`, before the last commit: 338 pass, 0 fail. `bun build --no-bundle` of five files that need the runtime, CommonJS interop, decorators, `using` and JSX, for three targets, plain, with `--public-path` and with `--format=cjs`: 45 outputs, the same bytes as canary. Nine of them import the runtime, which the printer spells `"bun:wrap"` whatever the path is, so this says that nothing that shows has changed, not that each arm was taken. After `Bun.plugin.clearAll()`, seven kinds of import give what canary gives. `BUN_JSC_validateExceptionChecks=1` on the tests of answers, of namespaces, of test files and of `Bun.ModuleGraph`, 15 of them: no report. With a plain entry point, and with `Module.runMain` of a path without its extension and of one that is missing: no report.
### What does this PR do? Removes four places where a borrow was widened to `'static`, or stored as a raw pointer, for no remaining reason. No behaviour changes, and none of these was a live bug. - **`FileReader.pending_view`.** A `&'static mut [u8]` into a JS typed array that was only ever written. `resolve_pending_read` gets the buffer from `pending_value` when the read completes, so a detached buffer reads as empty. The field is deleted, `on_pull` takes `&mut [u8]`, and the `unsafe` widening in the trait shim goes with it. - **HPACK `DecodeResult`.** `name` and `value` were `&'static [u8]` into a thread-local C buffer. They now borrow from `&mut self`, so the compiler checks that a caller copies them before its next `decode` or `encode`. The three callers needed no edits. Limit: every `HPACK` on a thread shares that buffer, so the borrow covers one instance only. - **`Resolver::resolve_via_tsconfig_paths`.** It widened `import_path` to `'static` and never used that. The two other widenings in `resolver.rs` are real and stay. - **CSS `StyleSheet.options`.** The sheet kept the whole `ParserOptions`, including `logger`, a raw pointer to a `Log`. In the bundler that is a stack local, and the sheet then moves into the bundle arena, so the pointer dangled. Nothing read it after the parse. The sheet now keeps the three values it reads: `filename`, `css_modules` and `flags`. `parse*` takes `ParserOptions<'_>` instead of `<'static>`, `logger` is private, and the three callers pass `default(Some(&mut log))`. `parse_bundler`'s `ManuallyDrop` + `ptr::read` copy of `options` had no second user and is deleted. ### How did you verify your code works? `cargo check --workspace` and `cargo clippy --workspace` are clean on Linux. Since nothing changes behaviour, there is no test that fails before this. The existing stream, HTTP/2, tsconfig paths and CSS suites cover these paths. Those suites and the other targets are left to CI.
Bumps `WEBKIT_VERSION` from `fb1167ebf2cb` to `1600131e46b5` (current oven-sh/WebKit `main`). The `autobuild-1600131e46b5af48bbda3559af8d8a3327230b6e` release exists. oven-sh/WebKit changes picked up: - [JSC] FTL inline-cache patchpoints must declare fpTempRegister as clobbered (oven-sh/WebKit#536) - [JSC] JSObject::makePropertiesImmutable(): an object's own properties and prototype stop changing, and its property attributes stay as they are (oven-sh/WebKit#759) - [JSC] Module records share an executable only when their sources have the same SourceOrigin and start position (oven-sh/WebKit#639) - [JSC] The check for an untouched StringObject is for the realm of the conversion (oven-sh/WebKit#766) Not built or tested locally; relying on CI.
…sh#44492) ### What does this PR do? Fixes a regression from oven-sh#44473, which is in no release. In a project with no `node_modules` directory, Bun [auto-installs](https://bun.com/docs/runtime/auto-install) a package it does not find. Since oven-sh#44473, what a runtime plugin's `onResolve` answers without a namespace goes through the resolver, and auto-install came along. So a plugin that serves a virtual module under a bare name has that name asked of the registry first, and a package of that name is downloaded and run in place of the plugin's module. ```js Bun.plugin({ name: "virtual", setup(build) { build.onResolve({ filter: /^virtual-.*\.js$/ }, ({ path }) => ({ path })); build.onLoad({ filter: /^virtual-.*\.js$/ }, () => ({ contents: "export default 1", loader: "js" })); }, }); await import("virtual-thing.js"); // GET <registry>/virtual-thing.js ``` ### What told the two apart before Before oven-sh#44473, `onResolve` was asked again about its answer. If it answered, that was the key and the resolver was not involved: the name was the plugin's own. If it declined, or no filter matched, the answer went through the resolver, auto-install included: that is how a plugin redirects to a package. No test of the filters stands in for that, and two were tried in this PR. "The filter of an `onLoad` matches" takes `"some-ui-lib/Button.svelte"` for the plugin's own when there is an `onLoad` for `/\.svelte$/`, which is a transform. "The filter of an `onResolve` matches" takes every redirect for the plugin's own when the filter is `/.*/`, as in the example in the documentation, whose callback declines what it does not know. ### Fix An answer that is a bare name, which is the only kind that can reach the registry, is put to `onResolve` once more. If it answers, the name is the plugin's own and is resolved with `GlobalCache::disable`, which is `--no-install`. What it answers is not used otherwise, but for one that is not valid, which is the error it is the first time. An answer that is the specifier is not asked about: what would be said is known. An absolute or a relative answer, or one in a namespace, is not asked about again, as oven-sh#44473 has it. `resolve_and_auto_install` already takes the mode. `VirtualMachine::_resolve` read it from the options; its one caller passes it now. `disable`, not `read_only`: tried, `read_only` asks the registry for the manifest, downloads and runs a package that is not in the cache, and lets a package in the cache take the place of the plugin's module. ### Measured Linux x64. A registry on the loopback that serves version 1.0.0 of whatever it is asked, whose code says that it ran. A project with no `node_modules`. "Before" is canary `7fe13e1b9`, which does not have oven-sh#44473. Thirteen kinds of answer, by an import statement, `import()`, `require()` and `require.resolve()`, each with an empty global cache and with one that already holds a package of every name used: 104 cases. **What the registry is asked (nothing, the manifest, the tarball) is what it was before oven-sh#44473 in all 104.** | `onResolve` | its answer | before oven-sh#44473 | `main` | this PR | |---|---|---|---|---| | answers the specifier with itself | a bare name, served by an `onLoad` or not | not asked | **asked** | not asked | | answers another name, and that name with itself | a bare name, served or not | not asked | **asked, and the package's code runs** | not asked | | its filter does not match its answer | `"real-package"`, `"real.package"`, `"real-package/index.js"` | installed and run, or found in the cache | the same | the same | | its filter does not match its answer | `"some-ui-lib/Button.svelte"`, with an `onLoad` for `/\.svelte$/` | installed, and the `onLoad` gets the file | the same | the same | | its filter is `/.*/`, and it declines all but one specifier | the four above | the same as the two rows above | the same | the same | | its filter does not match its answer | a bare name nothing serves | installed and run | the same | the same | | its filter does not match its answer | a bare name an `onLoad` serves | installed and run, in place of the plugin's module | the same | the same | 88 of the 104 are the same in what they print too. The other 16 are a name that is the plugin's own and that nothing serves, where the registry is not asked then or now: `Cannot find package` since oven-sh#44473, and before it `ENOENT reading`, or the name itself from `require.resolve()` and from an import of an extension with no loader. ### Not changed The last row: a plugin that serves a bare name which its own `onResolve` would not answer about has the registry asked first, in 1.4.2 as well. Nothing tells it from the row of `"some-ui-lib/Button.svelte"`: in both, the filter of `onResolve` does not match the answer and that of an `onLoad` does. Not installing what an `onLoad` would be called for is what an earlier commit of this PR did, and it broke that row. ### What else changes For a bare answer that a filter of `onResolve` matches and that is not the specifier, the callback runs twice, where oven-sh#44473 made it once and 1.4.2 has two or three times. The documentation and the comment in `bun.d.ts` say so. ### How did you verify your code works? Twelve tests in `plugins.test.ts`, with a registry on the loopback in the test's process. Each asserts the whole list of what the registry was asked, which always has an import that no plugin answers about, to show that auto-install is on in that project, and every call of `onResolve`. - Eight, one for each way to load or resolve (an import statement, `import()`, `require()`, `import.meta.require()`, `require.resolve()`, `import.meta.resolve()`, `Bun.resolveSync()`, `Bun.resolve()`): a name answered with itself and a name answered for another specifier, both served, are not asked of the registry. - One: nor are they when nothing serves them. - One: a redirect to a package is asked of the registry, also when the filter of an `onLoad` matches it, and when the filter of an `onResolve` that declines does. - One: an answer in a namespace that has an `onResolve` of its own is not put to it. - One: what `onResolve` says about the bare name is an error if it is not valid. They pass on 1.4.2, which does not have the regression, so: with the condition made false, which is `main`, the first ten fail; with the fix, they pass. The last two are about the second question itself: they fail on the commit that added it and pass on the next. `test/js/bun/plugin`, `node-module-module.test.js` and `mock-module.test.ts`, debug: 193 pass, 0 fail. `BUN_JSC_validateExceptionChecks=1` on these tests, those of what `onResolve` answers and those of how often it is asked, 34 of them: no report.
…ng the streams of a collected rewrite (oven-sh#43379) ### Problem - `element.onEndTag(fn)` makes `fn` a GC root (`ProtectedJSValue` in `EndTagHandler`, `src/runtime/api/html_rewriter.rs`). If `fn` reaches the output `Response` and the rewrite stops early, 200 of 200 Responses stay. - Main also uses the streams of a collected rewrite, in four places. `cancel_from_output()` and `abandon_suspension()` close a dead JS input (`SEGV` in `JSReadStreamIntoSinkOperation::result()`, or `ASSERTION FAILED: status() == Status::Pending`). `abandon_suspension()` writes to a freed output (`heap-use-after-free` in `ByteStream::on_data`). `Bun.ModuleGraph.dispose()` cancels a stream source that is dead, or that is freed under the call (a segfault on a release build with no options). The leak hid the first. ### Fix - `onEndTag` callbacks live in a JS array in `endTagHandlers`, a new visited slot of the transform cell. The lol-html handler keeps an index. - `cancel_from_output()` cuts the output's edge to the transform cell last, as `fail()` and `finish()` already do. - The pipe's reference to its own cell is a `JsRef` instead of a bare `JSValue`, so a dead, unswept cell reads as `None`. - The two entries that nothing reachable makes, the `abandon_suspension()` task and `NewSource`'s `on_abort` (`dispose()`), hold the wrapper that they read for the call, and leave a collected one to its finalizer. - Verified: 23 new tests (`test/js/workerd/html-rewriter-leak.test.ts`, `module-graph-gc.test.ts`). 11 fail on a debug build of main, 13 on release ASAN. Other suites: Notes. Self-reviewed: 3 concerns raised, 3 addressed. ### Background - `gcProtect` makes a value a GC root, so a cycle through it is never garbage. A visited slot is an edge that the collector traces. - The transform cell (`HTMLRewriterTransform`) keeps the input stream of one `transform()` alive. The native pipe holds that stream as a raw pointer. - Considered the cell in the `PipePin` guard of every entry point (an earlier version of this PR), and a `Strong` on the input (a root per rewrite). Notes say why not. ### Downsides - `Element` grows from 40 to 48 bytes, `RewriterPipe` from 272 to 304, the release binary by 8,960 bytes. - A handler without `onEndTag()` costs the same within noise (150 ns per element, base-to-base difference up to 1.2 ns). With it, 41 to 48 ns less. - Two leaks without `onEndTag()` remain (Notes). <details><summary>Notes</summary> **Report.** There is no GitHub issue. oven-sh#43210 measured the leak and left it for a follow-up, and its review asked for it: oven-sh#43210 (comment). This is the last `ProtectedJSValue` in `html_rewriter.rs`. **Why a visited slot.** `REVIEW.md` ("Root or copy every JSValue held beyond the current call") asks for WriteBarrier members declared in `.classes.ts`, and keeps `protect` for a justified self-keepalive. oven-sh#43210 applied that to the `handlers` slot. The third shape below also needs it: a rewrite whose input never ends reaches no terminal state, so only collection of the transform cell can free it, and a root prevents that collection. **Repro of the leak.** `MODE` is `ok`, `throw` or `cancel`. The `</div>` end tag never arrives. ```js const { heapStats } = require("bun:jsc"); let fired = 0; const fr = new FinalizationRegistry(() => fired++); const N = 200; const mode = process.env.MODE ?? "throw"; async function once() { const holder = {}; const rw = new HTMLRewriter() .on("div", { element(el) { el.onEndTag(() => holder.res); } }) .on("p", { element() { if (mode === "throw") throw new Error("boom"); } }); let ctrl; const res = rw.transform(new Response(new ReadableStream({ start(c) { ctrl = c; } }))); holder.res = res; fr.register(res, 0); const chunk = new TextEncoder().encode("<div><p>x</p>"); if (mode === "cancel") { const reader = res.body.getReader(); ctrl.enqueue(chunk); await reader.read(); await reader.cancel(); } else { ctrl.enqueue(chunk); ctrl.close(); try { await res.text(); } catch {} } } for (let i = 0; i < N; i++) await once(); for (let i = 0; i < 6; i++) { Bun.gc(true); await Bun.sleep(20); } console.log(mode, "retained", N - fired, "of", N, "protected Function", heapStats().protectedObjectTypeCounts.Function ?? 0); ``` **Measurements of the leak** (debug builds, retained Responses and protected Functions of 200, graphs of 15): | shape | main (367d939) | this PR | | --- | --- | --- | | rewrite completes | 0, 0 | 0, 0 | | a later handler throws, the body is read | 200, 200 | 0, 0 | | the output reader cancels | 200, 200 | 0, 0 | | the input never ends, all dropped, nothing pending on the output | 200, 200 | 0, 0 | | disposed `Bun.ModuleGraph` whose code left such a rewrite | 15 graphs | 1 graph | | the same graph code without `onEndTag()` (control) | 1 graph | 1 graph | Release builds of the merge base 37471e5 and of this PR give the same result for `throw` and `cancel`: 200 of 200, and 0 of 200. On main the heap snapshot of the new ModuleGraph test names the retainer: `root(ProtectedValues) Function -> JSLexicalEnvironment -> JSModuleEnvironment -> JSLexicalEnvironment -Variable:moduleGraph-> ModuleGraph`. **The dead input, case 1: `cancel_from_output()`.** An earlier version of this PR failed on the x64 ASAN lane: the new test "when the output reader is cancelled" aborted with `ASSERTION FAILED: status() == Status::Pending`. The cause is on main. `cancel_from_output()` starts with `detach_output()`, which clears the `owner` slot of the output stream. If script holds only the reader, that slot was the last GC path to the transform cell. The next step allocates the abort reason, so a collection can finish there. `detach_input_source()` then closes the sink controller of the JS input through `SourceHandle::JSController`, a raw pointer to a cell that only the transform cell kept alive. On main the protected `onEndTag` callback kept such a rewrite alive forever, so the window was closed for exactly the rewrites that the leak tests make. Main has the bug with no `onEndTag()` call. This script requests a collection before each `reader.cancel()`: ```js const encoder = new TextEncoder(); const N = 200; let cancelled = 0; for (let i = 0; i < N; i++) { let controller; const input = new ReadableStream({ start: c => void (controller = c), cancel() { cancelled++; } }); const reader = new HTMLRewriter().on("div", { element() {} }).transform(new Response(input)).body.getReader(); controller.enqueue(encoder.encode("<div><p>x</p>")); controller = undefined; if ((await reader.read()).done) throw new Error("done early"); Bun.gc(false); await reader.cancel(); } Bun.gc(true); console.log(JSON.stringify({ rewrites: N, cancelled })); ``` | release ASAN build | main | this PR | | --- | --- | --- | | plain run, 3 runs | `cancelled` is 195, 194, 200 of 200 | 200 of 200 | | `BUN_JSC_collectContinuously=1` | `SEGV on unknown address 0x000000000010` | 200 of 200 | The stack of the SEGV: `JSC::ClassInfo::isSubClassOf` < `JSReadStreamIntoSinkOperation::result()` < `Bun::WebStreams::rsisFinish` < `jsWebStreamsHandler_onReadStreamIntoSinkClose` < `JSC::runInternalMicrotask`. The failure needs a collection that finishes between `detach_output()` and the close of the input. It did not occur on a debug build or on a release build without ASAN. The regression test runs the script under `BUN_JSC_collectContinuously=1` on release builds (not on Windows, where that option is very slow), so it fails on main only on the release ASAN lanes. **The fix for case 1.** `detach_output()` moves to the end of `cancel_from_output()`, next to `release_input_roots()`. The reader that cancels then reaches the cell through the `owner` slot until the input is closed. This is the order of `fail()` and `finish()`, and the rule that the doc comment of `release_input_roots()` already states for the input's edges. Alternatives: - Keep the cell on the stack in the `PipePin` of `write`, `end_from_stream`, `resume`, `cancel_from_output` and `fail` (an earlier version of this PR). Each of those is entered by a peer whose own cell has an internal edge to the transform cell (`owner`, `sinkOwner`, the Response's `transform` slot, the context of a promise reaction). The guard mattered where the pipe itself cut that edge too early (this case), and where the first caller of the chain held nothing (cases 3 and 4). It does nothing for a cell that is already dead at the entry, so that version still crashes in case 4. It also made `EnsureStillAlive` a struct field. Everywhere else in the tree it is a local. - A `Strong` on the controller in `SourceHandle::JSController`. That is one more root per rewrite with a JS input, and a root is what made the leak. **The dead input, case 2: `abandon_suspension()`.** A review of this PR found it. It is on main too. A handler returns a promise that never settles, and script drops everything. The collector then frees the promise, and the destructor of its native context queues `abandon_suspension`. That task asked `cell.is_cell()` to learn if the transform cell is alive. The pipe's `cell` field was a bare `JSValue` that the cell's finalizer clears, and the finalizer runs at the sweep. A cell that is dead but not swept yet still passes `is_cell()`, so the task called `fail()`, and `fail()` closed the dead sink controller. That field is a hand-written weak reference. `JsRef` is the one the rest of the runtime uses for a native object's own wrapper (the stream sources next to this pipe do), and since oven-sh#39334 its `try_get()` gives `None` for a dead, unswept cell, which is the answer of `JSC::Weak::get()`. The field is now a `JsCell<JsRef>`, and every read of it goes through `try_get()`. The task clears the input and output handles without a call into them when that is `None`, as it already did for a swept cell. ```js const N = 50; const encoder = new TextEncoder(); const tick = () => new Promise(resolve => setImmediate(resolve)); let cancelled = 0; for (let i = 0; i < N; i++) { let controller; const input = new ReadableStream({ start: c => void (controller = c), cancel: () => void cancelled++ }); new HTMLRewriter().on("p", { element: () => new Promise(() => {}) }).transform(new Response(input)); controller.enqueue(encoder.encode("<p>x</p>")); } for (let i = 0; i < 5; i++) await tick(); for (let round = 0; round < 10; round++) { Bun.gc(false); const junk = []; for (let j = 0; j < 500; j++) junk.push({ j }); await tick(); } process.stdout.write(JSON.stringify({ rewrites: N, cancelled })); ``` Run it as a file. A release build of main exits with `panic(main thread): Segmentation fault at address 0x0` in 10 of 10 runs. A debug build of main stops at `ASSERTION FAILED: decontaminate()`. A release ASAN build of main reports the same `SEGV` as case 1, with this stack: `JSReadStreamIntoSinkOperation::result()` < `rsisFinish` < `pumpOnClose` < `sinkControllerOnClose` < `SourceHandle::cancel` < `RewriterPipe::fail` < `RewriterPipe::abandon_suspension`. This PR prints `{"rewrites":50,"cancelled":0}` on all three builds. Bun 1.4.2 does not crash on this script. oven-sh#37108 fixed the opposite direction, a controller destructor that reached a freed pipe. **The freed output, case 3: `abandon_suspension()` again.** It is on main too. The handler's promise is collected while script still holds the `Response`, so the task is queued for a reachable rewrite. Script drops the `Response` before the task runs. The cell is then unreachable, but no collection has found that out, so it reads as alive. `fail()` allocates (the error, the abort of the input), and a collection in there sweeps the cell and both streams. `fail()` then writes the error through `output`, a raw pointer to the freed `ByteStream`. This entry is a task that a destructor queued, so nothing on its stack reaches the cell. It keeps the cell that it read in a local `EnsureStillAlive` until it returns. ```js const encoder = new TextEncoder(); const tick = () => new Promise(resolve => setImmediate(resolve)); const responses = [], promises = []; function start() { let controller; const input = new ReadableStream({ start: c => void (controller = c) }); const response = new HTMLRewriter() .on("p", { element() { const p = new Promise(() => {}); promises.push(p); return p; } }) .transform(new Response(input)); response.body; responses.push(response); controller.enqueue(encoder.encode("<p>x</p>")); } for (let round = 0; round < 20; round++) { for (let i = 0; i < 4; i++) start(); await tick(); promises.length = 0; Bun.gc(true); responses.length = 0; await tick(); } ``` With `BUN_JSC_slowPathAllocsBetweenGCs=25` (a full collection at every 25th slow-path allocation) a release ASAN build of main reports the `heap-use-after-free` in 10 of 10 runs, and also for each of 7, 13, 20, 33, 50, 64, 80, 100 and 150. This PR exits with 0. A debug build and a release build without ASAN of main do not report it. **The dead or freed source, case 4: `Bun.ModuleGraph.dispose()`.** It is on main too, and needs no `onEndTag()`. `dispose()` cancels the source of every stream that the graph's script was given (`NewSource`'s `on_abort`, oven-sh#42590), from native code. The JS wrapper owns the source, and nothing on that stack reaches a wrapper that script has dropped. `AbortHandleOwner` has a `KeepAlive` type for what keeps the owner alive while `on_abort` runs. `NewSource` declared none. - A collection that finishes inside `cancel()` frees the source under the call, together with the rewrite that feeds it. - After a collection that finished before, the wrapper is dead and waits for its sweep. `cancel()` then tells the rewrite, which closes its dead input. `NewSource` now keeps its wrapper (`this_jsvalue.try_get()`) alive for the call. For a wrapper that is collected and not swept yet it does nothing: the peers are collected too, and the finalizer releases the source. A source whose wrapper is finalized while native refs remain (a child's pipe that nobody reads) is cancelled as before. ```js const TICKS = 0; // or 1 const encoder = new TextEncoder(); const tick = () => new Promise(resolve => setImmediate(resolve)); const rewrite = input => new HTMLRewriter().on("div", { element() {} }).transform(input); function start(chained) { for (let i = 0; i < 20; i++) { const input = new ReadableStream({ start: c => void c.enqueue(encoder.encode("<div>x")), cancel() {} }); const output = rewrite(new Response(input)); (chained ? rewrite(output) : output).body; } } for (const chained of [false, true]) { for (let i = 0; i < 20; i++) { const graph = new Bun.ModuleGraph(); graph.run(() => start(chained)); await tick(); Bun.gc(false); for (let t = 0; t < TICKS; t++) await tick(); graph.dispose(); } } ``` Run it as a file, with no options. 5 runs for each value of `TICKS`: | build | `TICKS = 0` | `TICKS = 1` | | --- | --- | --- | | main, release | segfault, 5 of 5 | segfault or abort, 5 of 5 | | main, debug | `ASSERTION FAILED: decontaminate()`, 5 of 5 | the same, 5 of 5 | | main, release ASAN | `decontaminate()` or `SEGV`, 5 of 5 | `SEGV`, 5 of 5 | | the version of this PR with the cell in `PipePin`, release ASAN | `ASSERTION FAILED: result`, 5 of 5 | `SEGV` or `decontaminate()`, 5 of 5 | | this PR, release ASAN | passes, 5 of 5 | passes, 5 of 5 | **Each of the changes is needed.** Release ASAN builds, the repros of cases 1 to 3: | build | case 1 (`slowPathAllocsBetweenGCs=1`, 10 rewrites) | case 2 | case 3 | | --- | --- | --- | --- | | main | `cancelled` is 0 of 10 | `SEGV` | `heap-use-after-free` | | this PR with only the `JsRef` | `cancelled` is 0 of 10 | passes | `heap-use-after-free` | | this PR | 10 of 10 | passes | passes | **Why `Element` has a back reference.** `on()` handlers find their list through the pipe whose lol-html call is on the stack. `onEndTag()` cannot: after an `await` in an async handler no lol-html call is on the stack, and inside a handler of a nested, synchronous rewrite the pipe on the stack is the inner one. Both cases work on main and have a test here. `handler_callback` gives the pipe to the wrapper when it makes it. `Element::invalidate()` clears it together with the lol-html pointer. That happens when the handler returns, or when the pipe drops the wrapper it parked, so the reference never outlives the pipe. **Replacement.** A second `onEndTag()` on the same element replaces the first callback, in one handler or across two `on()` handlers. That is the behavior of main (`handlers.clear()` then push). lol-html gives every matching handler the same `Element`, and a suspension moves the user data with the parked copy. So the first call pushes the one lol-html handler and records its slot as the element's user data, and a later call only stores the new callback into that slot. **A parked element that outlives its transform cell.** A handler returns a promise, the element is parked, then the promise and the cell are collected together. Until the queued `abandon_suspension` task runs, script can still call `onEndTag()` on the parked element. The cell is then swept, or dead and not swept yet. In both states `try_get()` is `None`, and `hold_end_tag_callback` stores nothing. It also stores nothing for a parked element whose rewrite was cancelled or failed. In all of these the end tag can never come. The last test in the file covers the collected case. **Return value.** Unchanged: the element while it is attached, `null` once it is detached. **Sizes.** `size_of` in the release profile, merge base then this PR: `Element` 40, 48 (one per element handler call, freed when the handler returns). `RewriterPipe` 272, 304 (one per `transform()`, alignment 16). `EndTagHandler` 24, 16. The transform cell gets one more `WriteBarrier` (8 bytes). The first `onEndTag()` call on an element now boxes a 4-byte index as the lol-html user data, and no longer adds an entry to the VM's table of protected values. `size` of the linux-x64 release binaries: text 80,681,674, then 80,690,634. Data and bss do not change. **Speed.** Release builds of the merge base and of this PR, linux-x64, pinned to one core. One process rewrites a document of 20,000 elements with one element handler, and reports the best of 40 runs in nanoseconds per element. 25 rounds run the processes in turn: base, PR, base again. The table gives the minimum of the rounds, and the median in parentheses. The third column is the same base binary, which shows the noise. | case | base | this PR | base again | | --- | --- | --- | --- | | `<p>x</p>` x 20,000, handler only | 150.1 (154.9) | 150.9 (155.7) | 151.3 (157.7) | | `<p>x</p>` x 20,000, handler calls `onEndTag()` | 298.3 (306.2) | 257.6 (266.8) | 298.0 (306.7) | | `<div>` nested 50 deep x 400, handler only | 150.1 (154.2) | 150.5 (154.4) | 150.1 (154.5) | | `<div>` nested 50 deep x 400, handler calls `onEndTag()` | 322.1 (331.4) | 274.6 (280.6) | 326.8 (332.7) | `try_get()` is one call into C++ per handler call. The version of this PR without it measured 151.0, 259.8, 151.9 and 273.8 in the same rounds. **Not changed.** Each of these keeps a rewrite alive on main and on this PR with no `onEndTag()` call at all (100 rewrites each): - A failed rewrite keeps the handler's error in the output body as a `Strong` until the body is read (`fail()`). If that error references the output `Response` (for example `error.response = res`) and nothing reads the body, 100 of 100 Responses stay. With the body read, 0 stay. An error that does not reference the Response retains nothing. - A `read()` that is pending on the output while the JS input stream can never produce again keeps 100 of 100 Responses, through the protected promise and buffer of the pending pull. A pending `.text()` leaves 100 protected Promises (`promise_value.protect()`, `src/runtime/webcore/Body.rs:453`). - A rewrite that fails or is cancelled keeps its lol-html rewriter (the parser arena) until the transform cell is collected. Only `finish()` frees it earlier. A parked wrapper can still point into it, so an earlier free needs its own change. **Tests.** 23 new tests. On a debug build of main 11 fail: the three leak shapes, four of the five kept-Response shapes, the parked rewrite that dies with its handler promise, the two `dispose()` tests of case 4, and the ModuleGraph test. On a release ASAN build of main the cancel test under continuous collection and the test of case 3 fail too. The other tests guard the new storage and pass before the fix: the callback stays alive until its end tag with nothing else referencing it (GC churn in between, half of the outputs dropped), slot reuse under nesting, one end tag that closes several elements, callbacks that run after a handler cancelled the output earlier in the same chunk, replacement (one handler, two handlers, across a suspension), `onEndTag()` after an `await`, an outer element used in a nested rewrite, an indexed accessor on `Array.prototype`, the parked element whose rewrite was collected, and the kept-Response case of a rewrite that completes. **Suites run on the debug (ASAN) build:** `test/js/workerd/html-rewriter-leak.test.ts` (49 pass, 1 skipped in debug), `test/js/bun/module-graph/module-graph-gc.test.ts` (33 pass), `test/js/workerd/html-rewriter.test.js` (186 pass), `test/js/workerd/html-rewriter-end-error.test.ts`, `test/js/web/html/html-rewriter-doctype.test.ts`, `test/regression/issue/htmlrewriter-additional-bugs.test.ts`, `test/regression/issue/text-chunk-null-access.test.ts`, `test/js/bun/http/serve-stream-body-error.test.ts`, and the HTMLRewriter cases of `test/js/web/workers/worker-terminate-lifetime.test.ts` and `test/js/bun/module-graph/module-graph-isolation.test.ts`. On the release ASAN build: the leak file with the CI environment of the ASAN lane (`BUN_JSC_validateExceptionChecks=1`, `BUN_GARBAGE_COLLECTOR_LEVEL=1`) and `html-rewriter.test.js`, 3 runs. Also run by hand, on the earlier version with the cell in `PipePin`: a nested document with `Bun.gc(true)` in handlers and callbacks under `BUN_JSC_collectContinuously=1`, and `terminate()` of workers that hold pending callbacks in each state. **A test of this file that is slow on main.** "HTMLRewriter does not leak element/document handler allocations" passes 15 s on a loaded release ASAN build, on main and on this PR. oven-sh#44359 and oven-sh#44377 change that test. **Self-review.** The review of the first commit asked for the ModuleGraph test, for the narrower wording of the measured shapes, and for the "Not changed" list. All three are in. </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/module-graph/module-graph-gc.test.ts <!-- robobun:evidence:end --> --------- Co-authored-by: Dylan Conway <dylan.conway567@gmail.com>
…etes (oven-sh#42350) ### Problem - Regression from oven-sh#42181, not released. `tls.connect({ socket: duplex })`, then `end()` before the handshake completes: `'finish'` fires, but the Duplex's `final()` never runs, so the peer sees no FIN. A healthy server then completes the handshake and the session stays open forever. Node v26.3.0 ends the Duplex at once. - `UpgradedDuplex::shutdown` (`src/runtime/socket/UpgradedDuplex.rs:551`) ran `SSL_shutdown` and nothing else. Only the close callback ended the transport, which needs the peer's close_notify or a `destroy()`. Mid-handshake, BoringSSL's `SSL_shutdown` returns 1 and does nothing. ### Fix - `shutdown()` sends the close_notify (none mid-handshake), then ends the write side of the transport. `us_internal_ssl_shutdown` does this on an fd, node's `TLSWrap::DoShutdown` on a stream. - `end()` in the turn that created the socket arrives before the engine exists and was dropped. It now sets `pending_shutdown`. `drain_pending` replays it after the staged input: first flight, then `end()`. - Correct because the engine keeps reading, and the `writableEnded` probe in `call_write_or_end` drops handshake output that follows. - Verified: `test/js/node/tls/node-tls-connect.test.ts`. 7 new cells fail on main, 6 assert the same report on node v26.3.0. Self-reviewed: 16 concerns raised, 13 addressed, 3 need no change. Remaining gaps: notes. ### Background - Bun has three TLS engines. `openssl.c` drives a socket on its fd. `UpgradedDuplex` runs an `SSLWrapper` over a `stream.Duplex` through `duplex.write()` and `duplex.end()`. `WindowsNamedPipe` does the same over a pipe. Only `UpgradedDuplex` changes. - node:net's `_final`, the shutdown hook of the writable side, calls `handle.shutdown()`: `UpgradedDuplex::shutdown` here. `'finish'` follows its callback. - The engine for a Duplex starts on a later event-loop turn. `drain_pending` replays what arrived before. <details><summary>Notes</summary> **Reports.** The fixture `test/js/node/tls/tls-shutdown-before-handshake-fixture.mjs` runs on both runtimes. "same turn" means the call is made in the turn that created the socket, before bun's engine exists. "after first flight" means the engine wrote its ClientHello and waits. | cell | node v26.3.0 | main | this branch | | --- | --- | --- | --- | | `end()`, stalled peer, same turn | ClientHello, `final()`, FIN, `finish` | `finish`, no FIN (times out) | same as node | | `end()`, stalled peer, after first flight | same | `finish`, no FIN (times out) | same as node | | `destroySoon()`, after first flight | `final()`, FIN, `finish`, `close` | transport destroyed, no `final()` | same as node | | `destroySoon()`, same turn | ClientHello, `final()`, FIN, `finish`, `close` | transport destroyed, no ClientHello, no `final()` | unchanged, see below | | `end()`, healthy TLS server, same turn | server: `tlsClientError ECONNRESET`, client closes | server: `secureConnection`, session left open | same as node | | server-side `TLSSocket` over a Duplex, `end()` same turn | `final()`, FIN | no FIN (times out) | same as node | | server-side, ClientHello already buffered | aborts: `ERR_INTERNAL_ASSERTION` in `JSStreamSocket.doWrite` | no FIN | server flight, then FIN (bun-only cell) | | `end()` after the handshake, server does not answer the close_notify | `final()` at once | `final()` never runs | same as node | The healthy-server cell on bun 1.4.3 (before oven-sh#42181): `_final` waited for the handshake, so the log is `secureConnect`, `finish`, close_notify, `final()`, `close`. Late, but closed. oven-sh#42181 removed that wait for the case with no queued write, which is right for the fd engine, where `us_internal_ssl_shutdown` sends the FIN. The stream engine had no equivalent. **Behaviour change after the handshake.** `end()` now ends the transport right after the close_notify. Before, the transport was ended only when the peer's close_notify arrived. With a peer that does not answer (a server with `allowHalfOpen`), the transport was never ended. Node ends it at once. Cell: `duplex-end-established`. **Against a healthy server, client side.** Node's engine also completes its half of the handshake after the FIN and writes its last flight into the ended stream, so node's client emits `secureConnect` and then `ERR_STREAM_WRITE_AFTER_END`. Bun drops that write (the `writableEnded` probe) and emits neither. The cell does not assert those two events. The server side is identical. **Not covered here.** - `destroySoon()` in the creating turn over a Duplex. Committed as an `it.failing` cell for bun. `endNT` (`src/js/node/net.ts:245`) calls the `_final` callback right after `shutdown()`, so `'finish'` fires before the engine exists, `destroySoon()`'s `destroy()` runs, and `_destroy` destroys the transport. Node completes the shutdown in the `stream.end()` callback, so `'finish'` comes after the transport's `final()`. Same on 1.4.3. The same cause puts `'finish'` ahead of the transport's `'finish'` when the transport's write is asynchronous. Gating `'finish'` on the transport is a separate change in `net.ts`. - `WindowsNamedPipe::shutdown` with TLS (`tls.connect({ path: pipe })`) has the same shape: `SSL_shutdown` only. `writer.end()` there closes the whole pipe, read side included, so it needs the `uv_shutdown` half-close from oven-sh#39727 first. - `tlsSocketFinal` in `src/js/node/_http2_upgrade.ts` ends its engine with `handle.end()`, a full close, not `shutdown()`. Not changed. - `end()` in the creating turn when the transport is a `net.Socket` (unflushed writes, named pipe): `Socket.prototype._final` parks on `'connect'`, which an upgraded socket never emits, so `shutdown()` is never reached. oven-sh#42343 clears `connecting` for such a transport and oven-sh#42340 emits the event. `end()` before the fd handle is attached is oven-sh#42339. oven-sh#42343 names the missing FIN on this engine as its open follow-up: that is this change. With the engine started, those transports get the FIN from this change (checked by hand with a net.Socket that has 4 MB of unflushed writes). **Replay order.** The pending shutdown runs after the staged bytes and EOF. A server-side socket that was handed a buffered ClientHello answers it first, so the client gets the server's flight and then the FIN. The first version replayed the shutdown first and dropped that flight. **Suites run with the debug build:** all of `test/js/node/tls/`, `test/js/node/net/`, `test/js/node/http2/`, `test/js/bun/net/socket.test.ts`, `socket-retention.test.ts`, `node-http-connect.test.ts`, `ws.test.ts`, `websocket-proxy.test.ts`, and 223 vendored `test-tls-*`, `test-https-*`, `test-http2-generic-streams*` scripts (222 exit 0). Every failure also fails on a debug build of main: tests that need public DNS or an IPv4 `localhost`, `test-https-timeout.js` (hangs on debug builds with and without this change), and `test-tls-client-allow-partial-trust-chain.js` (a `node:test` file). </details> <!-- robobun:evidence:begin --> --- **[human-review]** gate passed · iteration 1 · 3 files touched <details><summary>fails on main (without fix)</summary> ```console ASAN without fix: 7 failed, 18 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/tls/node-tls-connect.test.ts bun test v1.4.3 (4ff9193) test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [2.90ms] (pass) should thow ECONNRESET if FIN is received before handshake [362.75ms] (pass) initializes authorizationError to null in the TLSSocket constructor [12.06ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [173.06ms] (pass) should be able to grab the JSStreamSocket constructor [21.30ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [120.40ms] (pass) tls.connect > should have peer certificate when using self asign certificate [140.18ms] (skip) tls.connect > should have peer certificate (skip) tls.connect > getCipher, getProtocol, getEphemeralKeyInfo, getSharedSigalgs, getSession, exportKeyingMaterial and isSessionReused should work (skip) tls.connect > should process options correctly when connect is called with only options (skip) tls.connect > should process port ... (truncated) release without fix: 11 failed, 18 skipped bun test v1.4.3-canary.1 (4ff9193) test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [0.04ms] (pass) should thow ECONNRESET if FIN is received before handshake [6.73ms] (pass) initializes authorizationError to null in the TLSSocket constructor [0.18ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [4.02ms] (pass) should be able to grab the JSStreamSocket constructor [0.22ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [6.15ms] (pass) tls.connect > should have peer certificate when using self asign certificate [4.22ms] (skip) tls.connect > should have peer certificate (skip) tls.connect > getCipher, getProtocol, getEphemeralKeyInfo, getSharedSigalgs, getSession, exportKeyingMaterial and isSessionReused should work (skip) tls.connect > should process options correctly when connect is called with only options (skip) tls.connect > should process port and host correctly (skip) tls.connect > should process port, host, and callback correctly (skip) tls.connect > should handle the absence of a callback gracefully (skip) tls.c ... (truncated) ``` </details> <details><summary>passes on PR (with fix)</summary> ```console ASAN with fix: 18 skipped $ BUN_DEBUG_QUIET_LOGS=1 bun scripts/build.ts --profile=debug --quiet test "--reporter=junit" "--reporter-outfile=/tmp/pr_gate.xml" test/js/node/tls/node-tls-connect.test.ts bun test v1.4.3 (4ff9193) test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [3.28ms] (pass) should thow ECONNRESET if FIN is received before handshake [318.81ms] (pass) initializes authorizationError to null in the TLSSocket constructor [8.05ms] (pass) setMaxSendFragment mirrors OpenSSL's [512, 16384] acceptance without throwing [104.40ms] (pass) should be able to grab the JSStreamSocket constructor [13.33ms] (skip) tls.connect > should work with alpnProtocols (pass) tls.connect > Bun.serve() should work with tls and Bun.file() [79.45ms] (pass) tls.connect > should have peer certificate when using self asign certificate [88.70ms] (skip) tls.connect > should have peer certificate (skip) tls.connect > getCipher, getProtocol, getEphemeralKeyInfo, getSharedSigalgs, getSession, exportKeyingMaterial and isSessionReused should work (skip) tls.connect > should process options correctly when connect is called with only options (skip) tls.connect > should process port an ... (truncated) release with fix: 18 skipped $ bun scripts/build.ts --profile=release [configured] bun-profile → bun (stripped) in 612ms (unchanged) ninja: Entering directory `/workspace/bun/build/release' [1/22] gen generated_host_exports.rs generated_host_exports.rs: 122 exports (host=5, lazy=10, generic=107, rust=0); 243 extern-C blocks audited [2/22] gen JS modules (bundle-modules) Preprocess modules (7686ms) Bundle modules (44ms) Postprocesss modules (18ms) Bundle Functions (516ms) Generate Code (28ms) [8.30s] Bundled "src/js" for production 2599 kb 197 internal modules 13 native modules 50 internal functions across 16 files [2/6] cargo bun_runtime → libbun_runtime.a �[1m�[92m Compiling�[0m bun_runtime v0.0.0 (/workspace/bun/src/runtime) �[1m�[92m Finished�[0m `release` profile [optimized + debuginfo] target(s) in 4m 38s [3/6] link bun-profile [5/6] strip bun [5/6] bun-profile --revision 1.4.3-canary.1+75fa7a27d [build] done bun test v1.4.3-canary.1 (75fa7a2) test/js/node/tls/node-tls-connect.test.ts: (pass) should have checkServerIdentity [0.03ms] (pass) should thow ECONNRESET if FIN is received before handshake [6.16ms] (pass) initializes authorizationError to null in the TLSSocket ... (truncated) ``` </details> <details><summary>diff hotspot</summary> ``` src/runtime/socket/UpgradedDuplex.rs | 23 ++- test/js/node/tls/node-tls-connect.test.ts | 129 +++++++++++-- .../tls/tls-shutdown-before-handshake-fixture.mjs | 202 ++++++++++++++++++++- 3 files changed, 335 insertions(+), 19 deletions(-) ``` </details> **gate history** · 1 passed · 0 rejected · iteration 1 <details><summary>evidence per changed file</summary> ``` file reads edits tests src/runtime/socket/UpgradedDuplex.rs 2 8 20 test/js/node/tls/node-tls-connect.test.ts 3 5 18 …t/js/node/tls/tls-shutdown-before-handshake-fixture.mjs 3 3 23 ``` </details> <!-- robobun:evidence:end --> --------- Co-authored-by: Ciro Spaciari MacBook <ciro@anthropic.com>
… fails (oven-sh#44502) ### What does this PR do? `bun build` with source maps on sometimes crashes when the build fails in the link step. It should print the error and exit with code 1. ```js // a.js import { b } from "./b.js"; console.log(b, require("./b.js")); // b.js export const b = await 0; ``` ``` $ bun build --sourcemap=inline a.js panic(main thread): Segmentation fault at address 0x18 ``` Expected, and what the other runs print: ``` error: This require call is not allowed because the transitive dependency "b.js" contains a top-level await ``` The crash is not tied to top-level await. `import { nope } from "./b.js"` (`No matching export`) crashes the same way. It needs `--sourcemap`; without it there were 0 crashes in 40 runs. #### Cause 1. `LinkerContext::link` calls `compute_data_for_source_map`, which puts two tasks per reachable file on the worker pool. The only waits for them are in `generate_chunks_in_parallel`. 2. Every `?` and `return Err` in `link` after that point returns with the tasks still on the pool. The quickest is `return Err(ImportResolutionFailed)` in step 4 of `scan_imports_and_exports`. It comes before the blocking `worker_pool.each` of step 5, which otherwise gives the tasks time to start. 3. `generate_from_cli` then calls `deinit_without_freeing_arena`, which walks `workers_assignments` and reads `Worker.thread` of every entry. 4. At that moment a pool thread that runs its first task of the build is in `SourceMapDataTask::run_*` → `Worker::get` → `get_worker_slow`. It has inserted its `Box::<Worker>::new_uninit()` pointer into the map and has not reached `worker.write(...)`. So the main thread reads a `Worker` that was never written: - In a release build the fresh memory is zero. `thread` reads as `None`, `Worker::deinit` runs on the main thread, and the drop glue of `Option<WorkerData>` follows a null `Box<Define>`. That is the fault at `0x18`. - In a debug + ASAN build the memory is `0xbe…be`. `thread` reads as `Some(0xbebebebebebebebe)` and `Queue::push` faults at `0xbebebebebebebee6`. A debugger stopped at the ASAN fault shows the main thread in `deinit_soon` and pool threads at `ThreadPool.rs:448` and `:451`, inside the `Worker { .. }` literal of `get_worker_slow`, under `run_line_offset` and `run_quoted_source_contents`. `Bun.build` does not crash because its caller waits on the two groups on its error path. `generate_from_cli` and `generate_from_bake_production_cli` run the same teardown without that wait. #### Fix `deinit_without_freeing_arena` frees what the tasks use, so it now waits for them, before anything else. The first thing it did was to run the finalizers of native plugins, which free source text that these tasks read. The two waits in `Bun.build`'s caller are now redundant and are removed. That leaves the two arms of its `match` the same, so they are folded into one call. `source_maps` and the two groups have no user outside the crate any more and become `pub(crate)`. Waiting on a group that is already at zero takes a lock and returns. The fixing lines are the two `wait()` calls in `src/bundler/bundle_v2.rs`. #### Which build errors this covers - **Errors from before the link step** (resolve errors, syntax errors) never schedule these tasks. They did not crash before (0 of 10 each on the unfixed ASAN build). - **Errors from the link step** all leave through the same teardown, so the one wait covers them. - **Two `Ok` returns also skip the waits**: the dependency scanner branch of `generate_from_cli`, and `chunks.is_empty()` in `generate_from_bake_production_cli`. They are covered too. #### Not changed `enqueue_entry_points_*(...)?` in `generate_from_cli`, `generate_from_bake_production_cli` and `run_from_js_in_new_thread` returns before `wait_for_parse`, so an error there would reach the teardown with parse tasks on the pool. Every error I traced on that path is an allocation failure. I found no input that reaches it, so there is no test to write. Waiting there is not a drop-in fix either: `enqueue_entry_item` increments `pending_items` before its two fallible steps, so after such a failure `wait_for_parse` would never return. It is left as it is, and the `SAFETY` comment in `get_worker_slow` names the exception. ### How did you verify your code works? New test `default/TopLevelAwaitForbiddenRequireSourceMapCLI` in `test/bundler/esbuild/default.test.ts`, next to the existing test for this error. That test uses the `Bun.build` backend without source maps, which is why it never saw the crash. The new test runs the CLI build 40 times, four at a time, and expects the error line and exit code 1 from each. It checks the error line because an ASAN crash also exits with code 1. | binary | runs of the test | result | | --- | --- | --- | | release, this commit's parent (272ff43) | 40 | 40 fail | | release, this PR | 40 | 40 pass | | debug + ASAN, parent | 10 | 10 fail (time out: each ASAN report is slow) | | debug + ASAN, this PR (`bun bd test`) | 5 | 5 pass, 0.91 s to 0.96 s each | | Bun 1.4.2 (`USE_SYSTEM_BUN=1`) | 40 | 40 fail | The whole file run against the parent release build five times: 152 pass and this test fails, each time. With `bun bd test` and both new tests: 154 pass, 0 fail. `default/TopLevelAwaitForbiddenRequireSourceMapAPI` covers `Bun.build`, which now relies on the shared wait: 40 builds of the same two files in one subprocess. It passes on main, where the caller has its own wait, so its "before" is this branch with the two shared `wait()` calls removed. There ASAN reports a heap-use-after-free in `compute_quoted_source_contents`, which reads the freed linker graph. | binary | runs of the test | result | | --- | --- | --- | | release, shared waits removed | 40 | 40 fail | | debug + ASAN, shared waits removed | 10 | 10 fail | | release, this PR | 40 | 40 pass | | debug + ASAN, this PR | 10 | 10 pass | Crashes of 40 runs. The two release builds are of the same commit with the same toolchain and differ only by the fix: | build | before | after | | --- | --- | --- | | the two files above, `--sourcemap=inline` | 12 | 0 | | `No matching export`, two files, `--sourcemap=inline` | 10 | 0 | | ten-file graph the crash was found with, `--target=node --sourcemap=inline` | 11 | 0 | | the same with `--splitting` | 12 | 0 | Crashes of 10 runs on the debug + ASAN builds, all with `No matching export`: | build | before | after | | --- | --- | --- | | through `export *` | 9 | 0 | | `--splitting --sourcemap=external` | 10 | 0 | | `--compile --sourcemap=inline` | 9 | 0 | | `--minify --target=bun --sourcemap=linked` | 10 | 0 | Released versions, crashes of 40 runs of the ten-file graph: 1.3.0: 0, 1.4.0: 15, 1.4.2: 10. `Bun.build`, which lost its own wait: 540 failing builds with source maps in three processes, six at once, under ASAN. Before and after, every build rejects with the expected message and there is no report. `bun build --app` with a link error, the other caller without the wait: 15 crashes of 40 runs before and 0 of 50 after on release, 10 of 10 before and 0 of 30 after under ASAN. An independent A/B of the two release builds and the two ASAN builds: - **Failing builds:** 841 combinations of fixture and options. The fixed release build ran them 21,025 times with no crash, no hang, and the same stderr, exit code and output directory as the runs of the unfixed build that did not crash. - **Successful builds:** 433 builds with source maps on, output directories byte-identical. - **Hangs:** no timeout on one CPU (`taskset -c 0`), under `nice -n 19`, under CPU load, or with a graph of 10,000 files. - **Cost:** the two-file failing build has a median 0.13 ms to 0.32 ms higher, where the same binary against itself differed by 0.01 ms to 0.15 ms. Builds of 1,000 and 10,000 files, failing or not, are within that noise.
Fixes oven-sh#17502 ### Problem - `bun test --coverage-reporter=lcov` runs the tests, exits 0, and writes no report. It prints no warning. - The `--coverage-reporter` block of `parse_test_command_options` (`src/runtime/cli/Arguments.rs:1798`) sets `coverage.reporters` and never `coverage.enabled`. ### Fix - That block now sets `coverage.enabled = true`. The help text and `docs/snippets/cli/test.mdx` say `Implies --coverage`. - `--coverage-dir` and the bunfig key `coverageReporter` do not change (see Notes). - Verified: `test/cli/test/coverage.test.ts`, 5 new tests (lcov, the separate-argument form, text, both reporters, `--parallel=2`). The released build fails 5 of 5. A debug build with the change passes them, and the whole file passes (32 tests). ### Background - `bun test` collects coverage only when `coverage.enabled` is true. `--coverage` or `coverage = true` in `bunfig.toml` sets it. - A `--parallel` coordinator starts its workers with `--coverage` when `coverage.enabled` is true (`src/runtime/cli/test/parallel/runner.rs:422`), so the same switch covers that mode. - No other place was weighed: one block parses the flag. oven-sh#20736 and oven-sh#32420 made the same change, and a review of oven-sh#32420 asked for the reporter half only. ### Downsides - A run with `--coverage-reporter` and no `--coverage` now collects coverage. It is slower, it prints the table or writes `coverage/lcov.info`, and a `coverageThreshold` in `bunfig.toml` can now fail it. - `--coverage-dir` with neither flag is still dropped with no message. - Other runs pay nothing: one assignment at argument parsing, only when the flag is present. The help text grows by 20 bytes. <details><summary>Notes</summary> **Rows, before and after.** A directory with `f.ts` and `f.test.ts`: | command | released build | with the change | | --- | --- | --- | | `bun test --coverage` | text table | text table | | `bun test --coverage --coverage-reporter=lcov` | `coverage/lcov.info` | `coverage/lcov.info` | | `bun test --coverage-reporter=lcov` | nothing | `coverage/lcov.info` | | `bun test --coverage-reporter=text` | nothing | text table | | `bun test --coverage-dir=cov2 --coverage-reporter=lcov` | nothing | `cov2/lcov.info` | | `bun test` | nothing | nothing | **Why `--coverage-dir` stays.** Only the lcov writer reads the directory (`src/runtime/cli/test_command.rs:1631-1681`). `bun test --coverage-dir=out` alone would print the text table and never create `out/`. The comparable flags `--cpu-prof-dir` and `--heap-prof-dir` do not turn their switch on. They fail with `must be used with --cpu-prof` (`Arguments.rs:1392`, `:1449`). Whether `--coverage-dir` alone must be an error is an open question. **Why the bunfig key stays.** A reporter that is set in `bunfig.toml` must not turn coverage on for each `bun test` run. `coverage = true` is the key for that. **`bunfig.toml` wins over the flags today.** With `coverage = false` in `bunfig.toml`, `--coverage` does not turn coverage on, and `--coverage-reporter` does not either. With `coverageReporter = "lcov"` there, `--coverage-reporter=text` writes `lcov.info`. oven-sh#40348 (open) changes that order and edits the same function. **Threshold.** With `coverageThreshold = 1.0` and one function that no test calls: `bun test` exits 0, `bun test --coverage` exits 1, and `bun test --coverage-reporter=lcov` now exits 1 too. **Earlier pull requests.** oven-sh#17737, oven-sh#20736, oven-sh#32420. The last one was closed by a cleanup of stale pull requests, with no objection to the change. </details>
…e used (oven-sh#44508) ### What does this PR do? Fixes a `bun build` regression from oven-sh#42695 (not in a release; 1.4.2 is fine). Inside an import cycle through a barrel, a bundle could run a file before the file that ESM runs first. ```ts // index.ts export * from "./first.ts"; export * from "./second.ts"; // first.ts import { two } from "./index.ts"; export function viaSecond() { return two(); } const table = { a: 1 }; export function lookup(key) { return table[key]; } // second.ts import { lookup } from "./index.ts"; export function two() { return 2; } export const atLoad = lookup("a"); ``` ESM runs `first.ts` completely, then `second.ts`. The bundle printed `second.ts` in the middle of `first.ts`: `TypeError: undefined is not an object (evaluating 'table[key]')`. Same with and without `--splitting`. **Cause.** `for_each_edge` turned every cross-file `part.dependencies` entry into an `Edge::Import`, "for a file that the `import` statements did not reach". Every such target is reachable along `import` statements, so the edge only fires when the path goes through a file that is mid-visit, that is in a cycle. There it pulls the file ahead of the part that names it, also when that part only declares a function. The second loop (what a namespace object names) had the same effect on sibling order. **Fix.** - A file is placed by import records only. This holds for every file that runs or holds a value: JavaScript, JSON, SQLite, `.node`. - The dependency edges stay for CSS class names only. That is not about order: with `--splitting` each chunk prints its own copy of the class-name object, and a chunk that names it through a re-export in another chunk has no import record for the CSS file. - Namespace objects (`var exports_x = {}; __export(...)`) print ahead of the files of the chunk, after the runtime and the wrappers. In ESM a namespace object exists before any file runs. The old edges hid this by moving the whole file. This also fixes shapes that 1.4.2 gets wrong. | shape (stdout of the bundle; "ok" = same as the source) | 1.4.2 | main | this PR | |---|---|---|---| | a function / default export / method of `first` names `second`; `second` uses `first` at load | ok | `TypeError` / `ReferenceError` | ok | | the same, cycle in a chunk that two entry points share | `TypeError` (method: ok) | `TypeError` / `ReferenceError` | ok | | `first` calls a function, reads a `var`, or has a static initializer that reads `second` at load | ok | sees `second` already initialized | ok | | a file between the two (`first`, `second`, `third`) | ok | `third first second` | ok | | the namespace object of `first` names `third` | ok | `first third second` | ok | | a function of `first` names a SQLite import that the barrel imports later | ok | the database opens in the middle of `first` | ok | | `first` holds the namespace object of `second` at load | `undefined` | `second` runs first | ok | | a function of `second` reads `third[key]`, `first` calls it at load | `TypeError` | ok | ok | | CSS module named through a re-export in a shared chunk | ok | ok | ok | **Difference from main that stays.** `first` reads a JSON / TOML / asset import that the barrel imports later, at load, directly or through a function of `second`: `undefined`, as in 1.4.2 and as Node prints from source. Main gives the value, and so does Bun from source. Main gets it by printing the file ahead of `first`, which is the bug for a file that runs something. **Output bytes.** They change for any bundle that has a namespace object of a file that is not first in its chunk, not only for cycles: the object moves up and gets its own `// path` line, so `[hash]` names of such chunks move. `happy-dom` is one (its hashes in `bundler_bytecode_portable` are updated: all 7 Linux lanes of the first CI run reported the same three values, and apart from blank lines and `// path` lines the bundle has the same lines as on main). How many real packages change was not measured on this revision. ### How did you verify your code works? - `bun bd test test/bundler/bundler_splitting.test.ts`: 200 pass. 28 new tests, each graph with and without `--splitting`; most compare the bundle with the unbundled run. - `USE_SYSTEM_BUN=1` cannot be the "before" here, because no release has the regression: 1.4.2 passes 17 of the 28. The "before" is a build of main (369615c): 20 fail. - The 8 that pass on main are guards for what an earlier revision of this PR broke: the four CSS tests and one namespace graph (deleting the edges outright), and `NamespaceExportKeepsImportOrderOfJSON` (handling the namespace export part ahead of the `import` statements). - On the first commit of this PR, debug build, all of `test/bundler/*.test.ts`, `esbuild/`, `css/`, `transpiler/react-compiler*` except `bundler_bytecode_portable`, `bundler_compile`, `native-plugin` and `bun-build-api`: 6246 pass, 5 fail in `bun-build-compile`; a debug build of main fails the same 5 (timeouts and a `patchelf` test). - On the first commit, random graphs against the unbundled run (barrels with `export *` / `export {}` / `export * as`, `import * as`, cycles through the barrel, functions that only name other files, values and namespace objects read at load; JavaScript only), 150 graphs per row: | row | graphs that differ from the source: before oven-sh#42695 | main | this PR | |---|---|---|---| | no `--splitting`, nothing wrapped | 11 | 98 | 0 | | no `--splitting`, with `import()` and CommonJS | 31 | 65 | 27 | | `--splitting`, 1 to 3 entry points | 117 | 121 | 73 | No graph is right on main and wrong here. With `--splitting`, pairs of files of one chunk in the wrong order with no other entry point asking for that order: 284 on main, 0 here. What stays wrong is two entry points that need opposite orders of one shared chunk, files in different chunks, and wrapped files. Graphs without barrels (750): same bytes as main. - Not re-run after the last commit: the other bundler suites and the random graphs. Not run locally: release builds, other platforms.
Merge 24 upstream commits while preserving fork module hooks, runtime plugin import kinds, URL identity, process and HTTP/TLS compatibility changes. Adopt resolution-once loading and WebKit 1600131e46b5; remove the superseded prepared-key and transpiler-plugin paths. Allow recognized bun: built-ins in package imports as an explicit Bun extension, retaining Node 24.21 validation for every other target and exports. The 1,334-case W96 oracle preserves baseline results and Node outcomes, codes and messages. Qualify WebKit pin changes with the JSC-sensitive CI families on Linux and macOS. Reconcile cache, opt-in auto-install, data URL and TLS-negotiation fixtures with their preserved runtime contracts. Direct AWS Linux release build and all 133 selected files pass, along with the 97-case hooks oracle and 49 imported hooks tests. Final consumer, branch review and exact-head fork CI gates are recorded in the PR before landing. Upstream contributor history is retained by this merge.
steipete
added a commit
to openclaw/openclaw
that referenced
this pull request
Oct 3, 2026
Exercise deferred import and require through both native and captured loaders. Built-in URL targets remain invalid inside arrays, so the existing filesystem fallback is selected. The fork sync owns restricting its new built-in exception to scalar imports targets in openclaw/bun#87. The full conditions file passes 56/56 on Node 24 and pinned Bun e167; type-aware lint and independent P2 review pass. Production code is unchanged.
steipete
added a commit
to openclaw/openclaw
that referenced
this pull request
Oct 3, 2026
Fix retained package-import aliases and nested dependency capture timing under Bun. Prepare parent capture facts before preview classification, and keep native resolution policy in a sibling module while the generation artifact owns acquisition. This unblocks the OpenClaw prerequisite for openclaw/bun#87. Package-import array fallback semantics remain runtime-agnostic; the sync-only divergence is corrected by restricting the fork's built-in exception to scalar targets. Proof: final conditions 56/56 on Node 24 and checksum-verified Bun e167; unchanged production passed the 13-file sibling set on Node, e167, and the prior sync binary. Build, check-changed, P2 review, exact-head CI, and ClawSweeper pass. Production LOC -2; artifact 698/700. Co-authored-by: Peter Steinberger <steipete@gmail.com>
steipete
marked this pull request as ready for review
October 3, 2026 12:40
This was referenced Oct 3, 2026
github-actions Bot
pushed a commit
to Desicool/openclaw
that referenced
this pull request
Oct 4, 2026
) Fix retained package-import aliases and nested dependency capture timing under Bun. Prepare parent capture facts before preview classification, and keep native resolution policy in a sibling module while the generation artifact owns acquisition. This unblocks the OpenClaw prerequisite for openclaw/bun#87. Package-import array fallback semantics remain runtime-agnostic; the sync-only divergence is corrected by restricting the fork's built-in exception to scalar targets. Proof: final conditions 56/56 on Node 24 and checksum-verified Bun e167; unchanged production passed the 13-file sibling set on Node, e167, and the prior sync binary. Build, check-changed, P2 review, exact-head CI, and ClawSweeper pass. Production LOC -2; artifact 698/700. Co-authored-by: Peter Steinberger <steipete@gmail.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Sync 24 upstream commits from
4b02e1031d6195d96fc0446dfbff49297f89f2d6through7a503a7899dcf12186187c38df9f3c96b3ab9ad4, preserving fork and upstream history. Land with a merge commit, never squash.The original upstream merge is
dea988883c00472bfd0f9a560d4966855cb5d1a4. Follow-ups preserve runtime callback behavior, narrow Bun's package-import exception, and integrate fork main throughec4debf4ee106a1479cd8e02a1ed162e50c5b426. Reviewed head:2ae187ec5311ef342da59e218d8b20c86cbfc5b7; source tree:cbc39168875c7b5c940ef928c4800e4bcf3be2cc.Resulting behavior
Upstream's resolution-once loader replaces transpilation-time plugin callbacks. Runtime plugins retain their original import kinds, decoded filesystem importers, and complete authored file-URL requests. Native URL decoding follows plugin dispatch; the selected redirect owns its query/fragment suffix, including an empty suffix. Invalid URL diagnostics preserve the authored input. Node module hooks retain their metadata, ordering, URL identity, and reentrancy guards.
Only canonical real
bun:built-ins are accepted as scalar string package-import targets outside arrays. Arrays—including nested arrays and conditional targets within arrays—keep Node 24.21 validation and fallback.["bun:sqlite", "./fallback.cjs"]selects the fallback; an invalid target followed by a built-in raisesERR_INVALID_PACKAGE_TARGET. A valid relative target naming a missing file still produces a missing-module error without trying later alternatives. Unknown names, other URL schemes,node:targets, and exports retain Node validation. The compatibility documentation and positive/negative tests cover the boundary.The late-file cache policy from current fork main is retained after removal of
PluginRunner: successful runtime onLoad/onResolve registration enables native missing-file refresh through a small no-JS-call host setter. It does not restore speculative callbacks. The existing snapshot, child-IPC, inline package-scope and restored stack-formatter fixes remain included.Conflict resolutions and superseded duplicates
The original upstream merge had 14 conflicting files. Integrating the later cache fix added a changelog conflict and a modify/delete conflict at the obsolete registration notification. Current main's changelog is an exact prefix; sync notes are append-only.
Upstream oven-sh#44473 supersedes fork #83's prepared-key workaround by eliminating the second resolution; its regression remains. Removing plugin-path rewriting also supersedes #14's transpiler-cache exclusion. Registry fixtures explicitly opt into
--install=auto, keeping the fork's default-off policy. Bare-name onLoad support remains.Two inherited fixture mismatches have controls: the HTTP/2 fixture completes TLS without ALPN before requiring
HTTP2Unsupported(the previous HTTPS setup failed earlier withEPROTOon both binaries); the esbuild data-URL external expectation remains with separate execution coverage for Bun's intentional bundling.WebKit and qualification
WebKit advances
fb1167ebf2cb9edc1f6771a2c11771b024693ae0→1600131e46b5af48bbda3559af8d8a3327230b6e: six commits, no divergence, published prebuilts verified. Pin changes now select the JSC-sensitive Proxy/vm, compile-cache, Intl/Segmenter, regexp/lookbehind, GC/conservative-scan, plugin and module-hook URL suites in both fork lanes.Baseline results are reused only where the e167 binary, consumer source and test selection are unchanged. Private synthetic state and the repository's official test routing are used throughout.
Merge order
OpenClaw #164223 merged first, at 2026-10-03T11:23:35Z, as
e608169ef4fd8ab4599087ecb3933c08f8030f43. The final plugin comparison uses that exact merged revision. It fixes the capture-owner dependency that the old consumer had on speculative callbacks. The native/captured array mismatch is resolved at the Bun target-validation boundary; no synthetic resolve preflight is added.No release is included. Upstream contributor credit remains in imported commits.
CI builds synthetic merge
2964894af3b55b231214070845c7a66d61d4d601; its tree is exactlycbc39168875c7b5c940ef928c4800e4bcf3be2cc, matching the reviewed head and direct Linux source. Both diagnostic artifacts verify the selected tests plus two install prerequisites.