Skip to content

sdk: avoid duplicate REQ for subscriptions queued while disconnected - #1492

Merged
yukibtc merged 1 commit into
nostrdevkit:masterfrom
erskingardner:sdk/dedupe-queued-req-on-reconnect
Oct 15, 2026
Merged

yukibtc merged 1 commit into
nostrdevkit:masterfrom
erskingardner:sdk/dedupe-queued-req-on-reconnect

Conversation

@erskingardner

@erskingardner erskingardner commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Description

Here is the follow-up from the #1479 discussion, based on master.

Problem. Take a registered subscription. While the relay is disconnected, queue a REQ for it, either with relay.subscribe(..).with_id(id) or with a CLOSE + REQ sent through send_msg. After the reconnect the relay receives REQ <id> twice. First comes the queued frame, flushed from the outbound channel. Then the reconnect restore in resubscribe() sends it again. A relay that rejects an already-open subscription ID answers the second REQ with CLOSED duplicate:, and the SDK removes the subscription, so a live subscription is lost without any error.

Root cause. subscribed_connection was set to stats.success() when the REQ was queued. While disconnected, that is the count of the session that just ended. After the reconnect, success() is one higher, so should_resubscribe restores the subscription even though the queued REQ is still in the outbound channel and will go out on this same session. should_resubscribe already handled this for the first connection, but not for later reconnects.

Fix. Record the session that will actually write the frame. That is the current session when connected and the next one otherwise. Both values are read under the connection-state lock, so they can't straddle a reconnect. The session is recorded when a long-lived subscription is registered and again whenever a REQ for one is queued, which also covers raw send_msg. The subscription is only updated once the REQ is queued, so a failed send leaves it unchanged. The first-connection special case in should_resubscribe is no longer needed and has been removed. The public API does not change.

Notes to the reviewers

  • The tests drive ControlledRelay with a new recording_connection test helper, which records written frames and can inject relay replies. For both the subscribe and the CLOSE + REQ cases, they check three things: exactly one REQ live on the reconnected session, the subscription is still registered after replies from a relay that rejects duplicate IDs, and it is not marked for restore. Both fail on master and pass with this change. A third test checks that a REQ queued before the first connection is still sent once.
  • The existing reconnect tests still pass, including subscription_restoration_uses_connection_counter and the same-second restoration tests. Auto-closing subscriptions are still never restored.
  • A narrow race remains. If a REQ is queued while the status is still Connected and the session dies before the sender writes it, the frame goes out on the next session and is restored there too. Closing this would mean tracking frames at write time, which seemed out of scope.
  • Since sdk: reset subscription limit tracking when restoring a subscription #1493 merged first, a REQ queued for a later session now also resets the EOSE flag and event-limit count, like a restore does. It doesn't reset them for a REQ on the current session, where events of the previous query may still be in flight.

Checklist

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

🤖 Generated with Claude Code

@erskingardner
erskingardner marked this pull request as ready for review October 14, 2026 08:34
@codecov

codecov Bot commented Oct 14, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 99.17695% with 2 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
nostr-sdk/src/relay/inner.rs 99.06% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

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>
@yukibtc
yukibtc force-pushed the sdk/dedupe-queued-req-on-reconnect branch from 4075a5f to 99ad550 Compare October 15, 2026 11:37
@yukibtc
yukibtc merged commit 99ad550 into nostrdevkit:master Oct 15, 2026
42 checks passed
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