Skip to content

feat(#182): unsubscribe + deterministic ids for per-trade subscriptions - #255

Open
codaMW wants to merge 7 commits into
MostroP2P:mainfrom
codaMW:feat/182-subscription-lifecycle
Open

feat(#182): unsubscribe + deterministic ids for per-trade subscriptions#255
codaMW wants to merge 7 commits into
MostroP2P:mainfrom
codaMW:feat/182-subscription-lifecycle

Conversation

@codaMW

@codaMW codaMW commented Jul 30, 2026

Copy link
Copy Markdown
Collaborator

Closes the subscription-lifecycle half of #182 (the request_id half was done via #172; the chat side via #247).

Problem

subscribe_gift_wraps and subscribe_single_order (orders.rs) called client.subscribe(filter, None) with auto-generated ids and never unsubscribed, so the relay-side subscription outlived the event loop's exit (30-min idle timeout, shutdown, or completed trade). Every create/take spawned a new one they accumulated over a long session (bandwidth, duplicate notifications, relay pressure on mobile).

Fix

Mirrors the subscribe_incoming_chat pattern that #247 established:

  • Deterministic ids (trade_subscription_id / single_order_subscription_id) via subscribe_with_id, so a repeat subscribe for the same trade/order replaces in place instead of stacking. Full pubkey hex (not an 8-char prefix) to rule out collisions.
  • unsubscribe(&sub_id) at each loop's single exit point, so the relay-side subscription never outlives the task.

The limit(0) live-only replay-protection of subscribe_gift_wraps is unchanged only the subscription id changes, not the filter.

Scope

Central subscription registry deferred per the issue's own guidance (nostr-sdk re-establishes subscriptions on reconnect).

Tests

  • Unit tests for the deterministic-id logic: idempotent per pubkey, no cross-pubkey collision, expected format.
  • Unsubscribe-on-exit is covered by inspection at each single exit point, mirroring the proven subscribe_incoming_chat cleanup, since exercising it needs a live relay.
  • cargo test (213 passing), clippy -D warnings, flutter analyze all clean.

Summary by CodeRabbit

  • Bug Fixes

    • Improved reliability of order updates by preventing duplicate or stale subscriptions.
    • Ensured background watchers clean up subscriptions when they stop, time out, or shut down.
    • Stabilized repeated subscription and resubscription behavior to reduce missed updates and resource leaks.
    • Improved cleanup of pending order requests.
  • Tests

    • Added coverage for consistent subscription identification, safe watcher replacement, and pending-request cleanup.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: Pro Plus

Run ID: 0861ade4-f4dd-45b3-aa92-8ee87e41539a

Walkthrough

Relay watchers now use deterministic SubscriptionId values for gift-wrap and single-order subscriptions. Generation tracking prevents stale watchers from removing replacement subscriptions. Tests cover identifier generation and cleanup ownership.

Changes

Relay subscription lifecycle

Layer / File(s) Summary
Deterministic identifiers and ownership tracking
rust/src/api/orders.rs
Helper functions generate deterministic trade and order subscription IDs. Generation tracking identifies the current subscription owner. Tests verify formatting, uniqueness, generation advancement, and replacement-safe cleanup.
Gift-wrap watcher lifecycle
rust/src/api/orders.rs
subscribe_gift_wraps uses a deterministic per-trade ID. It removes pending state and unsubscribes only when it owns the current subscription generation.
Single-order watcher lifecycle
rust/src/api/orders.rs
subscribe_single_order uses a deterministic per-order ID and unsubscribes on exit only when it remains the current owner.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related issues

  • MostroP2P/app issue 182 — Covers deterministic subscription IDs, explicit unsubscription, and stale-watcher protection in rust/src/api/orders.rs.

Possibly related PRs

  • MostroP2P/app#274 — Both PRs modify subscription lifecycle handling in rust/src/api/orders.rs.

Poem

A rabbit tracks each relay stream,
With stable IDs kept clean.
New watchers claim their generation,
Stale watchers leave the subscription.
Pending requests rest safe and sound.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main changes: unsubscribe handling and deterministic IDs for per-trade subscriptions.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 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/api/orders.rs`:
- Around line 3073-3097: Add an asynchronous lifecycle regression test near
trade_subscription_id_is_deterministic_and_per_pubkey and
single_order_subscription_id_matches_expected_format that starts watcher A,
replaces it with watcher B for the same trade or order, waits for A to exit, and
verifies B remains subscribed. Exercise the existing watcher replacement/cleanup
APIs and assert the active subscription state after cleanup, without changing
the subscription ID construction tests.
- Around line 1503-1509: Make deterministic subscription cleanup
generation-aware: in rust/src/api/orders.rs lines 1503-1509, update the
gift-wrap watcher cleanup to unsubscribe only when its task still owns the
current generation or serialized subscription; in lines 2379-2382, apply the
same ownership guard to single-order cleanup. Before starting each replacement
watcher, cancel and await the existing task so stale tasks cannot unsubscribe
the replacement subscription.
🪄 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: c4129d97-788c-41fa-a319-b98472aebb60

📥 Commits

Reviewing files that changed from the base of the PR and between c0608d7 and 0989769.

📒 Files selected for processing (1)
  • rust/src/api/orders.rs

Comment thread rust/src/api/orders.rs Outdated
Comment thread rust/src/api/orders.rs
codaMW added 2 commits August 2, 2026 08:18
…bscriptions

The chat side (subscribe_incoming_chat) was already fixed via MostroP2P#247. This brings
the two remaining per-trade subscriptions in orders.rs to parity:

- subscribe_gift_wraps and subscribe_single_order called client.subscribe(_, None)
  with auto-generated ids and never unsubscribed, so the relay-side subscription
  survived the event loop's exit (30-min idle timeout, shutdown, or completed
  trade). Every create/take spawned a new one — they accumulated over a long
  session (bandwidth, duplicate notifications, relay pressure on mobile).

Fix, mirroring the subscribe_incoming_chat pattern:
- Deterministic ids (trade_subscription_id / single_order_subscription_id) via
  subscribe_with_id, so a repeat subscribe for the same trade/order replaces in
  place instead of stacking. Full pubkey hex, not an 8-char prefix, to rule out
  collisions.
- unsubscribe(&sub_id) at each loop's single exit point, so the relay-side
  subscription never outlives the task.

The limit(0) live-only semantics of subscribe_gift_wraps are unchanged — only
the subscription id changes, not the filter. Central subscription registry
deferred per the issue (nostr-sdk re-establishes subscriptions on reconnect).

Unit tests cover the deterministic-id logic (idempotent per pubkey, no collision,
expected format); the unsubscribe-on-exit is covered by inspection at the single
exit point, mirroring the proven subscribe_incoming_chat cleanup, since it needs
a live relay to exercise.
…le-task unsubscribe

CodeRabbit (Critical): because the subscription id is deterministic, a
re-subscribe for the same trade/order (e.g. a retry within the 30-min idle
window) creates watcher B under the same id, replacing A's relay subscription.
When A's task later exits, its unconditional unsubscribe would tear down B's
live subscription — the very thing that keeps B's trade responsive.

Fix: a per-id generation counter (subscription_generations, mirroring
pending_requests). Each watcher claims the id on subscribe (bumping the
generation) and, on exit, only unsubscribes if it still owns the current
generation. A superseded watcher skips cleanup and lets the newer owner keep
the subscription. Applied at both sites (subscribe_gift_wraps,
subscribe_single_order).

Guard-only, not cancel-and-await: the stale watcher self-terminates on its idle
timeout without touching the subscription, so tracking JoinHandles to cancel it
early adds machinery for no correctness gain.

Regression test asserts the ownership property directly: A then B claim the same
id, A no longer owns it (won't unsubscribe), B owns it (will) — testable as pure
logic without a live relay.
@codaMW
codaMW force-pushed the feat/182-subscription-lifecycle branch from aef6a17 to 1397ea8 Compare August 2, 2026 06:24
@codaMW

codaMW commented Aug 2, 2026

Copy link
Copy Markdown
Collaborator Author

@grunch Rebased onto current main (resolved parallel test-module conflicts from the dispute PRs). Heads-up on scope: CodeRabbit found a real stale-task-unsubscribe hazard that the deterministic ids introduced, so this now carries a small per-id generation guard to make cleanup ownership-aware a bit more than #182's minimal ask. If you'd rather keep it lean, the alternative is unique (auto) ids + unconditional self-unsubscribe, which fixes the leak without the guard but drops the re-subscribe dedup. Happy to go either way. Green on cargo test (237) / clippy / flutter analyze.

@ermeme ermeme Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Blocking issue found on the current head. The stale-task unsubscribe race is guarded now, but the same stale task can still mutate shared pending-request state before the ownership check. See inline comment.

Comment thread rust/src/api/orders.rs
…rship, not just unsubscribe

ermeme review: the gift-wrap watcher's exit ran purge_pending_request
unconditionally before the owns_subscription check that guards unsubscribe. With
deterministic ids, a replacement watcher (B) can claim the same id and register
its own pending request while the superseded watcher (A) is still alive; when A
exits it correctly skips unsubscribe but still purged B's pending request,
stranding the real daemon reply so the caller fell back to NoDaemonResponse
despite an active subscription.

Move purge_pending_request inside the owns_subscription guard so a watcher that
lost ownership performs no exit-side effect on the shared id. The single-order
watcher already had no unconditional purge, so this is the only affected path.

Test: a_stale_watcher_exit_does_not_purge_a_replacements_pending_request — A
claims, B replaces and registers pending state, A exits (stale) and must not
purge, B's pending request survives; B's own exit then legitimately purges.
@codaMW

codaMW commented Aug 12, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed in 0d66634. purge_pending_request is now inside the owns_subscription(&sub_id, sub_gen) guard alongside unsubscribe, so a stale watcher that lost ownership touches neither the subscription nor the shared pending-request state the replacement watcher's pending request survives and its daemon reply can still resolve. Added a regression test (orders.rs tests) covering exactly the scenario you described: watcher A claims the id, B replaces it and registers its own pending request, A exits and having lost ownership runs no exit-side effect (the purge is skipped), and B's pending state remains intact. Owner B's own exit still purges legitimately.

@codaMW codaMW self-assigned this Aug 13, 2026
@Catrya

Catrya commented Aug 27, 2026

Copy link
Copy Markdown
Member

@codaMW please fix the conflicts

@codaMW

codaMW commented Aug 27, 2026

Copy link
Copy Markdown
Collaborator Author

@Catrya conflicts resolved merged latest `upstream/main`. Two conflict regions in `orders.rs`: (1) the subscribe call, where I kept #182's deterministic `sub_id` + `subscribe_with_id` (required by the `claim_subscription`/`owns_subscription` ownership guard) while adopting upstream's `subscribe_daemon_messages` log label; (2) parallel test-module additions, kept both sides. Verified on the merged head: `cargo test --lib` (274 pass) and `cargo clippy --lib -- -D warnings` clean.

@Catrya Catrya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed at 23b29ce, merged locally against current main (the branch is 0 commits behind — it merged upstream today). On the merged result: 274 Rust tests pass and cargo clippy --locked -- -D warnings — the exact CI command — is clean.

Nothing here breaks existing behaviour. The change I'm asking for is in the defensive half, which is both unreachable and incomplete, plus some dead references.

First, the thing worth confirming: unsubscribing doesn't blind the client

The obvious risk in this PR is that a per-trade watcher exits at the 30-minute idle timeout and now also closes the relay-side subscription, on a trade that is still live. That's covered: ensure_global_dm_coverage (called at line 388 in create_order, 615 in take_order, 3446 in restore) keeps every derived trade key in a bulk Kind-14 filter that, per its own doc, covers the key "for the whole life of the process, not just while the temporary per-trade receiver runs" — and that feed replays history, where the per-trade one is limit(0) live-only. Coverage survives. That's what makes the change safe.

Main point: the generation guard defends a case the call graph can't produce — and still has a hole

Why it can't happen. Both callers of subscribe_daemon_messages (lines 459 and 687) call derive_trade_key() immediately before, with the comment "each order must use a unique derived key index". New key ⇒ new pubkey ⇒ new id. There are never two watchers on mostro-trade-<X>. And subscribe_single_order has exactly one caller (line 803), inside the successful-take handler, for an order that isn't taken twice. So the "a re-subscribe reuses the id and supersedes the previous watcher" scenario that motivated two review rounds (1397ea8, 0d66634) has no route today.

And if it did happen, the guard would not close it. The order is subscribe_with_id(...).awaitclaim_subscription(...). Claiming after subscribing leaves a window where B has already replaced the relay-side subscription but A is still the registered owner. If A exits in that window, owns_subscription(id, gen_a) returns true and A runs both destructive effects: it purges the pending request — which B had already inserted, since both call sites register it before subscribing — and unsubscribes the id, tearing down B's fresh subscription. The window isn't instruction-sized: it spans B's derive_trade_key / get_active_trade_keys and the full subscribe_with_id round trip. In create_order the outcome would be the bad one, because the publish happens after the subscribe: the order goes out, the daemon replies, the client can't correlate it, and the caller gets NoDaemonResponse for an order that exists on the daemon.

Both halves close together, either way:

  • If the case is unreachable (what I see): drop SUBSCRIPTION_GENERATIONS, claim_subscription, owns_subscription and their two tests, and unsubscribe directly on exit. The PR lands at ~20 lines and does exactly what the title says.
  • If there's a path I missed: document it, and claim before subscribing, with a rollback of the generation if subscribe_with_id fails — without that rollback a failed subscribe leaves A stale, its exit skips the unsubscribe, and that's the leak this PR exists to close.

Smaller things

SUBSCRIPTION_GENERATIONS is never pruned. One entry per trade key and per order, for the life of the process. It's bytes, not a real problem, but the natural cleanup point is the owner's exit (remove if it's still current) and it's one line. Moot if the guard goes.

Three references to a function that no longer exists. The comments at lines 2235, 2634 and 3614 point at subscribe_gift_wraps. main renamed it in the transport refactor (67df1ab / 56747c7), and CLAUDE.md is explicit that nothing in the v2 paths is called "gift wrap" any more. They send the reader looking for something that isn't there.

The guard tests assert a copy of the logic, not the logic. a_stale_watcher_exit_does_not_purge_a_replacements_pending_request re-implements the guard inside the test (if owns_subscription(...) { purge_pending_request(...) }) rather than exercising subscribe_daemon_messages. Delete the if from the real function and the test stays green. The PR is upfront that the unsubscribe itself is "covered by inspection" — worth not reading these as behavioural coverage either.

Test scope creep. take_matching_restore_matches_restore_records_only and take_matching_request_ignores_stale_events are new here (they don't exist on main) and cover the #215 correlation logic, not subscription lifecycle. Good tests, but they turn a ~40-line functional change into a +578 diff, which costs review time.

Also verified

  • Every loop exit reaches the cleanup block: inside the loop there are only break/continue, no return. The one early return is the failed-subscribe path, where there's nothing to clean up.
  • The limit(0) filter is untouched, as the description says — only the id changes. That's the replay protection that keeps an old reply from resolving an in-flight create_order.
  • No privacy regression: the deterministic id shows the relay the trade pubkey / order id, but the same subscription's filter already carries both (#p and the d-tag). Nothing new is disclosed.

Not a condition for merging

If a second subscriber for the same trade key ever does appear, the fix is a single owner of subscription lifecycle — the registry #182 lists as its optional third task — rather than a per-task ownership guard. Neither v1 (lib/features/subscriptions/, one subscription per type) nor Mostrix (a DM-router task owning subscribed_pubkeys / pubkey_to_subscription / subscription_to_order plus a targeted teardown) has anything like a generation guard, precisely because in both a single owner holds the subscriptions. I'll leave the detail on the issue so it isn't lost.

@Catrya

Catrya commented Aug 27, 2026

Copy link
Copy Markdown
Member

Update after #239 merged.

This needs a rebase, and it should shrink the diff

The branch now conflicts with main in rust/src/api/orders.rs, at three points, all inside the test module.

The cause isn't neighbouring edits — both PRs add the same code. These five are already on main via #239, byte-for-byte identical to the copies here:

  • insert_pending_create
  • insert_pending_take
  • local_uuid_of
  • take_matching_restore_matches_restore_records_only
  • take_matching_request_ignores_stale_events

When resolving, this branch's copies have to be dropped, not kept: two definitions of each won't compile.

Correcting my "Test scope creep" note above: I checked at the time and those two tests were not on main, so I read them as coverage added here. They were in #239 as well, which is where they landed. The practical upshot is better than what I wrote — removing the duplicates takes the diff from +578 down to roughly the functional change plus its own tests (trade_subscription_id_is_deterministic_and_per_pubkey, single_order_subscription_id_matches_expected_format, a_stale_watcher_*), which makes the two asks in the review the bulk of what remains.

No trait-stub fix needed here: this PR only touches orders.rs and implements no Storage, so the update_trade_peer_reputation addition that turned main red after #239 (fixed in #329) doesn't reach it.

Two corrections to the review above

On subscribe_gift_wraps. I wrote that main "renamed" it. That undersells it: 56747c7 removed every protocol-v1 gift-wrap path, and 67df1ab renamed what was left to match. This client speaks protocol v2 only and neither reads nor writes kind 1059, so those three comments point at a transport that is gone, not at a function with a new name.

The follow-up issue now exists: #325 — "Single owner for per-trade subscription lifecycle (remaining scope of #182)". That's where the v1 / Mostrix comparison and the single-owner argument live, so nothing from the last section of the review is lost.

…on-lifecycle

# Conflicts:
#	rust/src/api/orders.rs
@codaMW
codaMW force-pushed the feat/182-subscription-lifecycle branch from 23b29ce to 5894669 Compare August 29, 2026 08:01
@codaMW

codaMW commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator Author

Done. Merged current main (kept it a merge rather than a rebase to preserve the reviewed commits + ermeme's approval).

Dropped this branch's copies of the five tests now on main via #239 `insert_pending_create`, `insert_pending_take`, `local_uuid_of`, `take_matching_restore_matches_restore_records_only`, `take_matching_request_ignores_stale_events` keeping only #182's own tests (`trade_subscription_id_is_deterministic_and_per_pubkey`, `single_order_subscription_id_matches_expected_format`, and the two `a_stale_watcher_*`). That takes the diff from +578 to +181.

Also adopted main's `Wake`-wrapped `insert_pending_create`, and dropped the stale `subscribe_gift_wraps` comment now that the v1 gift-wrap transport is gone (56747c7 / 67df1ab) the log label is `subscribe_daemon_messages`.

Green on `cargo test --lib` (294) and `cargo clippy --locked -- -D warnings`.

@Catrya Catrya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@codaMW The only change since the last review is the merge of main. The three functional commits are untouched, and neither requested change has been made. The rebase was needed and is correct — the duplicated helpers and tests are now deduplicated (one copy each of insert_pending_create, insert_pending_take, local_uuid_of and the two #215 correlation tests), the branch is 0 commits behind and merges clean, and on that merge 294 tests pass with cargo clippy --locked -- -D warnings clean. But merging main does not address the review; the two blocking items are unchanged.

Still open

1. The generation guard still claims after subscribing. Line 1229 of this head is unchanged: subscribe_with_id(...).await, and only then claim_subscription(&sub_id). The window described in the last review is still open.

I re-checked reachability, because main gained a caller in the meantime: there are now three calls to subscribe_daemon_messagescreate_order, take_order, and a new one in restore_session from #239. That third one also calls derive_trade_key() as its first statement, so it produces a fresh index, a fresh pubkey and therefore a unique subscription id. subscribe_single_order still has a single caller. The conclusion is the same: two watchers can never hold mostro-trade-<pubkey> at the same time.

2. The three references to subscribe_gift_wraps are still there — lines 2473, 2869 and 4036. That function no longer exists; main removed the v1 gift-wrap paths in 56747c7 and renamed what was left in 67df1ab. The comments point readers at something that isn't in the codebase.

What to do — concretely

To avoid leaving this open-ended, here is the change I'm asking for rather than a choice between options:

Remove the generation machinery. Delete SUBSCRIPTION_GENERATIONS, subscription_generations(), claim_subscription, owns_subscription, and the two tests that exercise them (a_stale_watcher_does_not_unsubscribe_a_replacement, a_stale_watcher_exit_does_not_purge_a_replacements_pending_request). At each exit point, unsubscribe and purge unconditionally:

// subscribe_daemon_messages, at the single exit
purge_pending_request(&trade_pubkey_hex);
if let Ok(pool) = crate::api::nostr::get_pool() {
    pool.client().unsubscribe(&sub_id).await;
}

// subscribe_single_order, at the single exit
client.unsubscribe(&sub_id).await;

Reasons for removing rather than fixing the ordering:

  • The case it guards cannot occur today, as re-verified above against all four call sites.
  • As written it does not close the window it claims to, so keeping it would mean shipping a guard that gives false assurance.
  • Fixing the ordering correctly is not one line: claiming before subscribing requires rolling the
  • Fixing the ordering correctly is not one line: claiming before subscribing requires rolling the generation back when subscribe_with_id fails, or a failed subscribe leaves the previous watcher marked stale and its exit skips the unsubscribe — reintroducing exactly the leak this PR exists to close.
  • If a second subscriber for the same trade key ever appears, the fix is a single owner of subscription lifecycle, which is tracked in #325 — not a per-task generation counter. Note that #331, merged since the last review, already introduced that shape one layer down (lock_order, per-order serialization in the dispatcher). Those watchers are separate spawned tasks and do not go through that lock, so it isn't a drop-in — but the direction of the codebase is settled.

Fix the three comments to name subscribe_daemon_messages, and drop the "gift wrap" wording — per CLAUDE.md nothing in the v2 paths carries that name any more.

With those two done, the PR lands at roughly its functional core — deterministic ids plus unsubscribe on exit — which is what #182 asked for

…cribe unconditionally (MostroP2P#255 review)

Catrya's review: the generation guard (claim_subscription / owns_subscription)
claimed ownership *after* subscribing, so the window it described stayed open —
and the race it guards cannot occur today, since every caller of
subscribe_daemon_messages and subscribe_single_order derives a fresh trade key
and therefore a unique subscription id (re-verified against all four call
sites). Keeping it would ship a guard that gives false assurance, and fixing
the ordering correctly (claim-before-subscribe with rollback on a failed
subscribe) would reintroduce the very leak this closes.

- Remove SUBSCRIPTION_GENERATIONS, subscription_generations(), claim_subscription,
  owns_subscription, and the two a_stale_watcher_* tests that exercised them.
- Both exit points now unsubscribe (and, for the daemon-message watcher, purge
  the pending request) unconditionally.
- Fix the stale subscribe_gift_wraps comments to name subscribe_daemon_messages;
  the v1 gift-wrap paths were removed upstream (56747c7 / 67df1ab).

If a second subscriber for a trade key ever appears, the fix is a single owner
of subscription lifecycle (MostroP2P#325), not a per-task generation counter.

292 tests pass; cargo clippy --locked -- -D warnings clean.
@codaMW

codaMW commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator Author

Both done in this push.

1. Generation guard removed. You're right that it claimed after subscribing and that the race can't occur today every caller derives a fresh trade key, so the subscription id is always unique and two watchers never share `mostro-trade-`. Rather than reorder (which would need claim-before-subscribe with rollback on a failed subscribe, reintroducing the leak), I removed the machinery entirely `SUBSCRIPTION_GENERATIONS`, `subscription_generations()`, `claim_subscription`, `owns_subscription`, and the two `a_stale_watcher_*` tests. Both exit points now unsubscribe (and purge the pending request, for the daemon-message watcher) unconditionally, exactly as you laid out. If a second subscriber ever appears, #325's single-owner lifecycle is the right shape, not a per-task counter.

2. Stale comments fixed. The three `subscribe_gift_wraps` references now name `subscribe_daemon_messages`; the v1 gift-wrap paths were removed upstream (56747c7 / 67df1ab).

That lands the PR at its functional core — deterministic ids plus unsubscribe on exit. Green on `cargo test --lib` (292) and `cargo clippy --locked -- -D warnings`.

@codaMW

codaMW commented Aug 29, 2026

Copy link
Copy Markdown
Collaborator Author

Both done in this push.

1. Generation guard removed. You're right that it claimed after subscribing and that the race can't occur today — every caller derives a fresh trade key, so the subscription id is always unique and two watchers never share mostro-trade-<pubkey>. Rather than reorder (which would need claim-before-subscribe with rollback on a failed subscribe, reintroducing the leak), I removed the machinery entirely — SUBSCRIPTION_GENERATIONS, subscription_generations(), claim_subscription, owns_subscription, and the two a_stale_watcher_* tests. Both exit points now unsubscribe (and purge the pending request, for the daemon-message watcher) unconditionally, exactly as you laid out. If a second subscriber ever appears, #325's single-owner lifecycle is the right shape, not a per-task counter.

2. Stale comments fixed. The three subscribe_gift_wraps references now name subscribe_daemon_messages; the v1 gift-wrap paths were removed upstream (56747c7 / 67df1ab).

That lands the PR at its functional core. Green on cargo test --lib (292) and cargo clippy --locked -- -D warnings.

@Catrya Catrya left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re-reviewed at 9bd3303.

Both items from the last round are done. The generation machinery is gone — no SUBSCRIPTION_GENERATIONS, claim_subscription, owns_subscription or their two tests remain — and both exits now clean up unconditionally, with the reason in the comments. All three subscribe_gift_wraps references now name subscribe_daemon_messages. On the merge with current main (0 behind, merges clean): 292 tests pass, cargo clippy --locked -- -D warnings and cargo check --locked --target wasm32-unknown-unknown are clean.

With the diff down to its functional core I went through the whole thing rather than just the delta, and that turned up a defect that makes the central mechanism inert.

mostro-trade-<pubkey_hex> is 77 characters; the limit is 64

NIP-01:

<subscription_id> is an arbitrary, non-empty string of max length 64 chars.

SubscriptionId::new performs no length validation, so subscribe_with_id returns Ok regardless. Tested against relay.mostro.network, with a short id on the same filter as a control:

[probe] long id len=77  short id len=11
[probe] CLOSED id_len=77 msg=Subscription id should be non-empty string of max length 64 chars
[probe] RESULT long_id_events=0 short_id_events=5

The relay answers CLOSED, quoting the limit, and delivers zero events on that subscription. The control received five.

mostro-order-<uuid> is fine at 49 characters. Only the trade id is over.

This is a regression the PR introduces

On main, the same subscription uses client.subscribe(filter, None) with an auto-generated id, and nostr-sdk builds those as 16 bytes of hex — 32 characters, well inside the limit. So the change replaces a working subscription with one the relay refuses.

Why device testing didn't catch it

Three layers hide it:

  1. subscribe_with_id returns Ok — the REQ was sent; the refusal comes later.
  2. The CLOSED arrives asynchronously and the relay-notification loop only logs it (blog_warn "relay" closed sub=… msg=…). Nothing propagates it to the subscriber and nothing retries, so it's invisible unless someone reads the debug log.
  3. Decisively, the daemon's messages for that trade key still arrive over the global mostro-dm bulk feed, since ensure_global_dm_coverage includes the key. Create and take keep working, so the manual test passes.

Meanwhile the watcher loops for its full 30-minute idle window over a subscription that was never established, and unsubscribes it on exit.

Side effect worth noting

The limit(0) live-only replay protection the description says is untouched is not in effect for that key, because there is no subscription. Correlation falls entirely to the bulk feed, which does replay history. The request_id nonce still guards it, so there's no visible bug — but the property the PR claims to preserve isn't there.

The fix

Shorten the id. #182 originally suggested mostro-trade-<pubkey8>; this PR moved to the full hex to rule out collisions, which is reasonable in intent. A middle ground keeps both: the first 32 hex characters (128 bits) gives mostro-trade- + 32 = 45, no realistic collision risk and plenty of headroom.

Please also add an invariant test over both generators — assert!(id.to_string().len() <= 64) — so a later prefix change can't cross the limit again unnoticed.

The rest of the pass

Everything else holds up:

  • One watcher per id, verified against all four current call sites, including the third one restore_session gained from #239: all three derive a fresh trade key. The unconditional cleanup is correct.
  • No interaction with #331 — these watcher tasks don't go through lock_order, so no deadlock and no contention.
  • No privacy regression — the deterministic id shows the relay a trade pubkey and an order id that the same subscription's filter already carries.
  • Every loop exit reaches the cleanup; the only early return is the failed-subscribe path.

codaMW added 2 commits August 31, 2026 13:06
…har limit (MostroP2P#255 review)

Catrya's review: mostro-trade-<64-char-pubkey-hex> is 77 chars, over NIP-01's
64-char subscription-id cap. SubscriptionId::new does no length check and
subscribe_with_id returns Ok, but the relay answers CLOSED ("max length 64")
and delivers zero events — so the per-trade subscription was inert, its traffic
silently carried by the global mostro-dm bulk feed (which is why device testing,
create and take all kept working). This is a regression: main's auto-generated
id is 32 chars and works.

- trade_subscription_id now uses the first 32 hex chars (128 bits) — no realistic
  collision, and mostro-trade- + 32 = 45, well under the limit. This restores the
  limit(0) live-only replay protection the PR claims, since the subscription now
  actually establishes.
- mostro-order-<uuid> (49) was already fine; unchanged.
- Added subscription_ids_stay_within_the_nip01_64_char_limit over both generators
  so a later prefix change can't cross the limit unnoticed.

293 tests pass; cargo clippy --locked -- -D warnings clean.
@codaMW
codaMW requested a review from Catrya August 31, 2026 11:15
@codaMW

codaMW commented Aug 31, 2026

Copy link
Copy Markdown
Collaborator Author

Fixed in this push and thank you for the relay probe, that was the missing piece. You're right it was a regression: the 77-char id got CLOSED'd and delivered zero events, with the global `mostro-dm` bulk feed silently carrying the traffic, which is exactly why create/take and device testing all looked fine while the per-trade subscription was never actually established.

`trade_subscription_id` now uses the first 32 hex chars (128 bits) `mostro-trade-` + 32 = 45, well under NIP-01's 64. No realistic collision, and because the subscription now actually establishes, the `limit(0)` live-only replay protection the PR claims is genuinely in effect again rather than deferred to the bulk feed. `mostro-order-` (49) was already fine.

Added `subscription_ids_stay_within_the_nip01_64_char_limit` over both generators (`assert!(len <= 64)`) so a later prefix change can't cross the limit unnoticed. Merged current main (0 behind); 296 tests pass, `cargo clippy --locked -- -D warnings` clean

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants