From 2c56dcefa84563881804ae528924d758c0d68059 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sat, 24 Oct 2026 17:54:23 -0400 Subject: [PATCH 1/9] test(relay): pipeline REQ+CLOSE and AUTH+REQ in socket tests Both pairs are written in one flush, so the loop reads them back to back and the ordering between their handlers decides the outcome. Neither test passes on this tree: the CLOSE finds nothing to abort and the restricted REQ is refused before its AUTH is handled. --- crates/nostream-relay/tests/handlers.rs | 130 ++++++++++++++++++++++++ 1 file changed, 130 insertions(+) diff --git a/crates/nostream-relay/tests/handlers.rs b/crates/nostream-relay/tests/handlers.rs index 5ace1fed..ee0d820f 100644 --- a/crates/nostream-relay/tests/handlers.rs +++ b/crates/nostream-relay/tests/handlers.rs @@ -67,6 +67,25 @@ async fn send_event(server: &Server, event: &Event) -> String { client.expect_frame().await } +/// The text frames that arrive within `window`, read straight off the socket so a test can +/// bound a silence shorter than `Client::next_message`'s two-second deadline. +async fn frames_within(client: &mut common::Client, window: Duration) -> Vec { + use futures_util::StreamExt; + use tokio_tungstenite::tungstenite::Message; + + let deadline = tokio::time::Instant::now() + window; + let stream = client.stream(); + let mut frames = Vec::new(); + + while let Ok(Some(Ok(message))) = tokio::time::timeout_at(deadline, stream.next()).await { + if let Message::Text(text) = message { + frames.push(text.as_str().to_owned()); + } + } + + frames +} + // --------------------------------------------------------------------------------------- // EVENT (R9) // --------------------------------------------------------------------------------------- @@ -1025,6 +1044,72 @@ async fn a_close_aborts_an_in_flight_read_without_a_frame() { assert_eq!(frames[1], r#"["EOSE","s1"]"#); } +/// A `CLOSE` must not be handled ahead of the `REQ` it closes. +/// +/// Both frames leave in one flush, so the loop reads them back to back while the stored read +/// is parked at its gate — the window in which an unordered dispatch lets the `CLOSE` run +/// first, leave nothing to abort, and let the `REQ` register a subscription nobody will close. +/// The gate is part of the test: without it the in-memory read finishes in one poll and its +/// frames arrive even when the ordering is right. `multi_thread, worker_threads = 1` is the +/// flavour the defect was measured on (30/30); on `current_thread` a spawned handler cannot +/// be polled before the spawning task yields, so the test would pass on a wrong tree. +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +async fn a_close_that_overtakes_its_req_streams_nothing() { + use futures_util::SinkExt; + use tokio_tungstenite::tungstenite::Message; + + let fixture = Fixture::new(settings()); + let gate = fixture.events.gate_reads(); + fixture + .seed_event(signed_event(&CLIENT_SECRET, 1, vec![], "stored", NOW_SECS)) + .await; + + let server = start(fixture).await; + let (mut client, _challenge) = server.connect().await; + let live = signed_event(&CLIENT_SECRET, 1, vec![], "live", NOW_SECS); + + // Nothing may name the closed subscription: not a live dispatch, not the stored read. + let nothing_for_s1 = |frames: &[String]| { + assert!( + frames.iter().all(|frame| { + !frame.starts_with(r#"["EVENT","s1""#) && frame != r#"["EOSE","s1"]"# + }), + "a closed subscription streamed: {frames:?}" + ); + }; + + // One flush, so both frames are buffered before the loop reads either of them. + let stream = client.stream(); + stream + .feed(Message::Text( + r#"["REQ","s1",{"kinds":[1]}]"#.to_owned().into(), + )) + .await + .expect("feed the REQ"); + stream + .feed(Message::Text(r#"["CLOSE","s1"]"#.to_owned().into())) + .await + .expect("feed the CLOSE"); + stream.flush().await.expect("flush both frames"); + + // The window the `REQ` handler registers in and parks at the gated read. + tokio::time::sleep(Duration::from_millis(50)).await; + client.send(&event_frame(&live)).await; + + let frames = frames_within(&mut client, Duration::from_millis(150)).await; + + nothing_for_s1(&frames); + assert_eq!(frames.len(), 1, "only the live event's OK: {frames:?}"); + assert_eq!(frames[0], format!(r#"["OK","{}",true,""]"#, live.id)); + + // Releasing the read decides nothing new: the read either never opened or was aborted. + gate.add_permits(4); + + let frames = frames_within(&mut client, Duration::from_millis(300)).await; + + nothing_for_s1(&frames); +} + #[tokio::test] async fn a_new_filter_set_replaces_the_subscription() { let fixture = Fixture::new(settings()); @@ -1489,6 +1574,51 @@ async fn nip42_sessions_expire_at_the_ttl_boundary() { assert!(forever.is_authenticated(&pubkey, i64::MAX)); } +/// An `AUTH` must be handled before the restricted `REQ` that follows it in the stream. +/// +/// The existing NIP-42 tests read the `OK` before sending their `REQ`, which serialises the +/// pair; pipelined in one flush they exercise the loop's ordering. Two workers, the flavour +/// this symptom was measured on (10/10 refused before the ordering fix). +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn a_pipelined_auth_before_a_restricted_req_is_ordered() { + use futures_util::SinkExt; + use tokio_tungstenite::tungstenite::Message; + + let mut settings = settings(); + settings.nip42 = Some(nip42_settings()); + let server = start(Fixture::new(settings)).await; + let (mut client, challenge) = server.connect().await; + let auth = auth_event(&challenge, NOW_SECS); + + // One flush, so both frames are buffered before the loop reads either of them. + let stream = client.stream(); + stream + .feed(Message::Text( + format!( + r#"["AUTH",{}]"#, + serde_json::to_string(&auth).expect("auth json") + ) + .into(), + )) + .await + .expect("feed the AUTH"); + stream + .feed(Message::Text(r#"["REQ","s1",{"kinds":[4]}]"#.to_owned().into())) + .await + .expect("feed the REQ"); + stream.flush().await.expect("flush both frames"); + + let first = client.expect_frame().await; + let second = client.expect_frame().await; + + assert_eq!(first, format!(r#"["OK","{}",true,""]"#, auth.id)); + assert_eq!(second, r#"["EOSE","s1"]"#); + assert!( + !first.starts_with(r#"["CLOSED","s1""#) && !second.starts_with(r#"["CLOSED","s1""#), + "the REQ was refused: {first} {second}" + ); +} + // --------------------------------------------------------------------------------------- // Dispatch and the message limiter (R14) // --------------------------------------------------------------------------------------- From cf5827a7400d110c909cf36197c59b4a304695e1 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sat, 24 Oct 2026 17:57:24 -0400 Subject: [PATCH 2/9] fix(relay): run a handler's ordered prefix on the connection loop `handlers::dispatch` gave every message's whole handler to the per-socket `JoinSet`, so two frames read in order could run in either order: a `CLOSE` could be handled ahead of its own `REQ` and find nothing to abort, and a restricted `REQ` could be refused before the `AUTH` that preceded it. `handlers::begin` runs each message's ordering-sensitive prefix on the loop task, before the next frame is read, and returns the continuation for the `JoinSet`. The prefixes are `subscribe` (through `begin_read`), `unsubscribe` and `auth`; `EVENT` and `COUNT` keep their whole body as one spawned future. Continuations stay concurrent, so a `CLOSE` during a gated invite mint still aborts the read. --- crates/nostream-relay/src/handlers/auth.rs | 6 +- crates/nostream-relay/src/handlers/mod.rs | 53 ++++++++--- .../nostream-relay/src/handlers/subscribe.rs | 47 +++++++--- .../src/handlers/unsubscribe.rs | 3 + crates/nostream-relay/src/relay.rs | 16 +++- crates/nostream-relay/tests/connection.rs | 92 ++++++++++++++++++- 6 files changed, 181 insertions(+), 36 deletions(-) diff --git a/crates/nostream-relay/src/handlers/auth.rs b/crates/nostream-relay/src/handlers/auth.rs index 193e4e17..196867e5 100644 --- a/crates/nostream-relay/src/handlers/auth.rs +++ b/crates/nostream-relay/src/handlers/auth.rs @@ -10,7 +10,11 @@ use crate::handlers::HandlerContext; /// The seven outcomes are `nip42::authenticate`'s failures in the capture's order, then the /// replay check, then `["OK",,true,""]`. The challenge is never rotated by AUTH /// (`web-socket-adapter.ts:186-196`). -pub async fn handle(message: AuthMessage, context: &HandlerContext) { +/// +/// It contains no `.await`, so it runs whole as the message's prefix, on the connection loop: +/// a restricted `REQ` behind this `AUTH` in the stream is judged against the session it +/// records. +pub fn handle(message: AuthMessage, context: &HandlerContext) { let event = message.event; let now = context.now_secs(); let challenge = context.connection.challenge(); diff --git a/crates/nostream-relay/src/handlers/mod.rs b/crates/nostream-relay/src/handlers/mod.rs index 5b6a3cb4..121b48ee 100644 --- a/crates/nostream-relay/src/handlers/mod.rs +++ b/crates/nostream-relay/src/handlers/mod.rs @@ -5,6 +5,15 @@ //! write-point sink, the read-through settings, the clock, the connection state and the //! repository seams. +//! [`begin`] runs a message's **ordering-sensitive prefix** on the caller's task — the +//! connection loop — before the loop reads the next frame, and returns the rest of the +//! handler as a future to spawn. The rule for which handler takes a prefix: it is the +//! per-connection state a later frame's handler reads and expects to be there. `subscribe` +//! registers its subscription and its read, `unsubscribe` clears them, and `auth` records the +//! session a restricted `REQ` is judged against, so those three are prefixes. `event` and +//! `count` keep their whole body as one future: what they do before their first `await` +//! touches the rate limiters and the settings, which no later frame reads in arrival order. + pub mod auth; pub mod count; pub mod event; @@ -186,20 +195,40 @@ pub fn restricted_read_kinds(settings: Option<&Nip42Settings>) -> Vec + Send>>; -/// Runs one decoded client message as the capture's `messageHandlerFactory` switch does. -pub fn dispatch( +/// Runs one decoded client message's ordering-sensitive prefix on the caller's task, as the +/// capture's `messageHandlerFactory` switch does, and returns the rest of its handler. +/// +/// The connection loop calls this before it reads the next frame, so every statement a +/// handler executes before its first suspension point has run in arrival order. `None` means +/// the message was handled inside its prefix; `Some(future)` is the continuation, which is +/// spawned into the socket's handler set and stays concurrent with its neighbours. +pub fn begin( message: nostream_core::messages::ClientMessage, - context: Arc, -) -> HandlerFuture { + context: &Arc, +) -> Option { use nostream_core::messages::ClientMessage; - Box::pin(async move { - match message { - ClientMessage::Event(event) => event::handle(event, &context).await, - ClientMessage::Subscribe(message) => subscribe::handle(message, &context).await, - ClientMessage::Count(message) => count::handle(message, &context).await, - ClientMessage::Unsubscribe(message) => unsubscribe::handle(message, &context), - ClientMessage::Auth(message) => auth::handle(message, &context).await, + match message { + ClientMessage::Event(event) => { + let context = Arc::clone(context); + + Some(Box::pin(async move { event::handle(event, &context).await })) + } + ClientMessage::Count(message) => { + let context = Arc::clone(context); + + Some(Box::pin(async move { count::handle(message, &context).await })) } - }) + ClientMessage::Subscribe(message) => subscribe::begin(message, context), + ClientMessage::Unsubscribe(message) => { + unsubscribe::handle(message, context); + + None + } + ClientMessage::Auth(message) => { + auth::handle(message, context); + + None + } + } } diff --git a/crates/nostream-relay/src/handlers/subscribe.rs b/crates/nostream-relay/src/handlers/subscribe.rs index 53e1a10e..b0ca6cf0 100644 --- a/crates/nostream-relay/src/handlers/subscribe.rs +++ b/crates/nostream-relay/src/handlers/subscribe.rs @@ -9,12 +9,15 @@ //! //! The in-flight read is registered **before the mint's one `await`**, as the capture sets //! `activeSubscriptionId` as its first statement (`subscribe-message-handler.ts:53`), so a -//! `CLOSE` handled after that registration aborts the stored read and the subscription -//! streams nothing (a `CLOSE` handled ahead of its own `REQ` finds nothing to abort: -//! cameri/nostream#958). The invite `EVENT` is emitted in that case: the capture's -//! emit is unguarded (`subscribe-message-handler.ts:111`) and attaches its abort signal only -//! to the stored read (`:230`), and parity wins where the two disagree absent a decision -//! record (ADR 0005). +//! `CLOSE` aborts the stored read and the subscription streams nothing. Everything through +//! that registration is the handler's ordering-sensitive prefix and runs on the connection +//! loop, so a `CLOSE` behind this `REQ` in the stream cannot be handled ahead of it — the +//! defect cameri/nostream#958 reported, closed by the decision recorded in ADR 0005. The +//! invite `EVENT` is emitted even then: the capture's emit is unguarded +//! (`subscribe-message-handler.ts:111`) and attaches its abort signal only to the stored read +//! (`:230`), and parity wins where the two disagree absent a decision record (ADR 0005). + +use std::sync::Arc; use futures_util::StreamExt; use nostream_core::expiration; @@ -25,13 +28,33 @@ use nostream_core::{Filter, Kind}; use crate::connection::InFlightRead; use crate::error::RelayError; -use crate::handlers::HandlerContext; +use crate::handlers::{HandlerContext, HandlerFuture}; /// `EventKinds.NIP43_INVITE_REQUEST`. const INVITE_REQUEST_KIND: Kind = Kind(28_935); /// The handler's entry point (`handleMessage`, `:51-82`). -pub async fn handle(message: SubscribeMessage, context: &HandlerContext) { +/// +/// Its ordering-sensitive prefix — everything through `begin_read` — runs on the caller's +/// task, so the connection loop has the subscription and its read registered before it reads +/// the next frame. The continuation starts at the invite mint and is spawned into the +/// socket's handler set, where it interleaves with the frames that follow. +pub fn begin(message: SubscribeMessage, context: &Arc) -> Option { + let (read, filters) = register(message, context)?; + let context = Arc::clone(context); + + Some(Box::pin(async move { + maybe_issue_invite_code(read.subscription_id(), &filters, &context).await; + + fetch_and_send(read, &filters, &context).await; + })) +} + +/// The prefix (`handleMessage`, `:51-69`): what the capture runs before its first `await`. +fn register( + message: SubscribeMessage, + context: &HandlerContext, +) -> Option<(InFlightRead, Vec)> { let subscription_id = message.subscription_id; let search_enabled = context .settings @@ -43,7 +66,7 @@ pub async fn handle(message: SubscribeMessage, context: &HandlerContext) { if let Some(reason) = can_subscribe(&subscription_id, &filters, context) { context.send_notice(&format!("Subscription rejected: {reason}")); - return; + return None; } let restricted = context.restricted_read_kinds(); @@ -55,7 +78,7 @@ pub async fn handle(message: SubscribeMessage, context: &HandlerContext) { "auth-required: authentication is required to request these event kinds", None, ); - return; + return None; } context @@ -68,9 +91,7 @@ pub async fn handle(message: SubscribeMessage, context: &HandlerContext) { // Registered before the mint's only `await`, so the read a `CLOSE` aborts exists by then. let read = context.connection.begin_read(&subscription_id); - maybe_issue_invite_code(&subscription_id, &filters, context).await; - - fetch_and_send(read, &filters, context).await; + Some((read, filters)) } /// NIP-43: kind 28935 is never published by a client and never stored. A REQ for it asks the diff --git a/crates/nostream-relay/src/handlers/unsubscribe.rs b/crates/nostream-relay/src/handlers/unsubscribe.rs index 51b422a8..02746b5d 100644 --- a/crates/nostream-relay/src/handlers/unsubscribe.rs +++ b/crates/nostream-relay/src/handlers/unsubscribe.rs @@ -2,6 +2,9 @@ //! //! A `CLOSE` removes the subscription, aborts its in-flight read, and emits **no frame** //! (`:9-11`; measured: `CLOSE s1` → no frame). +//! +//! It runs whole as the message's prefix, on the connection loop, so a `CLOSE` behind its own +//! `REQ` in the stream always finds the subscription and the read it aborts. use nostream_core::messages::UnsubscribeMessage; diff --git a/crates/nostream-relay/src/relay.rs b/crates/nostream-relay/src/relay.rs index 003bd689..70fb1613 100644 --- a/crates/nostream-relay/src/relay.rs +++ b/crates/nostream-relay/src/relay.rs @@ -4,9 +4,11 @@ //! //! Each connection task owns its socket and the state machine R26 lists: `alive`, the //! subscription table, the NIP-42 session, the in-flight handler set and the sink. Outbound -//! frames travel through [`FrameSink`]; inbound text is rate-limited, decoded, and handed to -//! the handler `messageHandlerFactory` would pick. Accepted events fan out to this process's -//! connections through one broadcast channel (cross-worker fan-out stays a stub). +//! frames travel through [`FrameSink`]; inbound text is rate-limited, decoded, and given to +//! the handler `messageHandlerFactory` would pick — the handler's ordering-sensitive prefix +//! runs here, on the loop, before the next frame is read, and the rest of it is spawned into +//! the per-socket handler set. Accepted events fan out to this process's connections through +//! one broadcast channel (cross-worker fan-out stays a stub). use std::sync::{Arc, Mutex}; use std::time::Duration; @@ -478,6 +480,9 @@ impl Relay { /// One inbound text frame: the message limiter first, then the decoder, then the handler /// (`web-socket-adapter.ts:198-240`). + /// + /// The handler's prefix runs here, before the loop reads the next frame; its continuation + /// joins the socket's handler set. fn on_client_message( &self, text: &str, @@ -504,8 +509,9 @@ impl Relay { match messages::decode(text) { Ok(message) => { - let context = Arc::clone(context); - handlers.spawn(handlers::dispatch(message, context)); + if let Some(handler) = handlers::begin(message, context) { + handlers.spawn(handler); + } } Err(error) => context.send_notice(&error.notice()), } diff --git a/crates/nostream-relay/tests/connection.rs b/crates/nostream-relay/tests/connection.rs index 43f01c1b..5b6ee276 100644 --- a/crates/nostream-relay/tests/connection.rs +++ b/crates/nostream-relay/tests/connection.rs @@ -18,7 +18,7 @@ use std::sync::Arc; use common::{CLIENT_SECRET, NOW_MS, NOW_SECS, Server, fast_config, relay_keys, signed_event}; use nostream_core::clock::TestClock; use nostream_core::filter::Filter; -use nostream_core::messages::SubscribeMessage; +use nostream_core::messages::{AuthMessage, ClientMessage, SubscribeMessage, UnsubscribeMessage}; use nostream_core::{EventId, Pubkey}; use nostream_relay::connection::{Connection, Subscription}; use nostream_relay::sink::FrameSink; @@ -427,7 +427,7 @@ async fn closing_the_socket_aborts_an_in_flight_read() { let context = Arc::clone(&context); async move { - nostream_relay::handlers::subscribe::handle( + let continuation = nostream_relay::handlers::subscribe::begin( SubscribeMessage { subscription_id: "s1".to_owned(), filters: vec![Filter { @@ -437,7 +437,9 @@ async fn closing_the_socket_aborts_an_in_flight_read() { }, &context, ) - .await; + .expect("the REQ registers its subscription"); + + continuation.await; } }); @@ -510,7 +512,7 @@ async fn restricted_read_with(authenticate_mid_read: bool) -> Vec { let context = Arc::clone(&context); async move { - nostream_relay::handlers::subscribe::handle( + let continuation = nostream_relay::handlers::subscribe::begin( SubscribeMessage { subscription_id: "restricted".to_owned(), // A filter that names no kind is accepted while unauthenticated; the @@ -520,7 +522,9 @@ async fn restricted_read_with(authenticate_mid_read: bool) -> Vec { }, &context, ) - .await; + .expect("the REQ registers its subscription"); + + continuation.await; } }); @@ -604,3 +608,81 @@ async fn a_restricted_read_is_judged_against_the_authenticated_set_at_each_event assert!(authenticated[1].contains("encrypted")); assert_eq!(authenticated[2], r#"["EOSE","restricted"]"#); } + +// --------------------------------------------------------------------------------------- +// Handler dispatch order +// --------------------------------------------------------------------------------------- + +/// A message's prefix has run by the time `begin` returns, before its continuation is polled. +/// +/// No scheduler is involved: every assertion sits between two synchronous calls on this task, +/// so `Connection` either holds the registration or it does not. +#[tokio::test] +async fn a_handler_prefix_runs_before_its_continuation_is_polled() { + let fixture = Fixture::new(settings()); + let connection = connection(); + let (sink, mut frames) = FrameSink::channel(1024 * 1024); + let context = Arc::new(fixture.context( + Arc::new(TestClock::new(NOW_MS)), + relay_keys(), + Arc::clone(&connection), + sink, + )); + + let continuation = nostream_relay::handlers::begin( + ClientMessage::Subscribe(SubscribeMessage { + subscription_id: "s1".to_owned(), + filters: vec![Filter { + kinds: Some(vec![nostream_core::Kind(1)]), + ..Default::default() + }], + }), + &context, + ); + + // Nothing above was awaited and nothing was polled, yet the REQ has registered. + assert!(continuation.is_some(), "the REQ has a continuation"); + assert!(connection.read_in_flight("s1")); + assert!(connection.subscription("s1").is_some()); + + // Dropped unpolled: the registration is the whole of what this test needs from it. + drop(continuation); + + assert!( + nostream_relay::handlers::begin( + ClientMessage::Unsubscribe(UnsubscribeMessage { + subscription_id: "s1".to_owned(), + }), + &context, + ) + .is_none(), + "a CLOSE completes inside its prefix" + ); + assert!(!connection.read_in_flight("s1")); + assert!(connection.subscription("s1").is_none()); + + let event = signed_event( + &CLIENT_SECRET, + 22_242, + vec![ + vec!["relay", "wss://relay.example.com"], + vec!["challenge", "challenge"], + ], + "", + NOW_SECS, + ); + + assert!( + nostream_relay::handlers::begin(ClientMessage::Auth(AuthMessage { event }), &context) + .is_none(), + "an AUTH completes inside its prefix" + ); + + // The OK is already in the sink, so the AUTH's frame was written on this task. + let ok = frames.try_recv().expect("the AUTH's OK frame"); + + assert!( + ok.starts_with(r#"["OK","#) && ok.ends_with(r#"true,""]"#), + "{ok}" + ); +} From 0d713cc7eb46f5e98271fb224678b4db4e1f03dc Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sat, 24 Oct 2026 17:58:09 -0400 Subject: [PATCH 3/9] docs(rewrite): record the inbound dispatch order decision in ADR 0005 The ADR states the chosen shape (a prefix on the connection loop), the two rejected shapes with their reasons, the `AUTH`->`REQ` symptom, the `EVENT`/`COUNT` exclusion and the guarantee's boundary: prefix order, not handler serialisation. Its "invite emit stays unguarded" paragraph no longer describes a `CLOSE` overtaking its `REQ` as a live precondition. The changeset carries the release note; the TypeScript tree is untouched. --- .changeset/relay-inbound-dispatch-order.md | 21 ++++++++ .../decisions/0005-connection-concurrency.md | 48 ++++++++++++++++--- 2 files changed, 63 insertions(+), 6 deletions(-) create mode 100644 .changeset/relay-inbound-dispatch-order.md diff --git a/.changeset/relay-inbound-dispatch-order.md b/.changeset/relay-inbound-dispatch-order.md new file mode 100644 index 00000000..95800469 --- /dev/null +++ b/.changeset/relay-inbound-dispatch-order.md @@ -0,0 +1,21 @@ +--- +--- + +fix(relay): order an inbound frame's prefix before the next frame is read + +A per-socket handler ran as an independently scheduled task, so two frames read in order +could run in either order: a `CLOSE` handled ahead of its own `REQ` found nothing to abort +and the subscription kept streaming live events to a closed id, and a restricted `REQ` +pipelined behind its `AUTH` was refused for want of a session that had been sent but not +yet handled (cameri/nostream#958). + +An inbound frame now runs its ordering-sensitive prefix — `subscribe` through its read +registration, `unsubscribe`, and `auth` — on the connection loop before the next frame is +read. The rest of the handler is spawned and stays concurrent, so a `CLOSE` during an +invite mint still aborts the stored read and the invite `EVENT` is still emitted. `EVENT` +and `COUNT` are unchanged. + +The TypeScript tree is untouched: no `src/` or `test/` file changes, so the published +package has no user-visible change. The decision, the two rejected shapes and the +guarantee's boundary are recorded in +`docs/rewrite/decisions/0005-connection-concurrency.md`. diff --git a/docs/rewrite/decisions/0005-connection-concurrency.md b/docs/rewrite/decisions/0005-connection-concurrency.md index a4c9f090..817a75a1 100644 --- a/docs/rewrite/decisions/0005-connection-concurrency.md +++ b/docs/rewrite/decisions/0005-connection-concurrency.md @@ -1,6 +1,6 @@ # ADR 0005: Connection concurrency — read ownership, a bounded frame queue, and liveness under a blocked write (job 010) -- **Status:** accepted (job 010, Phase 2) +- **Status:** accepted (job 010 and job 012, Phase 2) - **Deciders:** the rewrite team, on the evidence in job 010's recon and spec - **Date:** 2026-10-24 @@ -134,11 +134,11 @@ on as soon as the write completes. ## The invite emit stays unguarded The read is registered before the invite mint's only `await`, and its cancellation is -checked before the stored read opens, so a `CLOSE` that is *handled after that registration* -ends the subscription with no stored event and no `EOSE`; one whose handler is scheduled -ahead of its own `REQ` finds nothing to abort, which is pre-existing and tracked as -cameri/nostream#958. The invite `EVENT` is nonetheless still -emitted for that closed subscription. That is deliberate parity, not an oversight: the +checked before the stored read opens, so a `CLOSE` ends the subscription with no stored +event and no `EOSE`. Since §"Inbound dispatch order" below, that `CLOSE` cannot be handled +ahead of its own `REQ` either: a handler's prefix runs on the connection loop before the next +frame is read, which is what closed cameri/nostream#958. The invite `EVENT` is nonetheless +still emitted for that closed subscription. That is deliberate parity, not an oversight: the capture emits the invite event unguarded (`src/handlers/subscribe-message-handler.ts:111`) and attaches its abort signal only to the stored read (`:230`), while it sets the abortable subscription id as the handler's first @@ -146,6 +146,42 @@ statement (`:53`). The TypeScript tree wins where the two trees disagree absent record, so the fix must not reach for the emit; the exposure it leaves is one stray `EVENT`, which is the capture's own. +## Inbound dispatch order — a prefix on the loop, not a serialized handler + +An inbound frame's handler was spawned whole into the per-socket `JoinSet`, so two frames +read in order ran in whichever order the scheduler picked: a `CLOSE` could be handled ahead +of its own `REQ` and find nothing to abort, and a restricted `REQ` could be refused before +the `AUTH` that preceded it had been handled. Both were measured through a real socket — +30/30 runs at one worker for the `REQ`/`CLOSE` pair, 10/10 at two for `AUTH`/`REQ` — and +neither was visible to the suite, because `#[tokio::test]` defaults to the one runtime flavour +in which a spawned handler cannot overtake the task that spawned it. + +The decision is the capture's own division of a message: its **ordering-sensitive prefix** +runs before the next frame is read, and its continuations stay concurrent. +`handlers::begin` runs each prefix on the connection loop and returns the continuation for +the `JoinSet`. The prefixes are `subscribe` (everything through `begin_read`, so the +subscription and its read exist before a following `CLOSE` is looked at), `unsubscribe` (the +whole handler) and `auth` (the whole handler, which has no `await`). `EVENT` and `COUNT` keep +their whole body as one spawned future: what they do before their first `await` touches the +rate limiters and the settings, which no later frame reads in arrival order, and moving it +would add loop latency to the hottest inbound path for no capture-visible difference. + +Two shapes were rejected. **Serializing whole handlers** — an ordered queue drained in the +loop — would run a `REQ`'s invite mint to completion before a `CLOSE` already in the buffer +is looked at, which the capture permits and `tests/handlers.rs` asserts, and it would put +every database write in front of the socket's whole read path. **An ordering token per +connection** — a per-id "closed at sequence N" consulted by `begin_read` — needs the second +structure in `InFlightReads` this record argues against in §"Read ownership", must be bounded +or evicted, needs a rule for what an "older" `REQ` means under a re-`REQ`, and orders +`AUTH` before a restricted `REQ` only with a second, unrelated rule. + +What this does not decide: it is a guarantee about prefixes, not about handler execution. +Continuations interleave as before, so a `CLOSE` during a gated invite mint still aborts the +read and the invite `EVENT` is still emitted. The cost accepted is that +`normalise_subscription` and `can_subscribe` run on the loop task, bounded by +`transport::max_payload_size` and, where configured, `max_filters` / `max_filter_values`; +the capture does the same work on its socket's thread. + ## Not decided here — the port's `CLOSE` semantics Whether a plain `CLOSE` aborts a stored read is out of scope for this record; it is tracked From 0c02d047659affccbfed63e81586f10c298dccde Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sat, 24 Oct 2026 17:58:29 -0400 Subject: [PATCH 4/9] style(relay): apply rustfmt to the new dispatch and test code --- crates/nostream-relay/src/handlers/mod.rs | 8 ++++++-- crates/nostream-relay/tests/handlers.rs | 4 +++- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/crates/nostream-relay/src/handlers/mod.rs b/crates/nostream-relay/src/handlers/mod.rs index 121b48ee..8b8dc8ad 100644 --- a/crates/nostream-relay/src/handlers/mod.rs +++ b/crates/nostream-relay/src/handlers/mod.rs @@ -212,12 +212,16 @@ pub fn begin( ClientMessage::Event(event) => { let context = Arc::clone(context); - Some(Box::pin(async move { event::handle(event, &context).await })) + Some(Box::pin( + async move { event::handle(event, &context).await }, + )) } ClientMessage::Count(message) => { let context = Arc::clone(context); - Some(Box::pin(async move { count::handle(message, &context).await })) + Some(Box::pin( + async move { count::handle(message, &context).await }, + )) } ClientMessage::Subscribe(message) => subscribe::begin(message, context), ClientMessage::Unsubscribe(message) => { diff --git a/crates/nostream-relay/tests/handlers.rs b/crates/nostream-relay/tests/handlers.rs index ee0d820f..9141b010 100644 --- a/crates/nostream-relay/tests/handlers.rs +++ b/crates/nostream-relay/tests/handlers.rs @@ -1603,7 +1603,9 @@ async fn a_pipelined_auth_before_a_restricted_req_is_ordered() { .await .expect("feed the AUTH"); stream - .feed(Message::Text(r#"["REQ","s1",{"kinds":[4]}]"#.to_owned().into())) + .feed(Message::Text( + r#"["REQ","s1",{"kinds":[4]}]"#.to_owned().into(), + )) .await .expect("feed the REQ"); stream.flush().await.expect("flush both frames"); From 8b4751a89a324bac81487ece97c30039356ed785 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sat, 24 Oct 2026 18:20:13 -0400 Subject: [PATCH 5/9] test(relay): anchor the CLOSE oracle on a loop-side barrier, not a sleep The oracle waited 50 ms for the REQ handler to register and then read frames for a fixed 150 ms window, asserting the window held exactly one frame. Both are load-dependent: a loaded runner could miss the live event's OK in the window and fail with the ordering correct, and a short wait at the head would let the test pass without the defect being exercised. The obstacle frame is now an undecodable one, whose NOTICE the connection loop writes itself, so receiving it proves the loop read the REQ and CLOSE ahead of it. The live event is sent after that barrier and its own OK, on the client's two-second deadline, ends the wait; the only timed assertion left is the post-gate silence, where a bounded window is the assertion. --- crates/nostream-relay/tests/handlers.rs | 74 ++++++++++++++++--------- 1 file changed, 48 insertions(+), 26 deletions(-) diff --git a/crates/nostream-relay/tests/handlers.rs b/crates/nostream-relay/tests/handlers.rs index 9141b010..ffaec0c6 100644 --- a/crates/nostream-relay/tests/handlers.rs +++ b/crates/nostream-relay/tests/handlers.rs @@ -86,6 +86,11 @@ async fn frames_within(client: &mut common::Client, window: Duration) -> Vec bool { + frame.starts_with(r#"["EVENT","s1""#) || frame == r#"["EOSE","s1"]"# +} + // --------------------------------------------------------------------------------------- // EVENT (R9) // --------------------------------------------------------------------------------------- @@ -1046,13 +1051,16 @@ async fn a_close_aborts_an_in_flight_read_without_a_frame() { /// A `CLOSE` must not be handled ahead of the `REQ` it closes. /// -/// Both frames leave in one flush, so the loop reads them back to back while the stored read -/// is parked at its gate — the window in which an unordered dispatch lets the `CLOSE` run -/// first, leave nothing to abort, and let the `REQ` register a subscription nobody will close. -/// The gate is part of the test: without it the in-memory read finishes in one poll and its -/// frames arrive even when the ordering is right. `multi_thread, worker_threads = 1` is the -/// flavour the defect was measured on (30/30); on `current_thread` a spawned handler cannot -/// be polled before the spawning task yields, so the test would pass on a wrong tree. +/// The `REQ` and the `CLOSE` leave in one flush, followed by a frame that cannot be decoded: +/// its `NOTICE` is written by the connection loop itself, so receiving it proves the loop has +/// read the two frames ahead of it. No wall-clock wait stands in for an observation — the +/// barrier orders the live event after both frames, and the live event's own `OK` (on the +/// client's deadline) is the only other anchor. The stored read stays parked at its gate until +/// the test releases it, which is what keeps the assertion about ordering: without the gate +/// the in-memory read finishes in one poll and its frames arrive even when the ordering is +/// right. `multi_thread, worker_threads = 1` is the flavour the defect was measured on (30/30); +/// on `current_thread` a spawned handler cannot be polled before the spawning task yields, so +/// the test would pass on a wrong tree. #[tokio::test(flavor = "multi_thread", worker_threads = 1)] async fn a_close_that_overtakes_its_req_streams_nothing() { use futures_util::SinkExt; @@ -1067,18 +1075,9 @@ async fn a_close_that_overtakes_its_req_streams_nothing() { let server = start(fixture).await; let (mut client, _challenge) = server.connect().await; let live = signed_event(&CLIENT_SECRET, 1, vec![], "live", NOW_SECS); + let accepted = format!(r#"["OK","{}",true,""]"#, live.id); - // Nothing may name the closed subscription: not a live dispatch, not the stored read. - let nothing_for_s1 = |frames: &[String]| { - assert!( - frames.iter().all(|frame| { - !frame.starts_with(r#"["EVENT","s1""#) && frame != r#"["EOSE","s1"]"# - }), - "a closed subscription streamed: {frames:?}" - ); - }; - - // One flush, so both frames are buffered before the loop reads either of them. + // One flush, so the loop reads the three frames in the order they are written here. let stream = client.stream(); stream .feed(Message::Text( @@ -1090,24 +1089,47 @@ async fn a_close_that_overtakes_its_req_streams_nothing() { .feed(Message::Text(r#"["CLOSE","s1"]"#.to_owned().into())) .await .expect("feed the CLOSE"); - stream.flush().await.expect("flush both frames"); + stream + .feed(Message::Text(r#"{"kind":"#.to_owned().into())) + .await + .expect("feed the undecodable frame"); + stream.flush().await.expect("flush the three frames"); - // The window the `REQ` handler registers in and parks at the gated read. - tokio::time::sleep(Duration::from_millis(50)).await; + // The loop's own answer to the third frame: the `REQ` and the `CLOSE` have been read. + let barrier = client.expect_frame().await; + assert!(barrier.starts_with(r#"["NOTICE","invalid: "#), "{barrier}"); + + // The live event's `OK` is the second anchor: nothing for `s1` may reach the client on + // the way to it, and the client's own deadline bounds the wait. client.send(&event_frame(&live)).await; - let frames = frames_within(&mut client, Duration::from_millis(150)).await; + loop { + let Some(message) = client.next_message().await else { + panic!("the live event's OK never arrived"); + }; + let Message::Text(frame) = message else { + continue; + }; + + assert!( + !names_s1(frame.as_str()), + "a closed subscription streamed: {frame}" + ); - nothing_for_s1(&frames); - assert_eq!(frames.len(), 1, "only the live event's OK: {frames:?}"); - assert_eq!(frames[0], format!(r#"["OK","{}",true,""]"#, live.id)); + if frame.as_str() == accepted { + break; + } + } // Releasing the read decides nothing new: the read either never opened or was aborted. gate.add_permits(4); let frames = frames_within(&mut client, Duration::from_millis(300)).await; - nothing_for_s1(&frames); + assert!( + frames.iter().all(|frame| !names_s1(frame)), + "a closed subscription streamed: {frames:?}" + ); } #[tokio::test] From e808769ebeaf080c4fa7857e6611016c104fb05f Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sat, 24 Oct 2026 18:39:51 -0400 Subject: [PATCH 6/9] docs(relay): state the prefix guarantee and the event limiter's ordering `begin`'s docblock promised that every statement a handler runs before its first suspension point has run in arrival order, which is false for the `Event` and `Count` arms: they are boxed whole, so none of their body is ordered. The postcondition is now stated about the prefix, with the single-caller precondition the guarantee rests on, and the two excluded arms say so at the edit site. The reason given for excluding `EVENT` and `COUNT` was untrue: the event limiter is keyed on this connection's address and each `EVENT` consults it from its own task, so two pipelined events may record their hits in either order. The module doc and ADR 0005 now give the criterion that does hold (no later frame needs an entry these handlers establish) and record that consequence as parity with the capture, whose limiter hit also sits after its first `await`. ADR 0005's cost clause now names the `AUTH` prefix, the expensive one, with its measured cost and the interleaving it gives up. --- crates/nostream-relay/src/handlers/mod.rs | 23 ++++++++++++++----- .../decisions/0005-connection-concurrency.md | 22 ++++++++++++------ 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/crates/nostream-relay/src/handlers/mod.rs b/crates/nostream-relay/src/handlers/mod.rs index 8b8dc8ad..aafbdf50 100644 --- a/crates/nostream-relay/src/handlers/mod.rs +++ b/crates/nostream-relay/src/handlers/mod.rs @@ -11,8 +11,13 @@ //! per-connection state a later frame's handler reads and expects to be there. `subscribe` //! registers its subscription and its read, `unsubscribe` clears them, and `auth` records the //! session a restricted `REQ` is judged against, so those three are prefixes. `event` and -//! `count` keep their whole body as one future: what they do before their first `await` -//! touches the rate limiters and the settings, which no later frame reads in arrival order. +//! `count` keep their whole body as one future: no later frame's handler needs an entry they +//! establish — `COUNT`'s work before its first `await` mutates nothing shared, and the event +//! limiter, though keyed on this connection, is consulted by each `EVENT` from its own task — +//! so none of it has to be ordered. The accepted consequence is that two pipelined `EVENT`s +//! may have their limiter hits recorded in either order, so which of the two is refused is not +//! fixed by arrival order; the capture's limiter hit also sits after +//! `await this.isEventValid(event)` (`event-message-handler.ts:66-86`), so that is parity. pub mod auth; pub mod count; @@ -198,10 +203,14 @@ pub type HandlerFuture = Pin + Send>>; /// Runs one decoded client message's ordering-sensitive prefix on the caller's task, as the /// capture's `messageHandlerFactory` switch does, and returns the rest of its handler. /// -/// The connection loop calls this before it reads the next frame, so every statement a -/// handler executes before its first suspension point has run in arrival order. `None` means -/// the message was handled inside its prefix; `Some(future)` is the continuation, which is -/// spawned into the socket's handler set and stays concurrent with its neighbours. +/// Nothing here can enforce when it is called: the guarantee holds because the connection loop +/// is the only caller, and it calls this before reading the next frame. For a message with a +/// prefix, every statement the prefix executes has run by the time this returns. `None` means +/// the prefix answered the message; `Some(future)` is the rest of the handler, which is spawned +/// into the socket's handler set and runs unordered with its neighbours. +/// +/// The postcondition is therefore about the prefix, not the handler: the `Event` and `Count` +/// arms box the whole handler, so none of their body is ordered. pub fn begin( message: nostream_core::messages::ClientMessage, context: &Arc, @@ -212,6 +221,7 @@ pub fn begin( ClientMessage::Event(event) => { let context = Arc::clone(context); + // Whole body by rule: `event` needs no prefix (see the module doc). Some(Box::pin( async move { event::handle(event, &context).await }, )) @@ -219,6 +229,7 @@ pub fn begin( ClientMessage::Count(message) => { let context = Arc::clone(context); + // Whole body by rule: `count` needs no prefix (see the module doc). Some(Box::pin( async move { count::handle(message, &context).await }, )) diff --git a/docs/rewrite/decisions/0005-connection-concurrency.md b/docs/rewrite/decisions/0005-connection-concurrency.md index 817a75a1..380bffd9 100644 --- a/docs/rewrite/decisions/0005-connection-concurrency.md +++ b/docs/rewrite/decisions/0005-connection-concurrency.md @@ -162,9 +162,12 @@ runs before the next frame is read, and its continuations stay concurrent. the `JoinSet`. The prefixes are `subscribe` (everything through `begin_read`, so the subscription and its read exist before a following `CLOSE` is looked at), `unsubscribe` (the whole handler) and `auth` (the whole handler, which has no `await`). `EVENT` and `COUNT` keep -their whole body as one spawned future: what they do before their first `await` touches the -rate limiters and the settings, which no later frame reads in arrival order, and moving it -would add loop latency to the hottest inbound path for no capture-visible difference. +their whole body as one spawned future: no later frame's handler needs an entry they establish, +so nothing they do before their first `await` has to be ordered. The accepted consequence is +that two pipelined `EVENT`s may have their rate-limiter hits recorded in either order, so which +of the two is refused is not fixed by arrival order — parity, because the capture's limiter hit +also sits after `await this.isEventValid(event)` +(`src/handlers/event-message-handler.ts:66-86`). Two shapes were rejected. **Serializing whole handlers** — an ordered queue drained in the loop — would run a `REQ`'s invite mint to completion before a `CLOSE` already in the buffer @@ -177,10 +180,15 @@ or evicted, needs a rule for what an "older" `REQ` means under a re-`REQ`, and o What this does not decide: it is a guarantee about prefixes, not about handler execution. Continuations interleave as before, so a `CLOSE` during a gated invite mint still aborts the -read and the invite `EVENT` is still emitted. The cost accepted is that -`normalise_subscription` and `can_subscribe` run on the loop task, bounded by -`transport::max_payload_size` and, where configured, `max_filters` / `max_filter_values`; -the capture does the same work on its socket's thread. +read and the invite `EVENT` is still emitted. The cost accepted is that the prefixes run on the +loop task. For `REQ` that is `normalise_subscription` and `can_subscribe`, bounded by +`transport::max_payload_size` and, where configured, `max_filters` / `max_filter_values`; for +`AUTH` it is the whole `nip42::authenticate`, measured on the 4-vCPU development box at 34.3 µs +per message (32.2 µs of it the signature verify, 1.28 µs the id hash), against 0.76 µs for +`REQ` and 0.053 µs for `CLOSE`. The capture does the same work on its socket's thread, but it +pays in awaited chunks that let other sockets interleave, where a prefix does not yield at all; +the same verify costs the capture 1.57–1.85 ms/op of CPU on Node, so the loop's per-`AUTH` cost +is well below the capture's, and what the prefix gives up is interleaving, not throughput. ## Not decided here — the port's `CLOSE` semantics From a891e6ac41fd77899ddbdd1f22f3eb24ae4a660d Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sat, 24 Oct 2026 18:55:18 -0400 Subject: [PATCH 7/9] docs(relay): correct the limiter's key, the None contract and the cost figures MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The event limiter is not keyed on the connection: `RateLimits::event_backoff` keys it on the event's author and window, in the table the whole relay shares, so the module doc and ADR 0005 now say that and keep the consequence — two pipelined `EVENT`s from one author record their hits in poll order. Both also give the capture's reason: it binds each `message` event to a handler it does not await, so its hits are unordered too. `begin`'s docblock said `None` means the prefix answered the message, which the `CLOSE` arm disproves: it returns `None` having written no frame. `None` now means the prefix handled the message and nothing is left to run, writing whatever frames its handler writes. The cost clause no longer presents a derived sum as a measurement: the `REQ` figure is named as its parts (0.11 µs normalisation plus 0.61–0.65 µs of subscription-table work, 0.72–0.76 µs), the verify as the 32.2–32.5 µs range it was measured over, and the attribution says this branch. It also drops the claim that the prefix costs only interleaving: the loop is occupied for those 34 µs, which caps a pipelining client's `AUTH` rate. ADR 0005's front matter now names both jobs in the title, the deciders' evidence and the context's four relationships. --- crates/nostream-relay/src/handlers/mod.rs | 26 ++++++------ .../decisions/0005-connection-concurrency.md | 42 ++++++++++--------- 2 files changed, 37 insertions(+), 31 deletions(-) diff --git a/crates/nostream-relay/src/handlers/mod.rs b/crates/nostream-relay/src/handlers/mod.rs index aafbdf50..6d934ca1 100644 --- a/crates/nostream-relay/src/handlers/mod.rs +++ b/crates/nostream-relay/src/handlers/mod.rs @@ -11,13 +11,14 @@ //! per-connection state a later frame's handler reads and expects to be there. `subscribe` //! registers its subscription and its read, `unsubscribe` clears them, and `auth` records the //! session a restricted `REQ` is judged against, so those three are prefixes. `event` and -//! `count` keep their whole body as one future: no later frame's handler needs an entry they -//! establish — `COUNT`'s work before its first `await` mutates nothing shared, and the event -//! limiter, though keyed on this connection, is consulted by each `EVENT` from its own task — -//! so none of it has to be ordered. The accepted consequence is that two pipelined `EVENT`s -//! may have their limiter hits recorded in either order, so which of the two is refused is not -//! fixed by arrival order; the capture's limiter hit also sits after -//! `await this.isEventValid(event)` (`event-message-handler.ts:66-86`), so that is parity. +//! `count` keep their whole body as one future: neither establishes per-connection state that a +//! later frame looks up. What they do touch is not per-connection either — the event limiter's +//! key is the event's author and window in a table the whole relay shares +//! (`RateLimits::event_backoff`) — so two pipelined `EVENT`s from one author record their hits +//! in whichever order their tasks are polled, and which of the two is refused is not fixed by +//! arrival order. The capture races the same way: each `message` event starts an async handler +//! the adapter does not await (`web-socket-adapter.ts:79`, `:229`), so the two hits are +//! unordered there too. pub mod auth; pub mod count; @@ -176,7 +177,7 @@ impl ReadGuard { } } -/// `getRestrictedReadKinds` (`src/utils/nip42.ts:15-23`): the configured kinds when +/// `getRestrictedReadKinds` (`src/utils/nip42.ts:17-24`): the configured kinds when /// restricted reads are enabled, the NIP-42 defaults when no list is given, none when off. pub fn restricted_read_kinds(settings: Option<&Nip42Settings>) -> Vec { let Some(settings) = settings else { @@ -204,10 +205,11 @@ pub type HandlerFuture = Pin + Send>>; /// capture's `messageHandlerFactory` switch does, and returns the rest of its handler. /// /// Nothing here can enforce when it is called: the guarantee holds because the connection loop -/// is the only caller, and it calls this before reading the next frame. For a message with a -/// prefix, every statement the prefix executes has run by the time this returns. `None` means -/// the prefix answered the message; `Some(future)` is the rest of the handler, which is spawned -/// into the socket's handler set and runs unordered with its neighbours. +/// is the only caller in `src`, and it calls this before reading the next frame. For a message +/// with a prefix, every statement the prefix executes has run by the time this returns. `None` +/// means the prefix handled the message and nothing is left to run — it writes whatever frames +/// its handler writes, and `CLOSE` writes none; `Some(future)` is the rest of the handler, which +/// is spawned into the socket's handler set and runs unordered with its neighbours. /// /// The postcondition is therefore about the prefix, not the handler: the `Event` and `Count` /// arms box the whole handler, so none of their body is ordered. diff --git a/docs/rewrite/decisions/0005-connection-concurrency.md b/docs/rewrite/decisions/0005-connection-concurrency.md index 380bffd9..ccedb36b 100644 --- a/docs/rewrite/decisions/0005-connection-concurrency.md +++ b/docs/rewrite/decisions/0005-connection-concurrency.md @@ -1,20 +1,21 @@ -# ADR 0005: Connection concurrency — read ownership, a bounded frame queue, and liveness under a blocked write (job 010) +# ADR 0005: Connection concurrency — read ownership, a bounded frame queue, liveness under a blocked write, and inbound dispatch order (jobs 010 and 012) - **Status:** accepted (job 010 and job 012, Phase 2) -- **Deciders:** the rewrite team, on the evidence in job 010's recon and spec +- **Deciders:** the rewrite team, on the evidence in job 010's recon and spec, and job 012's for §"Inbound dispatch order" - **Date:** 2026-10-24 ## Context -One task per socket owns the transport, and three of its concurrent relationships were -wrong. A re-`REQ` replaces the read registered for a subscription id and aborts the old +One task per socket owns the transport, and four of its concurrent relationships were wrong. +A re-`REQ` replaces the read registered for a subscription id and aborts the old one, and the old read's own cleanup then removed the *replacement's* cancellation sender, so a re-subscribed client could go silent without an `EOSE`. A `CLOSE` that landed while the NIP-43 invite mint was in flight aborted nothing, because the read was registered only after that mint returned, so the handler streamed a stored read the client had already closed. And the outbound frame queue was an unbounded channel whose only consumer was the -same select loop that performs the socket write, so a client that stopped reading let the -queue grow without limit while the liveness probe could not run. +queue grow without limit while the liveness probe could not run. And the handlers themselves +were spawned whole into that task's `JoinSet`, so two frames read in order could run in either +order (§"Inbound dispatch order"). Each fix had more than one plausible shape. This ADR records the shape chosen, the shape rejected, and why — the code shows the first and cannot show the second. @@ -162,12 +163,13 @@ runs before the next frame is read, and its continuations stay concurrent. the `JoinSet`. The prefixes are `subscribe` (everything through `begin_read`, so the subscription and its read exist before a following `CLOSE` is looked at), `unsubscribe` (the whole handler) and `auth` (the whole handler, which has no `await`). `EVENT` and `COUNT` keep -their whole body as one spawned future: no later frame's handler needs an entry they establish, -so nothing they do before their first `await` has to be ordered. The accepted consequence is -that two pipelined `EVENT`s may have their rate-limiter hits recorded in either order, so which -of the two is refused is not fixed by arrival order — parity, because the capture's limiter hit -also sits after `await this.isEventValid(event)` -(`src/handlers/event-message-handler.ts:66-86`). +their whole body as one spawned future: neither writes per-connection state a later frame looks +up, so nothing they do before their first `await` has to be ordered. What `EVENT` touches there +is not per-connection: the rate limiter, keyed on the event's author and window in a table the +whole relay shares. Two pipelined `EVENT`s from one author therefore record their hits in +whichever order their tasks are polled, so which of the two is refused is not fixed by arrival +order — parity, because the capture binds each `message` event to a handler it does not await +(`src/adapters/web-socket-adapter.ts:79`, `:229`), so its hits are unordered too. Two shapes were rejected. **Serializing whole handlers** — an ordered queue drained in the loop — would run a `REQ`'s invite mint to completion before a `CLOSE` already in the buffer @@ -181,14 +183,16 @@ or evicted, needs a rule for what an "older" `REQ` means under a re-`REQ`, and o What this does not decide: it is a guarantee about prefixes, not about handler execution. Continuations interleave as before, so a `CLOSE` during a gated invite mint still aborts the read and the invite `EVENT` is still emitted. The cost accepted is that the prefixes run on the -loop task. For `REQ` that is `normalise_subscription` and `can_subscribe`, bounded by -`transport::max_payload_size` and, where configured, `max_filters` / `max_filter_values`; for -`AUTH` it is the whole `nip42::authenticate`, measured on the 4-vCPU development box at 34.3 µs -per message (32.2 µs of it the signature verify, 1.28 µs the id hash), against 0.76 µs for -`REQ` and 0.053 µs for `CLOSE`. The capture does the same work on its socket's thread, but it +loop task: `REQ`'s `normalise_subscription` and `can_subscribe`, bounded by +`transport::max_payload_size` and, where configured, `max_filters` / `max_filter_values`, and +`AUTH`'s whole `nip42::authenticate`. Measured for this branch on the 4-vCPU development box: +`AUTH` 34.3 µs per message (a 32.2–32.5 µs signature verify plus a 1.28 µs id hash), `REQ` +0.72–0.76 µs (0.11 µs normalisation plus 0.61–0.65 µs of subscription-table work, measured +separately), `CLOSE` 0.053 µs. The capture does the same work on its socket's thread, but it pays in awaited chunks that let other sockets interleave, where a prefix does not yield at all; -the same verify costs the capture 1.57–1.85 ms/op of CPU on Node, so the loop's per-`AUTH` cost -is well below the capture's, and what the prefix gives up is interleaving, not throughput. +what the port buys is blast radius — one socket's read path rather than 1.57–1.85 ms/op of the +capture's only thread. The cost inside that radius is real: the loop is occupied for those +34 µs, and a client that pipelines `AUTH` is capped at roughly 29 000 per second. ## Not decided here — the port's `CLOSE` semantics From 72c61bb699e3f083d23365d072722fe67153631e Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sat, 24 Oct 2026 19:10:35 -0400 Subject: [PATCH 8/9] docs(relay): restore the queue clause and narrow two claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round 3's front-matter edit dropped a line from the ADR's Context paragraph, leaving "whose only consumer was the queue grow without limit" — the frame queue problem had vanished from the record. The clause is restored, so the paragraph again states the four relationships it counts. The module doc and ADR 0005 now say the one shared table the excluded handlers touch is relay-wide — the event limiter, keyed on the author and window — rather than denying that they touch per-connection state, which `COUNT`'s sink write and `EVENT`'s `remote_address` read contradict. The capture's parity mechanism is stated as it is: the socket fires each 'message' listener without awaiting it (:79, :198) and the handler is awaited inside that listener (:229), so two pipelined `EVENT`s run as interleaved chains there too. The old citation named the await as proof of the opposite. --- crates/nostream-relay/src/handlers/mod.rs | 14 +++++++------- .../decisions/0005-connection-concurrency.md | 13 +++++++------ 2 files changed, 14 insertions(+), 13 deletions(-) diff --git a/crates/nostream-relay/src/handlers/mod.rs b/crates/nostream-relay/src/handlers/mod.rs index 6d934ca1..68661e1e 100644 --- a/crates/nostream-relay/src/handlers/mod.rs +++ b/crates/nostream-relay/src/handlers/mod.rs @@ -12,13 +12,13 @@ //! registers its subscription and its read, `unsubscribe` clears them, and `auth` records the //! session a restricted `REQ` is judged against, so those three are prefixes. `event` and //! `count` keep their whole body as one future: neither establishes per-connection state that a -//! later frame looks up. What they do touch is not per-connection either — the event limiter's -//! key is the event's author and window in a table the whole relay shares -//! (`RateLimits::event_backoff`) — so two pipelined `EVENT`s from one author record their hits -//! in whichever order their tasks are polled, and which of the two is refused is not fixed by -//! arrival order. The capture races the same way: each `message` event starts an async handler -//! the adapter does not await (`web-socket-adapter.ts:79`, `:229`), so the two hits are -//! unordered there too. +//! later frame looks up, and the one shared table they touch is relay-wide — the event limiter, +//! keyed on the event's author and window (`RateLimits::event_backoff`). So two pipelined +//! `EVENT`s from one author record their hits in whichever order their tasks are polled, and +//! which of the two is refused is not fixed by arrival order. The capture races the same way: +//! the socket fires each `'message'` listener without awaiting it (`web-socket-adapter.ts:79`, +//! `:198`), and the handler is awaited inside that listener (`:229`), so two pipelined `EVENT`s +//! run as two interleaved chains there too. pub mod auth; pub mod count; diff --git a/docs/rewrite/decisions/0005-connection-concurrency.md b/docs/rewrite/decisions/0005-connection-concurrency.md index ccedb36b..7cdc1f92 100644 --- a/docs/rewrite/decisions/0005-connection-concurrency.md +++ b/docs/rewrite/decisions/0005-connection-concurrency.md @@ -13,6 +13,7 @@ so a re-subscribed client could go silent without an `EOSE`. A `CLOSE` that land the NIP-43 invite mint was in flight aborted nothing, because the read was registered only after that mint returned, so the handler streamed a stored read the client had already closed. And the outbound frame queue was an unbounded channel whose only consumer was the +same select loop that performs the socket write, so a client that stopped reading let the queue grow without limit while the liveness probe could not run. And the handlers themselves were spawned whole into that task's `JoinSet`, so two frames read in order could run in either order (§"Inbound dispatch order"). @@ -164,12 +165,12 @@ the `JoinSet`. The prefixes are `subscribe` (everything through `begin_read`, so subscription and its read exist before a following `CLOSE` is looked at), `unsubscribe` (the whole handler) and `auth` (the whole handler, which has no `await`). `EVENT` and `COUNT` keep their whole body as one spawned future: neither writes per-connection state a later frame looks -up, so nothing they do before their first `await` has to be ordered. What `EVENT` touches there -is not per-connection: the rate limiter, keyed on the event's author and window in a table the -whole relay shares. Two pipelined `EVENT`s from one author therefore record their hits in -whichever order their tasks are polled, so which of the two is refused is not fixed by arrival -order — parity, because the capture binds each `message` event to a handler it does not await -(`src/adapters/web-socket-adapter.ts:79`, `:229`), so its hits are unordered too. +up, so nothing they do before their first `await` has to be ordered. The one shared table they +touch is relay-wide: the event limiter, keyed on the event's author and window. Two pipelined +`EVENT`s from one author therefore record their hits in whichever order their tasks are polled, +so which of the two is refused is not fixed by arrival order — parity, because the socket fires +each `'message'` listener without awaiting it (`src/adapters/web-socket-adapter.ts:79`, `:198`) +and the handler is awaited inside that listener (`:229`), so its hits are unordered too. Two shapes were rejected. **Serializing whole handlers** — an ordered queue drained in the loop — would run a `REQ`'s invite mint to completion before a `CLOSE` already in the buffer From e05c741658e0848796c2e712d46b191209512fd6 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 00:55:25 +0000 Subject: [PATCH 9/9] docs(relay): record the AUTH ordering as a deliberate deviation, not parity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Greptile P2 on #978, and it is right: the capture's AuthMessageHandler awaits isEventIdValid and isEventSignatureValid before addAuthenticatedPubkey, so a REQ pipelined behind an AUTH can be judged against a session that has not been recorded and be refused. Rust verifies and records on the connection loop, so the session exists before the next frame is read — a stronger guarantee. ADR 0005 now says so, names the cost (the read path waits for two signature checks, once per AUTH frame) and the module doc points at it. Behaviour is unchanged; only the claim about it was wrong. --- crates/nostream-relay/src/handlers/mod.rs | 15 ++++++----- .../decisions/0005-connection-concurrency.md | 26 ++++++++++++++----- 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/crates/nostream-relay/src/handlers/mod.rs b/crates/nostream-relay/src/handlers/mod.rs index 68661e1e..e2e3a78a 100644 --- a/crates/nostream-relay/src/handlers/mod.rs +++ b/crates/nostream-relay/src/handlers/mod.rs @@ -10,12 +10,15 @@ //! handler as a future to spawn. The rule for which handler takes a prefix: it is the //! per-connection state a later frame's handler reads and expects to be there. `subscribe` //! registers its subscription and its read, `unsubscribe` clears them, and `auth` records the -//! session a restricted `REQ` is judged against, so those three are prefixes. `event` and -//! `count` keep their whole body as one future: neither establishes per-connection state that a -//! later frame looks up, and the one shared table they touch is relay-wide — the event limiter, -//! keyed on the event's author and window (`RateLimits::event_backoff`). So two pipelined -//! `EVENT`s from one author record their hits in whichever order their tasks are polled, and -//! which of the two is refused is not fixed by arrival order. The capture races the same way: +//! session a restricted `REQ` is judged against, so those three are prefixes. For `auth` this is +//! stronger than the capture, whose handler awaits two checks before recording the session, so a +//! `REQ` pipelined behind an `AUTH` may be refused there and is served here; ADR 0005 records the +//! deviation. `event` and `count` keep their whole body as one future: neither establishes +//! per-connection state that a later frame looks up, and the one shared table they touch is +//! relay-wide — the event limiter, keyed on the event's author and window +//! (`RateLimits::event_backoff`). So two pipelined `EVENT`s from one author record their hits in +//! whichever order their tasks are polled, and which of the two is refused is not fixed by +//! arrival order. The capture races the same way: //! the socket fires each `'message'` listener without awaiting it (`web-socket-adapter.ts:79`, //! `:198`), and the handler is awaited inside that listener (`:229`), so two pipelined `EVENT`s //! run as two interleaved chains there too. diff --git a/docs/rewrite/decisions/0005-connection-concurrency.md b/docs/rewrite/decisions/0005-connection-concurrency.md index 7cdc1f92..4a4b5240 100644 --- a/docs/rewrite/decisions/0005-connection-concurrency.md +++ b/docs/rewrite/decisions/0005-connection-concurrency.md @@ -163,12 +163,26 @@ runs before the next frame is read, and its continuations stay concurrent. `handlers::begin` runs each prefix on the connection loop and returns the continuation for the `JoinSet`. The prefixes are `subscribe` (everything through `begin_read`, so the subscription and its read exist before a following `CLOSE` is looked at), `unsubscribe` (the -whole handler) and `auth` (the whole handler, which has no `await`). `EVENT` and `COUNT` keep -their whole body as one spawned future: neither writes per-connection state a later frame looks -up, so nothing they do before their first `await` has to be ordered. The one shared table they -touch is relay-wide: the event limiter, keyed on the event's author and window. Two pipelined -`EVENT`s from one author therefore record their hits in whichever order their tasks are polled, -so which of the two is refused is not fixed by arrival order — parity, because the socket fires +whole handler) and `auth` (the whole handler, which has no `await`). + +`AUTH` is the one prefix that is deliberately **stronger** than the capture rather than parity +with it. The capture's `AuthMessageHandler.handleMessage` awaits `isEventIdValid` +(`src/handlers/auth-message-handler.ts:30`) and `isEventSignatureValid` (`:35`) before it +records the session in `addAuthenticatedPubkey` (`:65`), and the listener awaits the whole +handler, so a `REQ` pipelined behind an `AUTH` can be judged against a session that has not +been recorded yet and be refused as needing authentication. Rust verifies and records on the +loop (`handlers::auth::handle` is synchronous), so the session exists by the time the next +frame is read. The difference is one-sided — a pipelined `REQ` the capture may refuse, this +relay serves — and it is what a client reads NIP-42 as promising, so it is taken rather than +reproduced. The cost is real and named: the read path waits for two signature checks it could +have overlapped, once per `AUTH` frame rather than per connection. + +`EVENT` and `COUNT` keep their whole body as one spawned future: neither writes +per-connection state a later frame looks up, so nothing they do before their first `await` has +to be ordered. The one shared table they touch is relay-wide: the event limiter, keyed on the +event's author and window. Two pipelined `EVENT`s from one author therefore record their hits +in whichever order their tasks are polled, so which of the two is refused is not fixed by +arrival order — parity, because the socket fires each `'message'` listener without awaiting it (`src/adapters/web-socket-adapter.ts:79`, `:198`) and the handler is awaited inside that listener (`:229`), so its hits are unordered too.