feat(minimald): implement session destroy + RPC - #462
Merged
Conversation
📝 WalkthroughWalkthroughThis PR implements end-to-end session destruction. The ChangesSession Destruction End-to-End
Sequence Diagram(s)sequenceDiagram
participant Client
participant ManagerHandle
participant Manager
participant SessionHandle
participant SessionActor
participant HostHandle
participant DiskLoader
rect rgba(70, 130, 180, 0.5)
note over Client,ManagerHandle: Public API call
Client->>ManagerHandle: destroy_session(id)
ManagerHandle->>Manager: DestroySession(id, responder)
end
rect rgba(180, 100, 70, 0.5)
note over Manager,HostHandle: Cascading teardown
Manager->>Manager: resolve id → key
alt session running
Manager->>SessionHandle: destroy().await
SessionHandle->>SessionActor: Destroy(oneshot_tx)
SessionActor->>HostHandle: kill()
HostHandle-->>SessionActor: mainloop JoinHandle resolves
SessionActor-->>SessionHandle: oneshot_tx.send(())
end
end
rect rgba(70, 180, 100, 0.5)
note over Manager,Client: Disk cleanup and response
Manager->>DiskLoader: delete(key)
DiskLoader-->>Manager: Ok(())
Manager-->>Client: Ok(())
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
crates/minimald/src/sessions.rs (1)
361-394: ⚡ Quick winExercise destroy with an attached host, not just a running actor.
This test populates
running, butSession::hostremainsNone; it will not catch regressions in theHostHandle::kill/ hostJoinHandleteardown path. Add a manager-level test that opens a shell, callsdestroy_session(id), and asserts the channel closes and the session no longer resolves.🤖 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 361 - 394, Add a new separate async tokio test function that exercises the destroy path with an attached host. In this test, create a session using the manager, open a shell on the session (which attaches a host), then call destroy_session with that session id. After destroying, assert that the session channel is closed and that subsequent calls to get_session no longer resolve the session. This will exercise the HostHandle::kill and host JoinHandle teardown path that the current destroy_tears_down_a_running_session test does not cover.
🤖 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/session_host.rs`:
- Around line 822-826: The notify_remote_pty_err method is using tx.send().await
which can cause a deadlock during teardown if the binding queue is full or
blocked forwarding data. Replace the awaited tx.send() call with try_send() when
sending BindingMsg::TeardownDueToStdoutErr(e) to avoid blocking the host thread
during teardown. This allows the teardown notification to be sent best-effort
without creating a circular wait condition between the host and binding.
In `@crates/minimald/src/session.rs`:
- Around line 138-141: The SessionMessage::Destroy handler silently discards
errors from both the self.destroy().await operation and the r.send(()) call
without justification, violating Rust error handling standards. Since the
manager deletes the session's disk tree immediately after destroy returns,
teardown failures must be propagated before that deletion occurs. Replace the
current pattern in the Destroy handler to check and propagate errors from both
self.destroy().await and r.send(), ensuring that success is only acknowledged if
both operations actually succeed rather than silently discarding the results.
Apply the same error propagation fix to the similar patterns mentioned at lines
153-161 and 320-328.
In `@crates/sessions/src/store.rs`:
- Around line 89-96: The delete() method currently returns NotFound when the
session directory is already missing, leaving stale index entries in short_to_id
and name_to_id behind, which violates the documented contract that a missing
directory tree is not an error. Fix the delete() implementation (around line 389
where record.json is read) to handle the case where the directory is already
gone by either reading the record after index cleanup or catching the NotFound
error and proceeding with index cleanup regardless, ensuring that stale index
entries are always removed even when the directory tree is missing.
---
Nitpick comments:
In `@crates/minimald/src/sessions.rs`:
- Around line 361-394: Add a new separate async tokio test function that
exercises the destroy path with an attached host. In this test, create a session
using the manager, open a shell on the session (which attaches a host), then
call destroy_session with that session id. After destroying, assert that the
session channel is closed and that subsequent calls to get_session no longer
resolve the session. This will exercise the HostHandle::kill and host JoinHandle
teardown path that the current destroy_tears_down_a_running_session test does
not cover.
🪄 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: 83537a3d-953e-4233-8162-35945b18f274
📒 Files selected for processing (4)
crates/minimald/src/session.rscrates/minimald/src/session_host.rscrates/minimald/src/sessions.rscrates/sessions/src/store.rs
Comment on lines
+138
to
+141
| SessionMessage::Destroy(r) => { | ||
| self.destroy().await; | ||
| let _ = r.send(()); | ||
| return ControlFlow::Break(()); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C4 'SessionMessage::Destroy|async fn destroy|task\.await|destroy_session' crates/minimald/src/session.rs crates/minimald/src/sessions.rsRepository: gominimal/minimal
Length of output: 6873
🏁 Script executed:
cat crates/minimald/src/session.rs | head -170 | tail -40Repository: gominimal/minimal
Length of output: 1479
🏁 Script executed:
find . -name "rust-coding-standards.md" -o -name "*coding*standards*" | head -20Repository: gominimal/minimal
Length of output: 93
🏁 Script executed:
rg -n "SessionMessage::Destroy" crates/minimald/src/session.rs -A 10 -B 5Repository: gominimal/minimal
Length of output: 1350
🏁 Script executed:
cat docs/rust-coding-standards.mdRepository: gominimal/minimal
Length of output: 4815
🏁 Script executed:
rg -n "destroy_session" crates/minimald/src/sessions.rs -A 15 | head -50Repository: gominimal/minimal
Length of output: 1977
🏁 Script executed:
grep -n "store.delete\|destroy.*await" crates/minimald/src/sessions.rs | head -20Repository: gominimal/minimal
Length of output: 264
Propagate host teardown failures before acknowledging destroy — violates error handling standards.
The Rust coding standards explicitly forbid silent error discards without justification ("Never swallow errors. No let _ = result; without a comment justifying it"). Lines 138–141 and 153–161 discard JoinHandle results, causing SessionMessage::Destroy to acknowledge success even if the host task panicked or mainloop() failed. This breaks the semantic contract: the manager (sessions.rs:223–225) deletes the session's disk tree immediately after destroy returns, so teardown failures must propagate before that deletion occurs.
Suggested shape
- Destroy(oneshot::Sender<()>),
+ Destroy(oneshot::Sender<Result<(), std::io::Error>>), SessionMessage::Destroy(r) => {
- self.destroy().await;
- let _ = r.send(());
+ let result = self.destroy().await;
+ let _ = r.send(result);
return ControlFlow::Break(());
}- async fn destroy(&mut self) {
+ async fn destroy(&mut self) -> Result<(), std::io::Error> {
if let Some((host, task)) = self.host.take() {
// Signal the process to die, then await the runtime loop so the
// sandbox files backing its rootfs are released before the caller
// removes the session's directory tree.
let _ = host.kill().await;
- let _ = task.await;
+ let _exit = task.await.map_err(std::io::Error::other)??;
}
+ Ok(())
}- pub(crate) async fn destroy(&self) {
+ pub(crate) async fn destroy(&self) -> Result<(), std::io::Error> {
let (send, recv) = oneshot::channel();
// Ignore send errors - the recv will also fail.
let _ = self.0.send(SessionMessage::Destroy(send)).await;
// If the actor died before acking, the session is gone all the same.
- let _ = recv.await;
+ recv.await.unwrap_or(Ok(()))
}The manager caller should then propagate hnd.destroy().await? before store.delete(&k)?.
Also applies to lines 153–161 and 320–328.
🤖 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/session.rs` around lines 138 - 141, The
SessionMessage::Destroy handler silently discards errors from both the
self.destroy().await operation and the r.send(()) call without justification,
violating Rust error handling standards. Since the manager deletes the session's
disk tree immediately after destroy returns, teardown failures must be
propagated before that deletion occurs. Replace the current pattern in the
Destroy handler to check and propagate errors from both self.destroy().await and
r.send(), ensuring that success is only acknowledged if both operations actually
succeed rather than silently discarding the results. Apply the same error
propagation fix to the similar patterns mentioned at lines 153-161 and 320-328.
twitchyliquid64
force-pushed
the
tom/session-destroy
branch
from
June 17, 2026 23:59
1ed65bf to
0b89c27
Compare
norrietaylor
approved these changes
Jun 18, 2026
twitchyliquid64
force-pushed
the
tom/session-destroy
branch
from
June 18, 2026 00:19
0b89c27 to
74fcc62
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (1)
crates/sessions/src/store.rs (1)
89-96:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winStale docstring references record read that no longer occurs.
The implementation now looks up the session name from the in-memory index (line 400), not from the on-disk record. The documented error condition "if the record cannot be read" is no longer accurate.
Suggested fix
/// Deletes the session with the given key, dropping its index entries and /// removing its on-disk directory tree (record, workspace, home, cache). /// /// # Errors /// - /// Returns an I/O error if the record cannot be read or the index cannot be - /// flushed. A missing directory tree is not an error. + /// Returns an I/O error if the index cannot be flushed or the on-disk tree + /// cannot be removed. A missing directory tree is not an error. fn delete(&mut self, key: &Self::Key) -> Result<(), std::io::Error>;🤖 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/sessions/src/store.rs` around lines 89 - 96, The docstring for the delete method contains a stale error condition that references reading the on-disk record, but the current implementation retrieves the session name from the in-memory index instead. Update the # Errors section of the delete method's docstring to remove or correct the outdated reference to record reading, ensuring the documentation accurately reflects that the error conditions are limited to index flushing failures and not record I/O operations.
🤖 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.
Duplicate comments:
In `@crates/sessions/src/store.rs`:
- Around line 89-96: The docstring for the delete method contains a stale error
condition that references reading the on-disk record, but the current
implementation retrieves the session name from the in-memory index instead.
Update the # Errors section of the delete method's docstring to remove or
correct the outdated reference to record reading, ensuring the documentation
accurately reflects that the error conditions are limited to index flushing
failures and not record I/O operations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 29df0cd3-e14f-46e4-bdec-283908f67e2a
📒 Files selected for processing (6)
crates/minimald-rpc/src/lib.rscrates/minimald/src/rpc.rscrates/minimald/src/session.rscrates/minimald/src/session_host.rscrates/minimald/src/sessions.rscrates/sessions/src/store.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/minimald-rpc/src/lib.rs
- crates/minimald/src/sessions.rs
- crates/minimald/src/rpc.rs
- crates/minimald/src/session_host.rs
- crates/minimald/src/session.rs
twitchyliquid64
enabled auto-merge (rebase)
June 18, 2026 00:22
0chroma
added a commit
that referenced
this pull request
Jun 18, 2026
Wire cmd_destroy to the DestroySession RPC added in #462. Resolves the session by UUID or name via GetSessionRecord (matching cmd_attach), then issues the destroy RPC and prints confirmation.
0chroma
added a commit
that referenced
this pull request
Jun 19, 2026
…destroy) (#434) * feat(minimal2): implement client interface with SSH transport (#159) - Add CLI subcommands: activate, attach, destroy, dash (fzf picker stub) - Implement SSH client transport over UDS to minimald - Wire oneshot RPC calls for ls (list sessions), activate (create session) - Add exec_in_session for non-interactive command execution in sessions - PTY interactive shell is blocked on daemon PTY support (exec.rs:650-654) - Update Cargo.toml with minimald-rpc, russh, sessions, serde_json deps * feat(minimal2): add --raw flag and 'dash' fzf session picker (#159) - ls --raw: outputs one session ID per line for fzf piping - minimal dash: interactive fzf picker that chains into attach - Falls back to plain table if fzf is not installed - Update implementation plan with Phase 5 details * feat(minimal2): interactive attach via ssh ProxyCommand (#159) Implement twitchyliquid64's suggestion: shell out to ssh for interactive PTY attachment instead of reporting it as unsupported. The daemon's shell_request handler already mints PTY-backed session shells, so we avoid reimplementing termios/PTY management by deferring to openssh. Adds a hidden `proxy` subcommand that pipes stdio to the daemon UDS, used as ssh's ProxyCommand so we don't depend on socat or nc being installed. Also fixes pre-existing fmt/clippy failures. * feat(minimal2): wire up destroy command via DestroySession RPC (#159) Wire cmd_destroy to the DestroySession RPC added in #462. Resolves the session by UUID or name via GetSessionRecord (matching cmd_attach), then issues the destroy RPC and prints confirmation. * refactor(minimal2): migrate attach --command to ssh shell-out (#159) Per reviewer feedback, exec_in_session used the daemon's old exec codepath which doesn't hit the sessions module. Remove it entirely and shell out to ssh for both interactive and --command attachment — ssh handles termios/PTY reconfiguration and the daemon's shell_request handler mints the session shell. exec_request remains on the daemon side for git-receive-pack and vscode remote only. * feat(minimal2): auto-spawn minimald on Linux (#159) On Linux, check if the daemon UDS is connectable before each command. If not, spawn `minimald run` as a detached background process (setsid + null stdio) and poll the UDS until it becomes available (4s timeout). Also fixes resolve_socket_path() to match minimald's listen_on() on Linux ($XDG_STATE_HOME/minimal/providers/local-0/ssh.sock), which was previously broken — it only worked on macOS via minvmd. No state machine or state.toml — just socket polling. The lifecycle management PR (#435) can layer richer state tracking on top later. * fix(minimal2): thread --minimal-dir through auto-spawn Auto-spawn was checking the default UDS path instead of the --minimal-dir override, causing it to fail when the daemon was already running with a custom state dir. Also pass --minimal-state-dir to the spawned minimald so it listens on the same path we're polling. E2E verified: ls, activate, attach --command, destroy (by UUID and name), auto-spawn (default and --minimal-dir paths). * refactor(minimal2): remove temporary dash/fzf picker Remove the `dash` subcommand, `ls --raw` flag, and `LsArgs` struct. These were temporary scaffolding for an fzf-based session picker — a proper TUI will be built later. Net -125 lines. * revert(minimal2): add back ls --raw flag --raw has standalone value for scripting (e.g. `minimal ls --raw | fzf`), independent of the removed dash subcommand. * style(minimal2): add missing blank line between functions * feat(minimald,minimal2): detect starting daemon via PID file minimald now writes a PID file at startup. The minimal2 auto-spawn logic checks this file before spawning: if the PID is alive the daemon is already starting (e.g. spawned concurrently), so it waits for the UDS rather than spawning a duplicate. The spawn path uses Child::try_wait to detect a crash during startup and fail fast instead of exhausting the 4s timeout. Addresses review feedback on #434.
This was referenced Jul 13, 2026
This was referenced Jul 29, 2026
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
2 participants
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.
Summary by CodeRabbit
New Features
Bug Fixes