test(cli/upgrade): tolerate the canary re-upload 404 and add a hermetic FILE_SHARE_DELETE test#34300
test(cli/upgrade): tolerate the canary re-upload 404 and add a hermetic FILE_SHARE_DELETE test#34300robobun wants to merge 2 commits into
Conversation
…metic The 'zero arguments, should succeed' test set up a mock release server and passed GITHUB_API_DOMAIN pointing at it, but on canary builds (every CI build) bun upgrade constructs a hard-coded https://github.com/oven-sh/bun/releases/download/canary/<zip> URL and never calls get_latest_version, so the mock was never consulted and the test downloaded the real canary archive from github.com on every run. When the canary release is being re-uploaded the asset briefly 404s and the test fails with 'Canary builds are not available for this platform yet', as seen on the darwin-aarch64 lane in build 73614. Split the two concerns the old test mixed: - 'zero arguments, should succeed' is now a pure argument-parser test matching its siblings: the download is routed through a proxy nothing is listening on so it fails locally, and only the package-name error is asserted against. - The Windows FILE_SHARE_DELETE coverage moves to a new hermetic test that serves both the release JSON and the archive from a local server via --stable, and holds the temp-dir handle for the entire upgrade run (it was previously released immediately after Bun.spawn returned, so the handle was already closed by the time the child touched the temp dir).
|
Warning Review limit reached
Next review available in: 3 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
Comment |
|
Updated 12:55 AM PT - Jul 16th, 2026
❌ @robobun, your commit 81c36ce has 2 failures in
🧪 To try this PR locally: bunx bun-pr 34300That installs a local version of the PR into your bun-34300 --bun |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — test-only hermetic-ness fix that mirrors the existing sibling tests in this file. The one nit (lowercase proxy vars) is optional and can't reintroduce the flake given the narrow assertion.
Extended reasoning...
Overview
This PR touches a single test file, test/cli/install/bun-upgrade.test.ts, to fix a flake caused by the zero arguments, should succeed test silently downloading the live canary release from github.com on every CI run. The fix splits the test into (1) a pure arg-parser check that routes the download through a dead proxy, and (2) a new dedicated FILE_SHARE_DELETE test that uses --stable so GITHUB_API_DOMAIN is honored and the mock server is actually consulted. No src/ changes.
Security risks
None. Test-only change; the new test uses a local TLS mock server with the same tls fixture and NODE_TLS_REJECT_UNAUTHORIZED=0 pattern already used by the two sibling tests directly below it.
Level of scrutiny
Low-to-moderate. This is test infrastructure with no runtime impact, and the new FILE_SHARE_DELETE test is structurally near-identical to the existing recreates the staging directory and verifies the downloaded release archive tests in the same file (same assetNames loop, same mock-server shape, same --stable + GITHUB_API_DOMAIN + ASAN_OPTIONS env setup). The root-cause analysis in the PR description is detailed and verifiable — GITHUB_API_DOMAIN is indeed only read on the --stable path, so the old test's mock was dead code on canary CI builds.
Other factors
- The new test correctly moves outside
describe.concurrent()(globalopenTempDirWithoutSharingDeletestate) and holds the handle in atry/finallyfor the full child lifetime, fixing the prematurecloseTempDirHandle()the PR description calls out. - The
apiHits > 0assertion guards against the mock silently going unused again — the exact failure mode that let the old test pass for years while hitting github.com. - Resource management is sound:
using server,await using procscoped inside the try block so disposal precedescloseTempDirHandle(). - The one inline nit (lowercase
https_proxy/NO_PROXYprecedence) is real but cannot cause a flake: the assertion only checks that the arg-parser error line is absent, which holds regardless of whether the download fails, times out, or succeeds. Worth the four-line follow-up but not blocking. - Verified passing on linux-x64 and windows-x64 debug per the PR description.
…y re-upload 404 - 'zero arguments' runs the real canary upgrade against a copied binary again (end-to-end coverage), with the temp-dir handle held for the whole run, and only backs off the broad 'error:' assertion when stderr carries the specific 'Canary builds are not available for this platform yet' 404 that marks the canary release re-upload window. - the hermetic FILE_SHARE_DELETE test now counts the archive download separately and asserts both metadata and download hits, so a failure before filesystem.tmpdir() no longer satisfies the assertions. - clear both casings of the proxy env vars and NO_PROXY in the hermetic test so an ambient proxy on the runner cannot divert the mock traffic.
There was a problem hiding this comment.
Thanks for the follow-up — the proxy-env concern from my earlier note is now moot (the dead-proxy approach was dropped from zero arguments, and the new hermetic FILE_SHARE_DELETE test clears both the lowercase and uppercase proxy vars plus no_proxy). Deferring to a human to sign off on the final shape: the latest commit walked back to keeping zero arguments as a live github.com download (now tolerating the canary 404 window) while also adding the hermetic --stable variant, so both tests hold the FILE_SHARE_DELETE handle and the PR title/description ("stop … depending on the live canary release", "No external network") no longer quite match what ships.
Extended reasoning...
Overview
Test-only change to test/cli/install/bun-upgrade.test.ts. Two commits: 14b23e3 made zero arguments, should succeed hermetic via a dead-proxy override and split the Windows FILE_SHARE_DELETE coverage into a new hermetic --stable test that serves both the release JSON and the archive from a local TLS server. 81c36ce (responding to my earlier note) then dropped the dead-proxy approach in favour of keeping the live canary download but tolerating the "Canary builds are not available for this platform yet" 404 window, and applied the lowercase/no_proxy env clearing to the new hermetic test with a tightened { apiHits: 1, downloadHits: 1 } assertion.
Security risks
None. Test-only; no auth, crypto, or permission surface touched. NODE_TLS_REJECT_UNAUTHORIZED: "0" is scoped to a subprocess talking to a self-signed local server, matching neighbouring tests in the same file.
Level of scrutiny
Moderate. It's test infrastructure with no production impact, but it encodes a design choice about whether bun-upgrade.test.ts should keep a live-network end-to-end check. The final revision keeps the ~100 MB live canary download on every CI run (same as before the PR) and only adds a conditional skip for the specific 404 window that caused #11482 — which is fine, but the PR title and description still describe the earlier fully-hermetic approach, so a human should confirm this is the intended trade-off.
Other factors
- My prior inline nit is stale: the
HTTPS_PROXY: "http://127.0.0.1:1"block it referenced no longer exists; the concern was addressed by clearing all six proxy vars in the new hermetic test instead. - Both
zero arguments(insidedescribe.concurrent) and the new top-level test callopenTempDirWithoutSharingDelete()/closeTempDirHandle(), which back onto a process-global handle. They don't overlap (top-levelitruns after the concurrent block completes, and no other test in the concurrent block touches the helper), but it's worth a human glance to confirm the double coverage is intentional rather than an artefact of the mid-PR direction change. - The new hermetic test mirrors the shape of the two existing mock-server tests below it (same asset-name matrix, same
ASAN_OPTIONSleak-detection opt-out,exitCode === 1on the bogus zip), and the author reports all 9 tests pass on linux-x64 and windows-x64 debug.
robobun
changed the title
|
The diff is ready: |
1 participant
Problem
test/cli/install/bun-upgrade.test.tswent red on the darwin-aarch64 lane in build 73614:Cause
The
zero arguments, should succeedtest starts a local TLS server and passesGITHUB_API_DOMAIN=<mock>so the upgrade command would fetch release metadata from it. ButGITHUB_API_DOMAINis only read insideget_latest_version, and that function is only called on the--stablepath. On canary builds (every CI build hasIS_CANARY = true)bun upgradeconstructs a hard-codedhttps://github.com/oven-sh/bun/releases/download/canary/<zip>URL and downloads it directly, so the mock is never consulted and the test pulls the real canary archive from github.com on every run. Confirmed by request-counting the mock: it sees zero hits on a canary build.Build 73614 ran 03:57 to 06:17 UTC on 2026-07-16; the
bun-darwin-aarch64.zipasset on the canary release was re-created at 06:03:07 UTC. The test caught the brief 404 window between the old asset being removed and the new one appearing. The mock server was introduced in #10387 but the canary path predates it and has always been a direct download.Two adjacent issues surfaced while investigating:
closeTempDirHandle()is called immediately afterBun.spawn()returns, so the WindowsFILE_SHARE_DELETEhandle is released before the child process has done anything. The originalspawnSyncversion held it for the whole run; the async rewrite (bf1e492) inadvertently dropped that guarantee.browser_download_urlat an external R2 host, so even on the--stablepath the archive download left the machine.Fix
Keep the live end-to-end check (it is the only thing that exercises the full download/extract/verify/install path against a real release), but stop the one known non-bug transient from turning CI red, and add a hermetic backstop for the Windows temp-dir path.
zero arguments, should succeed: drop the mock server (dead code on canary builds), hold the WindowsFILE_SHARE_DELETEhandle for the entire child run instead of releasing it right afterBun.spawn(), and only back off the broadexpect(stderr).not.toContain("error:")when stderr carries exactly theCanary builds are not available for this platform yet404 that marks the canary release re-upload window. Any othererror:still fails the test. The argument-parser assertion is always enforced.completes the download when the OS temp dir is held open without FILE_SHARE_DELETE: a hermetic variant that passes--stablesoGITHUB_API_DOMAINis honored, serves both the release JSON and the archive from a local server (with ambient proxy env vars cleared in both casings), holds the temp-dir handle for the whole run, and asserts the mock saw exactly one metadata and one archive request. This covers the temp-dir staging path deterministically so it stays exercised even when the live test has to back off its broad assertion.Both tests intentionally wrap the upgrade in the
FILE_SHARE_DELETEhandle: the live test matches what #10387 originally set out to cover, the hermetic test guarantees the path is reached regardless of external network state.No
src/changes; this is a test-only robustness fix. The stash-src fail-before check does not apply.Verified: all 9 tests in the file pass on linux-x64 (debug) and windows-x64 (debug).
Fixes #11482