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

Why a plain replacement is not enough

Since #345 and #347 merged, a session can already exist at that call site for two unrelated reasons, and they must not be treated alike:

	how it got there	what it holds	what must happen
stale	a prior failed or timed-out take	an older trade_key_index, no peer material	replaced (#335)
current	apply_peer_reveal, when the daemon's first reply carried both trade pubkeys	the same index, plus peer_pubkey and shared_key	left untouched (#334/#345)

An unconditional replacement would wipe exactly the chat keys the peer-reveal path exists to establish. main's own comment above the call site says the duplicate-create error there is now expected for that reason.

The trade_key_index separates the two exactly: every take derives a fresh trade key, so a stale session always carries an older index and a peer-reveal session always carries the current one.

Changes

• Added SessionManager::install_session in rust/src/mostro/session.rs.
  • Replaces the session when the existing one carries a different trade_key_index; returns the existing one untouched when the index matches.
  • The replacement path resets peer_pubkey, shared_key and admin_shared_key. That is correct rather than lossy: a shared key derived from the superseded trade key is invalid, so carrying it over would fail the chat silently instead of rebuilding it.
  • The read and the write happen under one write lock, so a peer reveal cannot land between the decision and the write.
• take_order calls install_session and logs the error instead of discarding it. The only error it can return is an order-ID mismatch, i.e. a programming error.
• Updated contracts/orders.md: both branches of the install, and the fact that the generation gate reads the persisted trade_keys binding rather than Session.trade_key_index — which is why a superseded reply was already dropped even while the session held the previous take's index.

create_session

Kept, with its reject-on-duplicate behaviour and its tests. It has a production caller again: #345/#347 added one at the peer-reveal path. The earlier concern that this PR would leave it callerless no longer applies.

Test coverage and its limit

• retake_replaces_stale_session_trade_key_index — the retake wins, carrying its fresh index.
• install_session_discards_previous_peer_material — the retake wins even over a session holding peer material, and inherits none of it.
• install_session_keeps_this_takes_own_session_with_peer_material — the same-index case: the peer reveal's session survives take_order, with peer_pubkey and shared_key intact.

The third is mutation-checked: removing the index gate fails it with left: None, right: Some("aabbccdd").

These pin install_session itself. Reaching it through take_order needs a running daemon, so that seam is covered only by the manual run below — the test docstrings say so rather than implying coverage that does not exist.

Test plan

• cargo test --locked — 414 passed

• cargo clippy --locked -- -D warnings — clean

• cargo check --locked and --target wasm32-unknown-unknown — clean

• Manual regtest runs on the rebased branch (Polar + local relay + mostrod, mostro-cli as maker, app as taker in Chrome). The run in the original PR predated the rebase onto main and no longer covered this code, so it was redone — in two passes, because the first got the chat step wrong.

  1. Full happy path: order created, taken from the browser, buyer invoice, hold paid from an LND node other than the node's own (IN_FLIGHT, not settled), fiat-sent, release, counterparty rated from the app. Settles in LND.
  2. Chat with the trade active, which is what exercises this round. Verified against the relay rather than the UI: subscribing to kind 14 on the local relay, sending the message publishes a peer-to-peer event — neither author nor p tag is the node.

  Had install_session replaced the peer reveal's session, there would be no shared key, send_message would fall back to storing local-only, and nothing would reach the relay — silently, since all four of its fallback paths only log::warn! and the UI renders the message as sent either way. That is why this was checked on the wire and not in the app.

  Worth recording: the chat only works once the trade is active. Before the hold is paid the node has not revealed the counterparty pubkeys (payload=Order(..., buyer_pk=-, seller_pk=-) in the app log), so there is no peer material and a message sent then is correctly local-only. That was the mistake in the first pass.

All of the above on the toolchain CI pins (1.97.0).

Part 2 of #335

No code here. The generation gate already exists (rust/src/api/orders.rs:1710) and reads the persisted trade_keys binding rather than Session.trade_key_index, so part 1's bug never reached it. contracts/orders.md now records that. Whether this ticks part 2's box is a maintainer call — left unticked, raised in the review thread.

[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: 7ea6da1c-2156-4b55-9616-e1caabd09fa8

---

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.

[Catrya]

@Matobi98 hi, please, fix the conflicts with main

[Matobi98]

Rounds 1 and 2 — pushed in dd4a303. Round 1 answered your review; the rebase then changed what the fix has to do, so it needs reading as one piece.

Round 2 first, because it changes the shape of the fix

#345 and #347 merged in between. They added a second production path that creates the session for a take: apply_peer_reveal builds it, with peer_pubkey and shared_key already set, when the daemon's first reply carries both trade pubkeys. On current main the comment above take_order's call site says the duplicate-create error there is now expected — it protects that richer session.

So an unconditional install_session would have wiped exactly the chat keys #334/#345 exist to establish. Your Minor 2 said this was "not reachable today with peer material present". That was true on 2 Sep; those two merges made it reachable.

The replacement is now gated on trade_key_index, which separates the two cases exactly:

• different index — a prior failed or timed-out attempt left it behind, and it is stale. Replaced (Session: a retake keeps the stale session, and messages from a superseded take act on the current one #335, unchanged).
• same index — this take's own session, created by the peer reveal, strictly richer than what the call site would build. Left untouched.

Every take derives a fresh trade key, so a stale session always carries an older index and a peer-reveal session always carries the current one. The read and the write happen under one write lock: deciding outside it would let a reveal land in between and be discarded by a replacement decided when it did not yet exist.

I considered carrying the peer material over on replacement instead, and rejected it: a shared_key derived from the superseded trade key is invalid, so inheriting it would fail the chat silently rather than rebuild it. Deleting-then-creating was also rejected — that reopens the keyless window create_session_with_peer was added to close.

Conflict resolution keeps both helpers. create_session_with_peer and install_session are different operations and neither subsumes the other, so SessionManager now has a written rule for choosing: create-with-peer when the peer is already known, install at a confirmed take, and the index decides whether install replaces.

Round 1 — your review

Blocking: the test didn't exercise the bug. You were right, and the mutation check is now part of how I write these. The new test, install_session_keeps_this_takes_own_session_with_peer_material, fails when I remove the index gate: left: None, right: Some("aabbccdd"). The take_order → install_session seam is still out of reach without a daemon, and the test docstring says so rather than implying coverage that doesn't exist.

Blocking: create_session left callerless. This resolves itself on the rebase — #345/#347 added a production caller at the peer-reveal path, so it is in use again and the description's claim is true. No deletion needed; had they not landed I would have taken your first option.

Missing: the contract. Both edits are in, and the first one now records both branches rather than only the replacement.

Minor 1 — let _ replaced with a logged warning.
Minor 2 — docstring rewritten. It promised "replaces the whole session, so it discards peer material", which the gate makes false in half the cases.
Minor 3 — resolved by the merge order that happened: those two went in first, and this branch adapted to them.

Verified

On the toolchain CI pins (1.97.0): cargo test --locked 414 passed; cargo clippy --locked -- -D warnings clean; cargo check --locked and --target wasm32-unknown-unknown clean.

Unrelated, but noticed while verifying: adding --all-targets to that clippy invocation surfaces 27 await_holding_lock errors in test code, none in files this branch touches. CI doesn't pass --all-targets, so test code isn't linted at all today. Left alone here — worth its own issue if you agree.

Manual verification, redone on the rebased branch

The original PR's regtest run predated the rebase, so it no longer covered this code. Redone end to end against Polar + local relay + mostrod, mostro-cli as maker and the app in Chrome as taker: order taken from the browser, chat exercised, buyer invoice, hold paid from a non-daemon node (IN_FLIGHT, not settled), fiat-sent, release, counterparty rated.

The chat step is what exercises this round specifically, and it is the one that would have caught a regression: if install_session had replaced the peer reveal's session, there would be no shared key, send_message would fall back to storing local-only, and nothing would reach the relay. It is a silent failure by construction — all four of send_message's fallback paths only log::warn!, and the UI shows the message as sent either way — so I verified it against the relay rather than against the UI.

Subscribing to the local relay for kind 14 across the trade window, the message is there: a peer-to-peer event (neither author nor p tag is the node) published at the moment I sent it.

Two things worth recording from doing this, since neither is obvious and both cost me a wrong diagnosis first:

• The chat only works once the trade is active. Before the hold invoice is paid the node has not revealed the counterparty pubkeys (payload=Order(..., buyer_pk=-, seller_pk=-) in the app log), so there is no peer material and a message sent then is correctly local-only. Testing the chat right after the take measures nothing.
• mostro-cli cannot be used to observe app chat. Its getdm -f filters kind 14 p-tagged to its own trade key (util/events.rs:89-92), while the app addresses the chat envelope to the shared-key pubkey per the v2 spec. The message is on the relay; the CLI is looking in the wrong mailbox. Worth an issue if this isn't already known — as it stands, peer chat does not interoperate between the two clients in either direction.

Part 2 of #335 — a decision I'd rather not take alone

The PR adds no code for part 2, and I want that confirmed rather than assumed.

The gate part 2 asks for already exists, at rust/src/api/orders.rs:1710:

if let Some(bound) = lookup_trade_key_index(&oid).await {
    if trade_index < bound {
        // drop: addressed to superseded trade key (idx {} < bound {})
        return;
    }
}

What matters is the source it compares against. lookup_trade_key_index reads the persisted trade_keys binding, not Session.trade_key_index, and take_order rewrites that binding on every attempt via store_trade_key_index. So it was already comparing against a value part 1's bug never corrupted, and a superseded reply was already dropped even while the session held the previous take's index.

The issue reads as though the two halves were one problem — with a stale session, a gate comparing against the session would compare against the wrong number. That coupling doesn't exist, because the gate never read the session. Which is also why I didn't tick the box myself: "already satisfied" is a different claim from "implemented", and it's yours to accept.

If you'd rather have something concrete instead, there is one thing genuinely missing: a test pinning which source the gate reads, so a future refactor can't quietly move it onto Session.trade_key_index and reintroduce the coupling. The invariant is documented in contracts/orders.md now but nothing enforces it. Say the word and I'll add it to this PR.