chore(sessions): loadouts flow cleanup - #639

crates/minimal/src/main.rs (1)

508-518: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Consider aborting on SubmitVerdict failure to avoid leaked Pending records.

The gating-failure path calls send_abort, but a SubmitVerdict transport error (Line 511) or an Errorable::Err reply (Line 514) returns Err(()) without one. If the daemon hasn't processed/torn down the record, the Pending record and its stash slot leak until the startup reap — counting against MAX_PENDING_SESSIONS in the meantime. abort_session safely refuses non-Pending records, so a best-effort abort here is harmless if the session was already promoted.

The Fault arm correctly skips the abort (daemon already tore its side down).

♻️ Optional: best-effort abort on RPC/handler failure

     let resp = client
         .oneshot_rpc::<SubmitVerdict>(verdict)
         .await
-        .map_err(|e| eprintln!("SubmitVerdict RPC failed: {e}"))?;
+        .map_err(|e| eprintln!("SubmitVerdict RPC failed: {e}"));
+    let resp = match resp {
+        Ok(r) => r,
+        Err(()) => {
+            send_abort(client, session_id).await;
+            return Err(());
+        }
+    };
     let step = match resp {
         minimald_rpc::Errorable::Ok(s) => s,
         minimald_rpc::Errorable::Err { error } => {
             eprintln!("SubmitVerdict failed: {error}");
+            send_abort(client, session_id).await;
             return Err(());
         }
     };

🤖 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/main.rs` around lines 508 - 518, The SubmitVerdict failure
path in main should perform a best-effort abort before returning, because both
the oneshot_rpc transport error and the Errorable::Err branch currently exit
without cleaning up a possibly still-Pending session. Update the SubmitVerdict
handling around client.oneshot_rpc::<SubmitVerdict>(verdict) and the subsequent
match on minimald_rpc::Errorable so that any RPC/handler failure calls the same
abort_session/send_abort cleanup used by the gating-failure path, while leaving
the Fault arm unchanged since it already tears down the daemon side.

crates/minimald/src/sessions.rs (1)

1167-1202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing happy-path test for AbortSession on a Pending session.

Tests cover the two refusal paths (unknown id, Active session) but there's no test asserting that aborting an actual Pending session succeeds and removes both the in-memory stash entry and the on-disk record.

🤖 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 1167 - 1202, Add a happy-path
test for AbortSession using the existing manager() and create_and_unwrap_id
helpers to create a Pending session, then call abort_session on that id and
assert it succeeds. Verify the Pending session is removed from both the
in-memory stash and the persisted record by checking
get_record(SessionKeyPredicate::Id(id)) returns None after the abort, and keep
the test alongside abort_unknown_id_errors and abort_refuses_active_session for
coverage symmetry.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/minimald/src/sessions.rs (1)

1167-1202: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Missing happy-path test for AbortSession on a Pending session.

Tests cover the two refusal paths (unknown id, Active session) but there's no test asserting that aborting an actual Pending session succeeds and removes both the in-memory stash entry and the on-disk record.

🤖 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 1167 - 1202, Add a happy-path
test for AbortSession using the existing manager() and create_and_unwrap_id
helpers to create a Pending session, then call abort_session on that id and
assert it succeeds. Verify the Pending session is removed from both the
in-memory stash and the persisted record by checking
get_record(SessionKeyPredicate::Id(id)) returns None after the abort, and keep
the test alongside abort_unknown_id_errors and abort_refuses_active_session for
coverage symmetry.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

crates/minimald/src/rpc.rs (1)

264-283: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

serve_abort_session maps every error to a typed client-facing error, breaking the sibling-RPC convention.

Unlike serve_create_session/serve_submit_verdict, which only turn specific expected error kinds into Errorable::Err and let anything else fall through to ConnectionError::Internal (which triggers the tracing::warn! below and shows up in daemon logs), serve_abort_session converts any error — including the shutdown ConnectionRefused and any unexpected store.delete I/O failure — into Ok(Errorable::Err{...}). That keeps the outer res as Ok(()), so the tracing::warn! in this function never fires and a genuine internal failure is silently hidden from daemon logs while being reported to the client as an indistinguishable generic string.

🔧 Proposed fix

 async fn serve_abort_session(s: ServerStateHandle, c: RuChannel<Msg>) {
     let res = AbortSession
         .handle_channel(c, async |req| {
-            let res = s.sessions_manager().await.abort_session(req.id).await;
-            match res {
-                Ok(()) => Ok(Errorable::Ok(AbortSessionResponse)),
-                Err(e) => Ok(Errorable::Err {
-                    error: e.to_string(),
-                }),
-            }
+            match s.sessions_manager().await.abort_session(req.id).await {
+                Ok(()) => Ok(Errorable::Ok(AbortSessionResponse)),
+                // Expected, structured outcomes the client should
+                // handle as clean typed errors.
+                Err(e)
+                    if matches!(
+                        e.kind(),
+                        std::io::ErrorKind::NotFound | std::io::ErrorKind::InvalidInput
+                    ) =>
+                {
+                    Ok(Errorable::Err {
+                        error: e.to_string(),
+                    })
+                }
+                // Shutdown-refusal and any unexpected I/O error —
+                // surface as a transport failure so it's logged below.
+                Err(e) => Err(ConnectionError::Internal(e.to_string())),
+            }
         })
         .await;

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

/// `AbortSession`: drop a `Pending` session's stash entry and delete
/// its on-disk record. See the manager arm for the actor-side rules
/// (refuses `Active` records and unknown ids).
async fn serve_abort_session(s: ServerStateHandle, c: RuChannel<Msg>) {
    let res = AbortSession
        .handle_channel(c, async |req| {
            match s.sessions_manager().await.abort_session(req.id).await {
                Ok(()) => Ok(Errorable::Ok(AbortSessionResponse)),
                // Expected, structured outcomes the client should
                // handle as clean typed errors.
                Err(e)
                    if matches!(
                        e.kind(),
                        std::io::ErrorKind::NotFound | std::io::ErrorKind::InvalidInput
                    ) =>
                {
                    Ok(Errorable::Err {
                        error: e.to_string(),
                    })
                }
                // Shutdown-refusal and any unexpected I/O error —
                // surface as a transport failure so it's logged below.
                Err(e) => Err(ConnectionError::Internal(e.to_string())),
            }
        })
        .await;
    if let Err(e) = res {
        tracing::warn!("RPC handler for {} failed: {}", AbortSession::NAME, e);
    }
}

🤖 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/rpc.rs` around lines 264 - 283, serve_abort_session is
swallowing all failures into Errorable::Err, which breaks the same
client/internal error split used by serve_create_session and
serve_submit_verdict. Update the AbortSession.handle_channel callback to only
convert the expected session-level outcomes into Errorable::Err, and let
unexpected errors from sessions_manager().abort_session(req.id) propagate as
ConnectionError::Internal so the outer res in serve_abort_session can trigger
the tracing::warn! path. Keep the existing AbortSession::NAME-based logging
behavior and preserve client-facing typed errors only for the known, recoverable
cases.

