feat(#197): defer session removal so a trailing bond-slashed still decrypts - #258
feat(#197): defer session removal so a trailing bond-slashed still decrypts#258AndreaDiazCorreia wants to merge 9 commits into
Conversation
…ssion loss When retaking an order within the grace window, explicitly resolve any pending deferred removal from the canceled take so the fresh session created for the retake survives the old timer. create_session now clears the deferral after inserting the new session, and take_order calls resolve_deferred_removal before creating the session to ensure cleanup.
…ashed decryption Adds Session lifecycle on cancel section explaining the 60-second deferral that keeps the trade key in subscriptions so bond-slashed can be received and decrypted. Documents the status-based removal rules (immediate for Pending/Dispute, deferred otherwise), retake clearing the deferral, and the deliberate divergence from v1's user-initiated/bond-policy approach.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughCancellation now validates trade-key generations and applies status-based session cleanup. ChangesSession cleanup lifecycle
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CancellationHandler
participant SessionManager
participant DeferredRemovalScheduler
participant BondSlashedHandler
CancellationHandler->>SessionManager: validate current trade-key generation
CancellationHandler->>SessionManager: apply status-based cleanup
DeferredRemovalScheduler->>SessionManager: reconcile expired removal
BondSlashedHandler->>SessionManager: resolve deferred removal
CancellationHandler->>SessionManager: install accepted-take session
Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
📒 Files selected for processing (3)
rust/src/api/orders.rsrust/src/mostro/session.rsspecs/004-mostro-p2p-client/contracts/orders.md
…k to prevent retake races
|
@codex review |
There was a problem hiding this comment.
💡 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[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
There was a problem hiding this comment.
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:
- the old take has a deferred session;
- a retake creates a fresh session with a new
trade_key_index; - a delayed
Canceledfrom the old trade key arms a new order-level deferral; - the old
BondSlashedresolves 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
…ent retake collision
There was a problem hiding this comment.
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
…ted takes Introduces install_session that unconditionally installs a session for a daemon-confirmed take, replacing any stale session or clearing pending deferrals. Unlike create_session which fails on collision, install_session assumes the new take owns the order_id and logs a warning when replacing. Refactors session construction into build_session helper shared by both methods.
|
Thanks — all four findings were the same defect seen at successively narrower
What is not in this PR: serializing the gate together with the order-book and DB Verified: |
There was a problem hiding this comment.
Blocking scope/contract mismatch
I reviewed the full conversation and the current head (53b6c491fd). The latest install_session change fixes the session-loss half of the previously reported race: an accepted retake now replaces stale session state, and generation-bound deferred cleanup cannot delete that replacement. The remaining order-book/DB TOCTOU is pre-existing and is now explicitly tracked in #259, so I am not requiring this narrowly scoped PR to implement the dispatcher-wide lock.
However, the current PR still cannot be approved because its contract and closing metadata claim guarantees that this implementation does not establish:
#197requires thecanceled -> bond-slashedsequence to be received and decrypted end-to-end, but none of the new tests invokesdispatch_mostro_message, the per-trade receiver, or the global receiver.a_delivery_to_a_superseded_trade_key_is_rejectedonly calls the generation helper twice.- In the current v2 receive path, removing a
Sessiondoes not take its trade key out of the relay filter or discard the decryption key.subscribe_gift_wrapscapturesrecipient_keysin its spawned receiver, whileensure_global_dm_coverageretains freshly derived keys for the process lifetime. The documentation therefore imports a v1 premise that is not true for this implementation, and the pureSessionManagertests do not prove the stated transport invariant. - The contract also says an old-key delivery is stopped before it can reach the order book or DB. That is contradicted by the acknowledged #259 interleaving: the generation read at
orders.rs:1836is released before the later awaits, so a retake can overtake it.
Please choose one coherent scope before merging:
- Implement/verify the end-to-end guarantee: add a receiver/dispatcher-level regression that exercises
Canceledfollowed byBondSlashed, and wire session lifetime to transport key/subscription lifetime if that coupling is actually intended; or - Make this preparatory policy only: remove the false transport and stale-delivery guarantees, correct the nonexistent
process_gift_wrap_rumorreference, and stop auto-closing #197 until its end-to-end acceptance criterion can be exercised (likely with the bond flow from #208). Keep #259 as the separate dispatcher serialization work.
Verification:
- Current head
53b6c491fd; clean checkout cargo test --locked: 256 passed, 8 ignored, 0 failedcargo clippy --locked: clean- Live
mainadvanced toa149b8f160; merge is conflict-free and merged-treecargo test --locked: 257 passed, 8 ignored, 0 failed - Exact-head GitHub Rust, Flutter, and web checks: green
…rantee Rewrites BOND_SLASH_GRACE_SECS comment and Session lifecycle on cancel section to explain that unlike v1, the 60-second window does not make bond-slashed arrive—per-trade receiver and ensure_global_dm_coverage already retain trade keys for the process lifetime. The grace period is a conservative margin for handling that needs session state, not a transport dependency. Also renames process_gift_wrap_rumor references to dispatch_mostro_message
There was a problem hiding this comment.
Approved — scope and contract are now aligned
I re-reviewed the current head (55cbda5454) against both the current PR description and the full text/conversation of #197. The previous scope blocker is resolved:
- The PR now says
Refs #197, notCloses #197; the issue remains open and its end-to-end receive/decrypt criterion will not be auto-closed by this merge. - The description and contract now state accurately that v2 transport keys/subscriptions outlive
Session, so this PR is session-cleanup policy rather than a transport guarantee. - The nonexistent
process_gift_wrap_rumorreferences were corrected todispatch_mostro_message. - The remaining generation-check TOCTOU for order-book/DB writes is explicitly acknowledged and tracked in #259. It predates this session-cleanup policy and is no longer represented as solved here, so it is not a blocker for the clarified scope.
I also revalidated the previous session-specific blockers on the full current diff:
- sessions and deferred removals share one lock;
- deferred removal is bound to
trade_key_index; - resolution/timer cleanup cannot delete a replacement generation;
- an accepted retake uses
install_session, clearing stale session/deferral state instead of silently losing its session.
No blocking issue remains for the PR as now described.
Verification:
- Exact-head Rust, Flutter, and web GitHub checks: green
git diff --check: clean- Current head merges cleanly with live
main(a149b8f160) - #197: open, with no closing keyword in the PR body
… and retake isolation Adds daemon_message, slashed_bond_payload, and await_bond_slashed helpers. Tests that canceled preserves session for trailing bond-slashed and that canceled from a superseded trade key does not affect the retake's order book entry or session.
There was a problem hiding this comment.
Approved — the dispatcher regressions are meaningful
I reviewed the test-only delta from the previously approved head (55cbda5454..0e3c2c0e3). It does not change production behavior.
The two new tests cross the real dispatch_mostro_message boundary and cover the intended session-policy invariants:
Canceledleaves the session deferred, the trailingBondSlashedreaches the Dart-facing stream, and the deferral is settled.- A
Canceledaddressed to the superseded trade-key generation cannot remove the retake's order-book entry or affect its session.
These are mutation-sensitive for the relevant dispatcher wiring: removing the generation gate, deleting the session immediately, or failing to resolve the deferral would make the assertions fail. They deliberately start from an already-unwrapped message, so they do not alter the clarified scope: #197's receive/decrypt transport criterion remains open and is not auto-closed by this PR.
Non-blocking naming note: a_canceled_then_bond_slashed_sequence_is_handled_end_to_end and its doc comment would be more precise as ...through_the_dispatcher; the test constructs UnwrappedMessage directly and therefore does not exercise receiver/decryption. The PR description already states this distinction correctly, so this does not block the test-only commit.
Verification:
cargo test --locked: 258 passed, 8 ignored- Both new tests executed individually: 1 passed each
cargo clippy --locked: clean- Exact-head Rust, Flutter, and web checks: green
- Clean merge-tree with live
main(a149b8f160) - Working tree: clean
1 participant
Refs #197 — deliberately not auto-closing it, see Scope below. Part of the
anti-abuse bond epic #145.
Problem
Cancel is where a session's life ends, and on a timeout slash the daemon sends
canceledfirst withbond-slashed~150 ms behind it. v2 turned out never todelete sessions at all —
remove_sessionhad no production caller — so thepolicy had to be defined before #191 and #208 introduce that deletion, and the
leak closed with the deferral already in place.
Fix
SessionManagertracks deferred removals (defer / resolve / reconcile),with sessions and deferrals under one lock.
cancel_cleanupdecides from the status held before the cancel:Pendingremoves 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::Canceledreads that status before syncingCanceled;Action::BondSlashedsettles a pending deferral and is a no-op without one.index they were armed for, so a delivery from a superseded take cannot claim
the session of the one that replaced it.
install_session: an accepted take replaces whatever it finds instead ofdiscarding a
SessionAlreadyExistserror, which previously left a confirmedretake with no session at all.
Scope
This is session-cleanup policy, not a transport guarantee, and #197's
end-to-end acceptance criterion is not proven here — hence
Refs, notCloses.v1's rationale for the grace period does not carry over: there, deleting the
session drops the trade key from the subscription filter and discards its
decryption key. In v2 the per-trade receiver captures its own trade keys and
ensure_global_dm_coverageretains them for the life of the process, soreception and decryption never depend on the session. The window is a margin
for handling that needs session state, plus parity with v1's policy.
Exercising
canceled→bond-slashedend to end needs a receiver/dispatcherregression and the taker bond flow from #208. Serializing the generation gate
with the order-book and DB writes is tracked in #259.
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
16 session tests plus 6 over the cancel-cleanup and generation helpers.
cargo test256 passing,cargo clippyclean,flutter analyzeclean,flutter test195 passing,frb-generate --checkunchanged.Summary by CodeRabbit
Bug Fixes
Documentation