8000 fix(minvmd): reap VMM child when MarkRunning fails by gominimal-aw-bot[bot] · Pull Request #1841 · gominimal/minimal · GitHub
[go: up one dir, main page]
Skip to content

fix(minvmd): reap VMM child when MarkRunning fails - #1841

Merged
norrietaylor merged 1 commit into
mainfrom
inbox-patch/reap-vmm-on-markrunning-failure-47bca97f762be732
Oct 3, 2026
Merged

fix(minvmd): reap VMM child when MarkRunning fails#1841
norrietaylor merged 1 commit into
mainfrom
inbox-patch/reap-vmm-on-markrunning-failure-47bca97f762be732

Conversation

@gominimal-aw-bot
@gominimal-aw-bot gominimal-aw-bot Bot commented Oct 3, 2026 •
edited by coderabbitai Bot
Loading
Copy link
Copy Markdown
Contributor

Routing-Key: inbox-route/I_kwDOSUhdos8AAAABUt9rQw

The supervisor spawns the __krun-vmm child, then in Phase 2 takes the
lifecycle lock and transitions Starting → Running. If that transition or the
subsequent write_state fails, the supervisor returned via ? without
killing or reaping child_pid, leaving an orphaned VMM process.

Wrap the Phase 2 block in a closure and, on error, kill and wait the child
before returning — matching the existing boot_result and spawn-race cleanup
paths. The StartingGuard still resets state to Stopped on drop.

Verification

  • cargo fmt --all --check — clean
  • cargo build -p minvmd --locked — clean
  • cargo test -p minvmd --locked — 5 passed, 0 failed
  • cargo clippy -p minvmd --locked -- -D warnings — clean for minvmd
    (workspace-wide clippy fails on pre-existing args/remote-proto lints
    under the Rust 1.99 toolchain, unrelated to this change)

Summary by CodeRabbit

  • Bug Fixes
    • Improved cleanup when a virtual machine fails to start. The system now stops the partially launched instance and restores its status to stopped before reporting the error, reducing the chance of lingering processes or an inconsistent status after a failed startup.

@gominimal-aw-bot
fix(minvmd): reap VMM child when MarkRunning fails
6a96e25 
The supervisor spawns the VMM child, then in Phase 2 takes the
lifecycle lock and transitions Starting -> Running. If that
transition or the subsequent write_state fails, the supervisor
returned via `?` without killing or reaping child_pid, leaving an
orphaned __krun-vmm process.

Wrap the Phase 2 block in a closure and, on error, kill and wait
the child before returning, matching the existing boot_result and
spawn-race cleanup paths. The StartingGuard still resets state to
Stopped on drop.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@gominimal-aw-bot
gominimal-aw-bot Bot requested a review from a team as a code owner October 3, 2026 02:15
@gominimal-aw-bot gominimal-aw-bot Bot added the agent:fix Agentic fix PR eligible for review iteration; min-aw's worker-iterate gates on it. label Oct 3, 2026
@coderabbitai
coderabbitai Bot commented Oct 3, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Organization UI
  • Review profile: CHILL
  • Plan: Essentials
  • Run ID: 247d1b52-4801-4bc2-95e5-9e04278c42ec
📥 Commits

Reviewing files that changed from the base of the PR and between b176b33 and 6a96e25.

📒 Files selected for processing (1)
  • crates/minvmd/src/cmd/run.rs

Included review availability: This review used your included allowance. 4 included reviews remain after this review. Your included PR review attempts over the past 7 days set your current allowance at 5 reviews per hour.

📝 Walkthrough

Walkthrough

The supervisor now handles failures during the transition from Starting to Running by killing and waiting for the VMM child. The StartingGuard then resets lifecycle state to Stopped.

Changes

VMM startup cleanup

Layer / File(s) Summary
Running transition error handling
crates/minvmd/src/cmd/run.rs		 If the transition to Running fails, the supervisor kills and waits for the VMM child before returning the error. The StartingGuard resets state to Stopped.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: norrietaylor

Merge Risk: ⚪ Minimal · up to 6a96e

The startup cleanup change is mergeable after normal checks; no actionable merge-blocking issue was established.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly and concisely describes the main change: reaping the VMM child when marking it as Running fails.
Description check ✅ Passed The description explains the failure scenario and cleanup behavior, and it provides verification results. It uses “Verification” instead of the template’s “Testing” heading and omits the checklist, bu…
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

A rabbit watches startup begin
If the state change fails, cleanup steps in
The child is stopped and waits
The guard resets the state
Then quiet returns again

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

@norrietaylor norrietaylor changed the title fix: fix(minvmd): reap VMM child when MarkRunning fails fix(minvmd): reap VMM child when MarkRunning fails Oct 3, 2026
@gominimal-aw-bot gominimal-aw-bot Bot added the agent:resolve Consent gate: pr-conflict-resolver may rewrite this branch label Oct 3, 2026
norrietaylor
norrietaylor approved these changes Oct 3, 2026
View reviewed changes
@norrietaylor norrietaylor left a comment
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Defect validated against main (run.rs Phase 2 ? paths leak the VMM child); independent review approved; all lanes green.

@norrietaylor
norrietaylor added this pull request to the merge queue Oct 3, 2026
Merged via the queue into main with commit 9125f6a Oct 3, 2026
33 checks passed
@norrietaylor
8000 norrietaylor deleted the inbox-patch/reap-vmm-on-markrunning-failure-47bca97f762be732 branch October 3, 2026 05:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@norrietaylor norrietaylor norrietaylor approved these changes
@coderabbitai coderabbitai[bot] coderabbitai[bot] approved these changes

Assignees

No one assigned

Labels

agent:fix Agentic fix PR eligible for review iteration; min-aw's worker-iterate gates on it. agent:resolve Consent gate: pr-conflict-resolver may rewrite this branch

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@norrietaylor
0