Session: a retake keeps the stale session, and messages from a superseded take act on the current one

[Catrya]

Salvaged from #258 (closed): two session-correctness fixes that had nothing to do with the anti-abuse bond and are worth having on their own.

1. A retake keeps the stale session

take_order creates the session with:

let _ = crate::mostro::session::session_manager()
    .create_session(order_id.clone(), trade.role.clone(), trade_index, trade.order.clone())
    .await;

and create_session returns Err("SessionAlreadyExists") when one is already present (session.rs:98). The error is discarded, so a confirmed take over an order that already has a session keeps the old session with the old trade_key_index — and the chat key lookups that read session.trade_key_index then use the wrong index.

Reachable whenever the same order is taken twice in one process: a first take that the daemon rejected or that timed out, followed by a retake. Each take_order derives a fresh trade key, so the two takes hold different indexes.

#258 proposed install_session: an accepted take replaces whatever it finds instead of discarding the error.

2. A message from a superseded take acts on the current session

Daemon messages carry the trade index they were addressed to. Nothing checks it against the session's, so a late reply belonging to an earlier take can drive the Canceled / peer-pubkey handling of the take that replaced it. #258 proposed a generation gate: the deferral (and the exit-side effects) carry the trade key index they were armed for, and act only if it is still current.

Note: reachability here is more plausible than in the subscription-side case discussed in #325, because a retake reuses the same order_id with a different index — but it has not been verified against a live daemon, and that should be part of the work.

Related

• feat(#197): defer session removal so a trailing bond-slashed still decrypts #258 (closed) — where both fixes come from; its bond-specific half is deferred with the bond epic
• Peer chat: the counterparty pubkey is never persisted, so the maker has no chat room and nobody can send after a restart #334 — the counterparty pubkey is never persisted; different root cause, same code, likely the same PR
• Single owner for per-trade subscription lifecycle (remaining scope of #182) #325 — the same shape of problem one layer down, on subscription ownership

[Matobi98]

I'd like to take this one. Given the two parts have different risk/complexity, I'll split the work:

1. Retake keeps the stale session (part 1): straightforward fix — add an install_session that replaces whatever session exists instead of discarding the SessionAlreadyExists error in take_order, so a confirmed retake's trade_key_index is the one actually used. Will open this as its own PR first.
2. Message from a superseded take acting on the current session (part 2, the generation gate): this one is harder to get right and, as the issue notes, hasn't been verified against a live daemon yet. I'll investigate it separately once part 1 lands, and it may end up as its own PR rather than bundled with part 1.

[Matobi98]

Before starting on part 2, I looked into it — and I believe it is already covered on main.

What's already there

dispatch_mostro_message has a generation gate (rust/src/api/orders.rs, ~L1457-1487) that drops any daemon message addressed to a trade key strictly older than the one currently bound to the order (BondSlashed exempt by design, #197).

• It runs before every per-action arm, so it covers both the Canceled and the peer-pubkey handling this issue names.
• It applies on both delivery paths — subscribe_daemon_messages and handle_global_daemon_message — each of which passes the index of the key the message arrived on.
• It landed as commit adb8081 on fix(#259): serialize daemon message dispatch and take persistence per order id #331, added in a later review round — after that PR's description had already listed the session-side generation check as out of scope, which I suspect is why this issue still reads as open.

Manual verification

I verified it end-to-end against a local mostrod/regtest setup rather than by reading the code alone.

A real retake isn't reachable on demand, so I reproduced the condition the gate actually checks — a message arriving on a trade key older than the order's binding. I took two orders in the app (A → index 21, B → index 24) and published a forged but otherwise genuine protocol-v2 Canceled for order B, signed with the local node's key, addressed to A's older key:

  daemon-msg: action=Canceled order_id=Some(b0b24da0-…) trade_pubkey=6ae45df8 payload=None
  daemon-msg: drop Canceled order=b0b24da0: addressed to superseded trade key (idx 21 < bound 24)

Control — the same message delivered on B's own key (index 24) is applied, so the drop above is attributable to the gate and not to a malformed message:

daemon-msg: action=Canceled order_id=Some(b0b24da0-…) trade_pubkey=5a6b8104 payload=None
orders: Canceled before active — removed trade for order=b0b24da0-…
session: removed order=b0b24da0

Delivered on	Index vs binding	Result
A's older key	21 < 24	Dropped by the gate, order untouched
B's own key	24 == 24	Applied, order canceled

The existing test is load-bearing too: neutering the gate makes a_late_cancel_for_a_superseded_generation_is_dropped fail.

Two differences from the wording above

1. The gate compares against the order → trade-key binding (lookup_trade_key_index, DB-persisted) rather than Session.trade_key_index (in-memory). Same generation marker in practice — and with part 1 (fix(#335): replace-not-discard the session on a confirmed retake #375), both are written together in take_order.
2. The other half — "the deferral (and the exit-side effects) carry the trade key index they were armed for" — refers to feat(#197): defer session removal so a trailing bond-slashed still decrypts #258's deferred session removal, which isn't on main (that's Session lifecycle: defer session deletion on cancel to catch a trailing bond-slashed #197, still open), so there is nothing to gate yet.

Question

@Catrya does that match your read? If the only thing left here is the deferral half, it might be worth folding it into #197 rather than keeping this issue open.

[Catrya]

@Matobi98 yes, matches my read.

Just for the record i put here a more elaborated answer using Fable: I verified the claims: gate at orders.rs:1468-1485, runs before every per-action arm, on both paths (:1296, :3132); adb8081 merged via #331 (the #259 is the issue); neutering the gate does fail a_late_cancel_for_a_superseded_generation_is_dropped; and there's no session deferral on main.

Worth noting: the gate reads the marker that was always correct, take_order writes store_trade_key_index (:897) on every attempt, even the ones that left the session stale. That's why part 2 never manifested while part 1 was broken.

On the disposition, one correction: I wouldn't fold the remainder into #197. It's a sub-issue of the anti-abuse bond epic #145, which is parked until the app is more mature, and the remainder isn't a bond concern, it's a session-lifecycle invariant. Putting it there mixes two things and hides it in work nobody will pick up soon. #197 stays open as it is: it describes a real protocol trap (daemon sends canceled, then bond-slashed ~150 ms later) that still has to be solved when the bond work resumes.

The remainder also isn't a gap today, "the deferral carries the index it was armed for" presupposes a deferred session removal that doesn't exist. So: close #335 once #375 lands, and put the constraint where whoever builds the deferral will actually hit it — specs/004-mostro-p2p-client/contracts/orders.md:394-406 already documents the generation gate. One sentence there beats an issue comment.