Skip to content

Single owner for per-trade subscription lifecycle (remaining scope of #182) #325

Description

@Catrya

#182 is done as far as the leak goes: PR #255 unsubscribes on exit and gives the per-trade and single-order watchers deterministic ids. Its third task — the optional central registry — is not done, and this issue carries it.

Status: not broken today, required before #218

Nothing is broken right now. Each create_order / take_order calls derive_trade_key() first, so every per-trade subscription gets a unique pubkey and therefore a unique id; subscribe_single_order has a single caller, in the successful-take path. Two watchers over the same subscription id cannot currently happen.

#324 does not change that either: it forces a pool disconnect/connect, which drives the existing Online path in api::nostr (order book and chat subscriptions). It does not re-arm per-trade watchers.single caller, in the successful-take path. Two watchers over the same subscription id cannot currently happen.

#324 does not change that either: it forces a pool disconnect/connect, which drives the existing Online path in api::nostr (order book and chat subscriptions). It does not re-arm per-trade watchers.

Confirmed trigger: #218

#218 (priority: high, milestone M4) makes it reachable, by its own specification. Step 5 of the apply sequence is:

  1. Refresh gift-wrap subscriptions for the recovered trade keys.

with the matching acceptance criterion:

  • Gift-wrap subscriptions are live for all recovered trade keys after the apply

and the whole operation is required to be idempotent and retryable — "applying the same snapshot twice must not duplicate trades, sessions, disputes or mappings", and "a partial DB/key update (crash, kill, lost connection mid-apply) must be safely re-appliable".

Two applies of the same snapshot therefore re-arm a watcher for a trade key that already has one: two tasks owning mostro-trade-<pubkey>. The failure is not theoretical — the older watcher's exit path unsubscribes the newer one's live subscription and purges its pending request, which surfaces as NoDaemonResponse on an order the daemon actually accepted. A restore is exactly the moment a user cannot afford that.

So this is an ordering constraint, not a backlog item: it has to land before, or together with, #218 — and it stays small if it does, since all #218 needs is for subscribe_daemon_messages to be idempotent per trade key under a single owner.

(Note for #218: its step 5 and acceptance criteria say "gift-wrap subscriptions", which no longer exists — main renamed that transport in 56747c7 / 67df1ab. The subscription meant is the per-trade daemon-message one.)

Other paths that would reach the same place

The shape to copy

#182 dismissed the registry as "likely overkill" by comparing against v1's lib/features/subscriptions/ — a manager with four subscription types, session listeners and bulk filters. Mostrix is the closer reference (same language, same nostr-sdk, per-trade subscriptions like ours), and there it is three maps plus one function:

  • subscribed_pubkeys: HashSet<PublicKey> — idempotence by set membership, not by id
  • pubkey_to_subscription: HashMap<PublicKey, SubscriptionId>
  • subscription_to_order: HashMap<SubscriptionId, (order_id, trade_index)>
  • unsubscribe_dm_listener_subscriptions(client) — tears down only the DM-listener subscriptions and leaves the order/dispute scheduler ones intact

What it buys is a single owner of subscription lifecycle. v1 gets the same property from one subscription per type; Mostrix from one DM-router task owning the maps. Neither needs per-task ownership guards, because in neither does more than one task own a subscription.

Our deterministic ids are an advantage over Mostrix here: the id is recomputable from the trade pubkey, so a registry would not need pubkey_to_subscription at all.

If PR #255 merges with its generation guard still in place (SUBSCRIPTION_GENERATIONS / claim_subscription / owns_subscription), that guard is a per-task stand-in for this ownership and should be removed as part of this work — it also claims after subscribing, which leaves the window it is meant to close open.

Where it should live

Not in api/orders.rs — see #120. The relay/subscription layer (rust/src/nostr/) is the right home.

Acceptance criteria

  • Subscribing twice for the same trade key is idempotent: one relay-side subscription, one owner
  • Re-arming a watcher for a trade key that already has a live one neither tears down the live subscription nor purges its pending request
  • Teardown is targeted: closing per-trade subscriptions leaves the order-book and global DM subscriptions intact
  • Applying the same restore snapshot twice (Restore: idempotent transactional reconstruction of trades, sessions and disputes #218) leaves exactly one live subscription per recovered trade key

Supersedes the subscription-manager half of the closed #144 (its RequestId half shipped in #172).

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    area: protocolMostro protocol, transport, dispatcherarea: relaysRelay pool, discovery, healthpriority: mediumMedium priorityrefactorInternal restructure without behavior change

    Type

    No type

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions