Repository navigation
Conversation
When a tarball download fails (connection error or HTTP error), the error was logged but `any_failed_to_install` was not set to true. This caused `bun install` to exit with code 0 despite errors being printed. Now sets `any_failed_to_install = true` when a required network task fails during tarball download, ensuring the process exits with code 1. Fixes #2766 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
|
Updated 6:05 PM PT - Jan 14th, 2026
❌ Your commit 🧪 To try this PR locally: bunx bun-pr 26101That installs a local version of the PR into your bun-26101 --bun |
WalkthroughAdds a failure tracking flag to PackageManager that records when tarball downloads fail, enabling proper error exit codes. Includes a regression test validating that failed tarball downloads result in non-zero exit status for bun install. Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (6)**/*.zig📄 CodeRabbit inference engine (CLAUDE.md)
Files:
src/**/*.zig📄 CodeRabbit inference engine (src/CLAUDE.md)
Files:
**/*.test.ts?(x)📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/regression/issue/*.test.ts📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/**/*.test.ts?(x)📄 CodeRabbit inference engine (CLAUDE.md)
Files:
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}📄 CodeRabbit inference engine (test/CLAUDE.md)
Files:
🧠 Learnings (17)📓 Common learnings📚 Learning: 2026-01-05T16:32:07.551ZApplied to files:
📚 Learning: 2025-12-16T00:21:32.179ZApplied to files:
📚 Learning: 2025-12-16T00:21:32.179ZApplied to files:
📚 Learning: 2026-01-05T23:04:01.518ZApplied to files:
📚 Learning: 2025-11-24T18:36:59.706ZApplied to files:
📚 Learning: 2025-10-19T02:44:46.354ZApplied to files:
📚 Learning: 2025-12-16T00:21:32.179ZApplied to files:
📚 Learning: 2026-01-05T23:04:01.518ZApplied to files:
📚 Learning: 2026-01-05T23:04:01.518ZApplied to files:
📚 Learning: 2025-12-16T00:21:32.179ZApplied to files:
📚 Learning: 2025-12-16T00:21:32.179ZApplied to files:
📚 Learning: 2025-10-08T13:48:02.430ZApplied to files:
📚 Learning: 2025-10-26T01:32:04.844ZApplied to files:
📚 Learning: 2025-11-06T00:58:23.965ZApplied to files:
📚 Learning: 2026-01-14T21:08:10.407ZApplied to files:
📚 Learning: 2026-01-14T21:08:10.407ZApplied to files:
🧬 Code graph analysis (1)test/regression/issue/02766.test.ts (1)
🔇 Additional comments (6)
✏️ Tip: You can disable this entire section by setting Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/install/PackageManager/runTasks.zig (1)
592-621: Consider: Extraction failures may not setany_failed_to_install.The extraction failure path (when
task.status == .fail) logs an error but doesn't setany_failed_to_install = truein the else branch (lines 608-619). While this PR focuses on download failures, extraction failures could similarly result in a zero exit code if no callback is provided.This may be outside the scope of this PR (issue
#2766specifically mentions download failures), but worth noting for completeness.♻️ Optional: Set failure flag for extraction failures
} else { manager.log.addErrorFmt( null, logger.Loc.Empty, manager.allocator, "{s} extracting tarball from <b>{s}<r>", .{ `@errorName`(err), alias, }, ) catch |e| bun.handleOom(e); + manager.any_failed_to_install = true; }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/install/PackageManager/runTasks.zigtest/regression/issue/02766.test.ts
🧰 Additional context used
📓 Path-based instructions (6)
**/*.zig
📄 CodeRabbit inference engine (CLAUDE.md)
In Zig code, be careful with allocators and use defer for cleanup
Files:
src/install/PackageManager/runTasks.zig
src/**/*.zig
📄 CodeRabbit inference engine (src/CLAUDE.md)
src/**/*.zig: Use the#prefix for private fields in Zig structs, e.g.,struct {#foo: u32 };
Use Decl literals in Zig, e.g.,const decl: Decl = .{ .binding = 0, .value = 0 };
Place@importstatements at the bottom of the file in Zig (auto formatter will handle positioning)
Never use@import()inline inside functions in Zig; always place imports at the bottom of the file or containing struct
Files:
src/install/PackageManager/runTasks.zig
**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.test.ts?(x): Never usebun testdirectly - always usebun bd testto run tests with debug build changes
For single-file tests, prefer-eflag overtempDir
For multi-file tests, prefertempDirandBun.spawnover single-file tests
UsenormalizeBunSnapshotto normalize snapshot output of tests
Never write tests that check for 'panic', 'uncaught exception', or similar strings in test output
UsetempDirfromharnessto create temporary directories - do not usetmpdirSyncorfs.mkdtempSync
When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Do not write flaky tests - do not usesetTimeoutin tests; instead await the condition to be met
Verify tests fail withUSE_SYSTEM_BUN=1 bun test <file>and pass withbun bd test <file>- tests are invalid if they pass with USE_SYSTEM_BUN=1
Test files must end with.test.tsor.test.tsx
Avoid shell commands likefindorgrepin tests - use Bun's Glob and built-in tools instead
Files:
test/regression/issue/02766.test.ts
test/regression/issue/*.test.ts
📄 CodeRabbit inference engine (CLAUDE.md)
Place regression tests for specific GitHub issues in
test/regression/issue/${issueNumber}.test.tswith real issue numbers only
Files:
test/regression/issue/02766.test.ts
test/**/*.test.ts?(x)
📄 CodeRabbit inference engine (CLAUDE.md)
Always use
port: 0in tests - do not hardcode ports or use custom random port number functions
Files:
test/regression/issue/02766.test.ts
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}
📄 CodeRabbit inference engine (test/CLAUDE.md)
test/**/*.test.{ts,js,jsx,tsx,mjs,cjs}: Usebun bd test <...test file>to run tests with compiled code changes. Do not usebun testas it will not include your changes.
Usebun:testfor files ending in*.test.{ts,js,jsx,tsx,mjs,cjs}. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, usebun bd <file>instead ofbun bd test <file>since they expect exit code 0.
Do not set a timeout on tests. Bun already has timeouts built-in.
Files:
test/regression/issue/02766.test.ts
🧠 Learnings (17)
📓 Common learnings
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
📚 Learning: 2026-01-05T16:32:07.551Z
Learnt from: alii
Repo: oven-sh/bun PR: 25474
File: src/bun.js/event_loop/Sigusr1Handler.zig:0-0
Timestamp: 2026-01-05T16:32:07.551Z
Learning: In Zig codebases (e.g., Bun), treat std.posix.sigaction as returning void and do not perform runtime error handling for its failure. The Zig standard library views sigaction failures as programmer errors (unreachable) because they only occur with invalid signals like SIGKILL/SIGSTOP. Apply this pattern across Zig files that call sigaction (e.g., crash_handler.zig, main.zig, filter_run.zig, process.zig) and ensure failures are not handled as recoverable errors; prefer reaching an explicit unreachable/compile-time assumption when such failures are detected.
Applied to files:
src/install/PackageManager/runTasks.zig
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to test/regression/issue/*.test.ts : Place regression tests for specific GitHub issues in `test/regression/issue/${issueNumber}.test.ts` with real issue numbers only
Applied to files:
test/regression/issue/02766.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Verify tests fail with `USE_SYSTEM_BUN=1 bun test <file>` and pass with `bun bd test <file>` - tests are invalid if they pass with USE_SYSTEM_BUN=1
Applied to files:
test/regression/issue/02766.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun:test` for files ending in `*.test.{ts,js,jsx,tsx,mjs,cjs}`. For test files without .test extension in test/js/node/test/{parallel,sequential}/*.js, use `bun bd <file>` instead of `bun bd test <file>` since they expect exit code 0.
Applied to files:
test/regression/issue/02766.test.ts
📚 Learning: 2025-11-24T18:36:59.706Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: src/bun.js/bindings/v8/CLAUDE.md:0-0
Timestamp: 2025-11-24T18:36:59.706Z
Learning: Applies to src/bun.js/bindings/v8/test/v8/v8.test.ts : Add corresponding test cases to test/v8/v8.test.ts using checkSameOutput() function to compare Node.js and Bun output
Applied to files:
test/regression/issue/02766.test.ts
📚 Learning: 2025-10-19T02:44:46.354Z
Learnt from: theshadow27
Repo: oven-sh/bun PR: 23798
File: packages/bun-otel/context-propagation.test.ts:1-1
Timestamp: 2025-10-19T02:44:46.354Z
Learning: In the Bun repository, standalone packages under packages/ (e.g., bun-vscode, bun-inspector-protocol, bun-plugin-yaml, bun-plugin-svelte, bun-debug-adapter-protocol, bun-otel) co-locate their tests with package source code using *.test.ts files. This follows standard npm/monorepo patterns. The test/ directory hierarchy (test/js/bun/, test/cli/, test/js/node/) is reserved for testing Bun's core runtime APIs and built-in functionality, not standalone packages.
Applied to files:
test/regression/issue/02766.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Never use `bun test` directly - always use `bun bd test` to run tests with debug build changes
Applied to files:
test/regression/issue/02766.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Applies to test/**/*.test.{ts,js,jsx,tsx,mjs,cjs} : Use `bun bd test <...test file>` to run tests with compiled code changes. Do not use `bun test` as it will not include your changes.
Applied to files:
test/regression/issue/02766.test.ts
📚 Learning: 2026-01-05T23:04:01.518Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/CLAUDE.md:0-0
Timestamp: 2026-01-05T23:04:01.518Z
Learning: Organize regression tests for specific issues in `/test/regression/issue/${issueNumber}.test.ts`. Do not place regression tests in the regression directory if there is no associated issue number.
Applied to files:
test/regression/issue/02766.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : When spawning processes in tests, expect stdout before expecting exit code for more useful error messages on test failure
Applied to files:
test/regression/issue/02766.test.ts
📚 Learning: 2025-12-16T00:21:32.179Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-12-16T00:21:32.179Z
Learning: Applies to **/*.test.ts?(x) : Use `normalizeBunSnapshot` to normalize snapshot output of tests
Applied to files:
test/regression/issue/02766.test.ts
📚 Learning: 2025-10-08T13:48:02.430Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 23373
File: test/js/bun/tarball/extract.test.ts:107-111
Timestamp: 2025-10-08T13:48:02.430Z
Learning: In Bun's test runner, use `expect(async () => { await ... }).toThrow()` to assert async rejections. Unlike Jest/Vitest, Bun does not require `await expect(...).rejects.toThrow()` - the async function wrapper with `.toThrow()` is the correct pattern for async error assertions in Bun tests.
Applied to files:
test/regression/issue/02766.test.ts
📚 Learning: 2025-10-26T01:32:04.844Z
Learnt from: Jarred-Sumner
Repo: oven-sh/bun PR: 24082
File: test/cli/test/coverage.test.ts:60-112
Timestamp: 2025-10-26T01:32:04.844Z
Learning: In the Bun repository test files (test/cli/test/*.test.ts), when spawning Bun CLI commands with Bun.spawnSync for testing, prefer using stdio: ["inherit", "inherit", "inherit"] to inherit stdio streams rather than piping them.
Applied to files:
test/regression/issue/02766.test.ts
📚 Learning: 2025-11-06T00:58:23.965Z
Learnt from: markovejnovic
Repo: oven-sh/bun PR: 24417
File: test/js/bun/spawn/spawn.test.ts:903-918
Timestamp: 2025-11-06T00:58:23.965Z
Learning: In Bun test files, `await using` with spawn() is appropriate for long-running processes that need guaranteed cleanup on scope exit or when explicitly testing disposal behavior. For short-lived processes that exit naturally (e.g., console.log scripts), the pattern `const proc = spawn(...); await proc.exited;` is standard and more common, as evidenced by 24 instances vs 4 `await using` instances in test/js/bun/spawn/spawn.test.ts.
Applied to files:
test/regression/issue/02766.test.ts
📚 Learning: 2026-01-14T21:08:10.407Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/js/node/test/parallel/CLAUDE.md:0-0
Timestamp: 2026-01-14T21:08:10.407Z
Learning: Node.js compatibility tests pass by exiting with code 0
Applied to files:
test/regression/issue/02766.test.ts
📚 Learning: 2026-01-14T21:08:10.407Z
Learnt from: CR
Repo: oven-sh/bun PR: 0
File: test/js/node/test/parallel/CLAUDE.md:0-0
Timestamp: 2026-01-14T21:08:10.407Z
Learning: Run Node.js compatibility tests using `bun bd <file-path>` command, not `bun bd test <file-path>`
Applied to files:
test/regression/issue/02766.test.ts
🧬 Code graph analysis (1)
test/regression/issue/02766.test.ts (1)
test/harness.ts (1)
tempDir(277-284)
🔇 Additional comments (6)
test/regression/issue/02766.test.ts (4)
1-3: LGTM!Imports are appropriate for the test. Uses
bun:testandharnessutilities as expected.
14-51: LGTM!The mock registry setup correctly uses
port: 0to avoid hardcoded ports, and properly simulates a tarball download failure by returning 404 for.tgzrequests while serving valid package metadata.
53-55: LGTM!Proper cleanup of the mock server after each test.
57-85: LGTM!The test properly:
- Uses
usingfor automatic temp directory cleanup- Uses
await usingfor process cleanup- Gathers stdout/stderr before checking exit code via
Promise.all- Makes appropriate assertions for the failure scenario
Per coding guidelines, ensure this test fails with
USE_SYSTEM_BUN=1 bun test test/regression/issue/02766.test.tsand passes withbun bd test test/regression/issue/02766.test.tsto validate the fix.src/install/PackageManager/runTasks.zig (2)
384-397: LGTM!Correctly sets
any_failed_to_install = truewhen a required tarball download fails due to connection errors (null metadata). This ensures the install process will exit with a non-zero code when tarball downloads fail, addressing issue#2766.
451-462: LGTM!Correctly sets
any_failed_to_install = truewhen a required tarball download fails with an HTTP error status (> 399). This is the code path triggered by the test's 404 response, ensuring the install exits with a non-zero code.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
The test test/regression/issue/02766.test.ts passes in the system version of Bun. Fix the reproduction so that the test fails with the system version of Bun and passes with the debug build. |
|
Closing this PR because the bug described in issue #2766 was already fixed in PR #11828 (June 2024). The existing fix adds a My changes were adding redundant safety measures (explicitly setting The existing test at If there's interest in merging my changes as defense-in-depth code quality improvements, I can reopen this PR with updated description, but they don't fix any currently observable bug. |
2 participants
Summary
any_failed_to_install = trueto ensurebun installexits with code 1This fixes an issue where tarball download failures were logged as errors but the process still exited with code 0, making it difficult to detect install failures in CI pipelines.
Test plan
test/regression/issue/02766.test.tsbun installexits with non-zero code when tarball download failsFixes #2766
🤖 Generated with Claude Code