upgrade: pass a stack Progress instead of a leaked Box and raw ptrs

[robobun]

Problem

• No user-visible bug. bun upgrade leaks its two progress bars on purpose and drives them through raw pointers, 10 unsafe blocks in all.
• Cause: the version lookup took the progress bar and its node as two &mut parameters that borrow the same object, so the caller could only satisfy it by leaking the object.
• The lookup also had a SILENT mode and an Option<Version> return that nothing uses: the one caller is non-silent, and every non-silent path without a version exits the process, so Ok(None) was unreachable and the Option parameters were unwrapped 11 times.

Fix

• The lookup takes one &mut Progress and reaches the node through it; both call sites keep the progress bar as a stack local, so the leaks, raw pointers and unsafe go away.
• Sound because the HTTP client only touches the node during the blocking send, which returns before the frame owning the progress bar does. Progress has no Drop, so the only runtime change is two allocations that no longer happen.
• The SILENT parameter, the Option return and the caller's dead else branch are deleted; every remaining path returns a version or exits. A test comment that called the progress object leaked is updated.
• Verification: refactor only, no new test. Existing upgrade tests pass on a debug build (8 pass, 0 fail); cargo check and cargo clippy are clean.

Background

• Progress is bun's terminal progress bar. start() fills its public root node, refresh() redraws, root.end() finishes it. Since root is public, holding the Progress is enough to reach the node.
• The HTTP client stores an untracked NonNull<Node> and updates it from the HTTP thread while send_sync blocks; its soundness rests on the node outliving that call, before and after this change.
• Global::exit ends the process, so any return after it is dead code; that is what made the Option return unreachable.

Original description

What

UpgradeCommand::get_latest_version was declared as

pub(crate) fn get_latest_version<const SILENT: bool>(
    env_loader: &mut DotEnv::Loader,
    refresher: Option<&mut Progress::Progress>,
    mut progress: Option<&mut Progress::Node>,
    use_profile: bool,
) -> crate::Result<Option<Version>>

but its only instantiation is get_latest_version::<false> with both options Some. Of its six !SILENT blocks, five are error paths that each did progress.expect(..).end(); refresher.expect(..).refresh(); (10 expect calls standing in for a type invariant) and the sixth set progress_node from progress.as_deref_mut().unwrap(). To produce both references at once the caller leaked a Box<Progress> via heap::into_raw and kept two raw pointers into it, and the download block in _exec repeated the same pattern, plus an expect on NonNull::new of the leaked pointer (10 unsafe blocks between the two sites).

Progress::start writes the node into the pub root field, so a single &mut Progress reaches both objects. The function now takes

pub(crate) fn get_latest_version(
    env_loader: &mut DotEnv::Loader,
    progress: &mut Progress::Progress,
    use_profile: bool,
) -> crate::Result<Version>

and the error paths are progress.root.end(); progress.refresh();. Its single caller and the download block in _exec each own a plain stack Progress:

let mut progress = Progress::Progress::default();
progress.start(b"Fetching version tags", 0);
let version = Self::get_latest_version(&mut env_loader, &mut progress, use_profile)?;
progress.root.end();
progress.refresh();

The return type loses its Option: with the SILENT branches gone, every path that does not return a version ends in Global::exit, so the Ok(None) returns and the caller's else { return Ok(()) } were unreachable and are deleted. Net: removes 10 unsafe blocks, 2 leaked heap allocations, 11 expect calls and 1 unwrap, one const generic, and one impossible return state; 1 Rust file plus a one-word update to a comment in bun-upgrade.test.ts that described the progress object as intentionally leaked. This is the same shape standalone_graph/StandaloneModuleGraph.rs::download_to_path already uses (refresher.root.end(), refresher.refresh()). No dependency changes.

Why

Ownership of the progress bar is now visible in the types: it is a local of _exec, borrowed for the duration of get_latest_version, and the "no progress bar" state the Option parameters and Ok(None) return allowed is no longer representable. It is zero-cost: Progress has no Drop impl, the HTTP client receives the same NonNull<Node> as before, the start/end/refresh sequence is unchanged, and the only runtime difference is that two allocations that were never freed no longer happen.

Part of a series of small type-system hardening changes; each PR stands alone.

Verification

cargo check and cargo clippy are clean for the touched crates. Debug build succeeds on upgrade-stack-progress; bun bd test test/cli/install/bun-upgrade.test.ts: 8 pass, 0 fail.

[coderabbitai]

Warning

Review limit reached

@robobun, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 6 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: b46f1ad3-f28e-4285-90e9-738abeacd9d6

📥 Commits

Reviewing files that changed from the base of the PR and between 1b05c85 and 6e71b61.

📒 Files selected for processing (2)

• src/runtime/cli/upgrade_command.rs
• test/cli/install/bun-upgrade.test.ts

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

^{Updated 4:59 PM PT - Aug 11th, 2026}

✅ @robobun, your commit 6e71b61600c08ccfbba31fa53f7aa7d38a0c0948 passed in Build #92597! 🎉

---

🧪   To try this PR locally:

bunx bun-pr 37674

That installs a local version of the PR into your bun-37674 executable, so you can run:

bun-37674 --bun

[claude]

LGTM — mechanical ownership refactor that swaps leaked Box<Progress> + raw pointers for a stack local, matching the pattern already used in StandaloneModuleGraph.rs.

What was reviewed:

• Confirmed get_latest_version has a single caller with SILENT=false, so the removed Option return, Ok(None) arms, and const generic were all dead.
• Confirmed Progress::start writes into pub root and there is no Drop for Progress/Node, so the stack local + progress.root.end() shape is equivalent.
• Checked send_sync blocks until the HTTP callback completes, so NonNull::from(&mut progress.root) into a stack frame is sound (same as the existing download_to_path usage).

Extended reasoning...

Overview

Refactors UpgradeCommand::get_latest_version and the download block in _exec (src/runtime/cli/upgrade_command.rs) to own Progress as a plain stack local instead of a leaked Box with two raw pointers. Drops the SILENT const generic and the Option<Version> return, since the only instantiation was ::<false> and every non-version path already ended in Global::exit. The test change is a one-word comment update in bun-upgrade.test.ts removing a now-stale reference to the progress object being leaked.

Security risks

None. This is the bun upgrade CLI path; the change is purely internal ownership plumbing with no effect on the download URL, digest verification, TLS settings, or archive extraction logic.

Level of scrutiny

Low-to-medium. The interesting question is whether moving Progress from a leaked heap allocation to the stack changes the lifetime seen by async_http.client.progress_node (a raw NonNull<Node>). I traced send_sync in src/http/AsyncHTTP.rs: it schedules the request and blocks on read_item() until the callback fires, so the HTTP thread's accesses to progress_node are strictly bracketed by send_sync. The stack progress outlives that call in every path (including the ? early returns, where it lives in the caller's frame). This is the exact shape StandaloneModuleGraph::download_to_path and several src/install/ sites already use with a stack refresher and refresher.root.end().

Other factors

• Verified via grep that get_latest_version has no other callers, so removing the SILENT generic and Option wrapper cannot break anything.
• Progress and Node have no Drop impl, so the switch from leaked-forever to stack-dropped is a no-op at teardown.
• Net removal of 10 unsafe blocks, 11 expects, 1 unwrap, 2 leaked allocations, and one impossible return state — a strict simplification with no new control flow.
• CI build #92597 is green and bun-upgrade.test.ts (8 tests) passes per the PR description.
• No CODEOWNERS entry covers this file.