Skip to content

fix(relay): clean up in-flight websocket requests - #4998

Draft
stuartwdouglas wants to merge 2 commits into
block:mainfrom
stuartwdouglas:sdouglas/fix-relay-req-lifecycle
Draft

fix(relay): clean up in-flight websocket requests#4998
stuartwdouglas wants to merge 2 commits into
block:mainfrom
stuartwdouglas:sdouglas/fix-relay-req-lifecycle

Conversation

@stuartwdouglas

Copy link
Copy Markdown

Summary

  • track in-flight WebSocket REQ handlers by subscription ID and generation
  • cancel pending handlers when clients send CLOSE, replace a subscription ID, or disconnect
  • serialize REQ registration with CLOSE/disconnect cleanup so a late handler cannot reattach subscription state
  • make local subscription, fan-out index, and Redis topic-refcount transitions cancellation-safe
  • enforce the per-connection subscription ceiling at the serialized insertion point

Root cause

REQ handlers run in detached tasks because historical reads can take time. CLOSE and disconnect cleanup run independently.

A history timeout or other early cleanup could therefore produce this ordering:

  1. client sends REQ
  2. handler waits on access/database work
  3. client sends CLOSE (or disconnects)
  4. cleanup finds no registered subscription and completes
  5. the detached handler resumes and registers filters, fan-out indexes, and a Redis desired-topic refcount

On a long-lived WebSocket, that state was effectively leaked. Disconnect had the same race because its registry cleanup was one-shot and did not synchronize with REQ registration.

The fix uses cooperative cancellation plus the existing per-connection subscription mutex as the registration/cleanup barrier. Once registration begins, the registry and pub/sub transition runs to completion; teardown then removes it. If teardown wins the mutex, the REQ observes cancellation and does not register.

Testing

  • cargo test -p buzz-relay connection::tests:: --lib
  • cargo clippy -p buzz-relay --lib -- -D warnings
  • pre-push Rust tests and desktop Tauri checks

A full cargo test -p buzz-relay --lib run completed 852 tests successfully; nine unrelated database-backed admin/media tests failed because the local Postgres pool timed out.

Signed-off-by: Stuart Douglas <sdouglas@block.xyz>
Amp-Thread-ID: https://ampcode.com/threads/T-019fd4c7-3d09-71af-b091-05920b3b0957
Co-authored-by: Amp <amp@ampcode.com>

@wesbillman wesbillman left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Carl reviewing on behalf of Wes. The lifecycle barrier is directionally sound, but the NIP-50 path still permits output from a cancelled generation, so I don't think this head fully establishes the advertised CLOSE/replacement guarantee. Please also add a deterministic lifecycle regression that stages a blocked REQ, processes CLOSE/disconnect/replacement, resumes it, and asserts the local subscription map, fan-out registry, and Redis desired-topic refcount—not just token bookkeeping. Focused cargo test -p buzz-relay connection::tests:: passes at this exact head (10 tests), but those tests do not exercise the state race.

Comment thread crates/buzz-relay/src/handlers/req.rs Outdated
trace_state.as_ref(),
)
.await;
tokio::select! {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 — CLOSE/replacement can still be followed by stale search output. This outer select! only observes cancellation when handle_search_req yields. After its page hydration await, handle_search_req synchronously walks the hydrated page and calls conn.send for every accepted event, then sends EOSE (req.rs:719-765), with no cancellation check or yield. On a multi-thread runtime, the recv task can process CLOSE (or a replacement REQ) and enqueue CLOSED/new-generation output while this task continues enqueueing old-generation EVENTs and EOSE. Please pass the generation token into the search helper and check it before each emission and before EOSE (and make awaited phases cancellation-aware), with a deterministic regression proving no old-generation output is emitted after CLOSE/replacement wins.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in cd62a82. NIP-50 search now observes cancellation around search/hydration awaits and before every EVENT/EOSE emission. Each emission also holds the active request-generation guard through the enqueue, so CLOSE removal or same-ID replacement is serialized with old-generation output; once either operation wins, the old generation cannot enqueue another frame. Added deterministic CLOSE/replacement output coverage plus blocked-registration lifecycle regressions for CLOSE, disconnect, and replacement. Those tests poll the real cleanup/replacement path to Pending behind the connection subscription mutex, then resume registration and assert the connection-local map, fan-out registry, and PubSub topic refcount. Validation: connection tests (14), REQ tests (47), clippy -D warnings, and pre-push hooks all pass.

Signed-off-by: Stuart Douglas <sdouglas@block.xyz>
Amp-Thread-ID: https://ampcode.com/threads/T-019fd4c7-3d09-71af-b091-05920b3b0957
Co-authored-by: Amp <amp@ampcode.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