Repository navigation
feat(minimald): implement Shutdown RPC - #613
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughAdds a new oneshot ChangesGraceful Shutdown Feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant serve_shutdown
participant SessionsManager
participant ServerState
Client->>serve_shutdown: ShutdownRequest(force)
serve_shutdown->>SessionsManager: shutdown(force)
alt live sessions and not forced
SessionsManager-->>serve_shutdown: Err(())
serve_shutdown-->>Client: ShutdownResponse::SessionsLive
else proceed
SessionsManager->>SessionsManager: destroy sessions, set in_shutdown
SessionsManager-->>serve_shutdown: Ok(())
serve_shutdown->>ServerState: trigger_shutdown()
ServerState->>ServerState: cancel token, stop accept loop, drain with grace period
serve_shutdown-->>Client: ShutdownResponse::ShuttingDown
end
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/minimald/src/sessions.rs (1)
225-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated
in_shutdownguard.The same
if self.in_shutdown { return Err(SessionsError::new(ConnectionRefused, "in shutdown")); }block is duplicated verbatim inGetSession,CreateSession,RenameSession, andDestroySession. A small helper keeps the error text/kind consistent if it ever needs to change.♻️ Proposed helper
impl<L: Loader> Manager<L> { + fn ensure_not_shutting_down(&self) -> Result<(), SessionsError> { + if self.in_shutdown { + return Err(SessionsError::new( + std::io::ErrorKind::ConnectionRefused, + "in shutdown", + )); + } + Ok(()) + } + fn key_for(&self, pred: &SessionKeyPredicate) -> Result<Option<L::Key>, std::io::Error> {Then each guard site becomes
self.ensure_not_shutting_down()?;.Also applies to: 284-290, 340-345, 394-399
🤖 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/minimald/src/sessions.rs` around lines 225 - 230, Extract the repeated in_shutdown guard in Sessions by adding a small helper on the session manager (for example, ensure_not_shutting_down) that returns the existing SessionsError with ConnectionRefused and "in shutdown", then replace the duplicated checks in GetSession, CreateSession, RenameSession, and DestroySession with calls to that helper. Keep the helper’s error kind and message identical so all four paths stay consistent.
🤖 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 `@crates/minimald/src/sessions.rs`:
- Around line 432-449: Forced shutdown in ManagerMessage::Shutdown currently
destroys live sessions without first deregistering HostNet hostnames, unlike
DestroySession. Update the shutdown teardown path in sessions.rs to mirror the
deregistration step used by DestroySession: before calling hnd.destroy() for
each live session, remove the associated hostname entries from self.hostnames so
HostNet sessions do not leave stale mappings during the shutdown drain window.
Use the existing shutdown loop in ManagerMessage::Shutdown and the
session/hostname tracking logic around self.running and self.hostnames to place
the deregistration in the same teardown flow.
- Around line 432-449: ManagerMessage::Shutdown currently waits on each
SessionHandle::destroy() sequentially with no timeout, so one stuck session can
block shutdown indefinitely. Update the shutdown path in
ManagerMessage::Shutdown to use a bounded timeout or concurrent teardown for all
entries in self.running, and make sure self.in_shutdown is still set before
teardown begins. If you add helper logic, keep it anchored around
SessionHandle::destroy() and the ManagerMessage::Shutdown match arm so the
shutdown behavior remains controlled and cannot hang forever.
---
Nitpick comments:
In `@crates/minimald/src/sessions.rs`:
- Around line 225-230: Extract the repeated in_shutdown guard in Sessions by
adding a small helper on the session manager (for example,
ensure_not_shutting_down) that returns the existing SessionsError with
ConnectionRefused and "in shutdown", then replace the duplicated checks in
GetSession, CreateSession, RenameSession, and DestroySession with calls to that
helper. Keep the helper’s error kind and message identical so all four paths
stay consistent.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 76538df8-cf3b-4147-8255-1b61352a166e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
crates/minimald-rpc/src/lib.rscrates/minimald/Cargo.tomlcrates/minimald/src/rpc.rscrates/minimald/src/server.rscrates/minimald/src/sessions.rscrates/minimald/src/test_harness.rs
0chroma
approved these changes
Jul 2, 2026
Contributor
There was a problem hiding this comment.
LGTM! well thought out ^^
0chroma
added a commit
that referenced
this pull request
Jul 6, 2026
Adds a `minimal stop` command that sends the Shutdown RPC (PR #613) to the running minimald daemon. With no flags it requests a graceful shutdown — the daemon refuses if any session is live. Pass --force to tear down live sessions and shut down regardless. Includes three integration tests covering the no-sessions, refused, and forced paths. Also adds a bring_session_up helper to the test harness so tests can make sessions "live" for shutdown scenarios.
0chroma
added a commit
that referenced
this pull request
Jul 8, 2026
Adds a `minimal stop` command that sends the Shutdown RPC (PR #613) to the running minimald daemon. With no flags it requests a graceful shutdown — the daemon refuses if any session is live. Pass --force to tear down live sessions and shut down regardless. Includes three integration tests covering the no-sessions, refused, and forced paths. Also adds a bring_session_up helper to the test harness so tests can make sessions "live" for shutdown scenarios.
0chroma
added a commit
that referenced
this pull request
Jul 8, 2026
…nds, test harness, stop (#612) * feat(minimal): add CLI subcommands, test harness, and lib/main split Rebase the minimal2 CLI work onto main, adapting to the crate rename (minimal2 → minimal, minimal → mip). Combines the following features: - version, rename, init, add, and update subcommands - --json and --raw output flags for the ls command - anyhow-based error handling replacing ad-hoc eprintln!+Err(()) - resolve_session helper for UUID/name session lookups - integration test harness with 14 CLI tests exercising real daemon - lib/main split so integration tests can call cmd_* functions directly - minimald test-support feature exposing TestServer/TestClient externally - merged main's session-creation flow (SessionConfig, Pending/Ready, AbortOnUnapproved, drive_pending_to_active, send_abort) into the anyhow-based error handling - client.rs socket path now uses paths::minimal_state_dir() * feat(minimal): add stop subcommand to shut down minimald Adds a `minimal stop` command that sends the Shutdown RPC (PR #613) to the running minimald daemon. With no flags it requests a graceful shutdown — the daemon refuses if any session is live. Pass --force to tear down live sessions and shut down regardless. Includes three integration tests covering the no-sessions, refused, and forced paths. Also adds a bring_session_up helper to the test harness so tests can make sessions "live" for shutdown scenarios. * refactor(minimal): apply code review fixes to lib/main split Reduce duplication and improve test coverage after the lib/main split: extract SessionLookup/ensure_daemon helpers, make parse_ingress return anyhow::Error, privatize shell_quote, and capture stdout in ls integration tests via a new format_ls function. * fix(minimal): gate build-system commands to Linux for macOS compatibility The init/add/update subcommands depend on mctx → hakoniwa → libcgroups → procfs, which only compiles on Linux. Gating mctx/op behind target_os = "linux" keeps the minimal binary buildable on macOS, fixing the autospawn-e2e CI lane that builds and runs minimal on the self-hosted Apple Silicon runner.
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
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.
Tells the minimald to shutdown.
force=false- will error if there are active sessions.force=true- will kill active sessions/connections, drain them, then exit.Summary by CodeRabbit
New Features
ShutdownRPC with aforceoption.Bug Fixes
Tests
Chores