fix(minimal): gate min stop on the daemon being down, not on the RPC transport - #1011
Conversation
…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>
📝 WalkthroughWalkthrough
ChangesShutdown outcome handling
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/minimal/src/lib.rs (1)
2944-2956: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider 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), adaemon_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
📒 Files selected for processing (2)
crates/minimal/src/lib.rsscripts/session-e2e.sh
Root cause
min stopgated success on the RPC transport, not on the thingstopactually 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:Summary by CodeRabbit
min stophandling for successful shutdowns, active sessions, lost connections, and already-stopped daemons.