Skip to content

pack/publish: pack() consumes Context, returns Publish::Context<'a> - #37670

Open
robobun wants to merge 1 commit into
mainfrom
farm/c83f5856/pack-publish-consume-ctx-lifetimes
Open

pack/publish: pack() consumes Context, returns Publish::Context<'a>#37670
robobun wants to merge 1 commit into
mainfrom
farm/c83f5856/pack-publish-consume-ctx-lifetimes

Conversation

@robobun

@robobun robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

Problem

  • No user-visible change. bun pm pack and bun publish behave the same; this cleans up how the pack code hands its state on to the publish code.
  • pack() borrowed its Context but returned a Publish::Context<'static>, so both return sites rebuilt the manager and command context from raw pointers, justified by SAFETY comments calling them process-lifetime singletons.
  • The callers copied the pattern with raw pointers of their own for the lockfile load, error printing and the publish scripts. 15 unsafe blocks in all, none of which the compiler could check.

Fix

  • pack() takes its Context by value and returns Publish::Context<'a>, 'a being the lifetime of the manager and command context it was given. Both return sites move those fields across instead of casting.
  • The lockfile gets a second lifetime 'l, since both callers keep it in a local and one shared lifetime would pin the returned publish context to that local.
  • Callers use log_mut() and plain reborrows; pm_log and PackReturn are deleted. 15 unsafe blocks removed, none added. One raw projection (pm_workspace_cache) stays because pack() holds the entry it yields across other manager uses.
  • Property to check: each removed cast becomes a move or reborrow of the same value, so nothing changes at runtime.
  • Verification is existing tests only, as there is no behaviour to fail without the change: cargo check and clippy clean, the pack test file passes, and the publish test file has 14 failures reported as identical on main (a debug-build timeout kills the local registry).

Background

  • PackageManager is the state object shared by bun's install, pack and publish commands. Its Log collects errors and warnings, is a separate allocation, and is reached through log_mut().
  • pack::Context bundles what packing needs: the manager, the CLI command context, and an optional lockfile used only to resolve workspace versions.
  • pack::<FOR_PUBLISH>() serves both commands. For publish it returns a Publish::Context holding the tarball path plus the same manager and command context, which bun publish uses to upload and to run the publish and postpublish scripts.
  • A 'static reference in a return type claims the value lives for the whole process, and making one from a shorter borrow needs unsafe. Returning the caller's lifetime instead lets the borrow checker verify the claim.
  • A second lifetime parameter lets one field (the local lockfile) borrow something shorter-lived than another (the manager) without shortening what the function may return.
Original description

What

pack_command::pack() borrowed its pack::Context<'a> but returned Option<Publish::Context<'static, true>> (through the PackReturn alias), so both return sites (dry run and real pack) rebuilt manager and command_ctx from a raw pointer with SAFETY comments appealing to "process-lifetime singletons". Publish::Context::from_workspace inherited the 'static return type and used a manager_ptr of its own for the lockfile load, the package.json path and the pack::Context literal; PackCommand::exec_with_manager did the same with log_ptr/manager_ptr; pm_log was a raw-pointer helper for reaching manager.log inside pack(); and PublishCommand::exec kept a manager_ptr for one error message and a cmd_ctx_ptr to pass context.command_ctx to the publish/postpublish script runner.

