Skip to content

fix(minimal): gate min stop on the daemon being down, not on the RPC transport - #1011

Merged
norrietaylor merged 3 commits into
mainfrom
fix/inbox-363-min-stop-vm-exit-code
Jul 29, 2026
Merged

fix(minimal): gate min stop on the daemon being down, not on the RPC transport#1011
norrietaylor merged 3 commits into
mainfrom
fix/inbox-363-min-stop-vm-exit-code

Conversation

@norrietaylor

@norrietaylor norrietaylor commented Jul 29, 2026

Copy link
Copy Markdown
Member

Root cause

min stop gated success on the RPC transport, not on the thing stop
actually promises: the daemon being down.

On a VM target the daemon is the guest's pid-1, so an accepted shutdown takes
the SSH transport down with it — as a consequence of succeeding. The daemon
fires trigger_shutdown() before its reply is written:

// crates/minimald/src/rpc.rs:445-450
// Manager is down; tell the accept loop to stop and drain
// so the process can exit. Firing before the response is
// written is safe: the drain waits out the grace period,
// so this reply still flushes to the client.
s.trigger_shutdown().await;
ShutdownResponse::ShuttingDown

Summary by CodeRabbit

  • Bug Fixes
    • Improved min stop handling for successful shutdowns, active sessions, lost connections, and already-stopped daemons.
    • Prevented shutdown waits from blocking other asynchronous operations.
    • Preserved and displayed shutdown error details during session end-to-end checks, making failures easier to diagnose.

norrietaylor and others added 2 commits July 28, 2026 18:26
…C transport

`min stop` succeeds when the daemon is stopped, so judge it by that rather
than by the liveness of the transport that carried the request.

On a VM target minimald IS the guest's pid-1: an accepted Shutdown tears the
VM — and with it the SSH transport — down as a direct consequence of
succeeding. The client drains the channel and only then decodes what it
collected, so a reply lost on the way out decodes an empty buffer and fails
with "decode response for shutdown". Since #919 stopped swallowing RPC and
connection errors that surfaces as a non-zero exit for a stop that did
exactly what was asked — the assertion the nightly session-e2e-soak trips.

A failed Shutdown RPC no longer bails on the spot. It falls through to the
same wait an accepted shutdown runs, then re-asks the liveness probe the
command opened with: a native minimald has no lifecycle state to poll, so
its wait returns at once saying nothing, and only the probe can answer for
that backend. A daemon confirmed down is a successful stop; one still there
returns the original RPC error, context intact.

Only that failure arm probes. An accepted shutdown is still judged by its
wait alone, because the daemon acknowledges before it has finished going
down and asking again there would race its own teardown. SessionsLive keeps
its message and its non-zero exit, and the already-down fast path is
untouched.

The e2e's stop assertion now keeps stderr instead of discarding it: it threw
away the one piece of text that says why a stop failed, which is why this
needed a hypothesis rather than a log.

Closes gominimal/inbox#363

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Surface the RPC error the recovered stop suppresses, and make the
observation behind that recovery testable.

- The failed-RPC-but-daemon-down arm exited 0 silently, discarding the
  error that explains what went wrong. Print it on stderr, which is what
  the e2e script now captures and what scripted callers' >/dev/null
  leaves intact.
- Extract the liveness observation into daemon_confirmed_stopped so the
  real probe -- not a stub -- is exercised by a test, and document that
  it is a VM-backend recovery in practice: minimald holds its listener
  through SHUTDOWN_GRACE, so the native probe reports "running".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

min stop now centralizes shutdown outcome handling, confirms successful daemon termination after failed RPCs, avoids waiting on active-session refusals, and runs blocking waits off the async executor. Tests cover these cases, while the e2e script preserves stop errors for diagnostics.

Changes

Shutdown outcome handling

Layer / File(s) Summary
Shutdown outcome semantics
crates/minimal/src/lib.rs
stop_outcome distinguishes accepted shutdowns, active-session refusals, and failed RPCs; daemon_confirmed_stopped verifies termination using observation and liveness checks.
Stop integration and validation
crates/minimal/src/lib.rs, scripts/session-e2e.sh
cmd_stop drops the connection before waiting, async tests cover the outcome paths, and e2e failures now print captured stderr.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related issues

  • Issue 363 — Addresses the reported min stop failure when the daemon stops but the shutdown RPC connection is lost.

Possibly related PRs

Suggested reviewers: bryan-minimal

Poem

A rabbit watched the daemon fade,
While lost replies no longer stayed.
“If it is down, success!” I cheer,
And print the clues when faults appear.
Hop, stop, and rest—no waiters near.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description is incomplete for the required template; it only includes Root cause and omits Summary, Testing, and Checklist sections. Add the missing Summary, Testing, and Checklist sections, and include any relevant issue refs plus docs/BREAKING CHANGE notes if applicable.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes match issue #363 by treating daemon shutdown as success on liveness, preserving active-session refusal, and keeping stderr diagnostics.
Out of Scope Changes check ✅ Passed The diff stays focused on min stop shutdown handling and its E2E diagnostics, with no clear unrelated changes.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title is a clear conventional commit subject and accurately summarizes the main change.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
crates/minimal/src/lib.rs (1)

2944-2956: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider adding a native-backend case for daemon_confirmed_stopped.

Only daemon_confirmed_stopped(true, ...) (VM/minvmd) is exercised directly; given the doc comment on Lines 2159-2171 spends significant detail contrasting native vs VM behavior (native's probe "fires once, immediately" while a listener may still be bound during the drain grace), a daemon_confirmed_stopped(false, ...) case would close that documented gap.

🤖 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 `@crates/minimal/src/lib.rs` around lines 2944 - 2956, Add a Tokio test
alongside `the_real_probe_confirms_a_daemon_that_is_not_there` that calls
`daemon_confirmed_stopped(false, Some(...))` with a fresh temporary state
directory and asserts confirmation succeeds, covering the native-backend
behavior described by the function documentation.
🤖 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.

Nitpick comments:
In `@crates/minimal/src/lib.rs`:
- Around line 2944-2956: Add a Tokio test alongside
`the_real_probe_confirms_a_daemon_that_is_not_there` that calls
`daemon_confirmed_stopped(false, Some(...))` with a fresh temporary state
directory and asserts confirmation succeeds, covering the native-backend
behavior described by the function documentation.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 59a9bbd3-8b0d-465f-a874-69c8eb9739ac

📥 Commits

Reviewing files that changed from the base of the PR and between 5aba97a and 3bd2391.

📒 Files selected for processing (2)
  • crates/minimal/src/lib.rs
  • scripts/session-e2e.sh

@norrietaylor
norrietaylor merged commit d8c2f47 into main Jul 29, 2026
46 of 49 checks passed
@norrietaylor
norrietaylor deleted the fix/inbox-363-min-stop-vm-exit-code branch July 29, 2026 04:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants