feat(#182): unsubscribe + deterministic ids for per-trade subscriptions - #255
feat(#182): unsubscribe + deterministic ids for per-trade subscriptions#255codaMW wants to merge 3 commits into
Conversation
|
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)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughRelay watchers now use deterministic ChangesRelay subscription lifecycle
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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}
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
…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.
aef6a17 to
1397ea8
Compare
|
@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[bot]](https://avatars.githubusercontent.com/in/3690068?s=60&v=4){kind=small}
…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.
|
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. |
1 participant
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