fix(#335): replace-not-discard the session on a confirmed retake

[Matobi98]

Summary

Part 1 of #335.

take_order created the session with create_session and discarded the Err("SessionAlreadyExists") with let _ = ....

A retake of an order that already had a session (for example, the first take timed out or was rejected, and the retake succeeded with a fresh trade key) silently kept the stale session and its old trade_key_index, which chat key lookups then read.

Changes

• Added SessionManager::install_session in rust/src/mostro/session.rs.

  • It always replaces whatever session exists for the order instead of rejecting the write.
  • It replaces the session as a whole, so it discards peer material.
  • This is correct here because a retake's fresh trade key invalidates any shared key derived from the old one.
  • The behavior is documented so that a later caller cannot accidentally drop chat keys silently.

• take_order now calls install_session and logs the error instead of discarding it.

  • The only error it can return is an order ID mismatch, which indicates a programming error.

• Updated contracts/orders.md.

  • A confirmed take installs the session.
  • The generation gate reads the persisted trade_keys binding rather than Session.trade_key_index.
  • This is why a superseded reply was already dropped even while the session still held the previous take's index.

create_session

create_session is deliberately kept with its reject-on-duplicate behavior and its existing tests.

After this PR, it has no production callers.

However, #347 is open and keeps create_session because its changes to take_order would conflict with this PR. If #347 lands first, this becomes moot; otherwise, this approach should go forward.

Test Coverage and Its Limit

Two tests pin the replacement behavior:

• install_session_retake_replaces_stale_session — the retake wins for the same order, carrying the retake's fresh trade_key_index.
• install_session_discards_previous_peer_material — the retake still wins over a session that already holds peer_pubkey / shared material and inherits none of it.

These tests cover the replacement behavior itself, but do not cover take_order wiring the call site from create_session to install_session.

I verified the call site through mutation testing.

Reaching take_order requires a running daemon, so that seam is covered only by the manual regtest run below.

The earlier claim that the regression was fixed was true only in the compile sense, since the method did not exist yet. It is now actually fixed with the method implemented and wired into take_order.

Test Plan

• cargo test --locked — 331 tests passed
• cargo clippy --locked -- -D warnings
• Manually verified against a local setup (Polar + local relay + mostrod): a full sell trade (mostro-cli maker, app taker) completed successfully with the fix applied.

Part 2 of #335

Part 2 — a message from the current session — needs no work.

The generation gate compares against the trade_keys generation that take_order writes on every attempt, rather than against the session's trade_key_index as described in the contract.

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Team

Run ID: 2335f35b-77bb-4ad7-97b3-6781c44c22c3

---

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.}

[Catrya]

Reviewed at 23bed558, merged locally against current main (6261924). The branch is 5 commits behind and merges clean. On that merge: cargo test --locked 330 passed, cargo clippy --locked -- -D warnings clean, cargo check --locked --target wasm32-unknown-unknown clean.

The fix is the right one, and the manual regtest verification (two full trades, sell and buy, with mostro-cli as maker) is exactly what this needed. Three things to change before it goes in.

Blocking: the new test does not exercise the bug — verified by mutation

retake_replaces_stale_session_trade_key_index calls install_session twice directly. It never goes through take_order, which is where the bug lived.

I reverted the call site back to create_session — i.e. reinstated exactly the #335 part 1 bug, leaving install_session in place — and:

test result: ok. 330 passed; 0 failed

Nothing fails. The description says the test "reproduces the bug (fails without the fix, passes with it)"; that is only true in the compile sense (the method did not exist). With the method in the tree, anyone can move take_order back to create_session and CI stays green.

What would actually pin the fix is a test over take_order — which needs a daemon. A workable middle ground: assert the replacement semantics the caller depends on (two install_session calls with different indexes, the second winning even when peer material is already present). Failing that, the honest move is to say in the description that the take_order → install_session seam is covered only by the manual regtest run.

Blocking (small): create_session is left with no production caller

The description says "create_session keeps its reject-on-duplicate behavior — still used elsewhere and by its own idempotency test".

The first half is not true. After this PR, create_session has zero production callers — only the two tests (create_session_is_idempotent, new_session_has_no_peer_keys). On main its only caller was the very line this PR changes. So the "reject on duplicate" invariant no longer protects anything.

Not serious, but it should be a decision rather than a side effect: either delete create_session along with its tests, or say in the description that it is deliberately kept for future use. Dead code justified by an inaccurate claim is the part I would not merge as is.

Missing: the contract update

CLAUDE.md treats the specs as a living artifact — a behaviour change updates its contract in the same PR. This changes take-order behaviour and touches nothing in specs/. Two short edits:

1. contracts/orders.md, take_order → Side effects (~:122). The paragraph ends with "…the trade session/subscriptions start." It should say that a confirmed take installs the session:

…the order book entry is synced, and the trade session/subscriptions start.
A confirmed take **installs** that session, replacing whatever a prior
failed or timed-out attempt left behind: each attempt derives a fresh trade
key, so keeping the earlier session would leave chat key lookups reading a
superseded `trade_key_index` (#335).

2. contracts/orders.md, the generation-gate bullet (:394-406). Worth one sentence recording which marker the gate reads, since it is the thing that made #335 part 2 a non-issue and it cost a full investigation to establish:

The gate compares against the persisted `trade_keys` binding — written by
`take_order` on every attempt (`store_trade_key_index`) — not against
`Session.trade_key_index`, which a retake could leave stale until #335. That
is why a superseded reply was already dropped even while the session held the
previous take's index.

I would deliberately not add anything about a future deferred session removal carrying its generation: that documents a constraint on code which does not exist, and this repo consistently declines to do that (plan items 1.6 and 1.10 withdrawn rather than left "just in case"; #362 dropped the local_trade_status memoization "rather than adding pass-scoped state on spec"). If the bond work resumes, #197 is where that decision belongs.

Minor

1. take_order still discards the result with let _ = .... What it now swallows is the only error install_session can return — the order_id != order.id mismatch, i.e. a programming error. if let Err(e) = … { log::warn!(…) } costs one line. (Note: #347 rewrites this same line to log the create_session error — see the coordination point.)

2. install_session replaces the whole session, so it resets peer_pubkey, shared_key and admin_shared_key to None. For the case it fixes that is correct — a retake has a fresh trade key, so the old shared key is invalid — and it is not reachable today with peer material present, since the daemon will not confirm a second take of the same order. But the docstring should say it: "discards peer material; only for a confirmed take." If someone later calls it from elsewhere, losing the chat keys would be silent.

3. Coordination: this conflicts with #345 and #347. Merging #347 then #375 gives a conflict in rust/src/api/orders.rs and rust/src/mostro/session.rs. All three PRs rewrite session creation in take_order, and #347 adds upsert_peer_session while this one adds install_session — two new helpers in the same file with opposite merge semantics (one preserves role/index/order and touches only peer material; the other replaces everything). Worth deciding the order, and if both land, making sure SessionManager does not end up with three ways to write a session and no written rule for choosing.