fix(run): run COMMAND as the container init (docker semantics)#988
Merged
Conversation
`boxlite run IMAGE COMMAND` exec'd COMMAND as a tenant beside a throwaway init
built from the image's own ENTRYPOINT+CMD. When that default was short-lived
(oven/bun prints help and exits), its death tore down the pid namespace and
SIGKILLed the user's command — connections went to RST while the box still
reported Running — and the zombie init made the next `exec` panic in procfs.
COMMAND now replaces the image CMD (ENTRYPOINT prepended) and the result *is*
the container's init, as docker defines it. The box's lifetime is its main
command's lifetime.
Core:
- run/create: COMMAND -> BoxOptions.cmd; `create` gains a trailing COMMAND. The
stray `sh` fallback is gone.
- The host declares init's session id (ContainerInitRequest.execution_id =
container_id), mirroring kata's CreateContainerRequest{ExecId: c.id}; the
guest registers the id it is handed instead of re-deriving it.
- LiteBox::attach() addresses the main command, attach_exec(id) an exec session
(moby's ContainerAttach/ContainerExecAttach split). Python keeps docker-py's
attach(execution_id=None).
- `run -t` gives the main command a PTY (OCI process.terminal via the console
socket), not just an exec.
Ordering and lifecycle:
- Foreground `run` creates the container, attaches, then starts it (docker's
create -> attach -> start). Started first, a command that finishes instantly
can be gone before the attach lands.
- A box stops when its main command exits: the guest powers the VM off and a
host-side watcher records the exit. All three death paths (stop, the watcher,
runtime shutdown) report the code through one helper. exit.json
(boxlite_shared::layout::ExitRecord) carries it, per-run and per-container.
- exec/attach/cp/metrics on a finished *job* refuse rather than silently
restarting it (that would re-run the command); a box with no command of its
own still boots implicitly, which the cloud's auto-restart depends on.
- A handle whose VM has died refuses at the shared live_state() funnel instead
of serving the corpse.
REST/serve: a container-level /boxes/{id}/attach route so cloud `run` works;
the reaper never kills the main session; exit code on the REST wire.
Verified two-sided per CLAUDE.md (full CLI + boxlite integration suites green
apart from four failures that reproduce at the branch point with none of this
applied). Passed the CLAUDE.md preflight audit.
Claude-Session: https://claude.ai/code/session_01KYXByfJsxsWktqA2TjqKxi
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change separates container creation from init startup, adds main-command attachment and PTY handling, records init exit codes, prevents unsafe reruns, introduces unified lifecycle watching and guest process reaping, and updates CLI, REST, runtime, SDK, and integration-test behavior. ChangesInit lifecycle and command semantics
Estimated code review effort: 5 (Critical) | ~90+ minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CLI
participant BoxImpl
participant GuestService
participant MainSession
CLI->>BoxImpl: create container with command and tty
BoxImpl->>GuestService: Init without running init
GuestService->>MainSession: register main init session
CLI->>BoxImpl: attach main session
BoxImpl->>MainSession: stream init session
CLI->>BoxImpl: start container init
BoxImpl->>GuestService: Start container
GuestService->>MainSession: run and watch init
MainSession-->>BoxImpl: output and exit code
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
tester seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
1 similar comment
|
tester seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
added 6 commits
July 15, 2026 11:19
…runs
The container-init protocol had `ContainerInitRequest.defer_start`: a boolean
that made one RPC do two workflows — create-and-start when false, create-only
when true. That is the CLAUDE.md anti-pattern, and it diverged from the
reference protocol this change set otherwise follows, whose create and start
are two unconditional RPCs with no such flag.
Make the wire match: `Container.Init` always creates the container and never
runs init; `Container.Start` always runs it. The host sequences the two — it
calls Start right after Init for an ordinary boot, and attaches in between for
a foreground `run` (`start_attached`). The decision of *when* to start stays
host-side runtime logic; it no longer travels as a flag on the wire.
- proto: `defer_start` field removed; Init/Start docs say what each does.
- guest: `Container.Init` no longer branches on the flag; `run_container_init`
is reached only via the `Container.Start` RPC.
- host: `ContainerInterface::init` takes `tty: bool` directly (the
`InitProcessSetup{tty, defer_start}` grouping struct is deleted); the
guest-init task sends `Container.Start` after `init`, unless deferring.
Behavior is unchanged (create → [attach] → start); only the RPC shape and the
side the decision lives on move. Also scrubs reference-project name-drops from
the comments this PR introduced — the parity citation lives in the design doc.
Full rust integration 236/236; CLI 322/326 (the 4 failures are pre-existing at
the fork point); fmt + clippy clean. Passed the CLAUDE.md preflight audit.
Claude-Session: https://claude.ai/code/session_01KYXByfJsxsWktqA2TjqKxi
Close two coverage gaps in the run-command work that were unit-only or missing. - `run --url` had no end-to-end test — only route-registered / client-wired unit checks. `run_rest_e2e.rs` spins up `boxlite serve` on a free port and runs `run --url alpine sh -c 'echo …; exit 7'` through the whole wire, so a command's output and exit code must actually round-trip. That path's breakage (start-then-attach racing a fast command) was a release blocker; it is now guarded by something that exercises the socket. Teardown drains the server's boxes before killing it, since teardown over REST is async and serve stops its boxes only on SIGINT. - `--user` and `-w` now apply to init (COMMAND is init); assert it with `run --user 1000 alpine id -u` -> 1000 and `run -w /tmp alpine pwd` -> /tmp, which read init's own process spec back from the kernel. Full CLI suite 325/329 (+3 new; the 4 failures are pre-existing at the fork point). Test-only, no production change. Passed the CLAUDE.md preflight audit. Claude-Session: https://claude.ai/code/session_01KYXByfJsxsWktqA2TjqKxi
…ach + start
Booting a box and running its container's init were one pipeline step, so
holding init at the gate for a client to attach first needed a threaded flag
(defer_container_start) and a fused method (start_attached). Split them: boot
now only creates the container (Container.Init); running it (Container.Start)
is a separate, idempotent host step.
- start() = boot + run-container (single-flighted via a container_start
OnceCell; admits Running so a box booted by a bare attach() still
gets its init run)
- attach() = boot-only + subscribe (never runs the command, so it drops the
re-run guard exec/cp keep; refuses a stopped box)
- live_state() (exec/cp/metrics) = boot + run-container, behaviour unchanged
- run foreground = attach() then start(); over REST, /attach boots+subscribes
and /start runs, with start_box keeping a Running handle so run --url's
attach-then-start does not drop its live VM
Removes start_attached (BoxBackend trait, BoxImpl, RestBox, LiteBox facade) and
defer_container_start (flag + pipeline threading). An output-buffer bus was
considered and rejected: it lives in the guest and dies with the VM, so it does
not remove the ordering requirement. Net -54 LoC of machinery.
Verified: run_init_semantics 8/8, run_main_command (incl. new
attach_refuses_a_stopped_box, two-sided against the gate), run_over_rest e2e,
CLI exec 18/18, CLI run 53/53, working-dir/timeout/user.
Claude-Session: https://claude.ai/code/session_01KYXByfJsxsWktqA2TjqKxi
The main-command attach and the exec reattach were two BoxBackend methods.
Fold them into one `attach(execution_id: Option<&str>)`: `None` = the box's
main command session, `Some(id)` = reattach to that exec — docker's
ContainerAttach and ContainerExecAttach behind one verb. Behaviour unchanged:
- BoxImpl (local): None → main-attach; Some(_) → Unsupported (an in-process
exec never drops its stream, so there is nothing to reattach to by id).
- RestBox: None → /boxes/{id}/attach; Some(id) → /boxes/{id}/executions/{id}/attach.
- LiteBox facade + Python `attach(execution_id=None)` collapse to a pass-through.
- Callers moved to attach(None): run.rs, serve, tests.
Adds coverage for both arms — the exec-reattach path had zero test callers:
- REST unit (mock WS server): Some(id) → exec route + streams; None → missing
session-id header → Unsupported; Some(id) → 404 SessionReaped / 409
AlreadyExists (harness gained reject_upgrade_status to inject the rejection).
- local (real runtime): attach(Some(id)) → Unsupported (two-sided verified
against the guard).
Verified: 4 REST attach unit tests, run_main_command attach suite, run_over_rest
e2e; clean compile across boxlite/cli/python/node/guest.
Claude-Session: https://claude.ai/code/session_01KYXByfJsxsWktqA2TjqKxi
… into BoxWatcher A box ran two per-box tasks that raced to write BoxState on shim death: the always-on exit watcher and, when a HEALTHCHECK was configured, a separate health-check task. Collapse them into one BoxWatcher (litebox/watcher.rs) whose run() is a single select!: shutdown -> stand down; wait_for_exit -> record Stopped + exit_code (+ health_status Unhealthy if health-checked); and a health-tick arm present only with an Option<HealthProbe>. With None it degenerates to a pure exit watcher. Since shim-exit and health-tick are sibling select arms, the health path no longer detects shim death itself -- is_process_alive and force_status are gone, and the kill(pid,0) zombie bug with them: a tick fires only while the shim is alive. arm_exit_watcher / spawn_exit_watcher / spawn_health_check and the exit_watcher_armed + health_check_task fields are removed. One `watcher: OnceLock<JoinHandle>` replaces them (like `live` for the VM): arm_watcher get_or_inits it -- race-free "one watcher per box" across the many handouts the runtime makes -- and stop() aborts via get(). init_live_state fetches the guest and arms the full watcher UNDER the state write-lock that publishes Running+pid, so a concurrent handout's arm_watcher(None) cannot slip an exit-only watcher in ahead of the probe. Behavior change: a reattached box (one a fresh runtime recovers already Running) is watched exit-only -- its HEALTHCHECK no longer runs in the recovering process; freshly started/restarted boxes are unaffected. Adopted boxes are exit-only by design, so init_live_state builds no probe for them (no stranded probe, no discarded guest fetch). Tests: box_watcher_records_stopped_with_exit_code_when_the_shim_dies and box_watcher_auto_removes_the_box_after_its_shim_dies (stand-in shim, no VM); plus command_is_pid1, a_self_stopped, and both health_check.rs tests on VMs. Compiles clean across boxlite/cli/python/node/guest. Claude-Session: https://claude.ai/code/session_01KYXByfJsxsWktqA2TjqKxi
Resolve PR #988 conflicts against main networking and --rootfs work: keep both ensure_container_started and network() in box_impl.rs; fold docker-semantics create_box attach+start into main --rootfs in run.rs; drop the orphaned parse_command_args and its dead sh-defaulting tests; keep attach(Option) tests alongside ws_client_keepalive; merge the run/create README prose. Verified cargo check clean for boxlite default/rest/tests, boxlite-cli, boxlite-python; BoxWatcher and attach unit tests pass. Claude-Session: https://claude.ai/code/session_01KYXByfJsxsWktqA2TjqKxi
DorianZheng
marked this pull request as ready for review
July 16, 2026 03:11
📦 BoxLite review — couldn't completepowered by BoxLite |
![boxlite-agent[bot]](https://avatars.githubusercontent.com/in/4244458?s=60&v=4){kind=small}
DorianZheng
added
the
e2e-local
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/guest/src/container/lifecycle.rs (1)
99-108: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate lifecycle messaging for create-only, TTY-aware init.
The creation path no longer starts init and may use a PTY, but its public documentation and logs still describe the old behavior.
src/guest/src/container/lifecycle.rs#L99-L108: document deferred startup,run_init(), and thettyargument.src/guest/src/service/container.rs#L280-L309: report the selected I/O mode and “created/ready for attach and start.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/guest/src/container/lifecycle.rs` around lines 99 - 108, The container lifecycle documentation for start must describe deferred init startup, reference run_init(), and document the tty argument; update src/guest/src/container/lifecycle.rs lines 99-108 accordingly. In src/guest/src/service/container.rs lines 280-309, update creation logs to report the selected I/O mode and state that the container was created and is ready for attach and start.
🧹 Nitpick comments (1)
src/shared/proto/boxlite/v1/service.proto (1)
236-241: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider renaming
ContainerInitErroror creating a specificContainerStartError.Reusing
ContainerInitErrorfor theContainerStartResponseerror variant is functionally correct but slightly confusing semantically. Consider introducing a specificContainerStartErrormessage or renamingContainerInitErrorto a genericContainerErrorto better reflect its usage across different RPC responses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/shared/proto/boxlite/v1/service.proto` around lines 236 - 241, Clarify the error message naming used by ContainerStartResponse: either rename ContainerInitError to a generic ContainerError if it is shared across RPC responses, or introduce a dedicated ContainerStartError for this response and update the oneof reference and all affected usages consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/boxlite/src/litebox/box_impl.rs`:
- Around line 441-488: Update exit_code_from_file_when_portal_has_none to wait
for the asynchronously written ExitRecord before forwarding a negative portal
result. Await the relevant shim/init completion or retry ExitRecord::read with a
bounded timeout, ensuring the recovered record is used when available while
preserving the existing result forwarding behavior.
- Around line 930-945: Update ensure_container_started so only the caller that
successfully performs the get_or_try_init initialization returns true; callers
that observe an already-initialized cell, including those that waited for
another initializer, must return false. Preserve the existing container start
operation and error propagation.
In `@src/boxlite/src/litebox/watcher.rs`:
- Around line 140-158: Update the watcher loop and related run flow around
on_health_tick so shim.wait_for_exit() and shutdown.cancelled() remain
selectable while a health probe is in progress. Make the probe await cancellable
or run it through a nested select, and mark health probing disabled after an
unhealthy result instead of returning from run; continue observing and recording
later shim exits.
- Around line 161-189: Update on_shim_exit to use the active-state predicate for
the stop transition instead of checking only BoxStatus::Running, ensuring paused
and other active boxes are marked Stopped when the shim exits while preserving
the existing exit-code recovery behavior.
In `@src/boxlite/src/rest/client.rs`:
- Around line 326-328: Update the WebSocket connection flow around connect_async
to wrap the handshake await in tokio::time::timeout using the established
timeout duration. Handle timeout expiry by returning BoxliteError::Network,
while preserving map_ws_error handling for connection failures and the existing
behavior for both attach paths.
In `@src/boxlite/src/rest/litebox.rs`:
- Line 1809: Run rustfmt on the changed code in src/boxlite/src/rest/litebox.rs
at lines 1809-1809 and src/cli/src/commands/serve/mod.rs at lines 1035-1035,
applying all formatter-reported changes in both files.
In `@src/boxlite/src/runtime/rt_impl.rs`:
- Around line 23-33: The recovery flow must arm exit watchers for every verified
running box before any handle is requested, rather than relying on
litebox_from_impl. Update the recovery logic around the running-box verification
path to call arm_watcher for each adopted running box, and add a regression test
that recovers a running box without runtime.get(), kills its shim, and verifies
persisted Stopped state.
In `@src/boxlite/tests/run_main_command.rs`:
- Around line 38-41: Apply rustfmt’s standard formatting to the chained attach
call assigned to execution in the test, preserving its behavior and existing
error expectation.
In `@src/cli/src/commands/cp.rs`:
- Around line 51-61: Ensure the copy flow around handle.copy_into and the
corresponding copy operation always cleans up a box that was started implicitly,
including when the copy returns an error before reaching the existing stop call.
Update the copy API or surrounding control flow to own or report temporary
startup state, then stop only those temporarily started boxes while preserving
already-running boxes and propagating the original copy error.
In `@src/cli/src/commands/serve/mod.rs`:
- Around line 912-917: Update the cached-handle checks in
src/cli/src/commands/serve/mod.rs#L912-L917 and
src/cli/src/commands/serve/handlers/boxes.rs#L95-L100 to use status.is_active()
instead of comparing exclusively with BoxStatus::Running. Reuse cached handles
and avoid eviction for both Running and Paused VMs; evict only inactive
statuses.
- Around line 1031-1039: The register_main_session flow must not hold
state.executions across the potentially blocking litebox.attach(None) cold boot.
Add or reuse a per-box opening lock to serialize session creation, recheck for
an existing execution after acquiring it, perform attach outside the global
registry lock, then acquire state.executions.write() only for the final
lookup/insertion while preserving atomic registration behavior.
In `@src/guest/src/service/container.rs`:
- Around line 389-397: Update the exit-record handling in the container teardown
flow so `ExitRecord::write` must succeed durably before VM poweroff proceeds.
Retry the write or propagate its error instead of only issuing the existing
warning, and ensure the shutdown path does not power off while persistence
remains failed.
In `@src/guest/src/service/exec/mod.rs`:
- Around line 423-428: Validate the exec request’s container_id before passing
it to SharedGuestLayout::container() in the exit-file handling flow. Reject
absolute, traversal, or otherwise path-like values and only construct the
exit-file path after successful strict ContainerID validation, preserving normal
valid-container behavior and preventing registry/path access with the raw
string.
In `@src/guest/src/service/guest.rs`:
- Around line 130-142: Update the shutdown watcher handling in the timeout match
around watcher so Ok(Err(e)) and Err(_) return an RPC error instead of logging
and continuing. Preserve the successful completion path only for Ok(Ok(())),
ensuring teardown is not reported complete unless the exit record write ordering
is established.
---
Outside diff comments:
In `@src/guest/src/container/lifecycle.rs`:
- Around line 99-108: The container lifecycle documentation for start must
describe deferred init startup, reference run_init(), and document the tty
argument; update src/guest/src/container/lifecycle.rs lines 99-108 accordingly.
In src/guest/src/service/container.rs lines 280-309, update creation logs to
report the selected I/O mode and state that the container was created and is
ready for attach and start.
---
Nitpick comments:
In `@src/shared/proto/boxlite/v1/service.proto`:
- Around line 236-241: Clarify the error message naming used by
ContainerStartResponse: either rename ContainerInitError to a generic
ContainerError if it is shared across RPC responses, or introduce a dedicated
ContainerStartError for this response and update the oneof reference and all
affected usages consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 08a880cb-3e74-459c-852a-78103de09518
📒 Files selected for processing (52)
docs/investigations/collapse-start-attached.mddocs/investigations/init-build-via-zygote.mddocs/investigations/run-command-semantics-fix.mddocs/investigations/sdk-run-semantics-api.mddocs/reference/cli/README.mdsdks/node/src/options.rssdks/python/src/box_handle.rssrc/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/init/mod.rssrc/boxlite/src/litebox/init/tasks/container_rootfs.rssrc/boxlite/src/litebox/init/tasks/guest_init.rssrc/boxlite/src/litebox/mod.rssrc/boxlite/src/litebox/state.rssrc/boxlite/src/litebox/watcher.rssrc/boxlite/src/portal/interfaces/container.rssrc/boxlite/src/portal/interfaces/exec.rssrc/boxlite/src/rest/client.rssrc/boxlite/src/rest/litebox.rssrc/boxlite/src/rest/types.rssrc/boxlite/src/runtime/backend.rssrc/boxlite/src/runtime/layout.rssrc/boxlite/src/runtime/options.rssrc/boxlite/src/runtime/rt_impl.rssrc/boxlite/src/runtime/types.rssrc/boxlite/tests/run_main_command.rssrc/cli/README.mdsrc/cli/src/cli.rssrc/cli/src/commands/cp.rssrc/cli/src/commands/create.rssrc/cli/src/commands/inspect.rssrc/cli/src/commands/run.rssrc/cli/src/commands/serve/README.mdsrc/cli/src/commands/serve/handlers/boxes.rssrc/cli/src/commands/serve/handlers/executions.rssrc/cli/src/commands/serve/mod.rssrc/cli/src/commands/serve/types.rssrc/cli/tests/run.rssrc/cli/tests/run_init_semantics.rssrc/cli/tests/run_rest_e2e.rssrc/guest/src/container/command.rssrc/guest/src/container/lifecycle.rssrc/guest/src/container/spec.rssrc/guest/src/container/start.rssrc/guest/src/container/stdio.rssrc/guest/src/service/container.rssrc/guest/src/service/exec/mod.rssrc/guest/src/service/exec/registry.rssrc/guest/src/service/exec/state.rssrc/guest/src/service/guest.rssrc/guest/src/service/server.rssrc/shared/proto/boxlite/v1/service.protosrc/shared/src/layout.rs
Contributor
|
can |
BoxLite bot and CodeRabbit review findings, plus the post-merge rustfmt that was failing CI. ensure_container_started now sets a closure-local flag so only the single-flight winner reports the start (it returned Ok(true) to every concurrent caller, duplicating BoxStarted events). box_impl exit recovery bounded-polls for the guest async exit-file write, so a signal death reports docker 128+n instead of a raw negative. guest exec rejects a non-single-component container_id before joining it into a path (a client can override BOXLITE_EXECUTOR). cli cp restores a stopped box that a failed copy implicitly booted. serve, boxes and watcher use is_active (Running or Paused) where the VM-alive check is meant. watcher keeps shim-exit selectable while a health probe runs and stops probing, not the whole watcher, once unhealthy. rustfmt applied to the files the merge left unformatted. Claude-Session: https://claude.ai/code/session_01KYXByfJsxsWktqA2TjqKxi
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/boxlite/src/litebox/box_impl.rs`:
- Around line 959-971: Update the get_or_try_init closure in the container
startup flow to use async move, so it owns and can mutate the started_here flag
across the awaited operations while preserving the existing initialization and
return behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 8798af3c-574e-4c0b-a14e-9f7e162771f3
📒 Files selected for processing (8)
src/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/watcher.rssrc/boxlite/src/rest/litebox.rssrc/boxlite/tests/run_main_command.rssrc/cli/src/commands/cp.rssrc/cli/src/commands/serve/handlers/boxes.rssrc/cli/src/commands/serve/mod.rssrc/guest/src/service/exec/mod.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- src/cli/src/commands/cp.rs
- src/boxlite/tests/run_main_command.rs
- src/boxlite/src/litebox/watcher.rs
- src/cli/src/commands/serve/handlers/boxes.rs
- src/cli/src/commands/serve/mod.rs
- src/boxlite/src/rest/litebox.rs
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This was referenced Jul 16, 2026
Replace the per-pid blocking waitpid and the zygote Wait IPC with a single SIGCHLD-driven waitpid(-1) reaper in the guest agent. Exec tenants are now cloned CLONE_PARENT (youki as_sibling) so they reparent to guest main like the container init already did, and both wait via REAPER.wait(pid). Delete WaitVia, wait_via_zygote, and the zygote's Wait / do_wait / Zygote::wait. A process-wide waitpid(-1) races callers that wait for their own child: Command::output() in the storage paths lost its child to the reaper and failed with ECHILD (10 of 20 calls on a booted box). Add a reap fence — the sweep takes the write side, self-waiting callers hold the read side — and take it around every self-wait in the guest: mkfs, resize2fs, chown -R, and the userns helper's child from before its fork until it is reaped — on that helper's failure path the child is already dead when the parent looks, so a fence taken only at the wait would arrive too late. Hold an exit until its owner asks for it. Callers claim a pid with expect_waiter() once the spawn returns it, so an exit arriving before any Wait is kept; only ownerless strays age out via REAPED_TTL. The claim carries the instant read before the spawn, which is what separates our own exit — a short command can exit before its pid travels back from the zygote — from a previous owner's exit under a recycled pid. Without the hold, a detached exec, which sends no Wait until its caller chooses to, had its exit pruned and its later Wait hung. Tests: reaper unit tests covering the reaped-before-wait race, the ECHILD theft, the unclaimed-exit hold, both claim/reap orderings, TTL pruning, signal delivery and concurrent waiters; a concurrent distinct-exit-codes integration test; a self-terminate (SIGTERM -> 143) test. Claude-Session: https://claude.ai/code/session_01KYXByfJsxsWktqA2TjqKxi
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/investigations/init-build-via-zygote.md (1)
113-114: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winClarify the verification counts and status.
This says the full suite is green with 56 CLI integration tests, while the PR report lists 322/326 CLI integration tests and pre-existing failures. Identify 56 as an init-focused subset, or update the document with the actual suite results.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/investigations/init-build-via-zygote.md` around lines 113 - 114, Update the verification statement in the investigation document to accurately distinguish the 56 init-focused CLI integration tests from the full CLI suite, or replace it with the actual 322/326 results and note the pre-existing failures. Keep the guest unit-test and clippy status accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/investigations/init-build-via-zygote.md`:
- Around line 113-114: Update the verification statement in the investigation
document to accurately distinguish the 56 init-focused CLI integration tests
from the full CLI suite, or replace it with the actual 322/326 results and note
the pre-existing failures. Keep the guest unit-test and clippy status accurate.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: faedc0f5-0f3a-4c28-822a-190754f7a91b
📒 Files selected for processing (13)
docs/investigations/init-build-via-zygote.mdsrc/boxlite/tests/zygote_integration.rssrc/cli/tests/exec.rssrc/guest/Cargo.tomlsrc/guest/src/container/zygote.rssrc/guest/src/main.rssrc/guest/src/reaper.rssrc/guest/src/service/container.rssrc/guest/src/service/exec/mod.rssrc/guest/src/service/exec/state.rssrc/guest/src/storage/block_device.rssrc/guest/src/storage/idmap.rssrc/guest/src/storage/perms.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/guest/src/service/container.rs
Member
Author
|
@boxlite-agent review |
added 2 commits
July 20, 2026 17:01
The reaper exposed wait(pid) and callers parked on it, which forced two wait layers and two defects. wait() removed the cached exit and deliver() removed the waiter, so an exit was handed out exactly once and a second Wait RPC for the same execution could not be answered. And waiters.insert discarded the Option it returns, dropping any sender already registered for that pid, so a second wait on a pid someone was still waiting on ended the first wait with ExitStatus::Code(-1) — a status no process returned. Replace it with ExitSlot, a watch channel the execution owns. The reaper deposits and never reads; holders park on get(). Being level-triggered makes ordering irrelevant: an exit landing before its owner registers is already there when the owner arrives, and every later caller reads the same value. ExecutionState::wait_process is now the single place any caller waits, and is infallible. Delete Reaper::wait, the waiters map and the oneshot. expected and reaped collapse into one slots map. REAPED_TTL stays, but only to age out strays nobody holds. Pruning cannot strand a caller: register clears the stray mark before handing out a receiver, so a slot still marked stray has no holder. A slot carries two timestamps and they are not interchangeable. stray_since answers whether it may age out; settled_at answers whether the status is ours. Because the execution registry never evicts, a claimed slot outlives its execution holding a status, and pruning skips it — so only settled_at can tell that corpse from a fresh spawn's own exit once the kernel recycles the pid. register drops any slot that settled before the spawn, and deliver detaches an already-settled slot rather than overwrite it, so a recycled pid cannot hand the previous owner the new process's code. No peer reaper answers wait-by-pid: containerd broadcasts and filters at the receiver, kata deposits onto the Process and drops the sender, and tini, dumb-init and catatonit compare against one known pid. Where a repeat read is needed it always lives in a store separate from the reaper. Reproducers cover both defects and the recycle guard; rationale and prior art in docs/investigations/reaper-exit-slot.md. Claude-Session: https://claude.ai/code/session_01KYXByfJsxsWktqA2TjqKxi
Container init creation was the one clone3 left outside the zygote: it
ran libcontainer's build() on the multi-threaded tokio process, exposed
to the musl __malloc_lock fork deadlock, and its internal
waitpid(intermediate) raced the guest reaper's waitpid(-1) sweep
unfenced.
Add ZygoteRequest::BuildInit carrying {container_id, state_root,
bundle_path, console_socket}; stdio pipe fds ride the existing
SCM_RIGHTS channel. create_container_with_stdio becomes a thin blocking
client of it, and the dead direct-build create_container is deleted. The
response reuses BuildResult and returns init's pid; each request kind
gets its own response tag so a mismatched reply is a protocol error,
never a misattributed pid.
Init must stay guest main's child: libcontainer clones init CLONE_PARENT
off its intermediate, so init parents to whichever process calls
build(). Built in the zygote that would strand init's exit — nobody
reaps zygote children — so the build sets as_sibling(true), parenting
the whole chain to guest main exactly as tenants do; the intermediate
dies as a reaper stray and libcontainer tolerates the ECHILD by design.
The design doc's reparenting invariant is corrected accordingly.
Failed builds now close their SCM_RIGHTS fds before any fallible step —
previously an early builder error leaked all three into the long-lived
zygote (tenant path included).
Tests: zygote unit tests cover the new protocol, a mixed tenant/init
build concurrency race, failure propagation, a zygote death mid-request,
and fd-close-on-failure; integration adds a console-marker mechanism
proof, a restart re-record check, and a failed init build surfacing and
recovering end to end.
Claude-Session: https://claude.ai/code/session_01KYXByfJsxsWktqA2TjqKxi
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/investigations/reaper-exit-slot.md`:
- Around line 153-157: Convert the indented PID-reuse example in the
reaper-exit-slot documentation to a fenced text code block, preserving all
example lines and their formatting so markdownlint MD046 passes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a8e40483-22ca-40e3-a7aa-5b566baf59e5
📒 Files selected for processing (7)
docs/investigations/init-build-via-zygote.mddocs/investigations/reaper-exit-slot.mdsrc/boxlite/tests/zygote_integration.rssrc/cli/tests/run_init_semantics.rssrc/guest/Cargo.tomlsrc/guest/src/container/start.rssrc/guest/src/container/zygote.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/guest/Cargo.toml
- docs/investigations/init-build-via-zygote.md
added 2 commits
July 21, 2026 18:01
The container service spawned a watcher task per init that parked on the exit slot, wrote the exit record, and powered the VM off; its JoinHandle was kept in an init_watchers map solely so the Shutdown RPC could join it and know the record was on disk before the host reads the exit file. Fold that follow-up into the reaper: a claim may now register an ExitAction (Reaper::on_exit) that the reaper fires exactly once when it delivers the pid's exit — after its lock is released, so an action can be slow or call back into the reaper without blocking other deliveries. An exit that beats the registration fires the action immediately, and the recycle guards cannot discard a parked action: deliver takes it while settling, so a settled slot never holds one. The join contract is replaced, not dropped. Shutdown now derives the record from init's exit slot and writes it inline before answering the host; the action writes the same bytes from the same level-triggered slot, so the duplicate write is idempotent and only the ordering is new. The init_watchers JoinHandle map becomes an init_exits slot map, and the 128+n signal convention moves into ExitStatus::shell_code, shared by both write sites. Claude-Session: https://claude.ai/code/session_01KYXByfJsxsWktqA2TjqKxi
A box whose main command exits must power its VM off, but on x86_64 the microVM kernel has no pm_power_off backend (no ACPI): reboot(POWER_OFF) degrades to "Power off not available: System halted instead" and the VM stays up forever. Every observation path then reports the box Running — the whole self-stop family of e2e failures, present since the feature first ran on Linux CI. Switch the init-exit action to reboot(RESTART), the one command every VMM backend turns into a process exit: the kernel's reboot=k path ends in an i8042 CPU reset on x86 and a PSCI SYSTEM_RESET on aarch64, both trapped as a shutdown. It is also exactly what the VMM's own bundled init issues when its workload exits. Verified on the x86 CI runner: a self-stopping box now reaches "stopped 7" with the VMM gone, where before it sat "running" behind a halted VM. Also fold in the review fixes riding this branch: - run_main_command: plant a file where the boxes directory belongs instead of chmod 0500 — mode bits do not bind root, and CI runs the suite as root, so the old injector never fired there. - rest client: bound the WebSocket handshake with a 30s timeout mapped to a network error; a stalled connect blocked attach forever. - docs: fence the PID-reuse example (markdownlint MD046). Claude-Session: https://claude.ai/code/session_01KYXByfJsxsWktqA2TjqKxi
added 2 commits
July 21, 2026 20:52
A box created with a cmd is the user's main command, and exec refuses
to boot such a box as a side effect ("starting the box would run your
main command"). The cmd-override test predates that gate and exec'd a
configured box directly, failing with exactly that refusal on CI. Start
the box explicitly first — the same contract every caller now follows.
Claude-Session: https://claude.ai/code/session_01KYXByfJsxsWktqA2TjqKxi
`boxlite create` then `boxlite start` is docker's create → start: the box runs its main command in the background and the launching CLI gets out of the way. But a created box inherited detach=false, so when the `start` process exited its runtime's drop-time auto-stop and the shim's parent-death watchdog tore the VM down — on x86 that kill outran a fast `exit N`, so the exit code was never recorded and the box reported 0. On aarch64 the command won the race, which is why it only failed on Linux CI. `run` foreground escapes by waiting; `run -d` escapes via detach=true. Set detach on the `create` command itself (where config is written once — the drop-time auto-stop reads persisted config, so a per-boot flag could not reach it): a created box is a background box. `run` and the SDK default are untouched. auto_remove is forced off alongside, the same rule `run -d` enforces and `sanitize` requires (auto_remove ⊥ detach), since a detached box has no foreground watcher to remove it. Proven on the x86 CI runner: `create`+`start` of `exit 23` went from stopped/0 to stopped/23. run_init_semantics 11/11 and the create/exec/ rm/inspect/list/info CLI suites stay green. Claude-Session: https://claude.ai/code/session_01KYXByfJsxsWktqA2TjqKxi
This was referenced Jul 22, 2026
DorianZheng
added a commit
that referenced
this pull request
Jul 23, 2026
## Problem PR #988 split the guest's fused `Container.Init` into `Init` (create) + `Start` (run init). The guest agent is baked into the versioned guest-rootfs and reused per-box across restarts, so a box created before #988 boots its old agent — which has no `Container.Start`. The post-#988 host calls it, tonic returns `Unimplemented`, and it surfaced as `gRPC/tonic error … Unimplemented … (code=16)`, breaking restart/exec of pre-#988 boxes. ## Fix `ContainerInterface::start` now reads the gRPC status before `?` flattens it and treats `Unimplemented` as "already started": on a pre-#988 agent the fused `Init` (still called every boot) already ran init, so `Start` is a redundant no-op. Real errors still propagate. Also removes the spurious `boxlite/gvproxy` feature from `sdks/c` (introduced in #994), which linked the shim-side `libgvproxy-sys` CGO library into the C SDK — the SDK/CLI must not, per the existing `net::tests` guard. Matches Python/Node/CLI, which expose the same tunnel API with `rest` only. ## Test An in-process tonic stub guest returning `Unimplemented` reproduces the exact failure through `ContainerInterface::start`; the degrade turns it green while a genuine `Start` error still surfaces. Full boxlite + boxlite-shared unit suites pass (852 + 47). Deferred: the durable fix (re-inject/recreate the guest so restarts run the current agent, kata/Daytona-style) is not included here. https://claude.ai/code/session_01Vu4qDgCnMjnLPpNsroxeAP <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Container startup now succeeds when the service reports that the container start operation is unsupported, treating it as already started. * Genuine container startup errors continue to be reported normally. * **Compatibility** * Updated SDK configuration to use the REST feature without the optional networking feature. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: tester <t@t.test>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
boxlite run IMAGE COMMANDnow means what it means everywhere else: COMMAND replaces the image CMD, ENTRYPOINT is prepended, and the result is the container's init (PID 1). The box lives exactly as long as that command.Why
COMMAND was a
docker execindocker runclothing — it ran as a tenant beside an init built from the image's own ENTRYPOINT+CMD.When the image default was short-lived the consequences were silent and severe.
oven/bunprints help and exits; that init's death tore down the pid namespace and SIGKILLed the user's server (connections went to RST) whileboxlite psstill reported Running. The zombie init then made the nextexecpanic inside procfs. Confirmed side-by-side against podman 5.6.1, which keeps the container alive exactly as long as the user's command.What changed
Semantics — COMMAND →
BoxOptions.cmdinrunandcreate(creategains a trailing COMMAND, stored and run on start); the strayshfallback is deleted.run -tgives the main command a real PTY (OCIprocess.terminalover the console socket).Attach —
LiteBox::attach()addresses the box's main command,attach_exec(id)an exec session (moby'sContainerAttach/ContainerExecAttachsplit). Python keeps docker-py'sattach(execution_id=None).Session naming (kata parity) — the host declares init's session id in
ContainerInitRequest.execution_id = container_id, mirroring kata'sCreateContainerRequest{ExecId: c.id}. The guest registers the id it is handed rather than both sides hard-coding the convention.Ordering — foreground
rundoes create → attach → start (Container.Initwithdefer_start, thenContainer.Start). Started first, a command that finishes instantly can be gone before the attach lands and its output and exit code die with the VM.Lifecycle — a box stops when its main command exits: the guest powers the VM off and a host-side watcher marks the box Stopped with the exit code. All three death paths —
stop(), the watcher, and runtime shutdown — record the code through one helper.exec/cp/metricsagainst a finished job refuse rather than silently restarting it (that would re-run the command); a box with no command of its own still boots implicitly, which the cloud's auto-stop → next-SDK-call restart depends on. A handle whose VM has died refuses at the sharedlive_state()funnel instead of serving the corpse.Exit code —
containers/{cid}/exit.json(boxlite_shared::layout::ExitRecord), per-run and per-container, surfaced oninspect .State.ExitCodeand on the REST wire.REST / serve — a container-level
/v1/boxes/{id}/attachroute so cloudrunworks (docker'sPOST /containers/{id}/attach); the orphan reaper never kills the box's main session.Compatibility
run IMAGE quick-cmdthenexec/cplater. The box is now Stopped with the command's exit code and the operation is refused — same as docker, which treatsexecon a non-running container as an error, not an implicitstart. Before,exechit a zombie (procfs panic) or joined a still-alive image default.cpto/from a finished job is refused rather than silently rebooting it — a deliberate deviation fromdocker cp, because a boxlite container's filesystem lives inside the VM's disk and reaching it means booting, which runs init. Documented with its follow-up (a mount-without-run capability). Boxes without a main command are unaffected; volumes remain the way to get a job's output out.Verification
Two-sided per CLAUDE.md — production changes reverted, tests observed failing for the right reason, then restored. The three
run_init_semanticsreproducers use a full-tree revert; the PTY, reaper, spent-handle, poison-flag and shutdown-exit-code tests use a targeted revert of the single line that causes each bug (stated as such in the design doc rather than called a full two-side).boxliteintegrationboxlitelib unitboxlite-clibin unitThe four CLI failures and the one lib failure are pre-existing, reproduced at the branch's fork point with every change stashed:
127.0.0.1:Pand expect gvproxy's0.0.0.0:Pbind to fail, which it does on Linux (EADDRINUSE) but not on macOS/BSD (SO_REUSEADDRpermits the wildcard bind).auth whoami_updates_stale_profile_path_prefix—whoamidoesn't write the refreshedpath_prefixback.stress_disk bystander_writes_keep_progressing— a throughput threshold, not disk space.jailer::builder::builder_preset_shortcuts_pick_known_profiles— asserts a Linux-onlyseccomp_enabledon macOS;jailer/is untouched by this branch.Passed the repo's CLAUDE.md preflight audit.
Follow-ups (designed, not in this PR)
docs/investigations/init-build-via-zygote.md— init'sbuild()still clone3s on the multi-threaded guest, the last site outside the zygote's fork-safety guarantee.docs/investigations/sdk-run-semantics-api.md— the SDK surface (BoxOptions.cmd/ttyon Node/Python,box.attach(),box.wait(),rt.run()).serveboots a cold box under the registry-wide write lock; a per-box open lock would stop it stalling unrelated handlers.cp-on-a-finished-job deviation.https://claude.ai/code/session_01KYXByfJsxsWktqA2TjqKxi
Summary by CodeRabbit
boxlite runnow follows Docker-like COMMAND/ENTRYPOINT behavior, running the command as PID 1.-tconfigures PTY behavior for the main command at box creation time./attachflow (and reattach with an optional execution id).createcan store a command to run later via explicitstart.inspect, REST responses, and attach/exec lifecycles.