feat(guest): supervise container init lifecycle#982
Open
BatmanByte wants to merge 3 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds lifecycle-generation tracking and generation-scoped wait responses, introduces zygote-based init construction with supervised termination handling, continuously drains bounded init output tails, and adds tests for non-blocking large stdout and stderr writes. ChangesLifecycle-scoped init supervision
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant BoxBuilder
participant ContainerService
participant Container
participant Zygote
participant InitSupervisor
participant ContainerWaitClient
BoxBuilder->>ContainerService: send lifecycle generation
ContainerService->>Container: start init
Container->>Zygote: build init with stdio fds
Zygote-->>Container: return init build result
Container->>InitSupervisor: supervise PID and output completion
ContainerWaitClient->>ContainerService: Wait(container_id, lifecycle_generation)
ContainerService->>InitSupervisor: await terminal outcome
InitSupervisor-->>ContainerService: exit, signal, or failure
ContainerService-->>ContainerWaitClient: ContainerWaitResponse
Possibly related PRs
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
BatmanByte
marked this pull request as ready for review
July 13, 2026 07:39
📦 BoxLite review — 1 issues ·
|
Container init stdout and stderr were only read during diagnostics, so output beyond the pipe capacity could block the entrypoint indefinitely. Start dedicated bounded drain threads when the pipes are created and retain the latest 4 KiB for diagnostics. The diagnostic accessor now returns a snapshot instead of consuming pipe readers, allowing repeated failure reports without waiting for inherited writers to close. Add a VM regression test that writes 1 MiB before proving the entrypoint continued.
BatmanByte
force-pushed
the
codex/init-stdio-drain
branch
from
July 13, 2026 07:42
e9de25a to
bd36b9b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/guest/src/container/stdio.rs`:
- Around line 41-42: Remove the top-level cfg(test) std::os::unix::io::AsRawFd
import in stdio.rs. Keep the existing AsRawFd import inside the tests module
unchanged, since it is the import used by the tests.
🪄 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: 5ed1d573-a7d8-4f43-ac8c-aef9812dd071
📒 Files selected for processing (4)
src/boxlite/tests/init_stdio.rssrc/guest/src/container/lifecycle.rssrc/guest/src/container/stdio.rssrc/guest/src/service/exec/mod.rs
Create init through the pre-thread zygote and publish its terminal result through Container.Wait. The supervisor is the sole zygote waiter and only completes after stdout and stderr reach EOF, so a reconnecting host can retrieve an already-published result without losing the local output tail. Persist a monotonic lifecycle generation before each VM run and include it in Container.Init. Wait rejects stale generations, preventing an old monitor from observing a later init run.
BatmanByte
changed the title
![boxlite-agent[bot]](https://avatars.githubusercontent.com/in/4244458?s=60&v=4){kind=small}
Comment on lines
+126
to
+149
| let mut supervisors = self.init_supervisors.lock().await; | ||
| match supervisors.get(&container_id) { | ||
| Some(existing) if !existing.matches_generation(lifecycle_generation) => { | ||
| return Ok(Response::new(ContainerInitResponse { | ||
| result: Some(container_init_response::Result::Error(ContainerInitError { | ||
| reason: | ||
| "Container init already exists for another lifecycle generation" | ||
| .to_string(), | ||
| })), | ||
| })); | ||
| } | ||
| Some(_) => { | ||
| return Ok(Response::new(ContainerInitResponse { | ||
| result: Some(container_init_response::Result::Error(ContainerInitError { | ||
| reason: "Container init is already in progress".to_string(), | ||
| })), | ||
| })); | ||
| } | ||
| None => { | ||
| let supervisor = InitSupervisor::new(lifecycle_generation); | ||
| supervisors.insert(container_id.clone(), supervisor.clone()); | ||
| supervisor | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 init_supervisors map has no eviction
Failed/terminal supervisor entries are never removed from the HashMap; low risk today since container_id is unique per VM boot, but no cleanup path exists if that assumption changes.
Contributor
Author
There was a problem hiding this comment.
This registry is scoped to one guest VM, and each Box owns exactly one container. It is destroyed with the guest on stop/restart. The Container is retained while that VM lives for diagnostic output tails, so terminal eviction would either be dead code or prematurely discard diagnostics. No cleanup change is needed under the current one-container-per-VM model.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/guest/src/container/zygote.rs (1)
615-622: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a roundtrip test for
InitBuildResult::Failed.Only
Builtis tested; the analogousBuildResulttype tests bothSpawnedandFailedvariants (build_result_spawned_serde_roundtrip,build_result_failed_serde_roundtrip).Failed { error }carries opaque error text across the IPC boundary, so it's worth the same coverage.♻️ Proposed addition
#[test] fn init_build_result_failed_serde_roundtrip() { let result = InitBuildResult::Failed { error: "init build failed: container not found".to_string(), }; let json = serde_json::to_vec(&result).unwrap(); let decoded: InitBuildResult = serde_json::from_slice(&json).unwrap(); assert_eq!(result, decoded); }🤖 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/zygote.rs` around lines 615 - 622, Add a test alongside init_build_result_serde_roundtrip named init_build_result_failed_serde_roundtrip that constructs InitBuildResult::Failed with representative error text, serializes it with serde_json, deserializes it, and asserts the original and decoded values are equal.
🤖 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/guest/src/service/container.rs`:
- Around line 124-150: The init supervisor registry in the Container.Init flow
must allow later lifecycle generations after a prior supervisor terminates.
Update the storage and lookup logic around init_supervisors and InitSupervisor
so supervisors are keyed by both container_id and lifecycle_generation, or
rotate terminal supervisors while preserving generation-specific results for
reconnecting waiters; retain rejection only for an active conflicting generation
or duplicate in-progress init.
---
Nitpick comments:
In `@src/guest/src/container/zygote.rs`:
- Around line 615-622: Add a test alongside init_build_result_serde_roundtrip
named init_build_result_failed_serde_roundtrip that constructs
InitBuildResult::Failed with representative error text, serializes it with
serde_json, deserializes it, and asserts the original and decoded values are
equal.
🪄 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: 8c9337dc-20ca-4d46-8f21-d5a6e7603a81
📒 Files selected for processing (15)
src/boxlite/src/litebox/box_impl.rssrc/boxlite/src/litebox/init/mod.rssrc/boxlite/src/litebox/init/tasks/guest_init.rssrc/boxlite/src/litebox/init/types.rssrc/boxlite/src/litebox/state.rssrc/boxlite/src/portal/interfaces/container.rssrc/guest/src/container/lifecycle.rssrc/guest/src/container/mod.rssrc/guest/src/container/start.rssrc/guest/src/container/stdio.rssrc/guest/src/container/supervisor.rssrc/guest/src/container/zygote.rssrc/guest/src/service/container.rssrc/guest/src/service/server.rssrc/shared/proto/boxlite/v1/service.proto
🚧 Files skipped from review as they are similar to previous changes (1)
- src/guest/src/container/stdio.rs
DorianZheng
added
the
e2e-local
DorianZheng
added a commit
that referenced
this pull request
Jul 14, 2026
) ## Summary [Run 29324915498](https://github.com/boxlite-ai/boxlite/actions/runs/29324915498/job/87061427258) failed on fork PR #982: GitHub withholds secrets **and** repository variables from fork `pull_request` runs (`Secret source: None`), so the maintainer-applied `e2e-local` label sent the run into `start-runner`, where `create-github-app-token` failed with "Input required and not supplied: app-id". The label contract ("label ⇒ e2e runs") was implemented on an event type that structurally cannot honor it for forks. Switch the PR trigger to label-gated `pull_request_target` (base context: credentials available, workflow definition always from `main`), hardened per the standard pattern (protobuf `test_runner.yml`, GitHub security-hardening guidance): - **Per-push fork approval** — a fork PR runs only on the `e2e-local` labeling event, pinned to the labeled head SHA; pushes after approval are skipped with a notice until a maintainer re-labels. Same-repo PRs keep today's behavior (re-run on every push while labeled). - **Checkout pins `head.sha`** — `pull_request_target` resolves to the base branch by default, which would silently test `main`; `persist-credentials: false` so no token remains on the persistent runner when fork code runs. - **Read-only default token** — top-level `permissions: contents: read` (the `pull_request_target` default is write-capable); `should-run` gate inputs move from inline `${{ }}` interpolation into `env:`. Blast radius of an approved-then-malicious fork run stays contained: secrets exist only in `start-runner`/`stop-runner` (GitHub-hosted, never execute PR code), and the runner's instance profile allows only self-termination (`scripts/ci/setup-ci-runner.sh`). ## Verification - `actionlint`: identical 2 pre-existing findings as HEAD's version — zero new findings - Gate behavior: the `should-run` script extracted from the YAML and executed across 9 event scenarios passes 9/9 (push/dispatch run; same-repo labeled runs on label+synchronize; fork runs only on the `e2e-local` labeling event; stale/unapproved fork events skip) - Regression side: HEAD's gate script, given the failing run's exact inputs (`pull_request`, fork, `e2e-local` present), returns `run=true` — the path into the credential-less crash After merge: remove and re-add `e2e-local` on #982 to run e2e against its current head. https://claude.ai/code/session_01HkVMvjNbCKPrbwxps95jTp Co-authored-by: tester <t@t.test>
DorianZheng
added
e2e-local
This was referenced Jul 16, 2026
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.
Summary
Turn #982 into PR1 of the container-exit design: the guest now owns one init supervisor that observes the init process and publishes a terminal result only after stdout and stderr have drained. This is the guest-side foundation for #933; the shim's durable outcome file and host-state reconciliation remain follow-up work. Design context: Discussion #967.
Changes
Container.Wait; same-generation not-ready requests are retryable, while stale generations fail precondition. Reconnected waiters return an already-published outcome immediately.lifecycle_generationbefore each new VM run, then include it inContainer.Init.How to verify
make fmt:check:rustmake guestmake test:unit:rust FILTER=lifecycle_generationmake test:integration:rust FILTER=init_stdout_larger_than_a_pipe_does_not_block_entrypoint(requires a host wherekern.hv_supportis available)Risks / rollout
Container.Waitis guest-only in this PR. The next PR adds the shim monitor, crash-durable outcome file, and host-side reconciliation before exposing natural init exit as a runtime status.Summary by CodeRabbit