fix(relay): close per-trade and per-order subscriptions on task exit - #365
fix(relay): close per-trade and per-order subscriptions on task exit#365grunch wants to merge 1 commit into
Conversation
subscribe_daemon_messages and subscribe_single_order opened relay subscriptions with an auto-generated id and never closed them. Both tasks exit after 30 minutes idle, but the relay-side REQ lived on for the rest of the session, so every create and every take leaked one. Public relays cap concurrent REQs at roughly 10-20. Past the cap they answer CLOSED, which is only logged -- and the subscription they drop can be the order book's own feed, so a heavy session degrades into an order list that silently stops updating. Both now use stable ids and unsubscribe on exit, mirroring the pattern already established for chat subscriptions in messages.rs. Ids are keyed by trade pubkey and order id so one task's exit cannot close another's feed -- which is what the test pins.
|
Warning Review limit reachedNext included review available in 45 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
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 |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![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: 4817e4dfb9
ℹ️ 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".
| /// Stable so the task can drop the relay-side REQ when it exits. Keyed by | ||
| /// trade pubkey, so unsubscribing one trade cannot close another's feed. | ||
| fn daemon_message_subscription_id(trade_pubkey_hex: &str) -> nostr_sdk::SubscriptionId { | ||
| nostr_sdk::SubscriptionId::new(format!("mostro-daemon-{trade_pubkey_hex}")) |
There was a problem hiding this comment.
On relays enforcing NIP-01's 64-character subscription-ID limit, every dedicated daemon subscription is rejected: trade_pubkey_hex is always 64 characters, so this prefix produces a 78-character ID. subscribe_with_id can return after sending the REQ before the relay's asynchronous CLOSED response, causing the code to log the subscription as active; create, take, and restore operations then lack their synchronous per-trade reply feed and can time out whenever the bulk mostro-dm subscription is unavailable or does not cover the new key. Use a compact deterministic representation, such as a truncated hash, while keeping the complete ID at no more than 64 characters.
Useful? React with 👍 / 👎.
Catrya
left a comment
Catrya
left a comment
There was a problem hiding this comment.
Reviewed at 4817e4d, merged locally against current main (3641db9 — the branch is 9 commits behind and merges clean). On that merge: 294 Rust tests pass, cargo clippy --locked -- -D warnings and cargo check --locked --target wasm32-unknown-unknown — the exact CI commands — are clean.
The leak this closes is real, and I confirmed the cap it's based on. But as it stands the change replaces a working subscription with one that no relay accepts, so the mechanism it introduces never runs.
Overlap with #255 — and why I'd keep this one
#255 (feat(#182)) does the same two things in the same two functions: deterministic ids via subscribe_with_id, plus unsubscribe at each loop's exit. The two are mutually redundant and will conflict.
I'd continue with this PR, for reasons of substance rather than seniority:
- The diff. This is +60/−3. #255 is +497/−265, because a merge commit ran
cargo fmtover the whole oforders.rs(main's copy has 1263 lines of rustfmt drift; the branch's has zero). The functional change there is ~70 lines; the rest is churn that would conflict with the 13 open PRs that touchorders.rs. - The comments. #255 still carries three comment blocks that describe an ownership guard removed in an earlier round, and a doc-comment claiming "full trade pubkey hex, not a prefix" above a function that slices
[..32]. - The series. This is part of the coordinated #348 Phase 2 work.
But #255 is ahead on one thing that matters here, and it has to be carried over before this merges — see below. I'd suggest closing #255 as superseded once this lands, with the 64-char finding (which came out of its review) and its length test brought over.
Blocking: mostro-daemon-<trade_pubkey_hex> is 78 characters; NIP-01 caps ids at 64
daemon_message_subscription_id builds "mostro-daemon-" + 64 hex = 78. SubscriptionId::new performs no length validation and subscribe_with_id returns Ok regardless, so nothing surfaces locally.
Probed against both entries of DEFAULT_RELAYS, same filter on three ids at once — this PR's, #255's 45-char id, and a short control:
[probe] pr365_len=78 pr255_len=45 ctrl_len=10
[probe] subscribe len=78 -> ok=true
[probe] CLOSED relay=wss://relay.mostro.network id_len=78 msg=Subscription id should be non-empty string of max length 64 chars
[probe] CLOSED relay=wss://nos.lol id_len=78 msg=ERROR: bad req: invalid subscription id length
[probe] RESULT pr365(78)=0 pr255(45)=10 ctrl(10)=10
Zero events, on both relays, with independently worded errors — this is the NIP being enforced, not one relay's quirk.
It is a regression. On main the same subscription uses client.subscribe(filter, None), and nostr-sdk generates 16 bytes of hex — 32 characters, well inside the limit. So this change takes a subscription that works today and makes it one the relay refuses.
Why it isn't visible. Three layers hide it:
subscribe_with_idreturnsOk— the REQ was sent; the refusal comes later.- The
CLOSEDarrives asynchronously and the relay-notification loop only logs it (blog_warn "relay" closed sub=… msg=…). Nothing propagates it, nothing retries. - The daemon's messages for that trade key still arrive over the global
mostro-dmbulk feed, sinceensure_global_dm_coverageincludes the key. Create, take and restore keep working, so manual testing passes.
Meanwhile the watcher loops for its full 30-minute idle window over a subscription that was never established, and unsubscribes it on exit. The limit(0) live-only replay protection on that filter is also not in effect, because there is no subscription — correlation falls entirely to the bulk feed, which replays history. The request_id nonce still guards it, so no visible bug, but the property is gone.
Where the budget was lost. The pattern this mirrors — chat_subscription_id (messages.rs:889) — is "mostro-chat-" + "dispute-"? + a 36-char order id = 56 max. It fits because the key is an order id. Substituting a 64-char pubkey hex into the same shape crosses the limit.
The fix
Truncate: format!("mostro-daemon-{}", &trade_pubkey_hex[..32]) gives 46 characters. 128 bits rules out any realistic cross-trade collision, and it leaves headroom for a later prefix change.
Please also extend every_subscription_id_addresses_one_feed (or add a sibling) with a length invariant over both generators:
assert!(daemon_message_subscription_id(&a).to_string().len() <= 64);
assert!(single_order_subscription_id(order_id).to_string().len() <= 64);The existing collision test is the right test to have — it's the one thing #255 lacks — but it passes happily on an id no relay will accept. Without the length assert, the next prefix change crosses the limit again and the bulk feed hides it again.
Blocking: the new doc-comment landed on the wrong function
The helpers were inserted above orders_subscription_id rather than below it, so /// Stable subscription ID for the Kind 38383 order-book feed. now sits directly on top of /// Stable id for a trade's daemon-message subscription. — daemon_message_subscription_id carries two contradictory doc lines, and orders_subscription_id is left with none. One line to move.
The failure mode in the description is not the one that happens
I measured the cap on relay.mostro.network (40 REQs on one connection, with a stand-in "order book" subscription opened first):
[probe] eose_count=19 closed_count=22
[probe] CLOSED probe-018: Number of subscriptions exceeds limit
[probe] CLOSED probe-019: Number of subscriptions exceeds limit
…
[probe] book_was_closed=false
The relay accepts ~18 concurrent subscriptions and then rejects the newcomer; it does not evict anything already open — the order-book stand-in stayed alive and got its EOSE. So the symptom isn't "an order list that quietly stops updating"; it's "past roughly 18 trade actions in a session, new per-trade subscriptions are silently refused" (and the client falls back to the bulk feed). The leak is worth closing either way, but the description overstates the damage and should say what actually happens.
Not blocking, worth recording
Two watchers can share mostro-order-<uuid>. The id is keyed by order id, not by trade key. take_order only refuses when order.status != Pending, so an order that returns to Pending (taker timeout, cancellation) and is taken again in the same session starts a second watcher on the same id. Harmless in the normal case — both read the same client.notifications() broadcast, reset their idle timers on the same events and exit together — except that subscribe_single_order's recv arm is Ok(Err(_)) => break, which treats a broadcast Lagged as fatal. A lagged older watcher would then tear down the younger one's REQ and leave it looping on nothing for up to 30 minutes. subscribe_daemon_messages gets this right (Lagged => continue, Closed => break). Matching the two arms is a one-line change; otherwise this belongs on #325.
Also verified
- Merge with current
mainis clean, one file, +60/−3. - Unsubscribing doesn't blind the client:
ensure_global_dm_coverageruns on all three paths that open a per-trade watcher (create_order,take_order,restore_session) and keeps every derived trade key in the bulk Kind-14 filter for the life of the process. That's what makes the teardown safe. - One watcher per daemon-message id: all three callers call
derive_trade_key()first, so the pubkey — and the id — is fresh each time. Unconditional cleanup is correct there. - No privacy regression: the deterministic id shows the relay a trade pubkey / order id that the same subscription's filter already carries.
- Every loop exit reaches the cleanup; the only early
returnis the failed-subscribe path, where there is nothing to tear down. limit(0)is untouched — only the id changes.
2 participants
Phase 2, PR 2.6 of the optimization plan (#348). Independent of the other Phase 2 PRs.
Problem
Two subscriptions were opened with an auto-generated id and never closed:
subscribe_daemon_messages(orders.rs:1203) — one per create/takesubscribe_single_order(orders.rs:2376) — one per taken orderBoth tasks exit after 30 minutes idle (
:1213,:2387), but the relay-side REQ lives on for the rest of the session. There is nounsubscribeanywhere on those paths, so an active session leaks one subscription per trade action.Public relays cap concurrent REQs at roughly 10–20. Past the cap a relay answers
CLOSED, which the client only logs (orders.rs:3345-3357) — and the subscription it drops may be the order book's own feed. The symptom is not an error; it is an order list that quietly stops updating partway through a heavy session.Change
Both now subscribe with a stable id and unsubscribe on exit, mirroring the pattern already established for chat in
messages.rs(chat_subscription_id+ the cleanup block at:1115-1121, documented at:886). This PR brings the order-side subscriptions in line with it rather than inventing anything.Ids are keyed by trade pubkey and order id respectively, so one task's exit cannot close another task's feed — or the order book's.
Test plan
cargo test— 294 passed, 0 failedcargo clippy --all-targets— no new warnings./scripts/frb-generate.sh --check— clean, no bridge surface change