Skip to content

sdk: retain reconnect requests across connection task teardown - #1479

Closed
erskingardner wants to merge 3 commits into
nostrdevkit:masterfrom
erskingardner:codex/upstream-sdk-reconnect
Closed

erskingardner wants to merge 3 commits into
nostrdevkit:masterfrom
erskingardner:codex/upstream-sdk-reconnect

Conversation

@erskingardner

@erskingardner erskingardner commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Description

A reconnect requested after disconnect() can be lost while the previous connection task is still exiting, leaving the relay Pending without a running task. Subscription restoration also uses second-resolution timestamps, so two connections within one second can leave long-lived subscriptions inactive.

Serialize connection-task ownership and retain a reconnect request until the old task releases it. Reserve ownership before try_connect dials; cancellation releases that reservation, and a superseded dial cannot publish Connected. Clear stale termination permits before starting a replacement task. Track subscriptions by successful connection generation so immediate reconnect restores them reliably.

Notes to the reviewers

The public API signatures are unchanged. Disconnect/Terminated describe a termination request, not a task-exit barrier; the documentation now states this. Ban and shutdown cancel pending reconnects.

Deterministic teardown barriers exercise reconnect with automatic retries enabled and disabled, concurrent connect calls, stale termination permits, shutdown cancellation, and try_connect ownership conflicts. Additional regressions verify state changes while the admission policy is pending and release of connection ownership on dial cancellation or timeout. Loopback tests verify restored subscriptions and independent NIP-42 signers across reconnect.

Validation: just fmt and the complete just precommit passed locally on the pinned toolchain, including SDK tests with default and local-relay features, the configured WASM checks, Clippy, and documentation checks.

Checklist

  • I followed the contribution guidelines
  • I updated the relevant CHANGELOG.md (if applicable)
  • I understand and can explain all code in this PR

@codecov

codecov Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.41036% with 13 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
nostr-sdk/src/relay/inner.rs 97.23% 9 Missing ⚠️
nostr-sdk/src/relay/api/try_connect.rs 96.42% 4 Missing ⚠️

📢 Thoughts on this report? Let us know!

Retain both changelog entries while incorporating the merged COUNT and waiter fixes from upstream master.
yukibtc pushed a commit that referenced this pull request Oct 7, 2026
Record the connection count for each long-lived subscription so a new session restores it even when it starts in the same second.

Ref #1479

Signed-off-by: Yuki Kishimoto <yukikishimoto@protonmail.com>
@yukibtc yukibtc mentioned this pull request Oct 7, 2026
3 tasks done
yukibtc added a commit that referenced this pull request Oct 7, 2026
Keep one task per relay and retain connection requests while an old WebSocket closes. Share connection outcomes between `Relay::try_connect` callers and stop the task on ban, shutdown or the last Relay drop.

Replaces #1479

Pull-Request: #1491
Signed-off-by: Yuki Kishimoto <yukikishimoto@protonmail.com>
yukibtc pushed a commit that referenced this pull request Oct 7, 2026
Record the connection count for each long-lived subscription so a new session restores it even when it starts in the same second.

Ref #1479

Pull-Request: #1491
Signed-off-by: Yuki Kishimoto <yukikishimoto@protonmail.com>
@erskingardner

Copy link
Copy Markdown
Contributor Author

Can this one be closed now @yukibtc seems like it was incorporated in another PR?

@yukibtc

yukibtc commented Oct 12, 2026

Copy link
Copy Markdown
Member

Can this one be closed now @yukibtc seems like it was incorporated in another PR?

Yeah, but before closing this I'll wait that the other one will be merged.

yukibtc added a commit that referenced this pull request Oct 12, 2026
Keep one task per relay and retain connection requests while an old WebSocket closes. Share connection outcomes between `Relay::try_connect` callers and stop the task on ban, shutdown or the last Relay drop.

Replaces #1479

Pull-Request: #1491
Signed-off-by: Yuki Kishimoto <yukikishimoto@protonmail.com>
yukibtc pushed a commit that referenced this pull request Oct 12, 2026
Record the connection count for each long-lived subscription so a new session restores it even when it starts in the same second.

Ref #1479

Pull-Request: #1491
Signed-off-by: Yuki Kishimoto <yukikishimoto@protonmail.com>
@yukibtc

yukibtc commented Oct 12, 2026

Copy link
Copy Markdown
Member

@erskingardner, if you get the chance, could you give a try to #1491?

It should behave in the same way of this one. I've just rebased it on master branch, so includes also #1483

@erskingardner