crates/minimald/src/sessions.rs (1)

171-179: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

DestroySession doesn't clean up the new pending stash, leaking entries for destroyed Pending sessions.

ManagerMessage::List doesn't filter by status, so a Pending session is visible and can be targeted by a normal DestroySession call (instead of the new AbortSession). DestroySession's handler deletes the on-disk record and tears down running, but never calls self.pending.remove(&id). The stashed PendingComposeState then leaks in memory until daemon restart, silently eating into MAX_PENDING_SESSIONS capacity even though its backing record is gone.

🔧 Proposed fix

             ManagerMessage::DestroySession(id, r) => {
                 r.handle(async {
                     if self.in_shutdown {
                         return Err(SessionsError::new(
                             std::io::ErrorKind::ConnectionRefused,
                             "in shutdown",
                         ));
                     }
                     let k = self.store.find_by_id(&id)?.ok_or_else(|| {
                         std::io::Error::new(
                             NotFound,
                             format!("no session with ID `{}`", id.as_ref()),
                         )
                     })?;
+                    // A `Pending` session has no running host, but may
+                    // still hold a `PendingComposeState` stash entry —
+                    // drop it so DestroySession doesn't leak stash
+                    // capacity toward `MAX_PENDING_SESSIONS`.
+                    self.pending.remove(&id);

🤖 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 171 - 179, `DestroySession` is
leaving behind entries in the in-memory `pending` stash when it destroys a
`Pending` session, so update the `DestroySession` handler in
`Manager`/`Manager::handle_message` to also remove the matching `SessionId` from
`self.pending` alongside the existing on-disk cleanup and `running` teardown.
Use the `pending: BTreeMap<SessionId, PendingComposeState>` field as the
authoritative stash to clear, and make sure the normal destroy path mirrors the
new abort cleanup so a destroyed pending session fully releases its in-flight
state.