feat(#197): defer session removal so a trailing bond-slashed still decrypts

[AndreaDiazCorreia]

Closes #197. Part of the anti-abuse bond epic #145.

Problem

On a timeout slash the daemon sends canceled first and bond-slashed ~150 ms later. Deleting the session on canceled drops the trade key from the subscription filter and discards its decryption key, so the notice can never be received or decrypted.

v2 turned out never to delete sessions at all — remove_session had no production caller. This defines the policy before #191 and #208 introduce that deletion, and closes the leak with the deferral already in place.

Fix

• SessionManager tracks deferred removals (defer / resolve / reconcile).
• cancel_cleanup decides from the status held before the cancel: Pending removes at once (the cancel returns the bond), dispute and admin states keep the session (the admin chat still needs its keys), anything else — including an unknown status — defers 60 s.
• Action::Canceled reads that status before syncing Canceled; Action::BondSlashed settles a pending deferral and is a no-op without one.
• Retake guard (v1 §8): retaking inside the window clears the deferral, so the new take gets a fresh session under its own trade key.

Divergence from v1

v1 keys off !userInitiated && hadBond; this keys off the pre-cancel status, needing neither an outbound cancel marker nor a bond-policy lookup. It defers in a few cases v1 would not — the cost is a session held 60 s longer. Sessions are in-memory only, so v1's restart reconciliation does not apply. Documented in specs/004-mostro-p2p-client/contracts/orders.md.

Tests

9 pure session tests plus 4 through the dispatcher path. cargo test 247 passing, cargo clippy clean, frb-generate --check unchanged.

Summary by CodeRabbit

• Bug Fixes

  • Improved session cleanup when orders are canceled, with immediate removal, retention, or deferred cleanup based on order status.
  • Retaking an order now clears pending session removal during the bond grace period.
  • Bond-slashed orders now correctly resolve deferred cleanup.
  • Stale cancellation and bond-slashed events are safely ignored.
  • Cleanup now remains accurate across order replacements and trade-key changes.

• Documentation

  • Documented order cancellation, session lifecycle, bond grace-period behavior, and retake handling.

[coderabbitai]

Warning

Review limit reached

@AndreaDiazCorreia, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 37 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 61f1f67e-84d2-4d46-8345-f8524549571f

📥 Commits

Reviewing files that changed from the base of the PR and between 0aa2194 and 53b6c49.

📒 Files selected for processing (2)

• rust/src/api/orders.rs
• rust/src/mostro/session.rs

Walkthrough

Cancellation now validates trade-key generations and applies status-based session cleanup. SessionManager tracks generation-bound deferred removals, while retakes and bond-slashed events resolve them. Tests and order lifecycle contracts cover these paths.

Changes

Session cleanup lifecycle

Layer / File(s)	Summary
Deferred cleanup policy and manager rust/src/mostro/session.rs	Adds cleanup classification, generation-bound deferrals, early resolution, deadline reconciliation, scheduling, and focused tests.
Order cancellation and session flow rust/src/api/orders.rs, specs/004-mostro-p2p-client/contracts/orders.md	Validates generations, applies cleanup after cancellation, resolves deferrals for bond-slashed events and retakes, and documents the lifecycle with integration tests.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CancellationHandler
  participant SessionManager
  participant DeferredRemovalScheduler
  participant BondSlashedHandler
  CancellationHandler->>SessionManager: validate trade-key generation
  CancellationHandler->>SessionManager: apply cleanup policy
  DeferredRemovalScheduler->>SessionManager: reconcile expired removal
  BondSlashedHandler->>SessionManager: resolve deferred removal
  CancellationHandler->>SessionManager: create retaken session

Loading

Possibly related PRs

• MostroP2P/app#213: Both changes modify Rust cancellation, bond-slashed handling, and generation-safe session cleanup.

Poem

A rabbit checks each trade-key thread,
Defers removal when bonds are shed.
A slash resolves the waiting state,
Retakes clear the old gate.
Fresh sessions hop ahead.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: deferred session removal preserves trailing bond-slashed decryption.
Linked Issues check	✅ Passed	The changes implement deferred cancellation cleanup, bond-slashed resolution, voluntary-cancel handling, generation checks, and supporting tests required by issue [#197].
Out of Scope Changes check	✅ Passed	The session lifecycle, generation safety, dispatcher handling, tests, and contract updates all support the linked issue objectives [#197].
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/197-bond-slashed-session-defer

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1e87ed32f6

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[coderabbitai]

Actionable comments posted: 1

🤖 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 `@rust/src/mostro/session.rs`:
- Around line 127-134: Replace the separately guarded sessions and
deferred_removals maps with a shared SessionState protected by one RwLock, so
create_session atomically checks for stale sessions, inserts the new session,
and clears any deferred removal. Update resolve_deferred_removal and
reconcile_deferred_removals to atomically coordinate deadline removal with
session deletion, preserving existing behavior for due and unresolved entries.
Add a tokio::time::pause() test that interleaves reconciliation or resolution
with a concurrent retry and verifies no stale SessionAlreadyExists result.

🪄 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 Plus

Run ID: 715b2598-8f5d-4d92-b3df-e12212b1b481

📥 Commits

Reviewing files that changed from the base of the PR and between 2464ade and 1e87ed3.

📒 Files selected for processing (3)

• rust/src/api/orders.rs
• rust/src/mostro/session.rs
• specs/004-mostro-p2p-client/contracts/orders.md

[AndreaDiazCorreia]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: aa7441189a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[ermeme]

Blocking changes required

I reviewed the current head (aa7441189a) and revalidated the existing unresolved generation-binding thread. The shared SessionState lock fixes the previous two-map race, but cancellation cleanup is still correlated only by order_id.

On this exact head I reproduced the following sequence with a focused regression test:

1. the old take has a deferred session;
2. a retake creates a fresh session with a new trade_key_index;
3. a delayed Canceled from the old trade key arms a new order-level deferral;
4. the old BondSlashed resolves that deferral and removes the fresh session.

The added test fails at the assertion that the retaken session must survive. The same missing guard is also applied too late in the dispatcher: the Canceled arm removes the order-book entry and writes Canceled to the current DB trade before calling apply_cancel_cleanup. Therefore an old-generation delivery can corrupt the fresh retake's persisted/UI state even before its session is removed.

Please bind Canceled, BondSlashed, and deferred-removal state to the recipient trade_index (or another explicit session generation), reject generation mismatches before any order-book, DB, or session mutation, and add a dispatcher-level regression test covering delayed old-key Canceled/BondSlashed deliveries after a retake. This should extend the existing unresolved thread rather than merely adding another lock around order-ID state.

Verification on the current head:

• cargo test: 249 passed, 8 ignored, 0 failed
• Focused session/cancel tests: green
• Temporary stale-generation regression probe: fails, because the fresh session is deleted
• GitHub CI: Rust, Flutter, and web checks are green, but none exercises this event-ordering case

[ermeme]

Blocking race still remains

I reviewed the latest commit (0aa219486d) as a delta from the previously reviewed head. The new generation-tagged deferral correctly prevents an already superseded old-key delivery from deleting the replacement session, and it fixes the prior timer/resolution behavior. However, it does not cover an old Canceled handler that passes the new check and is then overtaken by a retake while suspended in the following awaits.

That remaining interleaving can still corrupt the fresh order-book/DB state and can leave an accepted retake without any session; see the inline blocker. The new tests call generation helpers sequentially, so they cannot exercise this dispatcher/caller ordering.

Required before approval: make generation validation and the affected order-book/DB/session transition one coordinated per-order operation (or make every persisted/cache mutation generation-conditional), handle create_session failure instead of discarding it, and add a real interleaving test that pauses the old Canceled after validation, completes the retake, then resumes it.

Verification on the current head:

• cargo test: 254 passed, 8 ignored, 0 failed
• GitHub Rust, Flutter, and web checks: all green
• Current branch and live base merge cleanly

[AndreaDiazCorreia]

Thanks — all four findings were the same defect seen at successively narrower
windows, and each round is in the branch:

• Two-map race (Codex, CodeRabbit): sessions and deferred_removals now
  share one RwLock<SessionState>, so no operation observes one mid-update
  against the other.
• Generation binding (Codex): Canceled and BondSlashed are gated on the
  recipient trade key before any mutation; deferrals carry the trade key index
  they were armed for, and neither the trailing notice nor the timer removes a
  session of another generation. Writing that exposed a real bug — the first
  version compared the generation against the deferral instead of against the
  session it was about to drop — which the new test caught.
• TOCTOU after the gate (ermeme): the severe half is closed. take_order
  now uses install_session, which replaces whatever it finds and clears any
  pending deferral under the same lock, so an accepted take keeps its session
  whoever wins the race. The SessionAlreadyExists error is no longer
  discarded.

What is not in this PR: serializing the gate together with the order-book and DB
writes. That needs a per-order keyed mutex across the dispatcher and test
scaffolding to drive dispatch_mostro_message, and the Canceled arm has
mutated book and DB without any generation check since well before this branch.
Tracked separately in #259 rather than growing a PR about not losing a
bond-slashed.

Verified: cargo test 256 passing, cargo clippy clean, flutter analyze
clean, flutter test 195 passing, frb-generate --check unchanged.