Copy link
Copy Markdown
Contributor Author

I'll have a look now.

@erskingardner

Copy link
Copy Markdown
Contributor Author

Thanks @yukibtc. I tried #1491 (31a0237) against what #1479 was fixing, and it covers our cases:

  • reconnect requested while the previous connection task is still exiting
  • try_connect ownership: a superseded dial can't publish Connected, and cancelling or timing out a dial releases ownership
  • ban/shutdown cancelling a pending reconnect
  • same-second reconnect restoring subscriptions

try_connect now waits for the old connection to close instead of returning an error. That works fine for us.

I also ran two tests from #1479 against #1491. Both pass, and they could be worth bringing over since #1491 doesn't test either case:

  • auth_gated_clients_keep_independent_signers_across_removal_and_reconnect: a loopback NIP-42 test that copies over unchanged.
  • try_connect_rechecks_state_after_connection_policy: ban/shutdown while the admission policy check is still pending.

There's one issue we hit on #1491 (I haven't checked whether it was already on master): a REQ queued while the relay is disconnected gets sent twice after reconnect. This happens with a plain Relay::subscribe() too, not only send_msg. The reconnect restore sends the subscription, then the queued REQ goes out as well. A relay that rejects duplicate subscription ids answers CLOSED duplicate:, and the SDK then deregisters the subscription, so it's silently lost. Relatedly, update_subscription doesn't reset the subscription's EOSE flag or event counter when a REQ is re-sent. Happy to open a separate PR for this if you'd rather keep #1491 focused.

Closing #1479 once #1491 merges is fine with me.

For context, a few more fork changes are still keeping us off upstream, and I'll open separate PRs for them:

  1. bounded, cancellable event acquisition (item/byte limits, fetched events kept out of live notifications and the seen-event DB)
  2. Relay::batch_msg (atomic CLOSE+REQ) and a public per-subscription EOSE query, which we use to repair live subscriptions after notification lag

@erskingardner

Copy link
Copy Markdown
Contributor Author

Quick follow-up on the duplicate REQ: I checked and it's already on master too, so it's not a regression from #1491 and doesn't need to block it.

Repro (loopback relay that answers a repeated subscription id with CLOSED duplicate:):

  1. Subscribe live, let the connection drop.
  2. While disconnected, either call relay.subscribe(..).with_id("live") again, or send_msg a CLOSE + REQ for it.
  3. After reconnect the relay receives REQ live twice (the queued frame, then the reconnect restore), answers the second with CLOSED duplicate:, and the subscription is gone from the registry.

I'll open a small PR with the fix and this test. Want it based on sdk-rework-relay-task or on master?

yukibtc added a commit that referenced this pull request Oct 13, 2026
Keep one task per relay and retain connection requests while an old WebSocket closes. Share connection outcomes between `Relay::try_connect` callers and stop the task on ban, shutdown or the last Relay drop.

Replaces #1479

Pull-Request: #1491
Signed-off-by: Yuki Kishimoto <yukikishimoto@protonmail.com>
yukibtc pushed a commit that referenced this pull request Oct 13, 2026
Record the connection count for each long-lived subscription so a new session restores it even when it starts in the same second.

Ref #1479

Pull-Request: #1491
Signed-off-by: Yuki Kishimoto <yukikishimoto@protonmail.com>
@yukibtc

yukibtc commented Oct 13, 2026 •

Copy link
Copy Markdown
Member

I also ran two tests from #1479 against #1491. Both pass, and they could be worth bringing over since #1491 doesn't test either case:

  • auth_gated_clients_keep_independent_signers_across_removal_and_reconnect: a loopback NIP-42 test that copies over unchanged.
  • try_connect_rechecks_state_after_connection_policy: ban/shutdown while the admission policy check is still pending.

Thanks, added at #1491

I'll open a small PR with the fix and this test. Want it based on sdk-rework-relay-task or on master?

Thanks, I've just merged #1491 so it can be based on master.

@yukibtc yukibtc closed this Oct 13, 2026
yukibtc pushed a commit to erskingardner/rust-nostr that referenced this pull request Oct 15, 2026
When `subscribe()` is called while disconnected, its REQ stays in the queue.
On reconnect, `resubscribe()` adds a second REQ because the subscription
still records the previous connection count.

Record the next connection count for REQs queued while disconnected.
Handle REQs sent through `send_msg()` the same way.

Ref nostrdevkit#1479 (comment)

Pull-Request: nostrdevkit#1492
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Yuki Kishimoto <yukikishimoto@protonmail.com>
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