Make installer gracefully stop a running instance during install - #768
Conversation
📝 WalkthroughWalkthroughThe CLI now probes daemon liveness before stopping, while the installer stops an existing daemon once before replacing component files. Tests and installer specifications cover lifecycle states, socket behavior, upgrade scenarios, and suppressed stop failures. ChangesDaemon lifecycle handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant LivenessProbe
participant Daemon
CLI->>LivenessProbe: check daemon state
LivenessProbe->>Daemon: inspect lifecycle or socket
Daemon-->>LivenessProbe: liveness result
LivenessProbe-->>CLI: running or not running
CLI->>Daemon: request shutdown when running
sequenceDiagram
participant Installer
participant InstalledMin
participant ComponentFiles
Installer->>InstalledMin: run stop --force once
InstalledMin-->>Installer: ignored status and suppressed output
Installer->>ComponentFiles: replace components
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/minimal/src/autospawn.rs (1)
258-310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBackend-selection duplication between
is_daemon_runningandensure_daemon_running.The macOS/Linux branching here mirrors
ensure_daemon_running(lines 236-249) line-for-line. The doc comment even calls this out as load-bearing ("the two cannot disagree about what running means"), but that guarantee is only true as long as both copies are kept manually in sync. A future edit to one without the other would silently break the invariant.Consider extracting a small shared backend-selection helper (e.g. an enum
Backend { Minvmd, Native(PathBuf) }resolved once fromuse_minvmd/minimal_dir) that bothensure_daemon_runningandis_daemon_runningdelegate to, so divergence becomes structurally impossible rather than just documented.🤖 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/autospawn.rs` around lines 258 - 310, Extract the duplicated backend-selection logic from ensure_daemon_running and is_daemon_running into one shared resolver, such as a Backend enum containing Minvmd or the resolved native socket path. Have both functions use this resolver while preserving the existing platform-specific behavior and probe semantics, so backend selection cannot diverge between the two call paths.
🤖 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/autospawn.rs`:
- Around line 258-310: Extract the duplicated backend-selection logic from
ensure_daemon_running and is_daemon_running into one shared resolver, such as a
Backend enum containing Minvmd or the resolved native socket path. Have both
functions use this resolver while preserving the existing platform-specific
behavior and probe semantics, so backend selection cannot diverge between the
two call paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 42c29ba8-b231-472b-b965-ab35d3705ea1
📒 Files selected for processing (5)
crates/minimal/src/autospawn.rscrates/minimal/src/lib.rsdocs/specs/07-spec-installer/07-spec-installer.mdscripts/install.shscripts/install_test.sh
min stopif it existsmin stoptiming out and complaining if the daemon or minvmd isnt running.Summary by CodeRabbit
Bug Fixes
min stopnow exits successfully with a clear message when the daemon is already stopped.Installer Improvements