Repository navigation
fix(minimal): report an already-destroyed session instead of failing - #2315
norrietaylor merged 4 commits into
Conversation
Two concurrent `min session destroy` calls race inside the daemon: the loser's DestroySession comes back "no session with ID ...", which the CLI surfaced as a hard error — reporting a failure for a session that is in exactly the state asked for. Thread the destroy's `--force` into destroy_session and treat that answer as the goal state: under `--force` the destroy prints "Session <name> is already destroyed" and exits 0; without it, the command still fails, but with wording that names the session rather than forwarding the daemon's internals. destroy_all_sessions passes its force through, so a mid-loop race loser is no longer counted a failure. Any other daemon error is unchanged. Proven by a harness test that destroys a session twice. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Warning Review limit reached
This review includes 7 billable files and costs up to $1.75. Or wait 53 minutes for your next included review. View limit details
Comment |
… via minimald-rpc
…ucceed The -f help text and the session destroy reference now say that a forced destroy treats an already-destroyed session as success, and that an unforced one fails naming it. The destroy_session comment no longer describes the forced path as the general one.
|
/review |
|
Approved: no findings at Nothing to do. Clobber: none: |
…ed lead ConfigureLoadout, FinalizeSession, RenameSession and AbortSession now build their not-found text from minimald_rpc::UNKNOWN_SESSION_ID_LEAD, as destroy does, so the wire text cannot drift from what the client matches. The message bytes are unchanged.
|
/review |
There was a problem hiding this comment.
APPROVE withheld: 1 unresolved thread — minimal-foundry[bot] crates/minimald/src/box_manager.rs:1439.
minimal-review — advisory review
Verdict COMMENT — No findings.
Panel: 1 of 3 personas ran (correctness); did not complete: security, supply-chain.
There was a problem hiding this comment.
minimal-review — approved
Verdict APPROVE — every review thread is resolved; @norrietaylor closed the last one. The review of ffed3ed7027e withheld this approval on the threads open then. Posted as a comment, as the review itself was; the check and the label carry it.
|
/review |
1 similar comment
|
/review |
There was a problem hiding this comment.
Approved on ARCH PASS at ffed3ed.
There was a problem hiding this comment.
The race's loser is told the session is gone, and the change reads that one answer through a shared constant on both sides rather than two spellings, which is the right fix. Under --force that is the goal state and a success; without it, the error names the session. The harness test covers both. One heads-up for whoever merges second: #2224 rewrites the DeleteSession handler in box_manager.rs around the line this PR touches, so the two will need a small rebase against each other, and #2224's side should pick up UNKNOWN_SESSION_ID_LEAD when it does.
Routing-Key: inbox-route/I_kwDOSUhdos8AAAABX3vZaA
Two concurrent
min session destroy <name>calls race in the daemon: theloser's DestroySession comes back "no session with ID ...", which the CLI
surfaced as a hard error — a failure reported for a session in exactly the
state asked for. Thread
--forceintodestroy_sessionand treat thatanswer as the goal state: under
--force, print "Session (name) isalready destroyed" and exit 0; without it, still fail, but naming the
session.
destroy_all_sessionspasses force through, so a mid-loop raceloser is not counted a failure. Other daemon errors are unchanged.
Verification
cargo fmt --all --check --manifest-path target/Cargo.toml— clean.cargo clippy --workspace --all-targets --locked --manifest-path target/Cargo.toml -- -D warnings— clean.cargo test --locked --manifest-path target/Cargo.toml -p minimal— 524 passed, 1 failed:tests::legacy_network_spellings_parse_with_hint, a pre-existing failure reproduced identically on the unmodified checkout (fd-2 capture quirk in the test's stderr pipe, unrelated to this change); the new test and the connection-inventory test it registers in both pass.a_destroy_of_an_already_destroyed_session_reports_itdestroys a session twice through a real in-process daemon — the second destroy is the race's loser — asserting the forced re-destroy reports "already destroyed" and succeeds, and the unforced one fails naming the session.