feat(#182): unsubscribe + deterministic ids for per-trade subscriptions - #255
Open
codaMW wants to merge 2 commits into
Open
feat(#182): unsubscribe + deterministic ids for per-trade subscriptions#255codaMW wants to merge 2 commits into
codaMW wants to merge 2 commits into
Conversation
…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.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughRelay watchers now use deterministic ChangesRelay subscription lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1⚔️ Resolve merge conflicts 💡
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
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
📒 Files selected for processing (1)
rust/src/api/orders.rs
…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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the subscription-lifecycle half of #182 (the request_id half was done via #172; the chat side via #247).
Problem
subscribe_gift_wrapsandsubscribe_single_order(orders.rs) calledclient.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_chatpattern that #247 established:trade_subscription_id/single_order_subscription_id) viasubscribe_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 ofsubscribe_gift_wrapsis 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
subscribe_incoming_chatcleanup, since exercising it needs a live relay.cargo test(213 passing),clippy -D warnings,flutter analyzeall clean.Summary by CodeRabbit
Bug Fixes
Tests