// before
pub(crate) struct Context<'a> { manager: &'a mut PackageManager, command_ctx: Command::Context<'a>, lockfile: Option<&'a Lockfile>, .. }
pub(crate) fn pack<const FOR_PUBLISH: bool>(ctx: &mut Context<'_>, ..) -> Result<PackReturn<'static, FOR_PUBLISH>, ..>
fn from_workspace(ctx: Command::Context<'a>, manager: &'a mut PackageManager) -> Result<Context<'static, true>, ..>

// after
pub(crate) struct Context<'a, 'l> { manager: &'a mut PackageManager, command_ctx: Command::Context<'a>, lockfile: Option<&'l Lockfile>, .. }
pub(crate) fn pack<'a, const FOR_PUBLISH: bool>(mut ctx: Context<'a, '_>, ..) -> Result<Option<Publish::Context<'a, true>>, ..>
fn from_workspace(ctx: Command::Context<'a>, manager: &'a mut PackageManager) -> Result<Context<'a, true>, ..>

The second lifetime is what makes this work: both callers load the lockfile into a function local, so with one lifetime the returned Publish::Context would be pinned to that local. pack() now takes the context by value and both return sites write manager: ctx.manager, command_ctx: ctx.command_ctx. The four helpers that take the context (archive_package_json, add_archive_entry, print_archived_files_and_packages, and pack itself) take or own Context<'_, '_> and the five pass-through calls become &mut ctx. exec_with_manager and from_workspace take let log = manager.log_mut();, call lockfile.load_from_cwd(Some(&mut *manager), log) (the LoadResult only borrows the local lockfile), report errors through log, and build the pack::Context literal with manager directly; the five pm_log(manager_ptr) uses inside pack() become ctx.manager.log_mut() and pm_log is deleted; PublishCommand::exec prints through manager.log_mut() in the from_tarball_path error arm and reborrows context.command_ctx for the two script runs, reading use_system_shell once beforehand as pm_version_command already does. PackReturn is deleted. pm_workspace_cache stays a raw projection because pack() keeps the &mut MapEntry it yields alive across every other use of ctx.manager; its comment now says so. Two call sites of pack(), 15 unsafe blocks removed (7 in pack_command.rs, 8 in publish_command.rs), none added; Publish::Context itself is unchanged.

Why

The &mut PackageManager and &mut ContextData that bun publish runs on now flow from exec through from_workspace and pack() into Publish::Context as ordinary moves with a single lifetime 'a, so the type signatures say where they come from and how long they live instead of 'static plus comments, and the lockfile's shorter lifetime is visible as 'l. It is zero-cost: the lifetimes are compile-time only, the moves replace pointer casts of the same two words, log_mut() is the same inline pointer load pm_log performed, use_system_shell was already read before each call, and the context is passed by value exactly once per bun pm pack / bun publish invocation.

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. bun bd test test/cli/install/bun-pack.test.ts: 76 pass, 0 fail; test/cli/install/bun-publish.test.ts: 24 pass, 14 fail: identical 14 failures on main (lifecycle scripts > should run in order hits the 5s timeout under the debug build, which kills the local Verdaccio registry and cascades into 13 ConnectionRefused failures in the rest of the file), so pre-existing and unrelated to this change.

pack() borrowed its pack::Context and returned Publish::Context<'static>, so both return sites re-derived manager and command_ctx from raw pointers, from_workspace inherited the 'static return type and re-derived its manager the same way, and exec_with_manager, from_workspace and publish exec reached the manager, the log and the command context through further raw pointers. pack::Context is now Context<'a, 'l>: manager and command_ctx carry 'a and the caller-local lockfile carries 'l. pack() takes the context by value and moves manager and command_ctx into the returned Publish::Context<'a, true>, and from_workspace returns Context<'a, true>. The log is reached through PackageManager::log_mut() instead of the local pm_log helper, and publish exec reborrows context.command_ctx directly instead of going through cmd_ctx_ptr; 15 unsafe blocks go away. The lifetimes are compile-time only and the references are moved rather than cast; the context is passed by value once per pack() call and the Publish::Context layout is unchanged.
@robobun
robobun requested a review from alii August 11, 2026 23:28
@coderabbitai

coderabbitai Bot commented Aug 11, 2026

Copy link
Copy Markdown
Contributor

Warning

Review limit reached

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

Next review available in: 1 minute

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: 68d2bbe8-b62a-4722-ade8-f28e1bba8a80

📥 Commits

Reviewing files that changed from the base of the PR and between 1d28cc4 and 6f281ac.

📒 Files selected for processing (2)
  • src/runtime/cli/pack_command.rs
  • src/runtime/cli/publish_command.rs

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

@robobun

robobun commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator Author
Updated 4:45 PM PT - Aug 11th, 2026

@robobun, your commit 6f281acdc09f884a037f82bdac2cafacb6b9737a passed in Build #92578! 🎉


🧪   To try this PR locally:

bunx bun-pr 37670

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

bun-37670 --bun

@claude claude Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — mechanical lifetime refactor that removes 15 unsafe blocks with no behavior change.

What was reviewed:

  • Confirmed PackageManager::log_mut() takes &self so let log = manager.log_mut(); load_from_cwd(Some(&mut *manager), log) borrow-checks without the old raw-pointer split.
  • Command::Context<'a> = &'a mut ContextData, so passing context.command_ctx to run_package_script_foreground is a plain reborrow; reading use_system_shell once beforehand matches pm_version_command.
  • PackReturn and pack::Context have no consumers outside the two changed files; the new 'l lifetime param reaches every use site.
Extended reasoning...

Overview

This PR threads proper lifetimes through bun pm pack / bun publish so the &mut PackageManager and &mut ContextData flow into the returned Publish::Context<'a> as ordinary moves instead of being reconstructed from raw pointers with 'static. Concretely: pack::Context gains a second lifetime 'l for the caller-local lockfile, pack() consumes its context by value and returns Publish::Context<'a, true>, from_workspace returns Context<'a, true>, the pm_log raw-pointer helper is deleted in favor of manager.log_mut(), and PublishCommand::exec reborrows context.command_ctx directly for the two lifecycle-script calls. Net: 15 unsafe blocks removed, none added; PackReturn type alias deleted.

Security risks

None. Single-threaded CLI code; the change replaces raw-pointer reborrows with compiler-checked borrows of the same objects. No new I/O, parsing, or trust-boundary crossings.

Level of scrutiny

Low-to-moderate. This is a Rust type-system hardening change where the borrow checker is the primary verifier — if it compiles, the lifetime relationships the old SAFETY comments asserted are now enforced. I checked that log_mut() takes &self (so the log + &mut *manager split at both load_from_cwd sites is a real disjoint borrow, not a hidden alias), that Command::Context<'a> is &'a mut ContextData (so the script-runner reborrow is sound and the use_system_shell hoist mirrors pm_version_command.rs), and that pack::Context/PackReturn have no consumers outside these two files.

Other factors

  • The one remaining raw-pointer projection (pm_workspace_cache) is intentionally kept and its comment now states the actual reason (the &mut MapEntry outlives other ctx.manager uses).
  • Existing coverage: bun-pack.test.ts 76/76 pass; bun-publish.test.ts failures match main exactly (pre-existing debug-build timeout cascade). No new tests needed for a zero-behavior-change refactor.
  • Part of a series of small standalone hardening PRs; scope is tight and self-contained.

@github-actions

Copy link
Copy Markdown
Contributor

This PR may be a duplicate of:

  1. refactor(rust): eliminate 57 borrowck-workaround allocs and unsafe launders #35399 - Removes the same cmd_ctx_ptr raw pointer and its four unsafe re-derives at the publish/postpublish run_package_script_foreground call sites in publish_command.rs, hoisting use_system_shell into a local — the same lines for the same purpose (this PR is a superset, also covering pack_command.rs).

🤖 Generated with Claude Code

@robobun

robobun commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

On the duplicate flag: #35399 is a 40 file sweep from July that, among other things, drops the same cmd_ctx_ptr re-derives in publish_command.rs. This PR's subject is pack_command.rs (pack() consuming its Context and returning the publish Context<'a>), and the publish_command.rs lines change here as a consequence of that signature. If #35399 lands first this rebases onto it with the publish hunk shrinking; the pack_command.rs half is not in #35399.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants