Repository navigation
Conversation
The HTTP thread built the default HTTPS context from the CA options of PackageManager::init and ended the process itself when a CA did not load. The main thread did not wait for that result, so a command that needs no network ended with exit 0 or exit 1 by a race. PackageManager::init now builds the SSL_CTX itself, reports a CA that does not load, and hands the finished context to the HTTP thread. InitOpts carries that context in place of the CA strings and the diverging on_init_error callback.
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 10 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Status Reproduced on release builds of 1.4.3-canary.1+367d939d9 and of main (9b4284f), Linux x64: mkdir /tmp/x && cd /tmp/x
printf '[install]\ncafile = "path/to/cafile"\n' > bunfig.toml
echo '{ "name": "x", "version": "0.0.0" }' > package.json
for i in $(seq 1 200); do
HOME=/tmp/x BUN_INSTALL_CACHE_DIR=/tmp/x/.cache bun install > out.txt 2>&1 || echo "iter $i: exit 1"
rm -rf node_modules bun.lock .cache
done28 of 200 runs exit 1 with PR: #46399 |
|
Updated 8:05 PM PT - Oct 21st, 2026
❌ @robobun, your commit abbc2db has 1 failures in
🧪 To try this PR locally: bunx bun-pr 46399That installs a local version of the PR into your bun-46399 --bun |
|
@Jarred-Sumner @dylan-conway One decision in this PR needs a maintainer. Should a
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline finding, I also checked the cross-thread SSL_CTX handoff: init_once gives the HttpThread its own SSL_CTX_up_ref clone before the spawn, so the main thread dropping http_thread_opts after init returns only releases its own reference; and the is_none() guard in ensure_https_context_init cannot re-enter since init_with_ssl_ctx sets secure unconditionally and runs only on the HTTP thread.
Extended reasoning...
The change moves user-CA SSL_CTX construction from the HTTP thread to PackageManager::init on the main thread, replacing raw pointer InitOpts and the diverging on_init_error callback with an Option; it touches TLS trust-store setup for bun install. The posted inline finding covers the remaining HTTP-thread exit path on Windows; the refcount handoff and lazy-init guard were examined and hold.
Additional findings (outside the current diff — GitHub can't attach inline comments there):
-
🟣
src/http/HTTPThread.rs— Windows users whose environment lacks%SystemRoot%still get the nondeterministic exit this PR removes for CA errors:bun pm pkg setcan leave package.json empty or written with exit 0 or 1 by a race. The check at src/http/HTTPThread.rs:1243 runs on the HTTP thread after spawn and callsGlobal::crash()at :1248 while the main thread keeps writing files; the PR moved only the CA exit to the main thread. Fix: perform every process-ending check that depends only on environment or config on the calling thread beforehttp_thread::initspawns, so no sibling site keeps the HTTP-thread exit race.Why this was flagged
On Windows, a process launched with a sanitized environment (CI runners,
spawnwith a customenvthat omitsSystemRoot, container images) runsbun pm pkg set description=xorbun install.on_startat src/http/HTTPThread.rs:1205 runs on the spawned HTTP thread;getenv_w("SystemRoot")at :1243 returns None andGlobal::crash()at :1248 calls exit_group(1) from that thread while the main thread is in the middle of writing package.json or placing node_modules. This is exactly the mechanism the PR documents for CA errors (392/400 exit 0 with the key written, 1/400 empty package.json on base) and leaves it in place for this sibling input. The check depends only on an environment variable and could run on the main thread before the spawn at src/http/HTTPThread.rs:1177, the same wayInitOpts::with_canow does for CA; REVIEW.md asks for the whole class to be fixed in the same PR. Remedy: hoist the%SystemRoot%check intoinit_onceon the calling thread before the spawn.Verification: The PR description says "Not done: the Windows
%SystemRoot%move". Trigger: Windows process lackingSystemRootrunning any install/pm command. Ongetenv_w("SystemRoot")returningNone,on_start()on the spawned HTTP thread callsbun_core::Global::crash()(src/http/HTTPThread.rs:1248), so the exit races whatever the main thread is writing. The base already fails by this exact route; merging makes nothing worse.
Related to #16103
Problem
ca/cafilethat does not load makes a command without network exit 0 or 1 by a race.bun install(no dependencies) exits 1 withHTTPThread: could not find CA file: '...'in 28 of 200 runs.bun pm pkg setleft package.json empty in 1 of 400.InitOpts.on_init_error(src/http/HTTPThread.rs:1335).Fix
PackageManager::initbuilds theSSL_CTXon the main thread (InitOpts::with_ca) and hands it to the HTTP thread. A bad CA exits 1 there with the same message, before the command prints or writes.test/cli/install/bun-install-registry.test.ts(10 new tests, 9 fail on main),config-precedence.test.ts,escape-control-characters.test.ts.%SystemRoot%move (Notes).Background
bun install. OnlyPackageManager::initpasses CA options.SSL_CTXis the TLS configuration. A user CA replaces its default trust store.initwaits for the HTTP thread's verdict (keeps the callback), or a report at the first request (a bad CA stays silent).Downsides
bun pm binwith a bad CA exits 0 in 99.3% of runs on main, now never. npm ignores a missingcafile(Missingcafileto be a warning instead of error #16103): a maintainer must decide.src/http/HTTPThread.rs:335(default trust store),:1248(Windows%SystemRoot%).Notes
All numbers are from release builds of main (9b4284f) and of this branch on Linux x64 with 12 cores. The measured branch build is e24c6b1b5. The commits after it change comments, docs and tests only.
Question for a maintainer
src/http/HTTPThread.rs:1329-1333on main: "validate now so a bad CA file fails the process at thread start"). The existing tests already pin it for every command that makes a request, also with a plainhttp://registry.cafileto be a warning instead of error #16103 asks for a warning whencafiledoes not exist. npm 11.16.0 ignores such a file. install: fix regressions since 1.4.3 and resolve peer dependencies more like pnpm #45898 made an unreadable.npmrcorbunfig.tomlnon-fatal, "like npm does".bun installwith a missingcafilealready exits 1 in 364 of 400 runs on main. The commands that now fail and mostly passed before are the fast local ones: on mainbun pm binexits 0 in 99.3% of runs,bun pm pkg getin 98.3%,bun removein 96.1%,bun installin an empty project in 86.4%.matcharm inPackageManager::init, plus the missing-cafilerows of the new tests and the two docs paragraphs. That answer must also say which trust store such a run uses: in bun a user CA replaces the default trust store.The race on main
cafile,cafilewith garbage PEM, emptycafile,cafilethat is a directory,--ca not-valid) times eight commands that need no network, in an empty project, 400 runs per cell.pm bin/pm pkg getprint their result and then exit 1.install86.4%,install --dry-run89.0%,install --offline92.4%,link91.0%,pm bin99.3%,pm pkg get name98.3%,remove foo96.1%,pm pack --dry-run2.6%.bun install --cafile <missing>on main ends withexit_group(1)on theHTTP Clientthread in 3 of 5 runs and withexit_group(0)on the main thread in 2 of 5. On this branch:exit_group(1)on the main thread in 5 of 5.Damage on main beyond the exit code (400 runs each,
--cafileof a file that does not exist)bun pm pkg set description=xbun install, 40 workspace packagesbun installinto a newnode_modules, 40 workspace packagesnode_modules, 16 exit 0Cost
bun pm binin a project with no dependencies, 300 runs per arm, interleaved main / branch / main, wall time with process start:--cafile, 1 certificate--cafile, 151 certificates (226 KB)bun pm binwith a validcafileexits on main before anySSL_CTXbuild in 5 of 5 runs.SSL_CTXalone takes 23 to 30 µs, and 2.5 to 2.9 ms for the 151 certificates (tls.createSecureContext({ caFile })in a loop).cafiletwice (SSL_load_client_CA_file, thenSSL_CTX_load_verify_locations). This PR does not change the loader.SSL_CTXbuild and 2 opens of thecafilewith a valid CA, on the main thread in place of theHTTP Clientthread (gdb breakpoint onus_ssl_ctx_build_raw,catch syscall openat). With no CA: 0 builds on both.bun pm bin: no CA 142 on both. Validcafile: 142 on main, 156 (1 certificate) and 271 (151 certificates) on this branch. Futex waits on the main thread: 0 on both.bun installwith no dependencies: 167 to 170 main-thread syscalls on both builds (12 runs each).fetch()call: thehttp_thread::init(&Default::default())site inFetchTasklet::queuegoes from 11 instructions (6 stores) to 4 (1 store).size_of::<InitOpts>()goes from 48 to 8 bytes.cmp+je(main: load, store,cmp,jne). One branch on both.bun-profile:.text81,904,485 -> 81,901,669 bytes (-2,816),.data-64,.bss-64 (llvm-size -A). The strippedbunhas the same file size.Design
InitOptsnow holds onlydefault_ssl_ctx: Option<OwnedSslCtx>. The one diverging function pointer type insrc/(fn(InitError, &InitOpts) -> !) is gone, with both of its call sites.init_oncestores the context in theHTTP_THREADstatic before it spawns the thread, andon_startmoves it intohttps_context. No closure carries it, sounsafe impl Send for InitOptsis deleted and none is added.HttpThread.lazy_https_initis gone: the first-use test readshttps_context.secure.http_thread::initwaits for the HTTP thread's verdict and runs the callback on the calling thread. It keeps the diverging callback type and its first-use call site, and the main thread waits for the thread start as well as the build.Tests
bun installandbun pm binin a project with no dependencies. Each expects an empty stdout, only the message on stderr and exit 1. One test checks thatbun pm pkg setleaves package.json byte for byte as it was. One runsbun pm binwith a--cafilethat loads.cafilenear the path buffer length no longer pass--silent. Its only purpose was to keep a line of the main thread out of a message that another thread printed.config-precedence.test.tsnow expects the message as the whole of stderr for a~/cafile.bun-install-registry.test.ts(498 pass before the last test edits, then its "certificate authority" block again: 32 pass),config-precedence.test.tsandescape-control-characters.test.ts(207 pass), theca/cafiletests ofnpmrc.test.ts,bun-install.test.tsandbun-pm-diff.test.ts, andfetch.tls.test.ts.fetch.tls.test.tshas one failure that main has too in this container (trusts a self-issued certificate without keyCertSign ...).detect_leaks=1), 480 runs ofbun pm bin/bun installwith a valid CA, a bad CA and no CA gave no sanitizer report.cargo checkofbun_httpandbun_installpasses forx86_64-pc-windows-msvc,aarch64-apple-darwinandaarch64-unknown-linux-gnu. Not run: the tests on macOS and Windows.Docs
docs/runtime/bunfig.mdxanddocs/pm/npmrc.mdxnow say when bun loads the certificate and what a CA that does not load does. Each sentence was run on this branch:bun x <name>with a missingcafilein~/.bunfig.tomlexits 0 when the binary is innode_modules/.bin, and exits 1 with the message when it must install.caorcafile, on main or on this branch. With a local HTTPS registry and its certificate incafile,bun addinstalls, andbun --install=force x.jsfails withDEPTH_ZERO_SELF_SIGNED_CERT.cathat holds a PEM key and no certificate is accepted. An emptycais not.cafile, acafilewith garbage PEM, an emptycafileandca=not-validall exit 0. Acafilethat is a directory exits 235.Not covered, and overlap
src/http/HTTPThread.rs:335, the first-use build of the default trust store. It takes no caller input.src/http/HTTPThread.rs:1248, the Windows%SystemRoot%check inon_start. It is the same kind of race for every caller ofhttp_thread::initon Windows. The self-review proposed to move it intoinit_once. I did not: it changes the thread that exits for all callers on Windows, and I cannot run Windows here. A falliblehttp_thread::init, which Report a refused HTTP, bundle, IO or process waiter thread as an error instead of a crash #39864 (open) adds for a thread that cannot start, is the shape that fixes it for every caller.is_main_thread()test from install: gate the exit-callback cache teardown to the main thread #31376 in the exit callback of the package manager stays, for those two exits. No test reaches it now.InitOptswith the CA strings and theon_init_errorcallback, http: stop making two mutable references to the same client or thread state #45476 changesHTTPThread.rsandHTTPContext.rs, Report a refused HTTP, bundle, IO or process waiter thread as an error instead of a crash #39864 changeshttp_thread::init. The one that merges second needs a rebase. If HTTP client: remove unsafe from src/http (h1/h2/h3 client, proxy tunnel, HTTP thread, keep-alive pool) #40385 keeps itson_start, the exit on the HTTP thread comes back and the new tests fail.