From d827c9780c24ce8fb4236db4cf99dc13987619db Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sat, 24 Oct 2026 23:42:35 -0400 Subject: [PATCH 01/26] feat(relay): bound the total queued frames and the sockets one address may hold (#959) The outbound queue was bounded per socket and their sum was bounded by nothing: one address could hold 8 MiB for every socket it kept, and a closed socket's queued bytes were never given back. Every socket now counts its queued payload against one process-wide FrameBudget (256 MiB, RelayConfig::frame_memory_budget_bytes), refunded when the socket's last sink handle drops, and one address may hold at most max_connections_per_ip live sockets (64) as a gauge beside the attempt limiter. Both refuse with an immediate 1013 close, a fixed reason and their own counter. See docs/rewrite/decisions/0008-frame-memory-budget.md and #959. --- .changeset/relay-frame-memory-budget.md | 22 + crates/nostream-relay/src/connection_cap.rs | 168 ++++++++ crates/nostream-relay/src/lib.rs | 1 + crates/nostream-relay/src/relay.rs | 126 +++++- crates/nostream-relay/src/sink.rs | 439 ++++++++++++++++++-- crates/nostream-relay/src/transport.rs | 8 + crates/nostream-relay/tests/connection.rs | 6 +- crates/nostream-relay/tests/limits.rs | 2 +- crates/nostream-relay/tests/strategies.rs | 2 +- crates/nostream-relay/tests/transport.rs | 274 ++++++++++++ 10 files changed, 978 insertions(+), 70 deletions(-) create mode 100644 .changeset/relay-frame-memory-budget.md create mode 100644 crates/nostream-relay/src/connection_cap.rs diff --git a/.changeset/relay-frame-memory-budget.md b/.changeset/relay-frame-memory-budget.md new file mode 100644 index 00000000..2292cfa7 --- /dev/null +++ b/.changeset/relay-frame-memory-budget.md @@ -0,0 +1,22 @@ +--- +--- + +feat(relay): bound the relay's total queued frames and the sockets one address may hold + +The outbound queue was bounded per socket and the number of sockets was bounded by nothing: +one address that opened sockets and stopped reading could hold 8 MiB for every socket it kept, +measured as 384 MiB from the shipped attempt limits, and nothing gave a closed socket's queued +bytes back. Two bounds now sit beside that one, both recorded in +`docs/rewrite/decisions/0008-frame-memory-budget.md`: + +- `RelayConfig::frame_memory_budget_bytes` (256 MiB) is one process-wide budget every socket's + queued payload is counted against, admitted frame by frame at the sink and refunded when a + socket's last handle drops. +- `RelayConfig::max_connections_per_ip` (64) caps the live sockets one address may hold, as a + gauge beside the attempt limiter rather than folded into it. + +Both refuse the way the policy comment on #959 asks: an immediate `1013` close with a fixed +reason and its own counter, never a text frame and never a silent drop. See #959. + +The TypeScript tree is untouched: no `src/` or `test/` file changes, so the published package +has no user-visible change. diff --git a/crates/nostream-relay/src/connection_cap.rs b/crates/nostream-relay/src/connection_cap.rs new file mode 100644 index 00000000..48641393 --- /dev/null +++ b/crates/nostream-relay/src/connection_cap.rs @@ -0,0 +1,168 @@ +//! The per-IP live-connection cap: a gauge, beside the attempt limiter and not part of it. +//! +//! `limits.rs` answers "how fast may this address open connections" over a sliding window; +//! this answers "how many may it hold at once". The capture has neither question +//! (`src/@types/settings.ts:163-167`: `ConnectionLimits` is `rateLimits`, `ipWhitelist` and +//! `ipBlacklist`), so the cap is the port's own policy with no counterpart there (ADR 0008). +//! +//! Two things it deliberately is not. It is not a second `RateLimits` store: no window, no +//! clock, no whitelist, nothing to expire — a count that only `Drop` changes. And it is not +//! advisory: the connection task holds a [`ConnectionSlot`] for as long as the socket lives, +//! so an address at the cap is refused before the NIP-42 challenge is sent rather than after +//! the client has already been spoken to. + +use std::collections::HashMap; +use std::sync::atomic::{AtomicU64, Ordering}; +use std::sync::{Arc, Mutex}; + +/// The live sockets per source address, with the cap each is checked against (ADR 0008). +#[derive(Debug)] +pub struct ConnectionCap { + max_per_ip: usize, + live: Mutex>, + refusals: AtomicU64, +} + +impl ConnectionCap { + /// A cap of `max_per_ip` sockets per address, with nothing counted. + pub fn new(max_per_ip: usize) -> Self { + Self { + max_per_ip, + live: Mutex::new(HashMap::new()), + refusals: AtomicU64::new(0), + } + } + + /// The most sockets one address may hold at once. + pub fn max_per_ip(&self) -> usize { + self.max_per_ip + } + + /// How many sockets `ip` holds now. + pub fn live(&self, ip: &str) -> usize { + self.live + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .get(ip) + .copied() + .unwrap_or(0) + } + + /// How many addresses hold a socket: an address that releases its last one leaves no entry. + pub fn tracked_addresses(&self) -> usize { + self.live + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()) + .len() + } + + /// How many sockets were refused because their address was at the cap. + pub fn refusals(&self) -> u64 { + self.refusals.load(Ordering::SeqCst) + } + + /// Takes a slot for `ip`, or counts the refusal and returns `None` at the cap. + /// + /// The slot releases on `Drop`, so a caller that holds it for the whole connection + /// releases it on every exit — the normal one, a registration failure, or one added + /// later — without a decrement at each. + pub fn acquire(self: &Arc, ip: &str) -> Option { + let mut live = self + .live + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + let held = live.get(ip).copied().unwrap_or(0); + + if held >= self.max_per_ip { + // Nothing is inserted for a refused address: an entry that must not be there + // would read as a socket held. + drop(live); + self.refusals.fetch_add(1, Ordering::SeqCst); + + return None; + } + + live.insert(ip.to_owned(), held + 1); + + Some(ConnectionSlot { + cap: Arc::clone(self), + ip: ip.to_owned(), + }) + } +} + +/// One socket's slot in a [`ConnectionCap`], given back when it drops. +#[derive(Debug)] +pub struct ConnectionSlot { + cap: Arc, + ip: String, +} + +impl Drop for ConnectionSlot { + fn drop(&mut self) { + let mut live = self + .cap + .live + .lock() + .unwrap_or_else(|poisoned| poisoned.into_inner()); + + if let Some(held) = live.get_mut(&self.ip) { + *held = held.saturating_sub(1); + + if *held == 0 { + live.remove(&self.ip); + } + } + } +} + +#[cfg(test)] +mod tests { + #![allow(clippy::expect_used)] + + use super::*; + + #[test] + fn the_cap_counts_live_slots_and_releases_each_once() { + let cap = Arc::new(ConnectionCap::new(2)); + + let first = cap.acquire("127.0.0.1").expect("a slot below the cap"); + let second = cap + .acquire("127.0.0.1") + .expect("the last slot below the cap"); + + assert_eq!(cap.live("127.0.0.1"), 2); + assert_eq!(cap.tracked_addresses(), 1); + + assert!(cap.acquire("127.0.0.1").is_none()); + assert_eq!(cap.refusals(), 1); + assert_eq!(cap.live("127.0.0.1"), 2, "a refusal holds no slot"); + + // A second address has its own count, and an address at the cap does not affect it. + let elsewhere = cap + .acquire("10.0.0.1") + .expect("another address is unaffected"); + + assert_eq!(cap.live("10.0.0.1"), 1); + assert_eq!(cap.tracked_addresses(), 2); + + drop(first); + + assert_eq!(cap.live("127.0.0.1"), 1); + assert_eq!(cap.tracked_addresses(), 2); + + drop(second); + + assert_eq!(cap.live("127.0.0.1"), 0); + assert_eq!( + cap.tracked_addresses(), + 1, + "an address that releases its last slot leaves no entry" + ); + + drop(elsewhere); + + assert_eq!(cap.tracked_addresses(), 0); + assert_eq!(cap.refusals(), 1); + } +} diff --git a/crates/nostream-relay/src/lib.rs b/crates/nostream-relay/src/lib.rs index 54de5777..5d1752cb 100644 --- a/crates/nostream-relay/src/lib.rs +++ b/crates/nostream-relay/src/lib.rs @@ -44,6 +44,7 @@ pub mod broadcast; pub mod connection; +pub mod connection_cap; pub mod error; pub mod handlers; pub mod hidden_content; diff --git a/crates/nostream-relay/src/relay.rs b/crates/nostream-relay/src/relay.rs index 70fb1613..ce6722e3 100644 --- a/crates/nostream-relay/src/relay.rs +++ b/crates/nostream-relay/src/relay.rs @@ -28,20 +28,21 @@ use nostream_core::messages::RelayMessage; use nostream_core::{Event, messages}; use crate::connection::{AcceptedTransport, Connection, ConnectionContext}; +use crate::connection_cap::ConnectionCap; use crate::handlers::{self, HandlerContext, Stores}; use crate::hidden_content::{DEFAULT_REFRESH_INTERVAL, HiddenContentCache}; use crate::limits::RateLimits; use crate::relay_key::RelayKeySource; -use crate::sink::FrameSink; +use crate::sink::{FrameBudget, FrameSink, Refusal}; use crate::transport; use crate::wot::WotGraph; /// The knobs a [`Relay`] is built with that are not settings: a constant the capture fixes in /// code (`liveness`, the NIP-56 refresh) or a policy the port adds with no counterpart there -/// (`frame_queue_bytes`, `write_timeout`; ADR 0005). Tests set the cadences they need -/// (`tests/common/mod.rs::fast_config`); the two policies run at their shipped defaults in -/// every test so far. A value the capture's settings document carries belongs in [`Settings`], -/// never here. +/// (`frame_queue_bytes`, `write_timeout`, the frame-memory budget and the per-IP connection +/// cap; ADR 0005, ADR 0008). Tests set the cadences they need (`tests/common/mod.rs::fast_config`); +/// the policies run at their shipped defaults except where a test sets one deliberately. A +/// value the capture's settings document carries belongs in [`Settings`], never here. #[derive(Debug, Clone)] pub struct RelayConfig { /// `WSS_CLIENT_HEALTH_PROBE_INTERVAL` (`web-socket-server-adapter.ts:18`). @@ -59,6 +60,13 @@ pub struct RelayConfig { /// cannot starve (`web-socket-server-adapter.ts:18,46`), while this loop performs the /// write itself. pub write_timeout: Duration, + /// The queued outbound payload bytes every socket of this relay shares. The capture has + /// no counterpart: it bounds neither one socket's queue nor their sum (ADR 0008). + pub frame_memory_budget_bytes: usize, + /// The live sockets one source address may hold at once. The capture has no counterpart + /// either: its connection limits are rates, not counts (`src/@types/settings.ts:163-167`; + /// ADR 0008). + pub max_connections_per_ip: usize, } impl Default for RelayConfig { @@ -76,6 +84,14 @@ impl Default for RelayConfig { // Shorter than the 120 s liveness interval, so a client that stops reading is // closed by the write path before a missed pong could close it. write_timeout: Duration::from_secs(30), + // 256 MiB: 32 sockets at the `frame_queue_bytes` ceiling can be fully buffered + // before the budget binds, which is below the 48 a single address gets through + // the shipped attempt limits alone (ADR 0008). + frame_memory_budget_bytes: 256 * 1024 * 1024, + // 64: four times the concurrent total one well-behaved client needs. The cap + // bounds sockets, the budget bounds bytes, so its own worst case (64 × 8 MiB) sits + // above the budget on purpose (ADR 0008). + max_connections_per_ip: 64, } } } @@ -92,6 +108,8 @@ pub struct Relay { hidden: Arc, hub: broadcast::Sender>, pow: Arc>, + frame_budget: Arc, + connection_cap: Arc, config: RelayConfig, } @@ -112,8 +130,8 @@ impl Relay { /// Everything the relay runs with is an argument, so a relay that mints from /// `SystemRandom` or resolves `WS_DRAIN_TIMEOUT_MS` from the environment is one whose /// composition root asked for it: `RelayConfig::default()` is the port's own defaults — the - /// capture's constants plus the two policies ADR 0005 adds — and the environment read is - /// [`transport::drain_timeout`] at the call site. + /// capture's constants plus the policies ADR 0005 and ADR 0008 add — and the environment + /// read is [`transport::drain_timeout`] at the call site. pub fn new( settings: Settings, stores: Arc, @@ -125,6 +143,8 @@ impl Relay { ) -> Arc { let settings = Arc::new(settings); let (hub, _receiver) = broadcast::channel(1024); + let frame_budget = Arc::new(FrameBudget::new(config.frame_memory_budget_bytes)); + let connection_cap = Arc::new(ConnectionCap::new(config.max_connections_per_ip)); Arc::new(Self { limits: Arc::new(RateLimits::new(&settings, Arc::clone(&clock))), @@ -137,6 +157,8 @@ impl Relay { hidden: HiddenContentCache::new(), hub, pow: Arc::new(Mutex::new(AdaptivePowState::default())), + frame_budget, + connection_cap, config, }) } @@ -147,6 +169,18 @@ impl Relay { &self.hidden } + /// The process-wide bound on queued outbound payload bytes. The counter it carries is what + /// a metrics exporter would read once the admin `/metrics` route is implemented; the warn + /// line on a refusal is what an operator has today (ADR 0008). + pub fn frame_budget(&self) -> &Arc { + &self.frame_budget + } + + /// The live-connection cap per source address, and its refusal counter. + pub fn connection_cap(&self) -> &Arc { + &self.connection_cap + } + /// The context one socket's handlers run in. fn handler_context(&self, connection: Arc, sink: FrameSink) -> HandlerContext { HandlerContext { @@ -242,7 +276,7 @@ impl Relay { ) { let max_payload = transport::max_payload_size(&self.settings); - let transport = match transport::upgrade(stream, max_payload).await { + let mut transport = match transport::upgrade(stream, max_payload).await { Ok(transport) => transport, Err(error) => { tracing::debug!(%peer, error = %error, "websocket handshake failed"); @@ -255,6 +289,30 @@ impl Relay { return; } + // After the attempt limiter, so a rate-limited socket is still dropped silently as it + // always was and takes no slot; before `run_connection`, because registering the + // `Connection` sends the NIP-42 challenge as a text frame and a refused client must + // receive no text frame (ADR 0008). The slot is held until this function returns, + // which covers the registration failure inside `run_connection` as well as its + // normal exit. + let ip = peer.ip().to_string(); + let Some(_slot) = self.connection_cap.acquire(&ip) else { + tracing::debug!( + %peer, + max_connections_per_ip = self.connection_cap.max_per_ip(), + live = self.connection_cap.live(&ip), + "client terminated: connection cap" + ); + self.close_frame( + &mut transport, + CloseCode::Again, + transport::CONNECTION_CAP_REASON, + ) + .await; + + return; + }; + self.run_connection(transport, peer, shutdown).await; } @@ -283,7 +341,10 @@ impl Relay { peer: std::net::SocketAddr, shutdown: ShutdownState, ) { - let (sink, mut frames) = FrameSink::channel(self.config.frame_queue_bytes); + let (sink, mut frames) = FrameSink::channel( + self.config.frame_queue_bytes, + Some(Arc::clone(&self.frame_budget)), + ); let connection_context = ConnectionContext { settings: Arc::clone(&self.settings), @@ -327,8 +388,8 @@ impl Relay { // Read at the top of every iteration, so a flag the producers set while the loop // was parked on a write is acted on as soon as that write returns. No `await` // sits between a write completing and this check. - if sink.overflowed() { - self.close_slow_consumer(&mut transport, peer).await; + if let Some(refusal) = sink.refusal() { + self.close_refused(&mut transport, peer, refusal).await; break; } @@ -428,24 +489,47 @@ impl Relay { connection.close(); } - /// Closes a client whose outbound frame queue overflowed: 1013 with the reason, then the - /// loop's cleanup (`handlers.abort_all()` and `connection.close()`). + /// Closes a client whose frame was refused, with the close its bound implies: 1013 and the + /// reason for that bound, then the loop's cleanup (`handlers.abort_all()` and + /// `connection.close()`). /// - /// The policy is a disconnect, not a silent break: a client cut mid-`REQ` with no frame - /// cannot tell a slow link from a relay that forgot it (ADR 0005). - async fn close_slow_consumer( + /// The policy is a disconnect, not a silent break, for both bounds: a client cut mid-`REQ` + /// with no frame cannot tell a slow link from a relay that forgot it (ADR 0005), and a + /// refusal that looked like a network fault is exactly what the two reason strings and + /// their counters exist to keep out of the logs (ADR 0008). + async fn close_refused( &self, transport: &mut AcceptedTransport, peer: std::net::SocketAddr, + refusal: Refusal, ) { - tracing::debug!(%peer, "closing a client that cannot keep up with its frame queue"); + match refusal { + Refusal::QueueFull => { + tracing::debug!(%peer, "closing a client that cannot keep up with its frame queue"); + + self.close_frame(transport, CloseCode::Again, transport::SLOW_CONSUMER_REASON) + .await; + } + Refusal::BudgetExhausted => { + tracing::debug!(%peer, "closing a client that exhausted the frame memory budget"); + + self.close_frame(transport, CloseCode::Again, transport::FRAME_BUDGET_REASON) + .await; + } + } + } + /// Writes one close frame straight to the transport, bounded by the write timeout so a + /// client that stopped reading cannot hold the task open. + async fn close_frame( + &self, + transport: &mut AcceptedTransport, + code: CloseCode, + reason: &'static str, + ) { let _ = tokio::time::timeout( self.config.write_timeout, - transport.close(Some(transport::close_frame( - CloseCode::Again, - transport::SLOW_CONSUMER_REASON, - ))), + transport.close(Some(transport::close_frame(code, reason))), ) .await; } diff --git a/crates/nostream-relay/src/sink.rs b/crates/nostream-relay/src/sink.rs index 9b8ed540..dfb236d1 100644 --- a/crates/nostream-relay/src/sink.rs +++ b/crates/nostream-relay/src/sink.rs @@ -24,39 +24,193 @@ //! depth — one line per sender, not one per connection — and every later send is dropped //! without queueing. The connection task reads the flag and closes the client with 1013, so //! the policy is a disconnect rather than a silent break (ADR 0005). +//! +//! **The sum is bounded too**, which the per-socket bound cannot do: one address that opens +//! sockets and stops reading held a multiple of the per-socket bound, so every socket's +//! queue is also counted against one process-wide [`FrameBudget`] (ADR 0008). An admission +//! needs room in both bounds; the budget has no empty-queue exception, so a frame that does +//! not fit the budget is refused however empty its own queue. The socket's share of the +//! budget is refunded when its last sink handle drops — see [`Account`] — because the next +//! socket's admission depends on it. use std::sync::Arc; -use std::sync::atomic::{AtomicBool, AtomicUsize, Ordering}; +use std::sync::atomic::{AtomicU8, AtomicU64, AtomicUsize, Ordering}; use tokio::sync::mpsc; use nostream_core::messages::RelayMessage; +/// Why a frame was refused: the bound that could not hold it. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Refusal { + /// The frame did not fit this socket's `frame_queue_bytes`. + QueueFull, + /// The frame did not fit the relay's `frame_memory_budget_bytes`. + BudgetExhausted, +} + +/// No refusal recorded yet; the two values below are this file's only refusal causes. +const NO_REFUSAL: u8 = 0; +const QUEUE_FULL: u8 = 1; +const BUDGET_EXHAUSTED: u8 = 2; + +/// The process-wide bound on queued outbound payload bytes, shared by every socket of one +/// relay (ADR 0008). +/// +/// The unit is `text.len()`, the same bytes `frame_queue_bytes` counts: not `String` +/// capacity, and not resident memory. +#[derive(Debug)] +pub struct FrameBudget { + limit_bytes: usize, + accounted_bytes: AtomicUsize, + refusals: AtomicU64, +} + +impl FrameBudget { + /// A budget of `limit_bytes`, with nothing accounted. + pub fn new(limit_bytes: usize) -> Self { + Self { + limit_bytes, + accounted_bytes: AtomicUsize::new(0), + refusals: AtomicU64::new(0), + } + } + + /// The byte bound. + pub fn limit(&self) -> usize { + self.limit_bytes + } + + /// The bytes admitted across every socket and not yet written or refunded. + pub fn accounted(&self) -> usize { + self.accounted_bytes.load(Ordering::SeqCst) + } + + /// How many sockets were refused because their frame did not fit. + pub fn refusals(&self) -> u64 { + self.refusals.load(Ordering::SeqCst) + } + + /// Admits `bytes` against the budget, or reports that they do not fit. + /// + /// A compare-and-exchange loop for the same reason [`FrameSink::send`] uses one: every + /// handler of every socket shares this counter, and a load-then-store would lose an + /// increment and admit more than the budget. + fn admit(&self, bytes: usize) -> bool { + let mut accounted = self.accounted_bytes.load(Ordering::SeqCst); + + loop { + // Strict: the whole frame has to fit what is left, with no exception for an + // empty budget — unlike the per-socket bound, whose exception keeps one large + // event from closing every client. + if bytes > self.limit_bytes.saturating_sub(accounted) { + return false; + } + + match self.accounted_bytes.compare_exchange_weak( + accounted, + accounted + bytes, + Ordering::SeqCst, + Ordering::SeqCst, + ) { + Ok(_) => return true, + Err(actual) => accounted = actual, + } + } + } + + /// Returns bytes to the budget: a written frame's, or a closing socket's remainder. + fn release(&self, bytes: usize) { + let _ = self + .accounted_bytes + .try_update(Ordering::SeqCst, Ordering::SeqCst, |accounted| { + Some(accounted.saturating_sub(bytes)) + }); + } + + /// Counts one socket refused for the budget. + fn refuse(&self) { + self.refusals.fetch_add(1, Ordering::SeqCst); + } +} + +/// One socket's accounting: the bytes it has admitted, its refusal cause, and the budget +/// those bytes are counted against when the relay has one. +/// +/// Every clone of the socket's sink shares this value, so the `Drop` that runs here is the +/// socket's accounting ending: a send holds a sink and therefore cannot be in flight or +/// begin once the drop starts. That is what makes the refund exactly this socket's +/// outstanding bytes, once, on every exit from the connection loop — the drain arm's +/// [`FrameSink::release`] keeps handling the bytes it wrote on the way out. +#[derive(Debug)] +struct Account { + queued_bytes: AtomicUsize, + refusal: AtomicU8, + budget: Option>, +} + +impl Account { + /// The refusal recorded for this socket, if a sender has been refused. + fn refusal(&self) -> Option { + match self.refusal.load(Ordering::SeqCst) { + QUEUE_FULL => Some(Refusal::QueueFull), + BUDGET_EXHAUSTED => Some(Refusal::BudgetExhausted), + _ => None, + } + } + + /// Records the first refusal for this socket, reporting whether this sender set it. + fn refuse(&self, refusal: Refusal) -> bool { + let cause = match refusal { + Refusal::QueueFull => QUEUE_FULL, + Refusal::BudgetExhausted => BUDGET_EXHAUSTED, + }; + + self.refusal + .compare_exchange(NO_REFUSAL, cause, Ordering::SeqCst, Ordering::SeqCst) + .is_ok() + } +} + +impl Drop for Account { + fn drop(&mut self) { + if let Some(budget) = &self.budget { + budget.release(self.queued_bytes.load(Ordering::SeqCst)); + } + } +} + /// A handle to one connection's outbound frame queue. #[derive(Debug, Clone)] pub struct FrameSink { sender: Option>, /// The byte bound the queue was built with. bound_bytes: usize, - /// The bytes admitted and not yet released, shared with the draining clone. - queued_bytes: Arc, - /// Set once a frame was dropped for the bound: from then on every send is dropped. - overflowed: Arc, + /// The socket's accounting, shared with every clone and with the draining task. + account: Arc, } impl FrameSink { - /// Creates the queue and its sink, admitting at most `bound_bytes` of queued frames. The - /// receiver is drained by the connection task, which [`FrameSink::release`]s each frame's - /// bytes once its write completes. - pub fn channel(bound_bytes: usize) -> (Self, mpsc::UnboundedReceiver) { + /// Creates the queue and its sink, admitting at most `bound_bytes` of queued frames. + /// + /// `budget` is the relay's process-wide bound, shared by every socket it serves: a frame + /// has to fit both. The receiver is drained by the connection task, which + /// [`FrameSink::release`]s each frame's bytes once its write completes. + pub fn channel( + bound_bytes: usize, + budget: Option>, + ) -> (Self, mpsc::UnboundedReceiver) { let (sender, receiver) = mpsc::unbounded_channel(); ( Self { sender: Some(sender), bound_bytes, - queued_bytes: Arc::new(AtomicUsize::new(0)), - overflowed: Arc::new(AtomicBool::new(false)), + account: Arc::new(Account { + queued_bytes: AtomicUsize::new(0), + refusal: AtomicU8::new(NO_REFUSAL), + budget, + }), }, receiver, ) @@ -68,37 +222,40 @@ impl FrameSink { Self { sender: None, bound_bytes: 0, - queued_bytes: Arc::new(AtomicUsize::new(0)), - overflowed: Arc::new(AtomicBool::new(false)), + account: Arc::new(Account { + queued_bytes: AtomicUsize::new(0), + refusal: AtomicU8::new(NO_REFUSAL), + budget: None, + }), } } /// Sends one frame. A closed socket drops the frame and is not an error. + /// + /// A queue-full refusal latches the socket shut, as the overflow always has: every later + /// send is dropped. A budget refusal does not — it is one frame that did not fit at that + /// instant, and the connection task closes the socket at its next iteration whatever the + /// sender does — so the accounting stays exact and a frame that fits later is admitted. pub fn send(&self, message: &RelayMessage) { let Some(sender) = &self.sender else { return; }; - if self.overflowed.load(Ordering::SeqCst) { - return; - } - let text = message.to_text(); let length = text.len(); - let mut queued = self.queued_bytes.load(Ordering::SeqCst); + let mut queued = self.account.queued_bytes.load(Ordering::SeqCst); // Admission is a compare-and-exchange, not a load-then-store: an in-flight read runs // in its own task and several readers share this sink, so two sends that interleaved // a load and a store would lose one increment and admit more than the bound. loop { - if self.overflowed.load(Ordering::SeqCst) { + if self.account.refusal() == Some(Refusal::QueueFull) { return; } - // An empty queue admits its frame whatever the size, so an event larger than the // whole bound still reaches the client instead of closing it. if queued > 0 && queued + length > self.bound_bytes { - self.overflowed.store(true, Ordering::SeqCst); + self.account.refuse(Refusal::QueueFull); tracing::warn!( bound_bytes = self.bound_bytes, queued_bytes = queued, @@ -107,7 +264,7 @@ impl FrameSink { return; } - match self.queued_bytes.compare_exchange_weak( + match self.account.queued_bytes.compare_exchange_weak( queued, queued + length, Ordering::SeqCst, @@ -119,29 +276,61 @@ impl FrameSink { } } + // The budget comes second so a socket that is already over its own bound refuses for + // that reason, as it did before this counter existed. It has no empty-queue + // exception: a frame that does not fit the budget is refused even on an idle socket. + if let Some(budget) = &self.account.budget + && !budget.admit(length) + { + // The frame is never queued, so the bytes counted a moment ago will never be + // released by the drain arm: take them back here, exactly what was counted. + let _ = self.account.queued_bytes.try_update( + Ordering::SeqCst, + Ordering::SeqCst, + |queued| Some(queued.saturating_sub(length)), + ); + + if self.account.refuse(Refusal::BudgetExhausted) { + budget.refuse(); + tracing::warn!( + budget_bytes = budget.limit(), + accounted_bytes = budget.accounted(), + "frame memory budget exhausted: dropping frames and closing the client" + ); + } + + return; + } + // The receiver is gone exactly when the connection task ended: the socket is not // open, and the capture's guard returns without sending. let _ = sender.send(text); } - /// Releases a frame's bytes once it has been written to the socket. + /// Releases a frame's bytes once it has been written to the socket, for this socket and + /// for the budget it counts against. pub(crate) fn release(&self, bytes: usize) { - let _ = self - .queued_bytes - .try_update(Ordering::SeqCst, Ordering::SeqCst, |queued| { - Some(queued.saturating_sub(bytes)) - }); + let _ = + self.account + .queued_bytes + .try_update(Ordering::SeqCst, Ordering::SeqCst, |queued| { + Some(queued.saturating_sub(bytes)) + }); + + if let Some(budget) = &self.account.budget { + budget.release(bytes); + } } - /// Whether a frame has been dropped for the bound. The connection task closes the client - /// once this is set. - pub fn overflowed(&self) -> bool { - self.overflowed.load(Ordering::SeqCst) + /// Why a frame was refused, if the connection is over its bound. The connection task + /// closes the client once this is set. + pub fn refusal(&self) -> Option { + self.account.refusal() } /// The bytes admitted into the queue and not yet written. pub fn queued_bytes(&self) -> usize { - self.queued_bytes.load(Ordering::SeqCst) + self.account.queued_bytes.load(Ordering::SeqCst) } } @@ -162,12 +351,12 @@ mod tests { let len = frame.to_text().len(); // Two frames fit exactly, and a third is dropped rather than queued. - let (sink, mut frames) = FrameSink::channel(2 * len); + let (sink, mut frames) = FrameSink::channel(2 * len, None); sink.send(&frame); sink.send(&frame); - assert!(!sink.overflowed()); + assert_eq!(sink.refusal(), None); assert_eq!(sink.queued_bytes(), 2 * len); assert!(frames.try_recv().is_ok()); assert!(frames.try_recv().is_ok()); @@ -175,7 +364,7 @@ mod tests { sink.send(&frame); - assert!(sink.overflowed()); + assert_eq!(sink.refusal(), Some(Refusal::QueueFull)); assert_eq!( sink.queued_bytes(), 2 * len, @@ -189,22 +378,22 @@ mod tests { // Once the flag is set every later send is dropped as well. sink.send(&frame); - assert!(sink.overflowed()); + assert_eq!(sink.refusal(), Some(Refusal::QueueFull)); assert_eq!(sink.queued_bytes(), 2 * len); // A frame larger than the bound still goes out while the queue is empty, so one // large event does not close every client. - let (sink, mut frames) = FrameSink::channel(len - 1); + let (sink, mut frames) = FrameSink::channel(len - 1, None); sink.send(&frame); - assert!(!sink.overflowed()); + assert_eq!(sink.refusal(), None); assert_eq!(sink.queued_bytes(), len); assert!(frames.try_recv().is_ok()); sink.send(&frame); - assert!(sink.overflowed()); + assert_eq!(sink.refusal(), Some(Refusal::QueueFull)); assert_eq!(sink.queued_bytes(), len); assert!(frames.try_recv().is_err()); @@ -214,7 +403,7 @@ mod tests { // needs, so this is a regression guard rather than a proof. const SENDERS: usize = 8; - let (sink, mut frames) = FrameSink::channel(SENDERS * len); + let (sink, mut frames) = FrameSink::channel(SENDERS * len, None); let start = std::sync::Barrier::new(SENDERS); std::thread::scope(|scope| { @@ -230,7 +419,7 @@ mod tests { } }); - assert!(!sink.overflowed(), "every frame fits the bound"); + assert_eq!(sink.refusal(), None, "every frame fits the bound"); assert_eq!(sink.queued_bytes(), SENDERS * len); for _ in 0..SENDERS { @@ -241,14 +430,14 @@ mod tests { // `release` is the drain side's half of the balance: bytes come back as a frame is // written, so a drained queue admits again and the count never drifts upwards. - let (sink, mut frames) = FrameSink::channel(2 * len); + let (sink, mut frames) = FrameSink::channel(2 * len, None); sink.send(&frame); sink.send(&frame); sink.release(2 * len); assert_eq!(sink.queued_bytes(), 0); - assert!(!sink.overflowed()); + assert_eq!(sink.refusal(), None); sink.send(&frame); sink.send(&frame); @@ -260,4 +449,166 @@ mod tests { assert!(frames.try_recv().is_ok()); assert!(frames.try_recv().is_err()); } + + #[test] + fn the_budget_binds_across_sockets_and_is_shared() { + let frame = notice("shared"); + let len = frame.to_text().len(); + let budget = Arc::new(FrameBudget::new(2 * len)); + + // Three sockets of a relay whose budget only two frames of this size fit in. + let (first, _first_frames) = FrameSink::channel(4 * len, Some(Arc::clone(&budget))); + let (second, _second_frames) = FrameSink::channel(4 * len, Some(Arc::clone(&budget))); + let (third, mut third_frames) = FrameSink::channel(4 * len, Some(Arc::clone(&budget))); + + first.send(&frame); + second.send(&frame); + + assert_eq!(budget.accounted(), budget.limit()); + assert!(budget.accounted() <= budget.limit()); + + third.send(&frame); + + assert_eq!(third.refusal(), Some(Refusal::BudgetExhausted)); + assert_eq!( + third.queued_bytes(), + 0, + "a refused frame is not counted against the socket either" + ); + assert_eq!( + budget.accounted(), + 2 * len, + "the refused frame is not accounted" + ); + assert_eq!(budget.refusals(), 1); + assert!(third_frames.try_recv().is_err()); + assert!(budget.accounted() <= budget.limit()); + + // The drain arm returning one written frame's bytes is all the room a waiting socket + // needs: the bound is bytes, not a queue slot. + first.release(len); + + third.send(&frame); + + assert_eq!(budget.accounted(), 2 * len); + assert_eq!( + budget.refusals(), + 1, + "the second frame needed no second refusal" + ); + assert_eq!(third.queued_bytes(), len); + assert!(third_frames.try_recv().is_ok()); + assert!(budget.accounted() <= budget.limit()); + } + + #[test] + fn the_budget_release_refunds_only_the_closing_sockets_bytes() { + let frame = notice("refunded"); + let len = frame.to_text().len(); + let budget = Arc::new(FrameBudget::new(4 * len)); + + let (first, _first_frames) = FrameSink::channel(len, Some(Arc::clone(&budget))); + let (second, _second_frames) = FrameSink::channel(len, Some(Arc::clone(&budget))); + + first.send(&frame); + second.send(&frame); + + assert_eq!(budget.accounted(), 2 * len); + assert_eq!(first.queued_bytes(), len); + assert_eq!(second.queued_bytes(), len); + + // Neither frame is released: this is the state of every exit from the connection loop + // except the drain arm, where the frames a socket admitted were never written. + drop(first); + + assert_eq!( + budget.accounted(), + len, + "the refund is the closing socket's bytes, not the whole counter" + ); + + drop(second); + + assert_eq!(budget.accounted(), 0); + + // Both handles are gone and nothing underflowed: the budget still admits what it + // should, which a race that subtracted the same bytes twice could not. + let (third, mut third_frames) = FrameSink::channel(len, Some(Arc::clone(&budget))); + + third.send(&frame); + + assert_eq!(third.refusal(), None); + assert_eq!(budget.accounted(), len); + assert!(third_frames.try_recv().is_ok()); + } + + #[test] + fn the_budget_is_never_over_committed_by_racing_senders() { + // The shape of the racing-sender guard above: a barrier makes the sends overlap but + // cannot force the interleaving a lost update needs, so this is a regression guard + // rather than a proof. + let frame = notice("raced"); + let len = frame.to_text().len(); + const SENDERS: usize = 8; + let budget = Arc::new(FrameBudget::new(SENDERS * len)); + let max_seen = AtomicUsize::new(0); + let start = std::sync::Barrier::new(SENDERS + 1); + + let mut sinks = Vec::new(); + + for _ in 0..SENDERS { + sinks.push(FrameSink::channel(len, Some(Arc::clone(&budget))).0); + } + + std::thread::scope(|scope| { + for sink in &sinks { + let frame = &frame; + let start = &start; + + scope.spawn(move || { + start.wait(); + sink.send(frame); + }); + } + + // A sampler reading the counter while the sends run: it stops when the budget is + // full, so it cannot outlive the sends it observes. + { + let budget = Arc::clone(&budget); + let max_seen = &max_seen; + let start = &start; + + scope.spawn(move || { + start.wait(); + + loop { + let accounted = budget.accounted(); + max_seen.fetch_max(accounted, Ordering::SeqCst); + + if accounted == SENDERS * len { + break; + } + + std::hint::spin_loop(); + } + }); + } + }); + + assert!(max_seen.load(Ordering::SeqCst) <= budget.limit()); + assert_eq!( + budget.accounted(), + SENDERS * len, + "every frame was admitted once" + ); + assert_eq!(budget.refusals(), 0); + + for sink in &sinks { + assert_eq!(sink.queued_bytes(), len); + } + + drop(sinks); + + assert_eq!(budget.accounted(), 0); + } } diff --git a/crates/nostream-relay/src/transport.rs b/crates/nostream-relay/src/transport.rs index e88706f8..b0080868 100644 --- a/crates/nostream-relay/src/transport.rs +++ b/crates/nostream-relay/src/transport.rs @@ -38,6 +38,14 @@ pub const DRAIN_REASON: &str = "relay shutting down"; /// own policy, with no counterpart in the capture (ADR 0005). pub const SLOW_CONSUMER_REASON: &str = "slow consumer: outbound frame queue full"; +/// The close reason a client whose outbound frame did not fit the relay's frame-memory budget +/// is closed with: the port's own policy, with no counterpart in the capture (ADR 0008). +pub const FRAME_BUDGET_REASON: &str = "frame memory budget exhausted"; + +/// The close reason a client above `max_connections_per_ip` is closed with: the port's own +/// policy, with no counterpart in the capture (ADR 0008). +pub const CONNECTION_CAP_REASON: &str = "connection cap: too many connections from this address"; + /// `network.maxPayloadSize` (`network.max_payload_size` in the shipped YAML), else 131072. pub fn max_payload_size(settings: &Settings) -> usize { settings diff --git a/crates/nostream-relay/tests/connection.rs b/crates/nostream-relay/tests/connection.rs index 5b6ee276..1654e8f6 100644 --- a/crates/nostream-relay/tests/connection.rs +++ b/crates/nostream-relay/tests/connection.rs @@ -411,7 +411,7 @@ async fn closing_the_socket_aborts_an_in_flight_read() { .await; // The queue's bound plays no part here: the frames are a few hundred bytes. - let (sink, mut frames) = FrameSink::channel(1024 * 1024); + let (sink, mut frames) = FrameSink::channel(1024 * 1024, None); let connection = Arc::new(Connection::new( "127.0.0.1".to_owned(), nostream_core::nip42::Nip42Session::with_challenge("challenge", 0), @@ -493,7 +493,7 @@ async fn restricted_read_with(authenticate_mid_read: bool) -> Vec { )) .await; - let (sink, mut frames) = FrameSink::channel(1024 * 1024); + let (sink, mut frames) = FrameSink::channel(1024 * 1024, None); // The session outlives the read: this connection is built directly rather than by // `Connection::accept`, so the TTL is the one named here and the settings' expiry (a // zero TTL) plays no part. @@ -621,7 +621,7 @@ async fn a_restricted_read_is_judged_against_the_authenticated_set_at_each_event 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 (sink, mut frames) = FrameSink::channel(1024 * 1024, None); let context = Arc::new(fixture.context( Arc::new(TestClock::new(NOW_MS)), relay_keys(), diff --git a/crates/nostream-relay/tests/limits.rs b/crates/nostream-relay/tests/limits.rs index 80263b24..f0431175 100644 --- a/crates/nostream-relay/tests/limits.rs +++ b/crates/nostream-relay/tests/limits.rs @@ -969,7 +969,7 @@ async fn a_failing_event_limiter_limits_the_event_without_a_backoff() { ..Default::default() })); - let (sink, mut frames) = nostream_relay::sink::FrameSink::channel(1024 * 1024); + let (sink, mut frames) = nostream_relay::sink::FrameSink::channel(1024 * 1024, None); let clock = Arc::new(TestClock::new(NOW_MS)); let connection = Arc::new(nostream_relay::connection::Connection::new( "127.0.0.1".to_owned(), diff --git a/crates/nostream-relay/tests/strategies.rs b/crates/nostream-relay/tests/strategies.rs index 83cc663f..0fbb605e 100644 --- a/crates/nostream-relay/tests/strategies.rs +++ b/crates/nostream-relay/tests/strategies.rs @@ -100,7 +100,7 @@ fn keys() -> RelayKeySource { /// A context whose sink a test can read, with its frames. fn socket(fixture: &Fixture) -> (HandlerContext, mpsc::UnboundedReceiver) { - let (sink, frames) = FrameSink::channel(64 * 1024); + let (sink, frames) = FrameSink::channel(64 * 1024, None); let connection = Arc::new(Connection::new( "127.0.0.1".to_owned(), Nip42Session::with_challenge("challenge", 0), diff --git a/crates/nostream-relay/tests/transport.rs b/crates/nostream-relay/tests/transport.rs index f3da1d54..76327fbb 100644 --- a/crates/nostream-relay/tests/transport.rs +++ b/crates/nostream-relay/tests/transport.rs @@ -21,6 +21,7 @@ use common::{ CLIENT_SECRET, Client, NOW_SECS, Server, fast_config, liveness_config, relay_keys, signed_event, }; use futures_util::SinkExt; +use nostream_relay::RelayConfig; use nostream_relay::testing::{Fixture, settings}; use nostream_relay::transport::{self, DEFAULT_MAX_PAYLOAD}; use tokio::io::{AsyncReadExt, AsyncWriteExt}; @@ -244,6 +245,16 @@ fn close_of(frame: &Frame) -> (u16, String) { ) } +/// Asserts that nothing was written to the client before its close frame: a frame the client +/// will never read cannot be the way to say why it is being closed. +fn assert_no_text_frame(frames: &[Frame]) { + assert!( + frames.iter().all(|frame| frame.opcode == 0x8), + "a refusal sends no text frame: {:?}", + frames.iter().map(Frame::text).collect::>() + ); +} + fn settings_with_payload(size: u64) -> nostream_config::settings::Settings { let mut settings = settings(); settings.network.max_payload_size = Some(size); @@ -251,6 +262,43 @@ fn settings_with_payload(size: u64) -> nostream_config::settings::Settings { settings } +/// A config with only the two bounds changed, on the fast intervals the suite runs with. +fn bounded_config(frame_memory_budget_bytes: usize, max_connections_per_ip: usize) -> RelayConfig { + RelayConfig { + frame_memory_budget_bytes, + max_connections_per_ip, + ..fast_config() + } +} + +/// Waits up to two seconds for `condition`: a closing socket's slot and bytes are given back +/// by its own task, which the client's close does not wait for. +async fn eventually(condition: impl Fn() -> bool) -> bool { + for _ in 0..200 { + if condition() { + return true; + } + + tokio::time::sleep(Duration::from_millis(10)).await; + } + + condition() +} + +/// The next text frame's payload, ignoring any other frame before it. +async fn next_text(client: &mut RawClient) -> String { + loop { + let frame = tokio::time::timeout(Duration::from_secs(5), client.next_frame()) + .await + .expect("a frame arrives") + .expect("the stream is open"); + + if frame.opcode == 0x1 { + return frame.text(); + } + } +} + // --------------------------------------------------------------------------------------- // The payload limit (R27) // --------------------------------------------------------------------------------------- @@ -617,6 +665,13 @@ async fn a_client_that_stops_reading_is_closed_with_1013() { } let server = Server::start_with(fixture, relay_keys(), fast_config()).await; + + assert_eq!( + server.relay.frame_budget().accounted(), + 0, + "nothing is counted before this client asks for anything" + ); + let (mut raw, _head) = RawClient::connect(server.address, None).await; // The socket's first frame is the NIP-42 challenge, which is a text frame on every @@ -681,4 +736,223 @@ async fn a_client_that_stops_reading_is_closed_with_1013() { close_of(last), (1013, transport::SLOW_CONSUMER_REASON.to_owned()) ); + + // This exit is the loop's overflow arm, not the drain arm's `release`: the frames this + // socket admitted and never wrote were still counted when the connection ended, so the + // refund on its last sink is the only thing that gives them back. This is the + // behaviour-level form of the criterion — a subsequent connection is admitted — and does + // not by itself tell a refund from a counter that happened to be zero; + // `sink.rs::the_budget_release_refunds_only_the_closing_sockets_bytes` is the test that does. + assert!( + eventually(|| server.relay.frame_budget().accounted() == 0).await, + "a closed socket's bytes return to the budget, found {}", + server.relay.frame_budget().accounted() + ); + + let (mut fresh, _head) = RawClient::connect(server.address, None).await; + let auth = tokio::time::timeout(Duration::from_secs(5), fresh.next_frame()) + .await + .expect("the fresh client is served") + .expect("the fresh client's challenge"); + + assert_eq!(auth.opcode, 0x1); + assert!( + auth.text().starts_with(r#"["AUTH","#), + "a fresh client is served after the refund: {}", + auth.text() + ); +} + +// --------------------------------------------------------------------------------------- +// The frame-memory budget and the per-IP cap (#959, ADR 0008) +// --------------------------------------------------------------------------------------- + +#[tokio::test] +async fn the_default_bounds_are_the_policy_numbers() { + let config = RelayConfig::default(); + + assert_eq!(config.frame_memory_budget_bytes, 268_435_456); + assert_eq!(config.max_connections_per_ip, 64); + + let server = Server::start_with(Fixture::new(settings()), relay_keys(), config).await; + + assert_eq!( + server.relay.frame_budget().limit(), + RelayConfig::default().frame_memory_budget_bytes + ); + assert_eq!( + server.relay.connection_cap().max_per_ip(), + RelayConfig::default().max_connections_per_ip + ); + assert_eq!(server.relay.frame_budget().accounted(), 0); + assert_eq!(server.relay.frame_budget().refusals(), 0); +} + +#[tokio::test] +async fn a_socket_above_the_per_ip_cap_is_closed_with_1013_and_no_text_frame() { + let server = Server::start_with( + Fixture::new(settings()), + relay_keys(), + bounded_config(256 * 1024 * 1024, 2), + ) + .await; + + let (mut first, first_head) = RawClient::connect(server.address, None).await; + let (mut second, second_head) = RawClient::connect(server.address, None).await; + + // Both admitted sockets get the challenge, which is the relay's first text frame. + for head in [&first_head, &second_head] { + assert!(head.starts_with("HTTP/1.1 101"), "{head}"); + } + + for client in [&mut first, &mut second] { + let challenge = tokio::time::timeout(Duration::from_secs(5), client.next_frame()) + .await + .expect("a challenge arrives") + .expect("a challenge frame"); + + assert_eq!(challenge.opcode, 0x1); + assert!(challenge.text().starts_with(r#"["AUTH","#)); + } + + let (mut refused, refused_head) = RawClient::connect(server.address, None).await; + + assert!(refused_head.starts_with("HTTP/1.1 101"), "{refused_head}"); + + let frames = tokio::time::timeout(Duration::from_secs(5), refused.frames_until(0x8)) + .await + .expect("the refused socket is closed"); + + assert_no_text_frame(&frames); + + assert_eq!(frames.len(), 1, "exactly one frame: the close: {frames:?}"); + assert_eq!(frames[0].opcode, 0x8); + assert_eq!( + close_of(&frames[0]), + (1013, transport::CONNECTION_CAP_REASON.to_owned()) + ); + + // The two admitted sockets are still serving. + first.send_text(r#"["REQ","s",{"kinds":[1]}]"#).await; + + assert_eq!(next_text(&mut first).await, r#"["EOSE","s"]"#); + + assert_eq!(server.relay.connection_cap().refusals(), 1); + assert_eq!(server.relay.connection_cap().live("127.0.0.1"), 2); +} + +#[tokio::test] +async fn the_per_ip_cap_releases_a_slot_when_a_socket_closes() { + let server = Server::start_with( + Fixture::new(settings()), + relay_keys(), + bounded_config(256 * 1024 * 1024, 1), + ) + .await; + + let (mut first, head) = RawClient::connect(server.address, None).await; + + assert!(head.starts_with("HTTP/1.1 101"), "{head}"); + assert!(next_text(&mut first).await.starts_with(r#"["AUTH","#)); + + let (mut second, _head) = RawClient::connect(server.address, None).await; + let frames = tokio::time::timeout(Duration::from_secs(5), second.frames_until(0x8)) + .await + .expect("the second socket is closed"); + + assert_no_text_frame(&frames); + + assert_eq!( + close_of(frames.last().expect("a close frame")), + (1013, transport::CONNECTION_CAP_REASON.to_owned()) + ); + assert_eq!(server.relay.connection_cap().live("127.0.0.1"), 1); + + drop(first); + + assert!( + eventually(|| server.relay.connection_cap().live("127.0.0.1") == 0).await, + "a closed socket's slot is released" + ); + assert_eq!( + server.relay.connection_cap().tracked_addresses(), + 0, + "an address that released its last slot leaves no entry" + ); + + let (mut third, head) = RawClient::connect(server.address, None).await; + + assert!(head.starts_with("HTTP/1.1 101"), "{head}"); + assert!(next_text(&mut third).await.starts_with(r#"["AUTH","#)); + assert_eq!( + server.relay.connection_cap().refusals(), + 1, + "the released slot admits again without a second refusal" + ); +} + +#[tokio::test] +async fn the_per_ip_cap_does_not_lapse_with_time() { + let server = Server::start_with( + Fixture::new(settings()), + relay_keys(), + bounded_config(256 * 1024 * 1024, 2), + ) + .await; + + let (mut first, _head) = RawClient::connect(server.address, None).await; + let (mut second, _head) = RawClient::connect(server.address, None).await; + + assert!(next_text(&mut first).await.starts_with(r#"["AUTH","#)); + assert!(next_text(&mut second).await.starts_with(r#"["AUTH","#)); + + // Past every rate window the shipped limits use, and past the liveness interval too: the + // cap is a count of live sockets, so time passing changes nothing. + server.clock.advance(3_600_000); + + let (mut refused, _head) = RawClient::connect(server.address, None).await; + let frames = tokio::time::timeout(Duration::from_secs(5), refused.frames_until(0x8)) + .await + .expect("the third socket is closed"); + + assert_no_text_frame(&frames); + + assert_eq!( + close_of(frames.last().expect("a close frame")), + (1013, transport::CONNECTION_CAP_REASON.to_owned()) + ); + assert_eq!(server.relay.connection_cap().live("127.0.0.1"), 2); +} + +#[tokio::test] +async fn a_frame_that_exceeds_the_budget_closes_the_socket_with_1013_and_no_text_frame() { + // One byte of budget: not even the AUTH challenge fits, so the very first frame a socket + // would send is refused and nothing is ever queued. + let server = Server::start_with( + Fixture::new(settings()), + relay_keys(), + bounded_config(1, 64), + ) + .await; + + for refusals in 1..=2 { + let (mut client, head) = RawClient::connect(server.address, None).await; + + assert!(head.starts_with("HTTP/1.1 101"), "{head}"); + + let frames = tokio::time::timeout(Duration::from_secs(5), client.frames_until(0x8)) + .await + .expect("the refused socket is closed"); + + assert_no_text_frame(&frames); + + assert_eq!(frames.len(), 1, "exactly one frame: the close: {frames:?}"); + assert_eq!(frames[0].opcode, 0x8); + assert_eq!( + close_of(&frames[0]), + (1013, transport::FRAME_BUDGET_REASON.to_owned()) + ); + assert_eq!(server.relay.frame_budget().refusals(), refusals); + assert_eq!(server.relay.frame_budget().accounted(), 0); + } } From 7f1635140f49e9557b89acc9c72efe3ac0481147 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sat, 24 Oct 2026 23:42:35 -0400 Subject: [PATCH 02/26] docs(rewrite): record the frame-memory budget and the per-IP cap (ADR 0008) Both numbers are deviations from the capture, with their arithmetic, the rejected accept-time shape, the refund on the last handle's Drop, and what an operator can see today (#959). --- .../decisions/0008-frame-memory-budget.md | 229 ++++++++++++++++++ 1 file changed, 229 insertions(+) create mode 100644 docs/rewrite/decisions/0008-frame-memory-budget.md diff --git a/docs/rewrite/decisions/0008-frame-memory-budget.md b/docs/rewrite/decisions/0008-frame-memory-budget.md new file mode 100644 index 00000000..762c0188 --- /dev/null +++ b/docs/rewrite/decisions/0008-frame-memory-budget.md @@ -0,0 +1,229 @@ +# ADR 0008: A global frame-memory budget and a per-IP live-connection cap (job 016) + +- **Status:** accepted (job 016, Phase 2) +- **Deciders:** the rewrite team, on the operator's policy comment on #959 and the evidence in job 016's recon and spec +- **Date:** 2026-10-24 + +## Context + +The outbound frame queue was bounded per socket and nothing bounded their sum. +`RelayConfig::frame_queue_bytes` (8 MiB; `crates/nostream-relay/src/relay.rs:57`) is enforced by +`FrameSink::send`'s compare-and-exchange on the socket's own counter +(`crates/nostream-relay/src/sink.rs:239-322`), and `Relay::serve` accepted every connection with +no count at all. The only per-address bookkeeping was the **attempt** limiter, which throttles how +fast connections open, not how many are held (`crates/nostream-relay/src/limits.rs`, windows +consumed at `crates/nostream-relay/src/relay.rs:327`). + +The operator's policy comment on #959 chose the numbers and the refusal and asked for two +consequences a reviewer can hold the change to: both bounds refuse the same way, and the budget is +released when a socket closes ("a leak here is worse than no bound"). + +**What the change is worth, measured.** One address that opens sockets and then stops reading held +`N × 8 MiB`. With the shipped attempt windows — 12 per 1 000 ms and 48 per 60 000 ms +(`resources/default-settings.yaml:215-224`) — one address gets 48 sockets a minute indefinitely, so +the pre-change worst case is **384 MiB**, and that figure is **arithmetic from named constants, not a measurement**; it is the same derivation the performance lens made, marked there as arithmetic. + +**The failure the counter exists to avoid was measured on the wire.** Twenty sockets opened back to +back on a fresh daemon: the first twelve received `HTTP/1.1 101 Switching Protocols` and then a text +frame starting `["AUTH","…`, and the remaining eight were upgraded and dropped with **no websocket +close frame at all** — the attempt limiter's silent return +(`crates/nostream-relay/src/relay.rs:287-290`). A refusal indistinguishable from a network fault is +exactly what the new bounds are not allowed to be, so each of them closes with a code, a fixed +reason and its own counter. + +## The two numbers, as deviations from the capture, with the arithmetic + +The capture has neither bound. Its connection limits are `ConnectionLimits` — `rateLimits`, +`ipWhitelist`, `ipBlacklist` and no count (`src/@types/settings.ts:163-167`) — and its outbound +queue is unbounded (`src/adapters/web-socket-adapter.ts:150-155`). Both numbers below are therefore +deviations from the capture, and both are correct because the operator's comment names them, not +because a captured transcript shows them. There is no parity oracle for either: `wire_parity` cannot +see a bound the capture does not have. + +| knob | default | the arithmetic | +|---|---|---| +| `frame_memory_budget_bytes` | **256 MiB** (`268_435_456` bytes) | 32 sockets at the 8 MiB `frame_queue_bytes` ceiling can be fully buffered before the budget binds — below the 48 a single address gets through the shipped attempt limits alone | +| `max_connections_per_ip` | **64** sockets | four times the concurrent total one well-behaved client needs, and above the 48 the attempt limiter alone allows in a minute | +| refusal | `1013`, immediate, no text frame | a frame the client will never read cannot be the way to say "you are not reading" | + +The cap's own worst case is 64 × 8 MiB = **512 MiB**, deliberately **above** the budget. That is the +policy's own division of labour restated: the cap bounds sockets, the budget bounds bytes, and a +process whose sockets are all small can hold more of them than the budget-implied 32 without being +refused for it. Neither number is a total-memory ceiling; see the next section. + +## What "frame memory" means here + +Accounted **outbound payload bytes**: the `text.len()` that `frame_queue_bytes` already counts +(`crates/nostream-relay/src/sink.rs:244-245`), not `String` capacity, not the channel node, and not +resident memory. The nearest measurement of the ratio is job 010's probe D, which saw roughly 2 KiB +resident per 1 KiB queued at 1 KiB frames, so **256 MiB accounted is not a process RSS ceiling** and +should not be read as one. + +Nothing else is counted. Inbound is bounded per message by `max_payload_size` (131 072 by default, +`crates/nostream-relay/src/transport.rs:50-57`), handler state is not counted at all, and the +hidden-content mirror and the event stores are outside the budget on purpose: they are not frames, +and folding them in would make one knob mean two things. + +## The config surface — `RelayConfig`, not settings + +Both fields are `RelayConfig` fields, `frame_memory_budget_bytes` at +`crates/nostream-relay/src/relay.rs:65` and `max_connections_per_ip` at `:69`, built into the relay's +two shared values in `Relay::new` (`:146-147`). ADR 0005 already decided the question for this class +of knob: + +> The bound and the timeout are `RelayConfig` fields and not settings. The capture has no +> counterpart for either … so a settings key would be an operator-facing surface mirroring nothing. +> `RelayConfig` is where this branch already keeps the knobs the capture fixes in code … + +— `docs/rewrite/decisions/0005-connection-concurrency.md:104-110` + +**The honest cost:** an operator cannot change either number without a rebuild. "Configurable" here +means the value is readable and settable through a `RelayConfig` literal rather than buried in a +call site — a settings key, a YAML entry and a validator/guided-catalogue entry are **#989** and out +of scope for this job. Say it plainly rather than letting "configurable" read as a knob an operator +can edit in production; today they cannot. + +`nostreamd` keeps `..Default::default()` in its `RelayConfig` literal +(`crates/nostreamd/src/main.rs:120-123`), and all five `Relay::new` call sites already did, so no +call site had to change. + +## The refusal — `1013`, immediate, no text frame + +Both bounds close the socket with `CloseCode::Again` (1013) written straight to the transport rather +than through the sink, bounded by `config.write_timeout` exactly as the existing slow-consumer close +is (`Relay::close_frame`, `crates/nostream-relay/src/relay.rs:524-535`). Three fixed reason strings +now exist, each lowercase and resource-stating, per the convention +`SLOW_CONSUMER_REASON` set (`crates/nostream-relay/src/transport.rs:37-47`): + +- `SLOW_CONSUMER_REASON` — "slow consumer: outbound frame queue full", unchanged. +- `FRAME_BUDGET_REASON` — "frame memory budget exhausted". +- `CONNECTION_CAP_REASON` — "connection cap: too many connections from this address". + +**Why not a `NOTICE`.** A NOTICE is a text frame, and an over-budget client is by definition one that +is not reading: the frame would sit in the very queue being refused, and a client at the cap has not +earned a JSON frame it did not ask for. The refusal has to be legible in the close frame, which every +websocket client reads, rather than in a payload it may never consume. + +**Neither refusal is a timeout, and neither is the attempt limiter's.** The attempt limiter's refusal +is still the silent upgrade-then-drop measured above; this job leaves `RateLimits`, its windows and +its whitelist untouched. The two new refusals are distinguishable from it and from each other by +code, reason string, and one counter each (`FrameBudget::refusals`, +`crates/nostream-relay/src/sink.rs:90`; `ConnectionCap::refusals`, +`crates/nostream-relay/src/connection_cap.rs:60`), which is what keeps them out of the "looks like a +network fault" bucket that started this. + +**A budget refusal refuses the frame, not the socket.** The refusal cause is recorded once — so the +counter counts one refused socket, not one per frame, and the warn line is written once — but only a +queue-full refusal latches the sink shut. A budget refusal that latched would also have to be +un-latched when the drain side frees room, and it buys nothing: the connection loop closes the socket +at its next iteration whatever the sender does. The accounting therefore stays exact under either +choice, and the unit test that proves room lets the next frame through +(`sink.rs::the_budget_binds_across_sockets_and_is_shared`) can say so. + +## Where the budget binds: per-frame admission, not an accept-time reservation + +The budget is enforced where the per-socket bound already is: in `FrameSink::send`, as a second +compare-and-exchange loop against `accounted + length > limit` +(`crates/nostream-relay/src/sink.rs:99-135`). Admission requires room in both bounds; the budget has +**no empty-queue exception** — unlike `frame_queue_bytes`, whose exception keeps one large event from +closing every client — because 256 MiB exceeds any single outbound frame the protocol can produce by +three orders of magnitude, and strictness is what makes a one-byte test budget meaningful. + +**The rejected shape: an accept-time reservation.** Refusing new sockets when the budget is already +full is the reading a "global budget" invites, and it is wrong for two reasons. It does not bound what +an admitted socket may queue — a socket admitted into an empty budget can still queue its 8 MiB, so +the bound has to exist at the sink regardless, and adding an accept-time gate buys a second bound +beside it rather than replacing it. And a reservation needs its own lifecycle: take bytes at accept, +refund them on every exit, and get the partial case right when a socket used less than it reserved — +which is a second refund path of exactly the kind section 7 exists to eliminate. Per-frame admission +has one lifecycle (bytes are counted when a frame is admitted, uncounted when it is written or when +the socket's accounting ends) and refuses the frames that actually need the memory. + +## The refund, and the shape rejected + +**Chosen: refund on the last handle's `Drop`.** `FrameSink` holds an `Arc` +(`crates/nostream-relay/src/sink.rs:185-191`); the account carries the socket's outstanding bytes, +its refusal cause and the shared budget, and `impl Drop for Account` +(`crates/nostream-relay/src/sink.rs:175-181`) returns whatever is left of that socket's bytes to the +budget. Every clone of the socket's sink shares the account, so the drop that runs is the last one: a +send holds a sink and therefore cannot be in flight or begin once that drop starts. That is what makes +the refund **exactly this socket's bytes, once**, whatever the connection loop did on the way out. + +**The rejected shape: a decrement at each of the loop's exits.** Today `release` has exactly one +caller, the drain arm (`crates/nostream-relay/src/relay.rs:407`), and every other exit leaves whatever +was still queued counted: the overflow close (`:391-394`), the `None` receiver (`:398`), a failed +write (`:409-411`), the inbound and close arms (`:414`, `:433-435`, `:440`), the lagged hub (`:460`), the +liveness arms (`:464-469`, `:479-481`) and the final cleanup (`:488-489`). Per socket that leak is invisible +because the counter dies with the sink; a process-wide budget makes it a leak of up to 8 MiB on every +disconnect. Writing the decrement at each of those ten sites is the change that looks right and rots: +the next exit added to the loop silently leaks, and no unit test can reach the exits that need a +failed handshake or a poisoned store to trigger. `Drop` covers all of them by construction, including +the registration failure inside `run_connection` — which matters, because a refund that ran only on +the happy exit would fail AC8's test. + +The drain arm's `release` is unchanged and still runs: it returns written bytes promptly, so the +budget is reusable while the socket lives, and the `Drop` refund then finds only what was never +written. + +## The per-IP cap + +A `ConnectionCap` (`crates/nostream-relay/src/connection_cap.rs`) counts **live sockets** per +`peer.ip().to_string()` — the same string the attempt limiter's buckets are keyed on — and is +checked in `handle_socket` **after** `is_connection_rate_limited` and **before** `run_connection`: + +- after the limiter, so a rate-limited socket is still dropped silently as it always was, takes no + slot, and needs no refund on that path; +- before `run_connection`, because `Connection::accept`'s first act is to send the NIP-42 challenge + as a text frame (`crates/nostream-relay/src/connection.rs:109-115`) — a client refused afterwards + would already have received a text frame, which the policy forbids. The window is + `crates/nostream-relay/src/relay.rs:292-312`. + +Acquisition returns a `ConnectionSlot` held for the rest of `handle_socket` +(`crates/nostream-relay/src/relay.rs:299`), so the normal exit, the registration failure inside +`run_connection`, and any early return added later all release it through `Drop` +(`crates/nostream-relay/src/connection_cap.rs:101-116`). A count that leaks makes the cap permanent +for that address, which is a defect and not a memory leak. The map entry is removed when its count +reaches zero (`:111-114`), so an address that once connected leaves nothing behind and the map is +bounded by live addresses. + +The cap applies to **every address, including whitelisted ones**. An exempt address would bypass the +memory bound the cap protects, and the limiter's `ipWhitelist` behaviour is untouched: the whitelist +answers "skip the rate window", not "hold unlimited sockets". + +This is a gauge, not a window: no store, no clock, no expiry. `RateLimits` and `limits.rs` are +unchanged, and the measured 20-socket behaviour — 12 served, 8 dropped by the limiter — is unchanged +with it. + +## What an operator can see today + +Two accessors in the `hidden_content()` style, `Relay::frame_budget()` and +`Relay::connection_cap()` (`crates/nostream-relay/src/relay.rs:175-182`), and the counters behind +them: `accounted()`, `limit()`, `refusals()` on the budget; `live(ip)`, `max_per_ip()`, +`refusals()`, `tracked_addresses()` on the cap. Plus two structured log lines, one per refusal — a +`warn!` carrying `budget_bytes` and `accounted_bytes` from the sink +(`crates/nostream-relay/src/sink.rs:294-298`), and a `debug!` carrying `peer`, +`max_connections_per_ip` and `live` from the connection task +(`crates/nostream-relay/src/relay.rs:300-305`). + +That is the whole surface, because **a Prometheus endpoint is out of reach right now**: the OTLP +exporter is a stub returning `NotImplemented` (`crates/nostreamd/src/telemetry.rs:47-52`) and the +admin `/metrics` controller is a Phase-1 stub (`crates/nostream-admin/src/controllers/get_metrics.rs:5-9`). +There is no registry to publish into. When `/metrics` lands, an exporter reads these two counters and +nothing here needs to change; until then the counters and the two log lines are the deliverable, and +this job deliberately adds no metrics subsystem. + +## What this does not decide + +- **The operator-facing settings key** for either number: #989, with the YAML entry, the validator + and the guided catalogue it drags in. +- **Any exporter, dashboard or alerting.** No registry, no `/metrics` controller, no admin or + telemetry change. +- **Anything per listener.** One `Relay` carries one budget, and a second listener served by the same + `Arc` shares it (`Acceptor::run` calls `relay.serve(listener)`, + `crates/nostreamd/src/acceptor.rs:56-60`). A `static` would give the same sharing and cost the test + suite the ability to run two relays with different budgets in one process; two `Relay::new` calls + are the only way to get two budgets, and production makes one. +- **The per-socket bound's own empty-queue exception**, its default, and `SLOW_CONSUMER_REASON`. +- **Inbound memory or handler state**, neither of which the budget counts. +- **Any validation of the two numbers.** A zero budget or a zero cap is a literal the operator wrote; + policing it is a settings decision this job does not own. From bb71b72745cd44f204a1ef515c53dd63a201b057 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sat, 24 Oct 2026 23:44:57 -0400 Subject: [PATCH 03/26] docs(relay): name the two new knobs where RelayConfig's list is, and the budget in the sink doc The RelayConfig doc enumerates the port-only policies, so it has to name all four; the sink's module doc is where a reader learns what the sum is bounded by. --- crates/nostream-relay/src/relay.rs | 8 ++++---- crates/nostream-relay/src/sink.rs | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/crates/nostream-relay/src/relay.rs b/crates/nostream-relay/src/relay.rs index ce6722e3..a5b9d9e1 100644 --- a/crates/nostream-relay/src/relay.rs +++ b/crates/nostream-relay/src/relay.rs @@ -39,10 +39,10 @@ use crate::wot::WotGraph; /// The knobs a [`Relay`] is built with that are not settings: a constant the capture fixes in /// code (`liveness`, the NIP-56 refresh) or a policy the port adds with no counterpart there -/// (`frame_queue_bytes`, `write_timeout`, the frame-memory budget and the per-IP connection -/// cap; ADR 0005, ADR 0008). Tests set the cadences they need (`tests/common/mod.rs::fast_config`); -/// the policies run at their shipped defaults except where a test sets one deliberately. A -/// value the capture's settings document carries belongs in [`Settings`], never here. +/// (`frame_queue_bytes`, `write_timeout`, `frame_memory_budget_bytes`, +/// `max_connections_per_ip`; ADR 0005, ADR 0008). Tests set the cadences they need +/// (`tests/common/mod.rs::fast_config`); the policies run at their shipped defaults. A value +/// the capture's settings document carries belongs in [`Settings`], never here. #[derive(Debug, Clone)] pub struct RelayConfig { /// `WSS_CLIENT_HEALTH_PROBE_INTERVAL` (`web-socket-server-adapter.ts:18`). diff --git a/crates/nostream-relay/src/sink.rs b/crates/nostream-relay/src/sink.rs index dfb236d1..f7c3fb06 100644 --- a/crates/nostream-relay/src/sink.rs +++ b/crates/nostream-relay/src/sink.rs @@ -27,11 +27,11 @@ //! //! **The sum is bounded too**, which the per-socket bound cannot do: one address that opens //! sockets and stops reading held a multiple of the per-socket bound, so every socket's -//! queue is also counted against one process-wide [`FrameBudget`] (ADR 0008). An admission -//! needs room in both bounds; the budget has no empty-queue exception, so a frame that does -//! not fit the budget is refused however empty its own queue. The socket's share of the -//! budget is refunded when its last sink handle drops — see [`Account`] — because the next -//! socket's admission depends on it. +//! queue also counts against one process-wide frame memory budget ([`FrameBudget`], +//! ADR 0008). An admission needs room in both bounds; the budget has no empty-queue +//! exception, so a frame that does not fit the budget is refused however empty its own +//! queue. The socket's share of the budget is refunded when its last sink handle drops — +//! see [`Account`] — because the next socket's admission depends on it. use std::sync::Arc; use std::sync::atomic::{AtomicU8, AtomicU64, AtomicUsize, Ordering}; From 74ff50d2b2494723e4fa4587d89f0293651fd5a2 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sat, 24 Oct 2026 23:45:00 -0400 Subject: [PATCH 04/26] docs(rewrite): correct ADR 0008's line citations to the final tree --- docs/rewrite/decisions/0008-frame-memory-budget.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/docs/rewrite/decisions/0008-frame-memory-budget.md b/docs/rewrite/decisions/0008-frame-memory-budget.md index 762c0188..aac2e086 100644 --- a/docs/rewrite/decisions/0008-frame-memory-budget.md +++ b/docs/rewrite/decisions/0008-frame-memory-budget.md @@ -176,7 +176,7 @@ checked in `handle_socket` **after** `is_connection_rate_limited` and **before** - before `run_connection`, because `Connection::accept`'s first act is to send the NIP-42 challenge as a text frame (`crates/nostream-relay/src/connection.rs:109-115`) — a client refused afterwards would already have received a text frame, which the policy forbids. The window is - `crates/nostream-relay/src/relay.rs:292-312`. + `crates/nostream-relay/src/relay.rs:292-315`. Acquisition returns a `ConnectionSlot` held for the rest of `handle_socket` (`crates/nostream-relay/src/relay.rs:299`), so the normal exit, the registration failure inside @@ -201,7 +201,7 @@ Two accessors in the `hidden_content()` style, `Relay::frame_budget()` and them: `accounted()`, `limit()`, `refusals()` on the budget; `live(ip)`, `max_per_ip()`, `refusals()`, `tracked_addresses()` on the cap. Plus two structured log lines, one per refusal — a `warn!` carrying `budget_bytes` and `accounted_bytes` from the sink -(`crates/nostream-relay/src/sink.rs:294-298`), and a `debug!` carrying `peer`, +(`crates/nostream-relay/src/sink.rs:295-299`), and a `debug!` carrying `peer`, `max_connections_per_ip` and `live` from the connection task (`crates/nostream-relay/src/relay.rs:300-305`). From 8ae2009a3c6e98e0d39ec6035680a0e2c70553bc Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 00:00:25 -0400 Subject: [PATCH 05/26] fix(relay): wake the connection loop on a refusal, and encode only frames it can queue Three static-review findings on the frame-memory budget, in one commit because they meet in FrameSink::send: - A refusal recorded a flag but woke nothing. A shared-budget refusal can arrive while this socket's own queue is empty, the socket idle and its read parked, so the 1013 waited for the next liveness tick instead of being immediate. The first refusal now signals a tokio::Notify the account owns, and the connection loop selects on FrameSink::refused(); notify_one stores a permit, so a refusal recorded between the loop's check and its select is not lost. - The queue-full check ran after to_text(), so a latching socket kept serialising events nobody would read. The early return is back before the encoding, with the in-loop check kept for racing senders. - The racing-sender test's sampler stopped only at the expected total, so the accounting bug it exists to catch could spin forever inside thread::scope. It now stops when the last sender finishes, and fails with numbers. The new wire test holds the budget from a socket that never reads and loses a frame on a socket whose own queue is empty: it times out without the signal and closes in 1.3 s with it. --- crates/nostream-relay/src/relay.rs | 6 ++ crates/nostream-relay/src/sink.rs | 66 ++++++++++++----- crates/nostream-relay/tests/transport.rs | 91 ++++++++++++++++++++++++ 3 files changed, 147 insertions(+), 16 deletions(-) diff --git a/crates/nostream-relay/src/relay.rs b/crates/nostream-relay/src/relay.rs index a5b9d9e1..1b733954 100644 --- a/crates/nostream-relay/src/relay.rs +++ b/crates/nostream-relay/src/relay.rs @@ -481,6 +481,12 @@ impl Relay { } } _ = drained.changed() => drain_seen = *drained.borrow_and_update(), + // A refusal can arrive with nothing else to wake this loop: the budget is + // shared, so the frame that did not fit may have left this socket's own queue + // empty, the socket itself idle, and its read parked on the store. The + // producer signals when it records the refusal, and the next iteration's + // check closes the client — immediate, not one liveness tick away (ADR 0008). + _ = sink.refused() => {} Some(_) = handlers.join_next(), if !handlers.is_empty() => {} } } diff --git a/crates/nostream-relay/src/sink.rs b/crates/nostream-relay/src/sink.rs index f7c3fb06..9cc26072 100644 --- a/crates/nostream-relay/src/sink.rs +++ b/crates/nostream-relay/src/sink.rs @@ -33,10 +33,11 @@ //! queue. The socket's share of the budget is refunded when its last sink handle drops — //! see [`Account`] — because the next socket's admission depends on it. +use std::future::Future; use std::sync::Arc; use std::sync::atomic::{AtomicU8, AtomicU64, AtomicUsize, Ordering}; -use tokio::sync::mpsc; +use tokio::sync::{Notify, mpsc}; use nostream_core::messages::RelayMessage; @@ -146,6 +147,14 @@ impl FrameBudget { struct Account { queued_bytes: AtomicUsize, refusal: AtomicU8, + /// Resolved when the first refusal is recorded. A refusal can arrive while this socket's + /// own queue is empty — the budget is shared, so another socket's frames are what left no + /// room — and the connection task may then have nothing else to wake it: signalling here + /// is what makes the close immediate rather than the next liveness tick. + /// + /// `notify_one` stores a permit when nothing is waiting, so a refusal recorded between the + /// loop's check and its `select!` is not lost, and two racing refusals leave one permit. + refusal_signal: Notify, budget: Option>, } @@ -166,9 +175,16 @@ impl Account { Refusal::BudgetExhausted => BUDGET_EXHAUSTED, }; - self.refusal + let first = self + .refusal .compare_exchange(NO_REFUSAL, cause, Ordering::SeqCst, Ordering::SeqCst) - .is_ok() + .is_ok(); + + if first { + self.refusal_signal.notify_one(); + } + + first } } @@ -209,6 +225,7 @@ impl FrameSink { account: Arc::new(Account { queued_bytes: AtomicUsize::new(0), refusal: AtomicU8::new(NO_REFUSAL), + refusal_signal: Notify::new(), budget, }), }, @@ -225,6 +242,7 @@ impl FrameSink { account: Arc::new(Account { queued_bytes: AtomicUsize::new(0), refusal: AtomicU8::new(NO_REFUSAL), + refusal_signal: Notify::new(), budget: None, }), } @@ -233,14 +251,21 @@ impl FrameSink { /// Sends one frame. A closed socket drops the frame and is not an error. /// /// A queue-full refusal latches the socket shut, as the overflow always has: every later - /// send is dropped. A budget refusal does not — it is one frame that did not fit at that - /// instant, and the connection task closes the socket at its next iteration whatever the - /// sender does — so the accounting stays exact and a frame that fits later is admitted. + /// send is dropped, and the check comes before the frame is encoded — a stored read can + /// keep producing events until the connection task closes it, and each discarded event + /// would otherwise cost an allocation and a serialisation that nothing reads. A budget + /// refusal does not latch — it is one frame that did not fit at that instant, and the + /// connection task closes the socket at its next iteration whatever the sender does — so + /// the accounting stays exact and a frame that fits later is admitted. pub fn send(&self, message: &RelayMessage) { let Some(sender) = &self.sender else { return; }; + if self.account.refusal() == Some(Refusal::QueueFull) { + return; + } + let text = message.to_text(); let length = text.len(); let mut queued = self.account.queued_bytes.load(Ordering::SeqCst); @@ -252,6 +277,7 @@ impl FrameSink { if self.account.refusal() == Some(Refusal::QueueFull) { return; } + // An empty queue admits its frame whatever the size, so an event larger than the // whole bound still reaches the client instead of closing it. if queued > 0 && queued + length > self.bound_bytes { @@ -328,6 +354,14 @@ impl FrameSink { self.account.refusal() } + /// Resolves when a refusal is recorded on this socket, so the connection task can close it + /// without waiting for a read, a write or a liveness tick to wake it. A permit is stored + /// when nothing is waiting, so a refusal recorded before the loop reaches its `select!` is + /// delivered rather than lost. + pub fn refused(&self) -> impl Future + '_ { + self.account.refusal_signal.notified() + } + /// The bytes admitted into the queue and not yet written. pub fn queued_bytes(&self) -> usize { self.account.queued_bytes.load(Ordering::SeqCst) @@ -552,6 +586,7 @@ mod tests { const SENDERS: usize = 8; let budget = Arc::new(FrameBudget::new(SENDERS * len)); let max_seen = AtomicUsize::new(0); + let finished = AtomicUsize::new(0); let start = std::sync::Barrier::new(SENDERS + 1); let mut sinks = Vec::new(); @@ -564,31 +599,30 @@ mod tests { for sink in &sinks { let frame = &frame; let start = &start; + let finished = &finished; scope.spawn(move || { start.wait(); sink.send(frame); + finished.fetch_add(1, Ordering::SeqCst); }); } - // A sampler reading the counter while the sends run: it stops when the budget is - // full, so it cannot outlive the sends it observes. + // A sampler reading the counter while the sends run. It stops when the last sender + // has finished rather than when the counter reaches the expected total, so the + // accounting bug this test exists to catch fails it with numbers instead of + // leaving it spinning inside the scope. { let budget = Arc::clone(&budget); let max_seen = &max_seen; + let finished = &finished; let start = &start; scope.spawn(move || { start.wait(); - loop { - let accounted = budget.accounted(); - max_seen.fetch_max(accounted, Ordering::SeqCst); - - if accounted == SENDERS * len { - break; - } - + while finished.load(Ordering::SeqCst) < SENDERS { + max_seen.fetch_max(budget.accounted(), Ordering::SeqCst); std::hint::spin_loop(); } }); diff --git a/crates/nostream-relay/tests/transport.rs b/crates/nostream-relay/tests/transport.rs index 76327fbb..1258236e 100644 --- a/crates/nostream-relay/tests/transport.rs +++ b/crates/nostream-relay/tests/transport.rs @@ -956,3 +956,94 @@ async fn a_frame_that_exceeds_the_budget_closes_the_socket_with_1013_and_no_text assert_eq!(server.relay.frame_budget().accounted(), 0); } } + +#[tokio::test] +async fn a_shared_budget_refusal_closes_the_socket_without_waiting_for_a_tick() { + // The budget is shared, so the frame that does not fit may be refused while *this* + // socket's own queue is empty: nothing here is ready to wake its loop — no queued frame, + // no inbound message, no handler finishing — and the close must not wait for the 600 s + // liveness tick. One socket holds the budget and never reads, so its write blocks with + // ~7 MiB of the 8 MiB still counted; the second socket's 2 MiB frame then does not fit + // what is left (7 + 2 > 8). + const BUDGET: usize = 8 * 1024 * 1024; + const HELD: usize = 7 * 1024 * 1024; + + let fixture = Fixture::new(settings()); + let held_content = "h".repeat(HELD); + let refused_content = "r".repeat(2 * 1024 * 1024); + + fixture + .seed_event(signed_event( + &CLIENT_SECRET, + 1, + vec![], + &held_content, + NOW_SECS, + )) + .await; + fixture + .seed_event(signed_event( + &CLIENT_SECRET, + 2, + vec![], + &refused_content, + NOW_SECS + 1, + )) + .await; + // A second kind-2 event, so the read that loses its frame stays in flight afterwards + // instead of finishing and waking the loop through `handlers.join_next()`. + fixture + .seed_event(signed_event( + &CLIENT_SECRET, + 2, + vec![], + "tail", + NOW_SECS + 2, + )) + .await; + + let server = Server::start_with(fixture, relay_keys(), bounded_config(BUDGET, 64)).await; + + // The socket that holds the budget: it never reads, so the loop's write of its one frame + // cannot finish and the bytes stay counted. + let (mut holder, _head) = RawClient::connect(server.address, None).await; + assert!(next_text(&mut holder).await.starts_with(r#"["AUTH","#)); + + holder.send_text(r#"["REQ","hold",{"kinds":[1]}]"#).await; + + assert!( + eventually(|| server.relay.frame_budget().accounted() > BUDGET / 2).await, + "the socket that never reads holds its frame in the budget, found {}", + server.relay.frame_budget().accounted() + ); + assert_eq!(server.relay.frame_budget().refusals(), 0); + + // From here every stored read waits on the gate, so the second socket's first event is + // delivered from its read task — with this loop parked in `select!` — rather than inside + // the `REQ`'s ordered prefix. + let gate = server.fixture.events.gate_reads_per_item(); + + let (mut refused, _head) = RawClient::connect(server.address, None).await; + assert!(next_text(&mut refused).await.starts_with(r#"["AUTH","#)); + + refused.send_text(r#"["REQ","now",{"kinds":[2]}]"#).await; + // The loop answers a ping in place, so the pong proves the `REQ` was taken and its read is + // parked: nothing in this socket's loop is ready after that. + refused.send_frame(true, false, 0x9, b"budget").await; + tokio::time::timeout(Duration::from_secs(5), refused.frames_until(0xa)) + .await + .expect("the loop answers the ping"); + + gate.add_permits(1); + + let frames = tokio::time::timeout(Duration::from_secs(2), refused.frames_until(0x8)) + .await + .expect("the refusal closes the socket without waiting for a liveness tick"); + + assert_no_text_frame(&frames); + assert_eq!( + close_of(frames.last().expect("a close frame")), + (1013, transport::FRAME_BUDGET_REASON.to_owned()) + ); + assert_eq!(server.relay.frame_budget().refusals(), 1); +} From c5bc366c2554f7927dd27c123d16e702ceeb5470 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 00:00:59 -0400 Subject: [PATCH 06/26] docs(rewrite): record the refusal signal in ADR 0008 and retarget its line citations --- .../decisions/0008-frame-memory-budget.md | 29 ++++++++++++++----- 1 file changed, 21 insertions(+), 8 deletions(-) diff --git a/docs/rewrite/decisions/0008-frame-memory-budget.md b/docs/rewrite/decisions/0008-frame-memory-budget.md index aac2e086..3bafffdb 100644 --- a/docs/rewrite/decisions/0008-frame-memory-budget.md +++ b/docs/rewrite/decisions/0008-frame-memory-budget.md @@ -9,7 +9,7 @@ The outbound frame queue was bounded per socket and nothing bounded their sum. `RelayConfig::frame_queue_bytes` (8 MiB; `crates/nostream-relay/src/relay.rs:57`) is enforced by `FrameSink::send`'s compare-and-exchange on the socket's own counter -(`crates/nostream-relay/src/sink.rs:239-322`), and `Relay::serve` accepted every connection with +(`crates/nostream-relay/src/sink.rs:260-334`), and `Relay::serve` accepted every connection with no count at all. The only per-address bookkeeping was the **attempt** limiter, which throttles how fast connections open, not how many are held (`crates/nostream-relay/src/limits.rs`, windows consumed at `crates/nostream-relay/src/relay.rs:327`). @@ -54,7 +54,7 @@ refused for it. Neither number is a total-memory ceiling; see the next section. ## What "frame memory" means here Accounted **outbound payload bytes**: the `text.len()` that `frame_queue_bytes` already counts -(`crates/nostream-relay/src/sink.rs:244-245`), not `String` capacity, not the channel node, and not +(`crates/nostream-relay/src/sink.rs:269-270`), not `String` capacity, not the channel node, and not resident memory. The nearest measurement of the ratio is job 010's probe D, which saw roughly 2 KiB resident per 1 KiB queued at 1 KiB frames, so **256 MiB accounted is not a process RSS ceiling** and should not be read as one. @@ -91,7 +91,7 @@ call site had to change. Both bounds close the socket with `CloseCode::Again` (1013) written straight to the transport rather than through the sink, bounded by `config.write_timeout` exactly as the existing slow-consumer close -is (`Relay::close_frame`, `crates/nostream-relay/src/relay.rs:524-535`). Three fixed reason strings +is (`Relay::close_frame`, `crates/nostream-relay/src/relay.rs:530-541`). Three fixed reason strings now exist, each lowercase and resource-stating, per the convention `SLOW_CONSUMER_REASON` set (`crates/nostream-relay/src/transport.rs:37-47`): @@ -108,7 +108,7 @@ websocket client reads, rather than in a payload it may never consume. is still the silent upgrade-then-drop measured above; this job leaves `RateLimits`, its windows and its whitelist untouched. The two new refusals are distinguishable from it and from each other by code, reason string, and one counter each (`FrameBudget::refusals`, -`crates/nostream-relay/src/sink.rs:90`; `ConnectionCap::refusals`, +`crates/nostream-relay/src/sink.rs:91`; `ConnectionCap::refusals`, `crates/nostream-relay/src/connection_cap.rs:60`), which is what keeps them out of the "looks like a network fault" bucket that started this. @@ -120,11 +120,24 @@ at its next iteration whatever the sender does. The accounting therefore stays e choice, and the unit test that proves room lets the next frame through (`sink.rs::the_budget_binds_across_sockets_and_is_shared`) can say so. +**The refusal wakes the loop.** Recording the cause is not enough on its own. A budget refusal +can arrive while *this* socket's queue is empty — the bytes that left no room are another +socket's — with the socket idle and its read parked on the store, and then nothing in the +connection task is ready: no frame to write, no inbound message, no handler finishing. The +account owns a `tokio::sync::Notify`, the first refusal signals it, and `run_connection` selects +on `FrameSink::refused()`, so the close is one iteration away rather than one liveness interval +away. `notify_one` stores a permit when nothing is waiting, which is what makes a refusal +recorded between the loop's check and its `select!` survive instead of being lost; two racing +refusals leave one permit, and the loop rechecks the cause it already reads at the top of every +iteration. The wire test that holds this down +(`tests/transport.rs::a_shared_budget_refusal_closes_the_socket_without_waiting_for_a_tick`) +times out at two seconds without the signal and closes in about 1.3 s with it. + ## Where the budget binds: per-frame admission, not an accept-time reservation The budget is enforced where the per-socket bound already is: in `FrameSink::send`, as a second compare-and-exchange loop against `accounted + length > limit` -(`crates/nostream-relay/src/sink.rs:99-135`). Admission requires room in both bounds; the budget has +(`crates/nostream-relay/src/sink.rs:100-136`). Admission requires room in both bounds; the budget has **no empty-queue exception** — unlike `frame_queue_bytes`, whose exception keeps one large event from closing every client — because 256 MiB exceeds any single outbound frame the protocol can produce by three orders of magnitude, and strictness is what makes a one-byte test budget meaningful. @@ -142,9 +155,9 @@ the socket's accounting ends) and refuses the frames that actually need the memo ## The refund, and the shape rejected **Chosen: refund on the last handle's `Drop`.** `FrameSink` holds an `Arc` -(`crates/nostream-relay/src/sink.rs:185-191`); the account carries the socket's outstanding bytes, +(`crates/nostream-relay/src/sink.rs:201-207`); the account carries the socket's outstanding bytes, its refusal cause and the shared budget, and `impl Drop for Account` -(`crates/nostream-relay/src/sink.rs:175-181`) returns whatever is left of that socket's bytes to the +(`crates/nostream-relay/src/sink.rs:191-197`) returns whatever is left of that socket's bytes to the budget. Every clone of the socket's sink shares the account, so the drop that runs is the last one: a send holds a sink and therefore cannot be in flight or begin once that drop starts. That is what makes the refund **exactly this socket's bytes, once**, whatever the connection loop did on the way out. @@ -201,7 +214,7 @@ Two accessors in the `hidden_content()` style, `Relay::frame_budget()` and them: `accounted()`, `limit()`, `refusals()` on the budget; `live(ip)`, `max_per_ip()`, `refusals()`, `tracked_addresses()` on the cap. Plus two structured log lines, one per refusal — a `warn!` carrying `budget_bytes` and `accounted_bytes` from the sink -(`crates/nostream-relay/src/sink.rs:295-299`), and a `debug!` carrying `peer`, +(`crates/nostream-relay/src/sink.rs:321-325`), and a `debug!` carrying `peer`, `max_connections_per_ip` and `live` from the connection task (`crates/nostream-relay/src/relay.rs:300-305`). From 1479c6c84ca016d956ed5e39ff26164c8e2d5da9 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 00:34:22 -0400 Subject: [PATCH 07/26] fix(relay): stop a stored read at the first refused frame, and correct the record Review round 1: a performance finding and two documentation defects, plus four notes. - A budget refusal did not stop the encode. The read loop in fetch_and_send encoded one frame per event and discarded each refused one; the window is the connection loop's write plus close_frame (30 s each) and is the normal case, since the budget binds when queues are full and loops are parked in writes. The read now ends at the top of its loop when the sink has a refusal, before pulling or encoding the next record; the sink still does not latch a budget refusal, so AC6's re-admission holds. New test in subscribe.rs: with a 64-byte budget every event frame is refused and the EOSE would fit, so an empty queue proves the read stopped (without the check it holds the 12-byte EOSE). - Two ADR citations were stale by six lines and one named the wrong arm; every citation in the file was re-read against the final tree. - The ADR presented the cap's refusal line as operator-visible; it is a debug! line and the daemon ships the info filter, so the record now says so, names the switch, and calls the gap a gap. Notes: the two new refusals are separated by the reason string, not by 1013; the shared budget's blast radius (one address below its cap can refuse every other client, including a newcomer's AUTH) is recorded; the cap's key is an address, so a [::] bind keys per /128; and RelayConfig's doc no longer claims the policies never move from their defaults. --- .../nostream-relay/src/handlers/subscribe.rs | 93 +++++++++++++++++++ crates/nostream-relay/src/relay.rs | 4 +- .../decisions/0008-frame-memory-budget.md | 73 +++++++++++++-- 3 files changed, 158 insertions(+), 12 deletions(-) diff --git a/crates/nostream-relay/src/handlers/subscribe.rs b/crates/nostream-relay/src/handlers/subscribe.rs index b0ca6cf0..0d95448a 100644 --- a/crates/nostream-relay/src/handlers/subscribe.rs +++ b/crates/nostream-relay/src/handlers/subscribe.rs @@ -268,6 +268,16 @@ async fn fetch_and_send(mut read: InFlightRead, filters: &[Filter], context: &Ha return; } + // A refused frame is never queued, and the connection loop is about to close this + // socket whatever this read does. Encoding the rest of the stream would only discard + // it, and with a shared budget the loop can be a whole blocked write away from + // running, so the window is not short. The sink must not latch on a budget refusal — + // a frame that fits later is admitted — which is why the stop lives here. + if context.sink.refusal().is_some() { + context.connection.end_read(read); + return; + } + let record = tokio::select! { changed = read.cancellation().changed() => { // An aborted read sends no frame at all, not even EOSE. @@ -408,3 +418,86 @@ fn can_subscribe( None } + +#[cfg(test)] +mod tests { + #![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + + use nostream_core::clock::TestClock; + use nostream_core::event::{EventId, Pubkey, Signature}; + use nostream_core::nip42::Nip42Session; + + use super::*; + use crate::connection::Connection; + use crate::relay_key::RelayKeySource; + use crate::sink::{FrameBudget, FrameSink, Refusal}; + use crate::testing::{Fixture, settings}; + + /// An event the fixture can hold, signed by `secret`. + fn signed_event(secret: &[u8; 32], created_at: i64) -> nostream_core::Event { + let mut event = nostream_core::Event { + id: EventId::from_byte_array([0; 32]), + pubkey: Pubkey::from_byte_array([0; 32]), + created_at, + kind: Kind(1), + tags: Vec::new(), + content: "stored".to_owned(), + sig: Signature::from_byte_array([0; 64]), + }; + + nostream_core::crypto::sign_event(secret, &mut event).expect("sign the test event"); + + event + } + + #[tokio::test] + async fn a_refused_sink_stops_the_read_instead_of_encoding_the_rest_of_it() { + // Three stored events and a budget of 64 bytes: every event frame is refused and the + // read's closing `EOSE` would fit. So the queue says which read ran — a read that + // stopped at the first refusal leaves nothing in it, and a read that encoded and + // discarded the rest of the stream ends with the `EOSE` that fits. + let fixture = Fixture::new(settings()); + + for index in 0..3 { + fixture + .seed_event(signed_event(&[7u8; 32], 1_700_000_000 + index)) + .await; + } + + let budget = Arc::new(FrameBudget::new(64)); + let (sink, mut frames) = FrameSink::channel(64 * 1024, Some(Arc::clone(&budget))); + let refused = sink.clone(); + let context = Arc::new(fixture.context( + Arc::new(TestClock::new(1_700_000_000_000)), + RelayKeySource::from_values(None, None), + Arc::new(Connection::new( + "127.0.0.1".to_owned(), + Nip42Session::with_challenge("challenge", 0), + )), + sink, + )); + + let message = SubscribeMessage { + subscription_id: "s".to_owned(), + filters: vec![Filter { + kinds: Some(vec![Kind(1)]), + ..Filter::default() + }], + }; + + let read = begin(message, &context).expect("the subscription is registered"); + read.await; + + assert_eq!(refused.refusal(), Some(Refusal::BudgetExhausted)); + assert_eq!(refused.queued_bytes(), 0); + assert!( + frames.try_recv().is_err(), + "the read stopped at the first refusal: no event frame and no EOSE reached the queue" + ); + assert_eq!( + budget.refusals(), + 1, + "one refusal for the socket, not one per event" + ); + } +} diff --git a/crates/nostream-relay/src/relay.rs b/crates/nostream-relay/src/relay.rs index 1b733954..2ec579ac 100644 --- a/crates/nostream-relay/src/relay.rs +++ b/crates/nostream-relay/src/relay.rs @@ -40,8 +40,8 @@ use crate::wot::WotGraph; /// The knobs a [`Relay`] is built with that are not settings: a constant the capture fixes in /// code (`liveness`, the NIP-56 refresh) or a policy the port adds with no counterpart there /// (`frame_queue_bytes`, `write_timeout`, `frame_memory_budget_bytes`, -/// `max_connections_per_ip`; ADR 0005, ADR 0008). Tests set the cadences they need -/// (`tests/common/mod.rs::fast_config`); the policies run at their shipped defaults. A value +/// `max_connections_per_ip`; ADR 0005, ADR 0008). Tests set the cadences they need and the +/// bounds a case asks for; every value here runs at its shipped default otherwise. A value /// the capture's settings document carries belongs in [`Settings`], never here. #[derive(Debug, Clone)] pub struct RelayConfig { diff --git a/docs/rewrite/decisions/0008-frame-memory-budget.md b/docs/rewrite/decisions/0008-frame-memory-budget.md index 3bafffdb..feaad0f5 100644 --- a/docs/rewrite/decisions/0008-frame-memory-budget.md +++ b/docs/rewrite/decisions/0008-frame-memory-budget.md @@ -106,11 +106,16 @@ websocket client reads, rather than in a payload it may never consume. **Neither refusal is a timeout, and neither is the attempt limiter's.** The attempt limiter's refusal is still the silent upgrade-then-drop measured above; this job leaves `RateLimits`, its windows and -its whitelist untouched. The two new refusals are distinguishable from it and from each other by -code, reason string, and one counter each (`FrameBudget::refusals`, -`crates/nostream-relay/src/sink.rs:91`; `ConnectionCap::refusals`, -`crates/nostream-relay/src/connection_cap.rs:60`), which is what keeps them out of the "looks like a -network fault" bucket that started this. +its whitelist untouched. What separates the two new refusals from the limiter's silent drop is that +they say something: a close frame with **code `1013`** and a fixed reason string. What separates +them **from each other** is the reason string and the counter, not the code — plus the log line's +fields (`budget_bytes`/`accounted_bytes` against `peer`/`max_connections_per_ip`/`live`) and, for a +test driving the relay directly, the accessor (`FrameBudget::refusals` at +`crates/nostream-relay/src/sink.rs:91` or `ConnectionCap::refusals` at +`crates/nostream-relay/src/connection_cap.rs:60`). Both bounds use `CloseCode::Again`, so `1013` on +its own tells a client that it was refused and says nothing about which bound did it; anything that +wants to tell the two apart reads the reason string. That is what keeps both out of the "looks like +a network fault" bucket that started this. **A budget refusal refuses the frame, not the socket.** The refusal cause is recorded once — so the counter counts one refused socket, not one per frame, and the warn line is written once — but only a @@ -120,6 +125,21 @@ at its next iteration whatever the sender does. The accounting therefore stays e choice, and the unit test that proves room lets the next frame through (`sink.rs::the_budget_binds_across_sockets_and_is_shared`) can say so. +**The refusal stops the reader as well.** The connection loop closes the socket at its next +iteration, but a stored read is a task of its own and keeps producing events until it notices: +`fetch_and_send` (`crates/nostream-relay/src/handlers/subscribe.rs:276-279`) encodes one frame per event, and +each refused frame is encoded and discarded. The window is not short — the loop can be parked in a +write and then in `close_frame`, each bounded only by the 30 s `write_timeout`, and +`handlers.abort_all()` runs only after both — and it is the normal case, because the budget binds +exactly when queues are full and loops are parked in writes. So the read checks +`context.sink.refusal()` at the top of its loop and ends there, before pulling or encoding the next +record, with no `EOSE` (there is no socket left to tell). The sink itself must not latch a budget +refusal — a frame that fits later is admitted, which the shared-budget test relies on — so the stop +belongs to the reader. `subscribe.rs::a_refused_sink_stops_the_read_instead_of_encoding_the_rest_of_it` +holds it down: with a 64-byte budget every event frame is refused and the `EOSE` would fit, so the +queue stays empty only if the read stopped at the first refusal (without the check it holds the +12-byte `EOSE`). + **The refusal wakes the loop.** Recording the cause is not enough on its own. A budget refusal can arrive while *this* socket's queue is empty — the bytes that left no room are another socket's — with the socket idle and its read parked on the store, and then nothing in the @@ -152,6 +172,16 @@ which is a second refund path of exactly the kind section 7 exists to eliminate. has one lifecycle (bytes are counted when a frame is admitted, uncounted when it is written or when the socket's accounting ends) and refuses the frames that actually need the memory. +**The blast radius is one address, and it is new.** The budget is shared, so it is only as fair as +the sockets already holding it. Eight sockets at the 8 MiB `frame_queue_bytes` ceiling are 64 MiB and +32 of them are 256 MiB, so **one address stays below its own 64-socket cap and still exhausts the +budget** — and then every other client's next frame is refused and its socket closed, including a +new client's AUTH challenge, which is a text frame like any other and is refused before that client +has been spoken to. Before this change there was no shared bound to exhaust: one address could hold +`N × 8 MiB` of its own, but it could not make another client's connection fail. That is the shape the +operator's comment chose — a shared bound over a per-address one — and it is recorded here as a cost +of it, not as a defect. + ## The refund, and the shape rejected **Chosen: refund on the last handle's `Drop`.** `FrameSink` holds an `Arc` @@ -164,11 +194,13 @@ the refund **exactly this socket's bytes, once**, whatever the connection loop d **The rejected shape: a decrement at each of the loop's exits.** Today `release` has exactly one caller, the drain arm (`crates/nostream-relay/src/relay.rs:407`), and every other exit leaves whatever -was still queued counted: the overflow close (`:391-394`), the `None` receiver (`:398`), a failed -write (`:409-411`), the inbound and close arms (`:414`, `:433-435`, `:440`), the lagged hub (`:460`), the -liveness arms (`:464-469`, `:479-481`) and the final cleanup (`:488-489`). Per socket that leak is invisible -because the counter dies with the sink; a process-wide budget makes it a leak of up to 8 MiB on every -disconnect. Writing the decrement at each of those ten sites is the change that looks right and rots: +was still queued counted: the drain itself (`:384-385`), the refusal close (`:391-394`), the `None` +receiver (`:398`), a failed write (`:409-411`), the inbound arms (`:414`, `:433-435`, `:440`, +`:443-445`), the closed hub (`:460`), the liveness arms (`:463-469`, `:479-481`) and the final +cleanup (`:494-495`); the lagged hub (`:455-457`) warns and does not leave. Per socket that leak is +invisible because the counter dies with the sink; a process-wide budget makes it a leak of up to +8 MiB on every disconnect. Writing the decrement at each of those sites is the change that looks +right and rots: the next exit added to the loop silently leaks, and no unit test can reach the exits that need a failed handshake or a poisoned store to trigger. `Drop` covers all of them by construction, including the registration failure inside `run_connection` — which matters, because a refund that ran only on @@ -207,6 +239,14 @@ This is a gauge, not a window: no store, no clock, no expiry. `RateLimits` and ` unchanged, and the measured 20-socket behaviour — 12 served, 8 dropped by the limiter — is unchanged with it. +**The key is an address, not an identity.** The cap counts source addresses, which is what the +policy's name says, and it holds on the shipped bind: `nostreamd` listens on +`SocketAddr::from((Ipv4Addr::UNSPECIFIED, port))` (`crates/nostreamd/src/main.rs:90`), so every +client has a distinct IPv4 source address. The relay is a library, though, and a listener bound to +`[::]` keys an IPv6 peer by its full address: one host holding a /64 may present 2^64 source +addresses, one socket each, and the cap then bounds nothing. Closing that is a policy about +prefixes, and it is not this job's. + ## What an operator can see today Two accessors in the `hidden_content()` style, `Relay::frame_budget()` and @@ -218,6 +258,19 @@ them: `accounted()`, `limit()`, `refusals()` on the budget; `live(ip)`, `max_per `max_connections_per_ip` and `live` from the connection task (`crates/nostream-relay/src/relay.rs:300-305`). +**Those two lines are not visible at the same level, and the daemon ships `info`.** The filter is +`--log-level`, then `LOG_LEVEL`, then `RUST_LOG`, else `info` +(`crates/nostreamd/src/telemetry.rs:10-23`). At the shipped default an operator therefore sees the +budget's `warn!` and **not** the cap's `debug!`: a socket refused at the cap leaves no line an +operator will find, which is the silent-invisibility failure this job exists to remove, restored on +one of the two paths. Seeing it takes `--log-level debug` (or `LOG_LEVEL=debug`, or a `RUST_LOG` +directive for `nostream_relay`). The reason the line is `debug!` and not something louder is that it +carries per-socket detail — `peer`, `live` — and would be log-amplifiable by exactly the client that +trips it; the process-wide fact belongs to the counter instead. Which leaves the counters as the +surface that does not depend on a log level, and they are readable only from a `Relay` handle today, +i.e. from a test or an embedder. **Making either refusal visible to an operator who has not raised +the filter is an open gap, and #989 or the `/metrics` work is where it closes.** + That is the whole surface, because **a Prometheus endpoint is out of reach right now**: the OTLP exporter is a stub returning `NotImplemented` (`crates/nostreamd/src/telemetry.rs:47-52`) and the admin `/metrics` controller is a Phase-1 stub (`crates/nostream-admin/src/controllers/get_metrics.rs:5-9`). From f688ae4cf9ae5ef5016bd495ab6fbc54604b06b8 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 00:35:24 -0400 Subject: [PATCH 08/26] docs(rewrite): point ADR 0008 at the cap's removal block, not the blank line above it --- docs/rewrite/decisions/0008-frame-memory-budget.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/rewrite/decisions/0008-frame-memory-budget.md b/docs/rewrite/decisions/0008-frame-memory-budget.md index feaad0f5..af08ec49 100644 --- a/docs/rewrite/decisions/0008-frame-memory-budget.md +++ b/docs/rewrite/decisions/0008-frame-memory-budget.md @@ -228,7 +228,7 @@ Acquisition returns a `ConnectionSlot` held for the rest of `handle_socket` `run_connection`, and any early return added later all release it through `Drop` (`crates/nostream-relay/src/connection_cap.rs:101-116`). A count that leaks makes the cap permanent for that address, which is a defect and not a memory leak. The map entry is removed when its count -reaches zero (`:111-114`), so an address that once connected leaves nothing behind and the map is +reaches zero (`:112-114`), so an address that once connected leaves nothing behind and the map is bounded by live addresses. The cap applies to **every address, including whitelisted ones**. An exempt address would bypass the From 5b23eebd40ed43a202aacb06d6150e264df0663a Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 00:47:26 -0400 Subject: [PATCH 09/26] fix(relay): hold a refused read until the close retires it, so the wake-up is what closes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Static round 2 showed the parked-read test passed with the refusal signal deleted: the read returned at the guard, its handler completed, and handlers.join_next() woke the loop. The two changes had cancelled each other's discriminating power, and the close's promptness came from a handler finishing rather than from the signal. A refused read now waits for the close that retires it (the cancellation watch, then end_read) instead of unwinding: the connection loop's abort_all/close stays the single owner of the teardown, and the close is prompt because the refusal woke the loop. The read's own test is restructured for the park — pending after the refusal, one record taken, no EOSE queued, retired by the connection's close — and the two tests now fail on their own mechanism: deleting the refused() arm makes the wire test time out at 2 s, and deleting the read's guard makes the unit test fail with the park message. --- .../nostream-relay/src/handlers/subscribe.rs | 55 ++++++++++++++----- 1 file changed, 41 insertions(+), 14 deletions(-) diff --git a/crates/nostream-relay/src/handlers/subscribe.rs b/crates/nostream-relay/src/handlers/subscribe.rs index 0d95448a..6462ac11 100644 --- a/crates/nostream-relay/src/handlers/subscribe.rs +++ b/crates/nostream-relay/src/handlers/subscribe.rs @@ -273,7 +273,15 @@ async fn fetch_and_send(mut read: InFlightRead, filters: &[Filter], context: &Ha // it, and with a shared budget the loop can be a whole blocked write away from // running, so the window is not short. The sink must not latch on a budget refusal — // a frame that fits later is admitted — which is why the stop lives here. + // + // The read then waits for the close that retires it instead of unwinding: it has + // nothing left to send, and the connection loop's `abort_all` and `close` are the one + // owner of the teardown, so this handler must not run `end_read` against the same + // state while the loop is closing the socket. Parking also keeps the close's + // promptness a property of the refusal signal rather than of this handler happening + // to finish. if context.sink.refusal().is_some() { + let _ = read.cancellation().changed().await; context.connection.end_read(read); return; } @@ -423,6 +431,8 @@ fn can_subscribe( mod tests { #![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + use std::time::Duration; + use nostream_core::clock::TestClock; use nostream_core::event::{EventId, Pubkey, Signature}; use nostream_core::nip42::Nip42Session; @@ -452,11 +462,13 @@ mod tests { #[tokio::test] async fn a_refused_sink_stops_the_read_instead_of_encoding_the_rest_of_it() { - // Three stored events and a budget of 64 bytes: every event frame is refused and the - // read's closing `EOSE` would fit. So the queue says which read ran — a read that - // stopped at the first refusal leaves nothing in it, and a read that encoded and - // discarded the rest of the stream ends with the `EOSE` that fits. + // Three stored events behind a per-item gate, and a 64-byte budget: every event frame + // is refused, so the read must neither take another record nor encode one. The gate is + // the measure — two permits left means one record was taken and the stream was not + // drained — and the queue is the other: a read that ran on to the end would leave the + // 12-byte `EOSE` in it, which does fit. let fixture = Fixture::new(settings()); + let gate = fixture.events.gate_reads_per_item(); for index in 0..3 { fixture @@ -467,13 +479,14 @@ mod tests { let budget = Arc::new(FrameBudget::new(64)); let (sink, mut frames) = FrameSink::channel(64 * 1024, Some(Arc::clone(&budget))); let refused = sink.clone(); + let connection = Arc::new(Connection::new( + "127.0.0.1".to_owned(), + Nip42Session::with_challenge("challenge", 0), + )); let context = Arc::new(fixture.context( Arc::new(TestClock::new(1_700_000_000_000)), RelayKeySource::from_values(None, None), - Arc::new(Connection::new( - "127.0.0.1".to_owned(), - Nip42Session::with_challenge("challenge", 0), - )), + Arc::clone(&connection), sink, )); @@ -485,19 +498,33 @@ mod tests { }], }; - let read = begin(message, &context).expect("the subscription is registered"); - read.await; + let mut read = begin(message, &context).expect("the subscription is registered"); + gate.add_permits(3); - assert_eq!(refused.refusal(), Some(Refusal::BudgetExhausted)); - assert_eq!(refused.queued_bytes(), 0); assert!( - frames.try_recv().is_err(), - "the read stopped at the first refusal: no event frame and no EOSE reached the queue" + tokio::time::timeout(Duration::from_millis(200), &mut read) + .await + .is_err(), + "the read parks once its frame is refused instead of draining the stream" ); + assert_eq!(refused.refusal(), Some(Refusal::BudgetExhausted)); + assert_eq!( + gate.available_permits(), + 2, + "one record was taken and the rest of the stream was never asked for" + ); + assert!(frames.try_recv().is_err(), "no EOSE reached the queue"); + assert_eq!(refused.queued_bytes(), 0); assert_eq!( budget.refusals(), 1, "one refusal for the socket, not one per event" ); + + // The close the read is waiting for ends it. + connection.close(); + tokio::time::timeout(Duration::from_secs(1), read) + .await + .expect("the connection's close ends the read"); } } From ff144dc08dfed2287dbea5ca352a5e8d7a86d821 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 01:07:29 -0400 Subject: [PATCH 10/26] docs(rewrite): say that a refused read waits for the close, and what that holds Round 2 (code, security, architecture) found one ADR paragraph that 5b23eeb had made false, the code comment copying it, and an ownership claim one clause too strong. - The read-stop paragraph said a refused read 'ends there'. It stops producing there and then waits for the close that retires it, so it now says that, names what a refused client therefore still holds, and no longer contradicts its own account of the two 30 s write windows. - The encode citation moved from the park comment to the send (subscribe.rs:325-330), and the guard is cited where it is (:284-288). - 'its read parked on the store' in the wake-up paragraph was the pre-1479c6c state; the relay.rs comment copied it. Both now say stopped and waiting for the close. - Section 7 records that the park holds the store stream to the close: latent today (unavailable::stores), but sqlx's &Pool executor holds a pooled connection per stream and DB_MAX_POOL_SIZE is 3, so whoever wires the store leaves the loop before parking. - The ownership comment claimed the loop's abort_all/close was the sole owner of teardown while end_read is reachable from six places. It now says what makes that safe: end_read removes the entry only while the generation still matches (connection.rs:290-297). Every line reference in the ADR was re-read from the final tree: 53 of them, all resolving. --- .../nostream-relay/src/handlers/subscribe.rs | 11 ++-- crates/nostream-relay/src/relay.rs | 4 +- .../decisions/0008-frame-memory-budget.md | 52 +++++++++++++------ 3 files changed, 44 insertions(+), 23 deletions(-) diff --git a/crates/nostream-relay/src/handlers/subscribe.rs b/crates/nostream-relay/src/handlers/subscribe.rs index 6462ac11..cd29b1ba 100644 --- a/crates/nostream-relay/src/handlers/subscribe.rs +++ b/crates/nostream-relay/src/handlers/subscribe.rs @@ -275,11 +275,12 @@ async fn fetch_and_send(mut read: InFlightRead, filters: &[Filter], context: &Ha // a frame that fits later is admitted — which is why the stop lives here. // // The read then waits for the close that retires it instead of unwinding: it has - // nothing left to send, and the connection loop's `abort_all` and `close` are the one - // owner of the teardown, so this handler must not run `end_read` against the same - // state while the loop is closing the socket. Parking also keeps the close's - // promptness a property of the refusal signal rather than of this handler happening - // to finish. + // nothing left to send, and parking keeps the close's promptness a property of the + // refusal signal rather than of this handler happening to finish. The `end_read` + // below is the same call every other exit of this function makes, and it is safe + // beside the loop's own `close` because it removes the entry only while the generation + // it carries still matches (`connection.rs:290-297`): whichever of the two runs + // second finds no entry of its own and does nothing. if context.sink.refusal().is_some() { let _ = read.cancellation().changed().await; context.connection.end_read(read); diff --git a/crates/nostream-relay/src/relay.rs b/crates/nostream-relay/src/relay.rs index 2ec579ac..c776c025 100644 --- a/crates/nostream-relay/src/relay.rs +++ b/crates/nostream-relay/src/relay.rs @@ -483,8 +483,8 @@ impl Relay { _ = drained.changed() => drain_seen = *drained.borrow_and_update(), // A refusal can arrive with nothing else to wake this loop: the budget is // shared, so the frame that did not fit may have left this socket's own queue - // empty, the socket itself idle, and its read parked on the store. The - // producer signals when it records the refusal, and the next iteration's + // empty, the socket idle, and its read stopped and waiting for this close. + // The producer signals when it records the refusal, and the next iteration's // check closes the client — immediate, not one liveness tick away (ADR 0008). _ = sink.refused() => {} Some(_) = handlers.join_next(), if !handlers.is_empty() => {} diff --git a/docs/rewrite/decisions/0008-frame-memory-budget.md b/docs/rewrite/decisions/0008-frame-memory-budget.md index af08ec49..58bd5e89 100644 --- a/docs/rewrite/decisions/0008-frame-memory-budget.md +++ b/docs/rewrite/decisions/0008-frame-memory-budget.md @@ -125,25 +125,33 @@ at its next iteration whatever the sender does. The accounting therefore stays e choice, and the unit test that proves room lets the next frame through (`sink.rs::the_budget_binds_across_sockets_and_is_shared`) can say so. -**The refusal stops the reader as well.** The connection loop closes the socket at its next -iteration, but a stored read is a task of its own and keeps producing events until it notices: -`fetch_and_send` (`crates/nostream-relay/src/handlers/subscribe.rs:276-279`) encodes one frame per event, and -each refused frame is encoded and discarded. The window is not short — the loop can be parked in a -write and then in `close_frame`, each bounded only by the 30 s `write_timeout`, and -`handlers.abort_all()` runs only after both — and it is the normal case, because the budget binds -exactly when queues are full and loops are parked in writes. So the read checks -`context.sink.refusal()` at the top of its loop and ends there, before pulling or encoding the next -record, with no `EOSE` (there is no socket left to tell). The sink itself must not latch a budget -refusal — a frame that fits later is admitted, which the shared-budget test relies on — so the stop -belongs to the reader. `subscribe.rs::a_refused_sink_stops_the_read_instead_of_encoding_the_rest_of_it` -holds it down: with a 64-byte budget every event frame is refused and the `EOSE` would fit, so the -queue stays empty only if the read stopped at the first refusal (without the check it holds the -12-byte `EOSE`). +**The refusal stops the reader too, and the reader then waits for the close.** The connection loop +closes the socket at its next iteration, but a stored read is a task of its own and keeps producing +events until it notices: `fetch_and_send` encodes one frame per event +(`crates/nostream-relay/src/handlers/subscribe.rs:325-330`), and each refused frame was encoded and +discarded. The window is not short — the loop can be parked in a write and then in `close_frame`, +each bounded only by the 30 s `write_timeout`, and `handlers.abort_all()` runs only after both — and +it is the normal case, because the budget binds exactly when queues are full and loops are parked in +writes. So the read checks `context.sink.refusal()` at the top of its loop, before pulling or +encoding the next record (`crates/nostream-relay/src/handlers/subscribe.rs:284-288`), and sends +nothing more: no further event, and no `EOSE`, which would be a frame this client is not reading. + +What the read does **not** do is finish there. It waits for the close that retires it — the +cancellation watch, then `end_read` — and what delivers that close is `handlers.abort_all()` and +`connection.close()` on the loop's way out (`crates/nostream-relay/src/relay.rs:494-495`). So a +refused client at this head holds two things until its socket actually closes: the read task, and +the store stream inside it (§7 records the second). The sink itself must not latch a budget refusal — +a frame that fits later is admitted, which the shared-budget test relies on — so the stop belongs to +the reader. `subscribe.rs::a_refused_sink_stops_the_read_instead_of_encoding_the_rest_of_it` holds it +down: with a 64-byte budget every event frame is refused and the `EOSE` would fit, so the queue stays +empty only if the read stopped at the first refusal, and the store's per-item gate shows it asked for +exactly one record (without the check it drains the stream and holds the 12-byte `EOSE`). **The refusal wakes the loop.** Recording the cause is not enough on its own. A budget refusal can arrive while *this* socket's queue is empty — the bytes that left no room are another -socket's — with the socket idle and its read parked on the store, and then nothing in the -connection task is ready: no frame to write, no inbound message, no handler finishing. The +socket's — with the socket idle and the read that lost the frame waiting for the very close that +recording the refusal is supposed to start, and then nothing in the connection task is ready: no +frame to write, no inbound message, no handler finishing. The account owns a `tokio::sync::Notify`, the first refusal signals it, and `run_connection` selects on `FrameSink::refused()`, so the close is one iteration away rather than one liveness interval away. `notify_one` stores a permit when nothing is waiting, which is what makes a refusal @@ -210,6 +218,18 @@ The drain arm's `release` is unchanged and still runs: it returns written bytes budget is reusable while the socket lives, and the `Drop` refund then finds only what was never written. +**The parked read holds its store stream, and that is what must change when a store is wired.** +Stopping at the refusal without leaving the loop keeps the stream from `find_by_filters` alive until +the close retires the read; the version before this one dropped it at the refusal. Latent today — the +daemon wires `unavailable::stores()` (`crates/nostreamd/src/main.rs:113`), whose reads are stubs — +but not for ever: sqlx's `&Pool` executor draws a pooled connection for as long as a query stream is +alive, and `DB_MAX_POOL_SIZE` defaults to **3** (`crates/nostream-storage/src/postgres/mod.rs:362`), +so three refused reads parked at once would hold every connection in the pool and stall every other +read in the process. Whoever wires the store leaves the loop before parking — returning from +`fetch_and_send`, or dropping the stream first — and in doing so takes the close's promptness back +onto the refusal signal, because a handler that returns is a handler whose completion wakes the loop +(static round 2 is what that looked like). The choice is recorded here so it is made on purpose. + ## The per-IP cap A `ConnectionCap` (`crates/nostream-relay/src/connection_cap.rs`) counts **live sockets** per From f67e726555d6a2f9ba8cfce72725c71f852958fd Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 01:07:47 -0400 Subject: [PATCH 11/26] docs(rewrite): record the park's test-power trade the right way round Returning from the loop before parking does not put the close's promptness onto the refusal signal - it removes the signal's necessity, because a returned handler wakes the loop on completion. Say that, and that the wire test stops discriminating in that world. --- docs/rewrite/decisions/0008-frame-memory-budget.md | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/docs/rewrite/decisions/0008-frame-memory-budget.md b/docs/rewrite/decisions/0008-frame-memory-budget.md index 58bd5e89..3cbd5502 100644 --- a/docs/rewrite/decisions/0008-frame-memory-budget.md +++ b/docs/rewrite/decisions/0008-frame-memory-budget.md @@ -226,9 +226,11 @@ but not for ever: sqlx's `&Pool` executor draws a pooled connection for as long alive, and `DB_MAX_POOL_SIZE` defaults to **3** (`crates/nostream-storage/src/postgres/mod.rs:362`), so three refused reads parked at once would hold every connection in the pool and stall every other read in the process. Whoever wires the store leaves the loop before parking — returning from -`fetch_and_send`, or dropping the stream first — and in doing so takes the close's promptness back -onto the refusal signal, because a handler that returns is a handler whose completion wakes the loop -(static round 2 is what that looked like). The choice is recorded here so it is made on purpose. +`fetch_and_send`, or dropping the stream first — and pays for it in test power: a handler that +returns wakes the loop on completion, so the close arrives promptly even with the refusal signal +deleted, and the wire test below then holds down nothing (static round 2 is exactly what that looked +like). That trade — a pooled connection against the signal being load-bearing — is recorded here so +it is made on purpose rather than by accident. ## The per-IP cap From 04186d4e7dea8b4a6cf72fa2d8b1b7679d91cd74 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 01:22:28 -0400 Subject: [PATCH 12/26] docs(rewrite): say that the abort, not the watch, retires a refused read Round 3 (code, architecture) measured the same clause independently: every connection-loop exit is a break, handlers.abort_all() runs before connection.close(), and a JoinSet-aborted task is dropped without being polled again. So on the refusal path the parked read dies inside the cancellation watch, its end_read never runs, and the store stream goes with the abort; the watch is the path a client CLOSE or a re-REQ takes, and the one the unit test exercises. Also: the refused frame is the only one encoded per read (and a queue-latched socket skips that too), the wire-test pointer no longer says 'below' with nothing below it, and section 7 gives the deployed pool size (prod compose 64, no replica, so held connections stall writes as well) beside the code default of 3. The two code comments that described the watch as the retirement now match. --- .../nostream-relay/src/handlers/subscribe.rs | 15 ++-- crates/nostream-relay/src/relay.rs | 2 +- .../decisions/0008-frame-memory-budget.md | 73 +++++++++++-------- 3 files changed, 53 insertions(+), 37 deletions(-) diff --git a/crates/nostream-relay/src/handlers/subscribe.rs b/crates/nostream-relay/src/handlers/subscribe.rs index cd29b1ba..6644a8f3 100644 --- a/crates/nostream-relay/src/handlers/subscribe.rs +++ b/crates/nostream-relay/src/handlers/subscribe.rs @@ -274,13 +274,14 @@ async fn fetch_and_send(mut read: InFlightRead, filters: &[Filter], context: &Ha // running, so the window is not short. The sink must not latch on a budget refusal — // a frame that fits later is admitted — which is why the stop lives here. // - // The read then waits for the close that retires it instead of unwinding: it has - // nothing left to send, and parking keeps the close's promptness a property of the - // refusal signal rather than of this handler happening to finish. The `end_read` - // below is the same call every other exit of this function makes, and it is safe - // beside the loop's own `close` because it removes the entry only while the generation - // it carries still matches (`connection.rs:290-297`): whichever of the two runs - // second finds no entry of its own and does nothing. + // The read then parks instead of unwinding: it has nothing left to send, and parking + // keeps the close's promptness a property of the refusal signal rather than of this + // handler happening to finish. On the refusal path the loop's `abort_all` drops the + // parked task before the watch below ever resolves, so the `end_read` after it is the + // path a client `CLOSE` or a re-`REQ` takes, not the refusal's. It is the same call + // every other exit of this function makes, and it is safe beside the loop's own + // `close` because it removes the entry only while the generation it carries still + // matches (`connection.rs:290-297`): whichever of the two runs second does nothing. if context.sink.refusal().is_some() { let _ = read.cancellation().changed().await; context.connection.end_read(read); diff --git a/crates/nostream-relay/src/relay.rs b/crates/nostream-relay/src/relay.rs index c776c025..2dcd1f3b 100644 --- a/crates/nostream-relay/src/relay.rs +++ b/crates/nostream-relay/src/relay.rs @@ -483,7 +483,7 @@ impl Relay { _ = drained.changed() => drain_seen = *drained.borrow_and_update(), // A refusal can arrive with nothing else to wake this loop: the budget is // shared, so the frame that did not fit may have left this socket's own queue - // empty, the socket idle, and its read stopped and waiting for this close. + // empty, the socket idle, and its read stopped and parked with nothing to run it. // The producer signals when it records the refusal, and the next iteration's // check closes the client — immediate, not one liveness tick away (ADR 0008). _ = sink.refused() => {} diff --git a/docs/rewrite/decisions/0008-frame-memory-budget.md b/docs/rewrite/decisions/0008-frame-memory-budget.md index 3cbd5502..962b8814 100644 --- a/docs/rewrite/decisions/0008-frame-memory-budget.md +++ b/docs/rewrite/decisions/0008-frame-memory-budget.md @@ -125,26 +125,37 @@ at its next iteration whatever the sender does. The accounting therefore stays e choice, and the unit test that proves room lets the next frame through (`sink.rs::the_budget_binds_across_sockets_and_is_shared`) can say so. -**The refusal stops the reader too, and the reader then waits for the close.** The connection loop +**The refusal stops the reader too, and the abort eventually retires it.** The connection loop closes the socket at its next iteration, but a stored read is a task of its own and keeps producing events until it notices: `fetch_and_send` encodes one frame per event -(`crates/nostream-relay/src/handlers/subscribe.rs:325-330`), and each refused frame was encoded and -discarded. The window is not short — the loop can be parked in a write and then in `close_frame`, -each bounded only by the 30 s `write_timeout`, and `handlers.abort_all()` runs only after both — and -it is the normal case, because the budget binds exactly when queues are full and loops are parked in -writes. So the read checks `context.sink.refusal()` at the top of its loop, before pulling or -encoding the next record (`crates/nostream-relay/src/handlers/subscribe.rs:284-288`), and sends -nothing more: no further event, and no `EOSE`, which would be a frame this client is not reading. - -What the read does **not** do is finish there. It waits for the close that retires it — the -cancellation watch, then `end_read` — and what delivers that close is `handlers.abort_all()` and -`connection.close()` on the loop's way out (`crates/nostream-relay/src/relay.rs:494-495`). So a -refused client at this head holds two things until its socket actually closes: the read task, and -the store stream inside it (§7 records the second). The sink itself must not latch a budget refusal — -a frame that fits later is admitted, which the shared-budget test relies on — so the stop belongs to -the reader. `subscribe.rs::a_refused_sink_stops_the_read_instead_of_encoding_the_rest_of_it` holds it -down: with a 64-byte budget every event frame is refused and the `EOSE` would fit, so the queue stays -empty only if the read stopped at the first refusal, and the store's per-item gate shows it asked for +(`crates/nostream-relay/src/handlers/subscribe.rs:326-331`), so a frame that does not fit has been +encoded by the time the sink refuses it. The window is not short — the loop can be parked in a write +and then in `close_frame`, each bounded only by the 30 s `write_timeout`, and `handlers.abort_all()` +runs only after both — and it is the normal case, because the budget binds exactly when queues are +full and loops are parked in writes. So the read checks `context.sink.refusal()` at the top of its +loop, before pulling or encoding the next record +(`crates/nostream-relay/src/handlers/subscribe.rs:285-289`), and sends nothing more: no further +event, and no `EOSE`, which would be a frame this client is not reading. That leaves the frame that +was refused as the only one encoded and thrown away per read — one, not one per event — and a socket +the queue bound has already latched does not even pay that, because `send` returns before `to_text()` +(`crates/nostream-relay/src/sink.rs:260-266`). + +The read does not finish there: it waits on the read's cancellation watch and then calls `end_read`. +**On the refusal path that wait is never resolved.** Every exit of the connection loop is a `break`, +`handlers.abort_all()` runs before `connection.close()` (`crates/nostream-relay/src/relay.rs:494-495`), +and a task aborted by the `JoinSet` it was spawned into (`crates/nostream-relay/src/relay.rs:377`, +`:602-603`) is dropped without being polled again: the parked read is dropped inside the watch, the +`end_read` below it never runs, and the store stream goes with it. What retires a refused read in +production is therefore the abort, not the watch — the watch is the path a client can still take, by +a `CLOSE` or a re-`REQ`, and it is the one the unit test exercises, because the test closes the +connection rather than aborting the task. So a refused client holds two things until the loop aborts +it, a close frame later: the read task, and the store stream inside it (§7 records the second). + +The sink itself must not latch a budget refusal — a frame that fits later is admitted, which the +shared-budget test relies on — so the stop belongs to the reader. +`subscribe.rs::a_refused_sink_stops_the_read_instead_of_encoding_the_rest_of_it` holds it down with a +64-byte budget, where every event frame is refused and the `EOSE` would fit: the queue stays empty +only if the read stopped at the first refusal, and the store's per-item gate shows it asked for exactly one record (without the check it drains the stream and holds the 12-byte `EOSE`). **The refusal wakes the loop.** Recording the cause is not enough on its own. A budget refusal @@ -220,17 +231,21 @@ written. **The parked read holds its store stream, and that is what must change when a store is wired.** Stopping at the refusal without leaving the loop keeps the stream from `find_by_filters` alive until -the close retires the read; the version before this one dropped it at the refusal. Latent today — the -daemon wires `unavailable::stores()` (`crates/nostreamd/src/main.rs:113`), whose reads are stubs — -but not for ever: sqlx's `&Pool` executor draws a pooled connection for as long as a query stream is -alive, and `DB_MAX_POOL_SIZE` defaults to **3** (`crates/nostream-storage/src/postgres/mod.rs:362`), -so three refused reads parked at once would hold every connection in the pool and stall every other -read in the process. Whoever wires the store leaves the loop before parking — returning from -`fetch_and_send`, or dropping the stream first — and pays for it in test power: a handler that -returns wakes the loop on completion, so the close arrives promptly even with the refusal signal -deleted, and the wire test below then holds down nothing (static round 2 is exactly what that looked -like). That trade — a pooled connection against the signal being load-bearing — is recorded here so -it is made on purpose rather than by accident. +the loop aborts the read, a close frame later; the version before this one dropped it at the refusal. +Latent today — the daemon wires `unavailable::stores()` (`crates/nostreamd/src/main.rs:113`), whose +reads are stubs — but not for ever: sqlx's `&Pool` executor draws a pooled connection for as long as a +query stream is alive. The size that matters is the deployed one, not the code's: `DB_MAX_POOL_SIZE` +defaults to **3** (`crates/nostream-storage/src/postgres/mod.rs:362`) but +`deploy/docker-compose.prod.yml` sets **64** (`:16`) and configures no replica, so the connection a +refused read holds is one the master cannot serve a *write* with either — and the pool goes at one +connection per parked read: three read only because the code default is 3, sixty-four because the +deployed one is 64. Whoever wires the store leaves the +loop before parking — returning from `fetch_and_send`, or dropping the stream first — and pays for it +in test power: a handler that returns wakes the loop on completion, so the close arrives promptly +even with the refusal signal deleted, and the wire test in the wake-up paragraph above then holds +down nothing (static round 2 is exactly what that looked like). That trade — a pooled connection +against the signal being load-bearing — is recorded here so it is made on purpose rather than by +accident. ## The per-IP cap From 82ade5805c71aa291d8f86477973c5b9335a87ae Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 02:05:38 -0400 Subject: [PATCH 13/26] feat(config): expose the frame budget and per-IP cap as settings keys --- Cargo.lock | 1 + crates/nostream-config/src/guided.rs | 12 ++++ crates/nostream-config/src/settings.rs | 4 ++ crates/nostream-relay/Cargo.toml | 1 + crates/nostream-relay/src/relay.rs | 31 ++++++++++ .../nostream-relay/tests/config_defaults.rs | 56 +++++++++++++++++++ crates/nostream-relay/tests/handlers.rs | 1 + crates/nostream-relay/tests/limits.rs | 2 + crates/nostreamd/src/main.rs | 13 +++-- crates/nostreamd/tests/check_flag.rs | 21 +++++++ resources/default-settings.yaml | 2 + 11 files changed, 138 insertions(+), 6 deletions(-) create mode 100644 crates/nostream-relay/tests/config_defaults.rs diff --git a/Cargo.lock b/Cargo.lock index eb3ccdf0..e68fdad8 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1203,6 +1203,7 @@ dependencies = [ "nostream-core", "nostream-storage", "serde_json", + "serde_yaml", "sha2 0.11.1", "thiserror", "tokio", diff --git a/crates/nostream-config/src/guided.rs b/crates/nostream-config/src/guided.rs index 5402a2a3..8d6183a1 100644 --- a/crates/nostream-config/src/guided.rs +++ b/crates/nostream-config/src/guided.rs @@ -206,6 +206,12 @@ fn build_categories() -> Vec { Number, SafeNonNegativeInteger, ), + validated( + "Frame memory budget (bytes)", + "network.frameMemoryBudgetBytes", + Number, + SafeNonNegativeInteger, + ), validated( "Worker count", "workers.count", @@ -263,6 +269,12 @@ fn build_categories() -> Vec { Number, SafeNonNegativeInteger, ), + validated( + "Max connections per IP", + "limits.connection.maxConnectionsPerIp", + Number, + SafeNonNegativeInteger, + ), validated( "Max filters per subscription", "limits.client.subscription.maxFilters", diff --git a/crates/nostream-config/src/settings.rs b/crates/nostream-config/src/settings.rs index d76a7290..c551cdba 100644 --- a/crates/nostream-config/src/settings.rs +++ b/crates/nostream-config/src/settings.rs @@ -68,6 +68,8 @@ pub struct Network { #[serde(skip_serializing_if = "Option::is_none")] pub max_payload_size: Option, #[serde(skip_serializing_if = "Option::is_none")] + pub frame_memory_budget_bytes: Option, + #[serde(skip_serializing_if = "Option::is_none")] pub remote_ip_header: Option, #[serde(skip_serializing_if = "Option::is_none")] pub trusted_proxies: Option>, @@ -306,6 +308,8 @@ pub struct ConnectionLimits { pub ip_whitelist: Option>, #[serde(skip_serializing_if = "Option::is_none")] pub ip_blacklist: Option>, + #[serde(skip_serializing_if = "Option::is_none")] + pub max_connections_per_ip: Option, } /// `limits.invoice` (`src/@types/settings.ts` `InvoiceLimits`). diff --git a/crates/nostream-relay/Cargo.toml b/crates/nostream-relay/Cargo.toml index 3dd1371f..d153dd90 100644 --- a/crates/nostream-relay/Cargo.toml +++ b/crates/nostream-relay/Cargo.toml @@ -24,6 +24,7 @@ tracing.workspace = true [dev-dependencies] base64.workspace = true +serde_yaml.workspace = true [lints] workspace = true diff --git a/crates/nostream-relay/src/relay.rs b/crates/nostream-relay/src/relay.rs index 2dcd1f3b..1e07e51d 100644 --- a/crates/nostream-relay/src/relay.rs +++ b/crates/nostream-relay/src/relay.rs @@ -96,6 +96,37 @@ impl Default for RelayConfig { } } +impl RelayConfig { + /// The configuration a relay built from `settings` runs with: [`RelayConfig::default`] for + /// every knob the capture fixes in code, with the two bounds an operator sets read from + /// their leaves — `network.frameMemoryBudgetBytes` and + /// `limits.connection.maxConnectionsPerIp` — when present and non-zero. + /// + /// This is the one place either setting is interpreted, so `nostreamd` and the tests cannot + /// disagree about it. `drain_timeout` stays the composition root's to resolve: it is an + /// environment read (ADR 0004), not a setting. + pub fn from_settings(settings: &Settings) -> Self { + let mut config = Self::default(); + if let Some(bytes) = settings.network.frame_memory_budget_bytes + && let Ok(bytes) = usize::try_from(bytes) + && bytes > 0 + { + config.frame_memory_budget_bytes = bytes; + } + if let Some(cap) = settings + .limits + .as_ref() + .and_then(|limits| limits.connection.as_ref()) + .and_then(|connection| connection.max_connections_per_ip) + && let Ok(cap) = usize::try_from(cap) + && cap > 0 + { + config.max_connections_per_ip = cap; + } + config + } +} + /// The relay's shared state: one value handed to every connection task. pub struct Relay { settings: Arc, diff --git a/crates/nostream-relay/tests/config_defaults.rs b/crates/nostream-relay/tests/config_defaults.rs new file mode 100644 index 00000000..8c6d8524 --- /dev/null +++ b/crates/nostream-relay/tests/config_defaults.rs @@ -0,0 +1,56 @@ +//! `RelayConfig::default()` and the shipped defaults document must state the same two bounds. +//! The document is what an operator reads and edits; the compiled default is what a relay runs +//! on when the key is absent. A divergence is silent in both directions, so it is asserted here +//! rather than left to whichever of the two a reader happens to open (ADR 0008, #989). + +// This integration-test target may unwrap and panic: a failed assertion here is a failed test, +// which is the point. The allowance names this file rather than exempting test targets +// workspace-wide, so a new test file still has to ask. +#![allow(clippy::unwrap_used, clippy::expect_used, clippy::panic)] + +use std::path::PathBuf; + +use nostream_config::settings::Settings; +use nostream_relay::relay::RelayConfig; + +fn defaults_settings() -> Settings { + let path = + PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("../../resources/default-settings.yaml"); + let raw = std::fs::read_to_string(&path).unwrap_or_else(|error| { + panic!( + "the shipped defaults {} are readable: {error}", + path.display() + ) + }); + serde_yaml::from_str(&raw).expect("the shipped defaults are valid YAML") +} + +#[test] +fn the_compiled_bounds_equal_the_shipped_defaults() { + let shipped = defaults_settings(); + let config = RelayConfig::default(); + + let budget = shipped + .network + .frame_memory_budget_bytes + .expect("the shipped defaults carry network.frameMemoryBudgetBytes"); + let cap = shipped + .limits + .as_ref() + .and_then(|limits| limits.connection.as_ref()) + .and_then(|connection| connection.max_connections_per_ip) + .expect("the shipped defaults carry limits.connection.maxConnectionsPerIp"); + + assert_eq!( + config.frame_memory_budget_bytes as u64, budget, + "network.frameMemoryBudgetBytes is {budget} in the defaults document and \ + {} in RelayConfig::default()", + config.frame_memory_budget_bytes + ); + assert_eq!( + config.max_connections_per_ip as u64, cap, + "limits.connection.maxConnectionsPerIp is {cap} in the defaults document and \ + {} in RelayConfig::default()", + config.max_connections_per_ip + ); +} diff --git a/crates/nostream-relay/tests/handlers.rs b/crates/nostream-relay/tests/handlers.rs index ffaec0c6..695a67d6 100644 --- a/crates/nostream-relay/tests/handlers.rs +++ b/crates/nostream-relay/tests/handlers.rs @@ -1715,6 +1715,7 @@ async fn the_connection_limiter_terminates_the_upgrade() { }], ip_whitelist: None, ip_blacklist: None, + max_connections_per_ip: None, }), ..Default::default() }; diff --git a/crates/nostream-relay/tests/limits.rs b/crates/nostream-relay/tests/limits.rs index f0431175..b6ed1ece 100644 --- a/crates/nostream-relay/tests/limits.rs +++ b/crates/nostream-relay/tests/limits.rs @@ -677,6 +677,7 @@ fn the_connection_level_uses_addr_connection_period_and_its_whitelist() { }], ip_whitelist: Some(vec!["10.0.0.1".to_owned()]), ip_blacklist: None, + max_connections_per_ip: None, }), ..Default::default() }); @@ -913,6 +914,7 @@ fn a_failing_store_reports_an_error_at_every_level() { }], ip_whitelist: None, ip_blacklist: None, + max_connections_per_ip: None, }), message: Some(MessageLimits { rate_limits: Some(vec![MessageRateLimit { diff --git a/crates/nostreamd/src/main.rs b/crates/nostreamd/src/main.rs index a5937dd1..987a5a17 100644 --- a/crates/nostreamd/src/main.rs +++ b/crates/nostreamd/src/main.rs @@ -11,7 +11,7 @@ use clap::Parser; use nostream_config::SettingsPaths; use nostream_core::clock::SystemClock; use nostream_core::lifecycle::ShutdownState; -use nostream_relay::{Relay, relay_key::RelayKeySource, unavailable}; +use nostream_relay::{Relay, relay::RelayConfig, relay_key::RelayKeySource, unavailable}; use nostreamd::{DaemonError, settings_gate, telemetry}; #[derive(Debug, Parser)] @@ -106,6 +106,11 @@ async fn serve( let acceptor = nostreamd::Acceptor::bind(listen).await?; let shutdown = ShutdownState::new(); + // The two bounds are the operator's; `from_settings` is where they are read. + let mut relay_config = RelayConfig::from_settings(&settings); + // The one environment read: the drain deadline is the operator's. + relay_config.drain_timeout = nostream_relay::transport::drain_timeout(); + // The storage backend is not wired yet (the LMDB environment is a stub), so the relay // runs over seams that report `NotImplemented`; the wire itself is served in full. let relay = Relay::new( @@ -116,11 +121,7 @@ async fn serve( std::sync::Arc::new(SystemClock), RelayKeySource::from_env(), std::sync::Arc::new(nostream_core::random::SystemRandom), - // The one environment read: the drain deadline is the operator's. - nostream_relay::relay::RelayConfig { - drain_timeout: nostream_relay::transport::drain_timeout(), - ..Default::default() - }, + relay_config, ); tracing::info!(at = %acceptor.local_addr(), "the acceptor is listening"); diff --git a/crates/nostreamd/tests/check_flag.rs b/crates/nostreamd/tests/check_flag.rs index 2297377f..8680eb2a 100644 --- a/crates/nostreamd/tests/check_flag.rs +++ b/crates/nostreamd/tests/check_flag.rs @@ -129,3 +129,24 @@ fn check_refuses_a_settings_yaml_that_is_a_directory() { assert_ne!(code, Some(0), "a directory must not read as a clean config"); assert!(stderr.contains("not a regular file"), "stderr: {stderr}"); } + +#[test] +fn check_accepts_an_operator_file_that_sets_both_bounds() { + // The two bounds #959 added are settings an operator writes, so the document has to carry + // their keys: a value the validator does not know is refused as `Unknown setting key`, and + // the daemon would keep running on the compiled defaults without saying so. + let directory = tempfile::tempdir().expect("tempdir"); + std::fs::write( + directory.path().join("settings.yaml"), + "network:\n frameMemoryBudgetBytes: 134217728\nlimits:\n connection:\n maxConnectionsPerIp: 128\n", + ) + .expect("write"); + + let config_dir = directory.path().to_string_lossy().into_owned(); + let (code, stdout, stderr) = run_check( + Duration::from_secs(30), + &["--check", "--config-dir", &config_dir], + ); + assert_eq!(code, Some(0), "exit code; stderr: {stderr}"); + assert_eq!(stdout.trim(), "settings valid: 0 issues"); +} diff --git a/resources/default-settings.yaml b/resources/default-settings.yaml index 115ea845..fa208859 100755 --- a/resources/default-settings.yaml +++ b/resources/default-settings.yaml @@ -150,6 +150,7 @@ nip56: hideActionableReports: false network: maxPayloadSize: 524288 + frameMemoryBudgetBytes: 268435456 # Uncomment only when using a trusted reverse proxy and configuring trustedProxies. # remoteIpHeader: x-forwarded-for # remoteIpHeader: cf-connecting-ip @@ -213,6 +214,7 @@ limits: - "10.10.10.1" - "::ffff:10.10.10.1" connection: + maxConnectionsPerIp: 64 rateLimits: - period: 1000 rate: 12 From 717e4cbf8433a10a91b45f80de86dd636ae74506 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 02:09:56 -0400 Subject: [PATCH 14/26] docs(rewrite): record the two bounds as operator settings, and what a zero means --- .changeset/relay-frame-budget-settings.md | 31 ++++++ CONFIGURATION.md | 2 + .../decisions/0005-connection-concurrency.md | 17 +++- .../decisions/0008-frame-memory-budget.md | 95 ++++++++++++++++--- resources/default-settings.yaml | 4 + 5 files changed, 133 insertions(+), 16 deletions(-) create mode 100644 .changeset/relay-frame-budget-settings.md diff --git a/.changeset/relay-frame-budget-settings.md b/.changeset/relay-frame-budget-settings.md new file mode 100644 index 00000000..a19bc051 --- /dev/null +++ b/.changeset/relay-frame-budget-settings.md @@ -0,0 +1,31 @@ +--- +--- + +feat(config): expose the frame budget and the per-IP cap as settings keys + +The two bounds #959 added were `RelayConfig` fields only: +"pub frame_memory_budget_bytes: usize," (`crates/nostream-relay/src/relay.rs:65`) and +"pub max_connections_per_ip: usize," (`crates/nostream-relay/src/relay.rs:69`). A running +daemon could not change either number without a rebuild, which is what #989 is about. + +Both are now settings the operator writes: + +- `network.frameMemoryBudgetBytes` in `resources/default-settings.yaml` — the byte count every + socket's queued outbound payload is counted against. +- `limits.connection.maxConnectionsPerIp` in `resources/default-settings.yaml` — the live + sockets one source address may hold at once. + +`RelayConfig::from_settings` reads both leaves, in the one place either is interpreted, and +falls back to the compiled default when a leaf is absent or zero: +"if let Some(bytes) = settings.network.frame_memory_budget_bytes" +(`crates/nostream-relay/src/relay.rs:110`). The daemon builds its config through it: +"let mut relay_config = RelayConfig::from_settings(&settings);" +(`crates/nostreamd/src/main.rs:110`), keeping its one environment read. + +The defaults document's values equal `RelayConfig::default()`'s, and a test in +`crates/nostream-relay/tests/config_defaults.rs` holds the two together. No validator rule is +added: the document is the validator's schema, so the two keys alone make an operator's file +valid. See ADR 0008 for what a zero means and why. + +The TypeScript tree is untouched: no `src/` or `test/` file changes, so the published package +has no user-visible change. diff --git a/CONFIGURATION.md b/CONFIGURATION.md index 0cdd8360..d311f35f 100644 --- a/CONFIGURATION.md +++ b/CONFIGURATION.md @@ -161,6 +161,7 @@ The settings below are listed in alphabetical order by name. Please keep this ta | limits.client.subscription.maxFilters | Maximum number of filters per subscription. Defaults to 10. Disabled when set to zero. | | limits.client.subscription.maxFilterValues | Maximum number of values allowed in each filter, counting its array criteria (`ids`, `authors`, `kinds`, `#`) together. Defaults to 2500. Disabled when set to zero. | | limits.client.subscription.maxSubscriptions | Maximum number of subscriptions per connected client. Defaults to 10. Disabled when set to zero. | +| limits.connection.maxConnectionsPerIp | Maximum number of live sockets one source address may hold at once; a socket above the count is closed with `1013` and the reason `connection cap: too many connections from this address`. Defaults to 64. A value of zero means the default, never unlimited. | | limits.event.content[].kinds | List of event kinds to apply limit. Use `[min, max]` for ranges. Optional. | | limits.event.content[].maxLength | Maximum length of `content`. Defaults to 1 MB. Disabled when set to zero. | | limits.event.createdAt.maxNegativeDelta | Maximum number of seconds an event's `created_at` can be in the past. Defaults to zero. Disabled when set to zero. | @@ -199,6 +200,7 @@ The settings below are listed in alphabetical order by name. Please keep this ta | mirroring.static[].limits.event | Event limit overrides for this mirror. See configurations under limits.event. | | mirroring.static[].secret | Secret to pass to relays. Nostream relays only. Optional. | | mirroring.static[].skipAdmissionCheck | Disable the admission fee check for events coming from this mirror. | +| network.frameMemoryBudgetBytes | The queued outbound payload bytes every socket of this relay shares, in bytes; a frame that would overrun it is refused and the socket closed with `1013` and the reason `frame memory budget exhausted`. Defaults to 268435456 (256 MiB). A value of zero means the default, never unlimited. | | network.maxPayloadSize | Maximum number of bytes accepted per WebSocket frame | | network.remoteIpHeader | HTTP header from proxy containing IP address from client. | | nip05.domainBlacklist | List of domains blocked from NIP-05 verification. Authors with NIP-05 at these domains will be rejected. | diff --git a/docs/rewrite/decisions/0005-connection-concurrency.md b/docs/rewrite/decisions/0005-connection-concurrency.md index 4a4b5240..e1b6d20c 100644 --- a/docs/rewrite/decisions/0005-connection-concurrency.md +++ b/docs/rewrite/decisions/0005-connection-concurrency.md @@ -39,7 +39,8 @@ once per read either way, and identity would add a *second* watch handle to ever sender, plus a receiver the entry never awaits), while the generation adds one `u64` beside the one handle the table already holds. The generation is also the value a returning read can still carry after its own handle has been replaced, and the value that names the owner in the -table; the rule is tested through behaviour (`tests/connection.rs:208-231`), not by value. It +table; the rule is tested through behaviour +(`crates/nostream-relay/tests/connection.rs:208-231`), not by value. It fits the locking the file already has: `impl Connection` contains no `.await`, so no lock is ever held across a suspension point, and the counter is read and written inside the same `insert` that replaces the previous sender. No second lock, no allocation outside the guard, @@ -109,6 +110,20 @@ key would be an operator-facing surface mirroring nothing. `RelayConfig` is wher already keeps the knobs the capture fixes in code — with the tests setting the cadences they need — and these two port-only policies, which every test so far runs at their defaults. +**Reversed for the two bounds, by #989.** The reason above does not hold for +`frame_memory_budget_bytes` and `max_connections_per_ip`: mirroring nothing in the capture is an +argument against porting a *captured* key, and these two are not captured knobs. They are the +port's own denial-of-service bounds, and a bound an operator cannot set in production is a policy +constant wearing a config field's name — the state #989 was filed against. Both are now settings +(`network.frameMemoryBudgetBytes` and `limits.connection.maxConnectionsPerIp`), read into +`RelayConfig` in one place, so the compiled default and the document cannot disagree: +"pub fn from_settings(settings: &Settings) -> Self {" +(`crates/nostream-relay/src/relay.rs:108`), from leaves the document carries: +"pub frame_memory_budget_bytes: Option," (`crates/nostream-config/src/settings.rs:71`). The +write timeout and the per-socket queue bound stay +`RelayConfig`-only, as this section decided; ADR 0008 carries what a zero means for the two that +moved. + ## Liveness under a blocked write — every write carries a timeout The write happens inside the select arm, so while it is pending the same task cannot poll diff --git a/docs/rewrite/decisions/0008-frame-memory-budget.md b/docs/rewrite/decisions/0008-frame-memory-budget.md index 962b8814..a379184a 100644 --- a/docs/rewrite/decisions/0008-frame-memory-budget.md +++ b/docs/rewrite/decisions/0008-frame-memory-budget.md @@ -34,7 +34,7 @@ reason and its own counter. ## The two numbers, as deviations from the capture, with the arithmetic The capture has neither bound. Its connection limits are `ConnectionLimits` — `rateLimits`, -`ipWhitelist`, `ipBlacklist` and no count (`src/@types/settings.ts:163-167`) — and its outbound +`ipWhitelist`, `ipBlacklist` and no count, in the capture's settings types — and its outbound queue is unbounded (`src/adapters/web-socket-adapter.ts:150-155`). Both numbers below are therefore deviations from the capture, and both are correct because the operator's comment names them, not because a captured transcript shows them. There is no parity oracle for either: `wire_parity` cannot @@ -77,15 +77,36 @@ of knob: — `docs/rewrite/decisions/0005-connection-concurrency.md:104-110` -**The honest cost:** an operator cannot change either number without a rebuild. "Configurable" here -means the value is readable and settable through a `RelayConfig` literal rather than buried in a -call site — a settings key, a YAML entry and a validator/guided-catalogue entry are **#989** and out -of scope for this job. Say it plainly rather than letting "configurable" read as a knob an operator -can edit in production; today they cannot. - -`nostreamd` keeps `..Default::default()` in its `RelayConfig` literal -(`crates/nostreamd/src/main.rs:120-123`), and all five `Relay::new` call sites already did, so no -call site had to change. +**The honest cost, as this job left it:** an operator could not change either number without a +rebuild, because "configurable" meant readable and settable through a `RelayConfig` literal rather +than buried in a call site. **#989 resolved that, and the resolution is recorded here rather than +in a second document.** Both numbers are now operator settings, `network.frameMemoryBudgetBytes` and +`limits.connection.maxConnectionsPerIp` in `resources/default-settings.yaml`, typed as optional +leaves beside the keys that already live in those sections: +"pub max_connections_per_ip: Option," (`crates/nostream-config/src/settings.rs:312`). + +One function reads them and nothing else does — the daemon and the test share it, so they cannot +drift: "pub fn from_settings(settings: &Settings) -> Self {" +(`crates/nostream-relay/src/relay.rs:108`). The daemon calls it once and keeps its one environment +read: "let mut relay_config = RelayConfig::from_settings(&settings);" +(`crates/nostreamd/src/main.rs:110`). `RelayConfig::default()` remains the single source of the two +numbers — the document mirrors it, and a test in `crates/nostream-relay/tests/config_defaults.rs` +fails when they part — because a missing key must still mean a working relay, not a relay that +needs the document at start-up. The compiled values are unchanged: +"frame_memory_budget_bytes: 256 * 1024 * 1024," (`crates/nostream-relay/src/relay.rs:90`) and +"max_connections_per_ip: 64," (`crates/nostream-relay/src/relay.rs:94`). + +Two consequences a reader should have in one place. The document gains its first key whose name +states a byte unit, `frameMemoryBudgetBytes`; the convention it breaks is that the unit lives in +prose (`network.maxPayloadSize` is "bytes accepted per WebSocket frame" in `CONFIGURATION.md` and +carries no suffix). The suffix stays because #989 names the field `frame_memory_budget_bytes`, and +a YAML key that disagreed with the Rust field would be a second name for one number; the comment +above the key states the unit regardless. And no validator rule was added: the document is the +schema the shape validator walks, so the two keys alone make an operator's file acceptable, and +every existing message stays byte-identical (`crates/nostream-config/tests/ts_parity.rs` pins +them). + +**A zero is not "disabled" for either number** — see *What `0` means*, below. ## The refusal — `1013`, immediate, no text frame @@ -306,7 +327,9 @@ carries per-socket detail — `peer`, `live` — and would be log-amplifiable by trips it; the process-wide fact belongs to the counter instead. Which leaves the counters as the surface that does not depend on a log level, and they are readable only from a `Relay` handle today, i.e. from a test or an embedder. **Making either refusal visible to an operator who has not raised -the filter is an open gap, and #989 or the `/metrics` work is where it closes.** +the filter is an open gap. #989 did not close it either: the in-budget fix is a `debug!` → `warn!` +flip that reverses the log-amplification reasoning above, and the real surface is two +`NotImplemented` stubs, so #989 recorded the gap as a disposition and moved no log level.** That is the whole surface, because **a Prometheus endpoint is out of reach right now**: the OTLP exporter is a stub returning `NotImplemented` (`crates/nostreamd/src/telemetry.rs:47-52`) and the @@ -317,8 +340,9 @@ this job deliberately adds no metrics subsystem. ## What this does not decide -- **The operator-facing settings key** for either number: #989, with the YAML entry, the validator - and the guided catalogue it drags in. +- **~~The operator-facing settings key~~ for either number** — settled by #989, and recorded in + *The config surface* above: two YAML keys, the `RelayConfig::from_settings` constructor, and no + validator rule. - **Any exporter, dashboard or alerting.** No registry, no `/metrics` controller, no admin or telemetry change. - **Anything per listener.** One `Relay` carries one budget, and a second listener served by the same @@ -328,5 +352,46 @@ this job deliberately adds no metrics subsystem. are the only way to get two budgets, and production makes one. - **The per-socket bound's own empty-queue exception**, its default, and `SLOW_CONSUMER_REASON`. - **Inbound memory or handler state**, neither of which the budget counts. -- **Any validation of the two numbers.** A zero budget or a zero cap is a literal the operator wrote; - policing it is a settings decision this job does not own. +- **Any validation of the two numbers.** A zero budget or a zero cap is still not refused — it is + read as the default, which is the answer *What `0` means* gives and the only reading that cannot + turn a typo into a waiver of the bounds. There is no rule for it in the validator. + +## What `0` means + +#989 left this question here by name and this section answers it, after the code was final. + +**What `0` means: the default.** Neither key's zero disables its bound; the relay keeps the +compiled `256 * 1024 * 1024` bytes and `64` sockets and says nothing at start-up. Neither `0` is +refused. The repository had two precedents to choose from: + +- **"use the default"** — `transport::max_payload_size` reads an absent or zero + `network.maxPayloadSize` as the compiled value: + ".filter(|size| *size > 0)" (`crates/nostream-relay/src/transport.rs:55`). +- **"disabled"** — `max_subscriptions` treats a zero as no limit: + " if max_subscriptions > 0" (`crates/nostream-relay/src/handlers/subscribe.rs:376`), which is + what `CONFIGURATION.md` documents for the `limits.client.subscription` family. + +These two numbers follow the first. "Disabled" is defensible for a subscription count: zero +subscriptions is a legal relay for a read-only deployment, and the operator loses only a feature +they asked to lose. It is not defensible for a memory budget or a connection cap, because +"disabled" there means an unbounded outbound queue — the 384 MiB-per-address state the bound at +`crates/nostream-relay/src/relay.rs:90` exists to end — and an unlimited socket count per address, +which is the exposure measured in *The two numbers* above. A `0` typed by mistake would then be +strictly worse than the default it replaced, which is the wrong direction for a typo to fail in. + +The constructor implements exactly that, and nothing else: + +- the budget is taken only when the leaf is present, convertible and positive — + " if let Some(bytes) = settings.network.frame_memory_budget_bytes" + (`crates/nostream-relay/src/relay.rs:110`); +- the cap likewise, through the two optional levels the typed schema forces — + " .and_then(|connection| connection.max_connections_per_ip)" + (`crates/nostream-relay/src/relay.rs:120`); +- otherwise the field keeps `RelayConfig::default()`'s value, so a zero, an absent key and a + number too large for `usize` all behave as "the shipped default". + +**The cost, stated:** an operator who wants no budget has no way to ask for one, and this record +is the place that says so rather than a surprise at the console. That is the safe reading, not a +neutral one, and it is reversible later by a validator rule that rejects `0` with a message of its +own — the change this job deliberately did not make, because it would move validation output that +`crates/nostream-config/tests/ts_parity.rs` pins byte-for-byte. diff --git a/resources/default-settings.yaml b/resources/default-settings.yaml index fa208859..61cd8358 100755 --- a/resources/default-settings.yaml +++ b/resources/default-settings.yaml @@ -150,6 +150,8 @@ nip56: hideActionableReports: false network: maxPayloadSize: 524288 + # The queued outbound payload bytes every socket of this relay shares, in bytes. + # Defaults to 268435456 (256 MiB); a value of 0 means the default (ADR 0008). frameMemoryBudgetBytes: 268435456 # Uncomment only when using a trusted reverse proxy and configuring trustedProxies. # remoteIpHeader: x-forwarded-for @@ -214,6 +216,8 @@ limits: - "10.10.10.1" - "::ffff:10.10.10.1" connection: + # The live sockets one source address may hold at once. + # Defaults to 64; a value of 0 means the default (ADR 0008). maxConnectionsPerIp: 64 rateLimits: - period: 1000 From 47fc464d7b62ffff3f0bc8499d6c8142ba55c4c7 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 02:20:17 -0400 Subject: [PATCH 15/26] test(relay): prove from_settings reads the operator's bounds --- .../nostream-relay/tests/config_defaults.rs | 68 +++++++++++++++++++ 1 file changed, 68 insertions(+) diff --git a/crates/nostream-relay/tests/config_defaults.rs b/crates/nostream-relay/tests/config_defaults.rs index 8c6d8524..12f21982 100644 --- a/crates/nostream-relay/tests/config_defaults.rs +++ b/crates/nostream-relay/tests/config_defaults.rs @@ -54,3 +54,71 @@ fn the_compiled_bounds_equal_the_shipped_defaults() { config.max_connections_per_ip ); } + +/// A `Settings` whose two bounds are whatever the caller asks for — `None` for a key the operator +/// did not write. Everything else is the shipped defaults, so a difference in the result can only +/// come from the two leaves. +fn settings_with_bounds(budget: Option, cap: Option) -> Settings { + let mut settings = defaults_settings(); + settings.network.frame_memory_budget_bytes = budget; + settings + .limits + .as_mut() + .and_then(|limits| limits.connection.as_mut()) + .expect("the shipped defaults carry limits.connection") + .max_connections_per_ip = cap; + settings +} + +#[test] +fn a_non_default_setting_is_the_value_the_relay_runs_with() { + // The behaviour #989 exists for: what the operator wrote is what the relay gets. Asserted by + // value on both bounds, because "differs from the default" passes when the constructor swaps + // one number for the other. + let config = RelayConfig::from_settings(&settings_with_bounds(Some(1_234), Some(7))); + let defaults = RelayConfig::default(); + + assert_eq!(config.frame_memory_budget_bytes, 1_234); + assert_eq!(config.max_connections_per_ip, 7); + assert_eq!( + config.frame_queue_bytes, defaults.frame_queue_bytes, + "the settings must not change any other field" + ); + assert_eq!(config.write_timeout, defaults.write_timeout); + assert_eq!(config.liveness_interval, defaults.liveness_interval); + assert_eq!(config.drain_timeout, defaults.drain_timeout); +} + +#[test] +fn a_zero_setting_falls_back_to_the_compiled_default() { + // The answer ADR 0008's `What 0 means` gives, for each bound on its own so a constructor that + // read only one leaf cannot pass both cases. + let budget_only = RelayConfig::from_settings(&settings_with_bounds(Some(0), Some(7))); + assert_eq!( + budget_only.frame_memory_budget_bytes, + RelayConfig::default().frame_memory_budget_bytes + ); + assert_eq!(budget_only.max_connections_per_ip, 7); + + let cap_only = RelayConfig::from_settings(&settings_with_bounds(Some(1_234), Some(0))); + assert_eq!(cap_only.frame_memory_budget_bytes, 1_234); + assert_eq!( + cap_only.max_connections_per_ip, + RelayConfig::default().max_connections_per_ip + ); +} + +#[test] +fn an_absent_setting_falls_back_to_the_compiled_default() { + let config = RelayConfig::from_settings(&settings_with_bounds(None, None)); + let defaults = RelayConfig::default(); + + assert_eq!( + config.frame_memory_budget_bytes, + defaults.frame_memory_budget_bytes + ); + assert_eq!( + config.max_connections_per_ip, + defaults.max_connections_per_ip + ); +} From d07f8982fbaaca75c38a61923a1d4df00de3195a Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 02:31:14 -0400 Subject: [PATCH 16/26] fix(relay): cap a setting's bound so an absurd value cannot exhaust usize --- crates/nostream-config/src/guided.rs | 4 +- crates/nostream-relay/src/relay.rs | 36 ++++++++--- .../nostream-relay/tests/config_defaults.rs | 60 +++++++++++++++++++ 3 files changed, 90 insertions(+), 10 deletions(-) diff --git a/crates/nostream-config/src/guided.rs b/crates/nostream-config/src/guided.rs index 8d6183a1..ed0800bb 100644 --- a/crates/nostream-config/src/guided.rs +++ b/crates/nostream-config/src/guided.rs @@ -207,7 +207,7 @@ fn build_categories() -> Vec { SafeNonNegativeInteger, ), validated( - "Frame memory budget (bytes)", + "Frame memory budget (bytes, 0 means the default)", "network.frameMemoryBudgetBytes", Number, SafeNonNegativeInteger, @@ -270,7 +270,7 @@ fn build_categories() -> Vec { SafeNonNegativeInteger, ), validated( - "Max connections per IP", + "Max connections per IP (0 means the default)", "limits.connection.maxConnectionsPerIp", Number, SafeNonNegativeInteger, diff --git a/crates/nostream-relay/src/relay.rs b/crates/nostream-relay/src/relay.rs index 1e07e51d..d1f249e1 100644 --- a/crates/nostream-relay/src/relay.rs +++ b/crates/nostream-relay/src/relay.rs @@ -37,12 +37,13 @@ use crate::sink::{FrameBudget, FrameSink, Refusal}; use crate::transport; use crate::wot::WotGraph; -/// The knobs a [`Relay`] is built with that are not settings: a constant the capture fixes in +/// The knobs a [`Relay`] is built with. Most are not settings: a constant the capture fixes in /// code (`liveness`, the NIP-56 refresh) or a policy the port adds with no counterpart there -/// (`frame_queue_bytes`, `write_timeout`, `frame_memory_budget_bytes`, -/// `max_connections_per_ip`; ADR 0005, ADR 0008). Tests set the cadences they need and the -/// bounds a case asks for; every value here runs at its shipped default otherwise. A value -/// the capture's settings document carries belongs in [`Settings`], never here. +/// (`frame_queue_bytes`, `write_timeout`; ADR 0005). The two bounds are settings an operator +/// writes — `network.frameMemoryBudgetBytes` and `limits.connection.maxConnectionsPerIp` — and +/// [`RelayConfig::from_settings`] is what reads them; [`RelayConfig::default`] is the value a +/// relay runs with when the key is absent. Tests set the cadences they need and the bounds a +/// case asks for. A new value the capture's settings document carries belongs in [`Settings`]. #[derive(Debug, Clone)] pub struct RelayConfig { /// `WSS_CLIENT_HEALTH_PROBE_INTERVAL` (`web-socket-server-adapter.ts:18`). @@ -72,7 +73,8 @@ pub struct RelayConfig { impl Default for RelayConfig { /// The capture's own constants, and no environment read: `WS_DRAIN_TIMEOUT_MS` is /// resolved by [`transport::drain_timeout`], which the composition root calls (ADR 0004); - /// the queue bound and the write timeout are the port's additions (ADR 0005). + /// the queue bound and the write timeout are the port's additions (ADR 0005). The two + /// bounds are the shipped default an operator's setting overrides, not the only value. fn default() -> Self { Self { liveness_interval: Duration::from_millis(120_000), @@ -96,6 +98,18 @@ impl Default for RelayConfig { } } +/// The largest value either bound accepts from a setting: 1 TiB of queued frames, and a million +/// sockets from one address. Both are far above any deployment and far below `usize::MAX`, so a +/// stray digit cannot reach [`FrameBudget::admit`]'s arithmetic. A value above it is treated like +/// a zero: the default stands (ADR 0008). +pub const MAX_SETTING_BOUND: usize = 1024 * 1024 * 1024 * 1024; + +/// Whether an operator's value for a bound is one the relay can run with: positive and at most +/// [`MAX_SETTING_BOUND`]. +fn in_range(value: usize) -> bool { + value > 0 && value <= MAX_SETTING_BOUND +} + impl RelayConfig { /// The configuration a relay built from `settings` runs with: [`RelayConfig::default`] for /// every knob the capture fixes in code, with the two bounds an operator sets read from @@ -105,11 +119,17 @@ impl RelayConfig { /// This is the one place either setting is interpreted, so `nostreamd` and the tests cannot /// disagree about it. `drain_timeout` stays the composition root's to resolve: it is an /// environment read (ADR 0004), not a setting. + /// + /// A leaf is taken when it is present, non-zero and at most [`MAX_SETTING_BOUND`]; anything + /// else keeps the default. The ceiling is not cosmetic: `usize::MAX` fits a `usize`, so it + /// would pass a bare `> 0` check and reach [`FrameBudget::admit`], whose fit guard uses + /// `saturating_sub` while its accounting adds unchecked — a budget that cannot be + /// exhausted, and an add that overflows. pub fn from_settings(settings: &Settings) -> Self { let mut config = Self::default(); if let Some(bytes) = settings.network.frame_memory_budget_bytes && let Ok(bytes) = usize::try_from(bytes) - && bytes > 0 + && in_range(bytes) { config.frame_memory_budget_bytes = bytes; } @@ -119,7 +139,7 @@ impl RelayConfig { .and_then(|limits| limits.connection.as_ref()) .and_then(|connection| connection.max_connections_per_ip) && let Ok(cap) = usize::try_from(cap) - && cap > 0 + && in_range(cap) { config.max_connections_per_ip = cap; } diff --git a/crates/nostream-relay/tests/config_defaults.rs b/crates/nostream-relay/tests/config_defaults.rs index 12f21982..bb61d38f 100644 --- a/crates/nostream-relay/tests/config_defaults.rs +++ b/crates/nostream-relay/tests/config_defaults.rs @@ -89,6 +89,66 @@ fn a_non_default_setting_is_the_value_the_relay_runs_with() { assert_eq!(config.drain_timeout, defaults.drain_timeout); } +#[test] +fn a_setting_above_the_ceiling_falls_back_to_the_compiled_default() { + // `usize::MAX` is a `usize`, so a bare `> 0` check would pass it to `FrameBudget::admit`, + // whose guard cannot trip and whose accounting adds unchecked. + let config = RelayConfig::from_settings(&settings_with_bounds(Some(u64::MAX), Some(u64::MAX))); + let defaults = RelayConfig::default(); + + assert_eq!( + config.frame_memory_budget_bytes, + defaults.frame_memory_budget_bytes + ); + assert_eq!( + config.max_connections_per_ip, + defaults.max_connections_per_ip + ); + + // The documented ceiling is accepted; one byte over it is not. Spelled out here rather than + // imported, so lowering the constant cannot quietly lower this test with it. + let ceiling = 1024 * 1024 * 1024 * 1024_usize; + let at_ceiling = RelayConfig::from_settings(&settings_with_bounds( + Some(ceiling as u64), + Some(ceiling as u64), + )); + assert_eq!(at_ceiling.frame_memory_budget_bytes, ceiling); + assert_eq!(at_ceiling.max_connections_per_ip, ceiling); + + let over = RelayConfig::from_settings(&settings_with_bounds( + Some(ceiling as u64 + 1), + Some(ceiling as u64 + 1), + )); + assert_eq!( + over.frame_memory_budget_bytes, + defaults.frame_memory_budget_bytes + ); + assert_eq!(over.max_connections_per_ip, defaults.max_connections_per_ip); +} + +#[test] +fn absent_sections_fall_back_to_the_compiled_default() { + // Not just absent leaves: an operator file may omit `limits` entirely, or `limits` without + // `connection`, and the two-level `and_then` chain must survive both. + let mut without_limits = defaults_settings(); + without_limits.limits = None; + + let mut without_connection = defaults_settings(); + without_connection + .limits + .as_mut() + .expect("the shipped defaults carry limits") + .connection = None; + + for settings in [without_limits, without_connection] { + let config = RelayConfig::from_settings(&settings); + assert_eq!( + config.max_connections_per_ip, + RelayConfig::default().max_connections_per_ip + ); + } +} + #[test] fn a_zero_setting_falls_back_to_the_compiled_default() { // The answer ADR 0008's `What 0 means` gives, for each bound on its own so a constructor that From 03180b898208faaef960c0fedb678e1394d00bd7 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 02:33:18 -0400 Subject: [PATCH 17/26] docs(rewrite): say what a bound above the ceiling does, and what the catalogue does not --- .changeset/relay-frame-budget-settings.md | 10 ++--- CONFIGURATION.md | 4 +- .../decisions/0008-frame-memory-budget.md | 42 ++++++++++++++++--- resources/default-settings.yaml | 10 +++-- 4 files changed, 49 insertions(+), 17 deletions(-) diff --git a/.changeset/relay-frame-budget-settings.md b/.changeset/relay-frame-budget-settings.md index a19bc051..1a28997c 100644 --- a/.changeset/relay-frame-budget-settings.md +++ b/.changeset/relay-frame-budget-settings.md @@ -15,11 +15,11 @@ Both are now settings the operator writes: - `limits.connection.maxConnectionsPerIp` in `resources/default-settings.yaml` — the live sockets one source address may hold at once. -`RelayConfig::from_settings` reads both leaves, in the one place either is interpreted, and -falls back to the compiled default when a leaf is absent or zero: -"if let Some(bytes) = settings.network.frame_memory_budget_bytes" -(`crates/nostream-relay/src/relay.rs:110`). The daemon builds its config through it: -"let mut relay_config = RelayConfig::from_settings(&settings);" +`RelayConfig::from_settings` reads both leaves, in the one place either is interpreted, and falls +back to the compiled default when a leaf is absent, zero, or above the ceiling that keeps an +operator's number out of the frame accounting: +" value > 0 && value <= MAX_SETTING_BOUND" (`crates/nostream-relay/src/relay.rs:109`). The daemon +builds its config through it: "let mut relay_config = RelayConfig::from_settings(&settings);" (`crates/nostreamd/src/main.rs:110`), keeping its one environment read. The defaults document's values equal `RelayConfig::default()`'s, and a test in diff --git a/CONFIGURATION.md b/CONFIGURATION.md index d311f35f..5529b87c 100644 --- a/CONFIGURATION.md +++ b/CONFIGURATION.md @@ -161,7 +161,7 @@ The settings below are listed in alphabetical order by name. Please keep this ta | limits.client.subscription.maxFilters | Maximum number of filters per subscription. Defaults to 10. Disabled when set to zero. | | limits.client.subscription.maxFilterValues | Maximum number of values allowed in each filter, counting its array criteria (`ids`, `authors`, `kinds`, `#`) together. Defaults to 2500. Disabled when set to zero. | | limits.client.subscription.maxSubscriptions | Maximum number of subscriptions per connected client. Defaults to 10. Disabled when set to zero. | -| limits.connection.maxConnectionsPerIp | Maximum number of live sockets one source address may hold at once; a socket above the count is closed with `1013` and the reason `connection cap: too many connections from this address`. Defaults to 64. A value of zero means the default, never unlimited. | +| limits.connection.maxConnectionsPerIp | Maximum number of live sockets one source address may hold at once. Defaults to 64. Zero means the default, never unlimited. | | limits.event.content[].kinds | List of event kinds to apply limit. Use `[min, max]` for ranges. Optional. | | limits.event.content[].maxLength | Maximum length of `content`. Defaults to 1 MB. Disabled when set to zero. | | limits.event.createdAt.maxNegativeDelta | Maximum number of seconds an event's `created_at` can be in the past. Defaults to zero. Disabled when set to zero. | @@ -200,7 +200,7 @@ The settings below are listed in alphabetical order by name. Please keep this ta | mirroring.static[].limits.event | Event limit overrides for this mirror. See configurations under limits.event. | | mirroring.static[].secret | Secret to pass to relays. Nostream relays only. Optional. | | mirroring.static[].skipAdmissionCheck | Disable the admission fee check for events coming from this mirror. | -| network.frameMemoryBudgetBytes | The queued outbound payload bytes every socket of this relay shares, in bytes; a frame that would overrun it is refused and the socket closed with `1013` and the reason `frame memory budget exhausted`. Defaults to 268435456 (256 MiB). A value of zero means the default, never unlimited. | +| network.frameMemoryBudgetBytes | The queued outbound payload bytes every socket of this relay shares. Unit: bytes. Defaults to 268435456 (256 MiB). Zero means the default, never unlimited. | | network.maxPayloadSize | Maximum number of bytes accepted per WebSocket frame | | network.remoteIpHeader | HTTP header from proxy containing IP address from client. | | nip05.domainBlacklist | List of domains blocked from NIP-05 verification. Authors with NIP-05 at these domains will be rejected. | diff --git a/docs/rewrite/decisions/0008-frame-memory-budget.md b/docs/rewrite/decisions/0008-frame-memory-budget.md index a379184a..d40a0a57 100644 --- a/docs/rewrite/decisions/0008-frame-memory-budget.md +++ b/docs/rewrite/decisions/0008-frame-memory-budget.md @@ -381,14 +381,44 @@ strictly worse than the default it replaced, which is the wrong direction for a The constructor implements exactly that, and nothing else: -- the budget is taken only when the leaf is present, convertible and positive — - " if let Some(bytes) = settings.network.frame_memory_budget_bytes" - (`crates/nostream-relay/src/relay.rs:110`); +- the budget is taken only when the leaf is present, convertible, positive and within the + ceiling — " value > 0 && value <= MAX_SETTING_BOUND" (`crates/nostream-relay/src/relay.rs:109`), + applied to the leaf at " if let Some(bytes) = settings.network.frame_memory_budget_bytes" + (`crates/nostream-relay/src/relay.rs:130`); - the cap likewise, through the two optional levels the typed schema forces — " .and_then(|connection| connection.max_connections_per_ip)" - (`crates/nostream-relay/src/relay.rs:120`); -- otherwise the field keeps `RelayConfig::default()`'s value, so a zero, an absent key and a - number too large for `usize` all behave as "the shipped default". + (`crates/nostream-relay/src/relay.rs:140`); +- otherwise the field keeps `RelayConfig::default()`'s value, so a zero, an absent key — absent + leaf, absent `limits`, absent `limits.connection` — and a number above the ceiling all behave as + "the shipped default". + +**Why a ceiling, and not just `> 0`.** `usize::MAX` *is* a `usize`, so a bare positivity check +passes it through, and the budget then stops bounding anything: the fit guard subtracts with +` if bytes > self.limit_bytes.saturating_sub(accounted) {` +(`crates/nostream-relay/src/sink.rs:107`), which can never be true against a limit of +`usize::MAX`, while the accounting that follows adds unchecked — +" accounted + bytes," (`crates/nostream-relay/src/sink.rs:113`) — so it panics in a +debug build and wraps in a release one, and a wrapped counter refills the budget it was meant to +exhaust. The ceiling is what keeps an operator's number out of that arithmetic: +"pub const MAX_SETTING_BOUND: usize = 1024 * 1024 * 1024 * 1024;" +(`crates/nostream-relay/src/relay.rs:105`). A TiB of queued frames and a million sockets from one +address are far above any deployment; a too-large value falls back exactly as a zero does, and a +value at the ceiling is accepted. **The unchecked add itself is not this job's** — it predates the +bounds and is unreachable through settings now — so it is reported rather than changed here. + +**The guided catalogue does not bound these two, and no validator in the repository does.** The +catalogue has two validators — " SafeNonNegativeInteger," (`crates/nostream-config/src/guided.rs:25`) +and `NonEmpty` — and `SafeNonNegativeInteger`'s ceiling is the capture's maximum safe integer: +" Ok(number) if number <= 9_007_199_254_740_991 => None," +(`crates/nostream-config/src/guided.rs:77`). That is the capture's only number validator +(`src/utils/settings-guided-schema.ts` has one), so both entries carry it as its precedent +`network.maxPayloadSize` does, and an editor accepts about 9 exabytes of budget and about 9 +quadrillion sockets — values the constructor then treats as the shipped default, at the operator's +console. A bounded-integer validator is the fix and is **not** in this job: it is a new validator +kind with its own messages, and it belongs with the guided catalogue's unimplemented consumers. +Both entries say what the number accepts and what a zero means in their labels — "Frame memory +budget (bytes, 0 means the default)" (`crates/nostream-config/src/guided.rs:210`) — so the editor +at least does not mislabel the value. **The cost, stated:** an operator who wants no budget has no way to ask for one, and this record is the place that says so rather than a surprise at the console. That is the safe reading, not a diff --git a/resources/default-settings.yaml b/resources/default-settings.yaml index 61cd8358..5d0d38a2 100755 --- a/resources/default-settings.yaml +++ b/resources/default-settings.yaml @@ -150,8 +150,9 @@ nip56: hideActionableReports: false network: maxPayloadSize: 524288 - # The queued outbound payload bytes every socket of this relay shares, in bytes. - # Defaults to 268435456 (256 MiB); a value of 0 means the default (ADR 0008). + # The queued outbound payload bytes every socket of this relay shares. Unit: bytes; the guided + # editor calls it "Frame memory budget (bytes, 0 means the default)". Defaults to 268435456 + # (256 MiB); 0 — or anything above 1099511627776 (1 TiB) — means the default (ADR 0008). frameMemoryBudgetBytes: 268435456 # Uncomment only when using a trusted reverse proxy and configuring trustedProxies. # remoteIpHeader: x-forwarded-for @@ -216,8 +217,9 @@ limits: - "10.10.10.1" - "::ffff:10.10.10.1" connection: - # The live sockets one source address may hold at once. - # Defaults to 64; a value of 0 means the default (ADR 0008). + # The live sockets one source address may hold at once. Unit: sockets; the guided editor calls + # it "Max connections per IP (0 means the default)". Defaults to 64; 0 — or anything above + # 1099511627776 — means the default (ADR 0008). maxConnectionsPerIp: 64 rateLimits: - period: 1000 From 645a85b2e2a0ce053b27174f6dceb36d9b9f2592 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 02:49:23 -0400 Subject: [PATCH 18/26] fix(daemon): report a bound setting the relay will replace with its default --- crates/nostream-relay/src/relay.rs | 62 +++++++++++++++++++-------- crates/nostreamd/src/settings_gate.rs | 30 +++++++++++++ crates/nostreamd/tests/check_flag.rs | 28 ++++++++++++ 3 files changed, 102 insertions(+), 18 deletions(-) diff --git a/crates/nostream-relay/src/relay.rs b/crates/nostream-relay/src/relay.rs index d1f249e1..44a30963 100644 --- a/crates/nostream-relay/src/relay.rs +++ b/crates/nostream-relay/src/relay.rs @@ -90,24 +90,29 @@ impl Default for RelayConfig { // before the budget binds, which is below the 48 a single address gets through // the shipped attempt limits alone (ADR 0008). frame_memory_budget_bytes: 256 * 1024 * 1024, - // 64: four times the concurrent total one well-behaved client needs. The cap - // bounds sockets, the budget bounds bytes, so its own worst case (64 × 8 MiB) sits - // above the budget on purpose (ADR 0008). + // 64: four times the concurrent total one well-behaved client needs. The cap is per + // address while the budget is process-wide, so what the pair really bounds is + // `min(budget, addresses × cap × frame_queue_bytes)` — the cap alone cannot be + // read as the memory ceiling (ADR 0008). max_connections_per_ip: 64, } } } -/// The largest value either bound accepts from a setting: 1 TiB of queued frames, and a million -/// sockets from one address. Both are far above any deployment and far below `usize::MAX`, so a -/// stray digit cannot reach [`FrameBudget::admit`]'s arithmetic. A value above it is treated like -/// a zero: the default stands (ADR 0008). +/// The ceiling a setting's bound may carry, in that bound's own unit: 1 TiB (1 099 511 627 776) +/// of queued frames for `network.frameMemoryBudgetBytes`, and 1.09 million sockets for +/// `limits.connection.maxConnectionsPerIp`. One number because one check reads both; it is far +/// above any deployment each way, and far below `usize::MAX`, so a stray digit cannot reach +/// [`FrameBudget::admit`]'s arithmetic. A value above it is treated like a zero: the default +/// stands (ADR 0008). pub const MAX_SETTING_BOUND: usize = 1024 * 1024 * 1024 * 1024; /// Whether an operator's value for a bound is one the relay can run with: positive and at most -/// [`MAX_SETTING_BOUND`]. -fn in_range(value: usize) -> bool { - value > 0 && value <= MAX_SETTING_BOUND +/// [`MAX_SETTING_BOUND`]. Public because the daemon's settings gate reports the same verdict +/// before a relay exists, and one predicate is what keeps `--check` and the relay from +/// disagreeing (ADR 0008). +pub fn in_range(value: u64) -> bool { + value > 0 && value <= MAX_SETTING_BOUND as u64 } impl RelayConfig { @@ -127,22 +132,43 @@ impl RelayConfig { /// exhausted, and an add that overflows. pub fn from_settings(settings: &Settings) -> Self { let mut config = Self::default(); - if let Some(bytes) = settings.network.frame_memory_budget_bytes - && let Ok(bytes) = usize::try_from(bytes) - && in_range(bytes) - { - config.frame_memory_budget_bytes = bytes; + + if let Some(bytes) = settings.network.frame_memory_budget_bytes { + if in_range(bytes) { + config.frame_memory_budget_bytes = bytes as usize; + } else { + // `warn!`, not `debug!`: the value passes `--check` and is then quietly + // replaced, so this line and the gate's report are the only places an operator + // learns that the number they wrote is not the number the relay runs with. + tracing::warn!( + setting = "network.frameMemoryBudgetBytes", + value = bytes, + ceiling = MAX_SETTING_BOUND, + default = config.frame_memory_budget_bytes, + "setting out of range for a bound; using the default" + ); + } } + if let Some(cap) = settings .limits .as_ref() .and_then(|limits| limits.connection.as_ref()) .and_then(|connection| connection.max_connections_per_ip) - && let Ok(cap) = usize::try_from(cap) - && in_range(cap) { - config.max_connections_per_ip = cap; + if in_range(cap) { + config.max_connections_per_ip = cap as usize; + } else { + tracing::warn!( + setting = "limits.connection.maxConnectionsPerIp", + value = cap, + ceiling = MAX_SETTING_BOUND, + default = config.max_connections_per_ip, + "setting out of range for a bound; using the default" + ); + } } + config } } diff --git a/crates/nostreamd/src/settings_gate.rs b/crates/nostreamd/src/settings_gate.rs index 52136271..c82a2390 100644 --- a/crates/nostreamd/src/settings_gate.rs +++ b/crates/nostreamd/src/settings_gate.rs @@ -17,12 +17,42 @@ //! request, per message or per streamed record. use nostream_config::LoadOutcome; +use nostream_relay::relay::{MAX_SETTING_BOUND, in_range}; use crate::error::DaemonError; +/// The two settings whose value the relay bounds at run time, with the default it falls back to. +/// A value out of range is not a schema fault — the key is valid and the number is a number — so +/// it is neither an issue nor fatal; but `--check` is where an operator looks before starting, so +/// a value about to be replaced is reported here rather than only at start-up (ADR 0008). +fn out_of_range_bounds(settings: &nostream_config::settings::Settings) -> Vec<(&'static str, u64)> { + let mut dropped = Vec::new(); + if let Some(bytes) = settings.network.frame_memory_budget_bytes + && !in_range(bytes) + { + dropped.push(("network.frameMemoryBudgetBytes", bytes)); + } + if let Some(cap) = settings + .limits + .as_ref() + .and_then(|limits| limits.connection.as_ref()) + .and_then(|connection| connection.max_connections_per_ip) + && !in_range(cap) + { + dropped.push(("limits.connection.maxConnectionsPerIp", cap)); + } + dropped +} + /// Applies the gate to one load outcome: report the findings, decide whether they are fatal. pub fn apply_settings_gate(outcome: &LoadOutcome, check: bool) -> Result<(), DaemonError> { if outcome.issues.is_empty() { + for (setting, value) in out_of_range_bounds(&outcome.settings) { + eprintln!( + "{setting}: {value} is above the {MAX_SETTING_BOUND} a bound accepts; \ + the relay runs with its default" + ); + } if check { println!("settings valid: 0 issues"); } diff --git a/crates/nostreamd/tests/check_flag.rs b/crates/nostreamd/tests/check_flag.rs index 8680eb2a..deb48a49 100644 --- a/crates/nostreamd/tests/check_flag.rs +++ b/crates/nostreamd/tests/check_flag.rs @@ -150,3 +150,31 @@ fn check_accepts_an_operator_file_that_sets_both_bounds() { assert_eq!(code, Some(0), "exit code; stderr: {stderr}"); assert_eq!(stdout.trim(), "settings valid: 0 issues"); } + +#[test] +fn check_reports_a_bound_it_would_replace_with_the_default() { + // The key and the number are both valid, so this is not an issue and not fatal — but a value + // discarded at start-up used to be silent, and `--check` is where an operator looks first. + let directory = tempfile::tempdir().expect("tempdir"); + std::fs::write( + directory.path().join("settings.yaml"), + "network:\n frameMemoryBudgetBytes: 1099511627777\nlimits:\n connection:\n maxConnectionsPerIp: 1099511627777\n", + ) + .expect("write"); + + let config_dir = directory.path().to_string_lossy().into_owned(); + let (code, stdout, stderr) = run_check( + Duration::from_secs(30), + &["--check", "--config-dir", &config_dir], + ); + assert_eq!(code, Some(0), "exit code"); + assert_eq!(stdout.trim(), "settings valid: 0 issues"); + assert!( + stderr.contains("network.frameMemoryBudgetBytes: 1099511627777"), + "stderr: {stderr}" + ); + assert!( + stderr.contains("limits.connection.maxConnectionsPerIp: 1099511627777"), + "stderr: {stderr}" + ); +} From 99b78a764f84b53b6e22253fe1b67a4080441b01 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 02:50:58 -0400 Subject: [PATCH 19/26] docs(rewrite): re-anchor every citation, and name the ceiling where the number is typed --- .changeset/relay-frame-budget-settings.md | 9 ++- CONFIGURATION.md | 4 +- .../decisions/0005-connection-concurrency.md | 2 +- .../decisions/0008-frame-memory-budget.md | 75 +++++++++++++------ resources/default-settings.yaml | 2 +- 5 files changed, 63 insertions(+), 29 deletions(-) diff --git a/.changeset/relay-frame-budget-settings.md b/.changeset/relay-frame-budget-settings.md index 1a28997c..c5fa2cba 100644 --- a/.changeset/relay-frame-budget-settings.md +++ b/.changeset/relay-frame-budget-settings.md @@ -18,9 +18,12 @@ Both are now settings the operator writes: `RelayConfig::from_settings` reads both leaves, in the one place either is interpreted, and falls back to the compiled default when a leaf is absent, zero, or above the ceiling that keeps an operator's number out of the frame accounting: -" value > 0 && value <= MAX_SETTING_BOUND" (`crates/nostream-relay/src/relay.rs:109`). The daemon -builds its config through it: "let mut relay_config = RelayConfig::from_settings(&settings);" -(`crates/nostreamd/src/main.rs:110`), keeping its one environment read. +" value > 0 && value <= MAX_SETTING_BOUND as u64" (`crates/nostream-relay/src/relay.rs:115`). The +daemon builds its config through it: "let mut relay_config = RelayConfig::from_settings(&settings);" +(`crates/nostreamd/src/main.rs:110`), keeping its one environment read. A value it replaces is not +silent: the constructor warns with the setting, the value, the ceiling and the default +(`crates/nostream-relay/src/relay.rs:148`), and `nostreamd --check` reports the same before a relay +exists (`crates/nostreamd/src/settings_gate.rs:28-45`). The defaults document's values equal `RelayConfig::default()`'s, and a test in `crates/nostream-relay/tests/config_defaults.rs` holds the two together. No validator rule is diff --git a/CONFIGURATION.md b/CONFIGURATION.md index 5529b87c..03716e0e 100644 --- a/CONFIGURATION.md +++ b/CONFIGURATION.md @@ -161,7 +161,7 @@ The settings below are listed in alphabetical order by name. Please keep this ta | limits.client.subscription.maxFilters | Maximum number of filters per subscription. Defaults to 10. Disabled when set to zero. | | limits.client.subscription.maxFilterValues | Maximum number of values allowed in each filter, counting its array criteria (`ids`, `authors`, `kinds`, `#`) together. Defaults to 2500. Disabled when set to zero. | | limits.client.subscription.maxSubscriptions | Maximum number of subscriptions per connected client. Defaults to 10. Disabled when set to zero. | -| limits.connection.maxConnectionsPerIp | Maximum number of live sockets one source address may hold at once. Defaults to 64. Zero means the default, never unlimited. | +| limits.connection.maxConnectionsPerIp | Maximum number of live sockets one source address may hold at once. Defaults to 64. Zero, or a value above 1099511627776, means the default. | | limits.event.content[].kinds | List of event kinds to apply limit. Use `[min, max]` for ranges. Optional. | | limits.event.content[].maxLength | Maximum length of `content`. Defaults to 1 MB. Disabled when set to zero. | | limits.event.createdAt.maxNegativeDelta | Maximum number of seconds an event's `created_at` can be in the past. Defaults to zero. Disabled when set to zero. | @@ -200,7 +200,7 @@ The settings below are listed in alphabetical order by name. Please keep this ta | mirroring.static[].limits.event | Event limit overrides for this mirror. See configurations under limits.event. | | mirroring.static[].secret | Secret to pass to relays. Nostream relays only. Optional. | | mirroring.static[].skipAdmissionCheck | Disable the admission fee check for events coming from this mirror. | -| network.frameMemoryBudgetBytes | The queued outbound payload bytes every socket of this relay shares. Unit: bytes. Defaults to 268435456 (256 MiB). Zero means the default, never unlimited. | +| network.frameMemoryBudgetBytes | The queued outbound payload bytes every socket of this relay shares. Unit: bytes. Defaults to 268435456 (256 MiB). Zero, or a value above 1099511627776 (1 TiB), means the default. | | network.maxPayloadSize | Maximum number of bytes accepted per WebSocket frame | | network.remoteIpHeader | HTTP header from proxy containing IP address from client. | | nip05.domainBlacklist | List of domains blocked from NIP-05 verification. Authors with NIP-05 at these domains will be rejected. | diff --git a/docs/rewrite/decisions/0005-connection-concurrency.md b/docs/rewrite/decisions/0005-connection-concurrency.md index e1b6d20c..df9efd0e 100644 --- a/docs/rewrite/decisions/0005-connection-concurrency.md +++ b/docs/rewrite/decisions/0005-connection-concurrency.md @@ -118,7 +118,7 @@ constant wearing a config field's name — the state #989 was filed against. Bot (`network.frameMemoryBudgetBytes` and `limits.connection.maxConnectionsPerIp`), read into `RelayConfig` in one place, so the compiled default and the document cannot disagree: "pub fn from_settings(settings: &Settings) -> Self {" -(`crates/nostream-relay/src/relay.rs:108`), from leaves the document carries: +(`crates/nostream-relay/src/relay.rs:133`), from leaves the document carries: "pub frame_memory_budget_bytes: Option," (`crates/nostream-config/src/settings.rs:71`). The write timeout and the per-socket queue bound stay `RelayConfig`-only, as this section decided; ADR 0008 carries what a zero means for the two that diff --git a/docs/rewrite/decisions/0008-frame-memory-budget.md b/docs/rewrite/decisions/0008-frame-memory-budget.md index d40a0a57..ceeaa08c 100644 --- a/docs/rewrite/decisions/0008-frame-memory-budget.md +++ b/docs/rewrite/decisions/0008-frame-memory-budget.md @@ -7,12 +7,12 @@ ## Context The outbound frame queue was bounded per socket and nothing bounded their sum. -`RelayConfig::frame_queue_bytes` (8 MiB; `crates/nostream-relay/src/relay.rs:57`) is enforced by +`RelayConfig::frame_queue_bytes` (8 MiB; `crates/nostream-relay/src/relay.rs:58`) is enforced by `FrameSink::send`'s compare-and-exchange on the socket's own counter (`crates/nostream-relay/src/sink.rs:260-334`), and `Relay::serve` accepted every connection with no count at all. The only per-address bookkeeping was the **attempt** limiter, which throttles how fast connections open, not how many are held (`crates/nostream-relay/src/limits.rs`, windows -consumed at `crates/nostream-relay/src/relay.rs:327`). +consumed at `crates/nostream-relay/src/relay.rs:404`). The operator's policy comment on #959 chose the numbers and the refusal and asked for two consequences a reviewer can hold the change to: both bounds refuse the same way, and the budget is @@ -27,7 +27,7 @@ the pre-change worst case is **384 MiB**, and that figure is **arithmetic from n back on a fresh daemon: the first twelve received `HTTP/1.1 101 Switching Protocols` and then a text frame starting `["AUTH","…`, and the remaining eight were upgraded and dropped with **no websocket close frame at all** — the attempt limiter's silent return -(`crates/nostream-relay/src/relay.rs:287-290`). A refusal indistinguishable from a network fault is +(`crates/nostream-relay/src/relay.rs:364-367`). A refusal indistinguishable from a network fault is exactly what the new bounds are not allowed to be, so each of them closes with a code, a fixed reason and its own counter. @@ -67,7 +67,7 @@ and folding them in would make one knob mean two things. ## The config surface — `RelayConfig`, not settings Both fields are `RelayConfig` fields, `frame_memory_budget_bytes` at -`crates/nostream-relay/src/relay.rs:65` and `max_connections_per_ip` at `:69`, built into the relay's +`crates/nostream-relay/src/relay.rs:64` and `max_connections_per_ip` at `:67`, built into the relay's two shared values in `Relay::new` (`:146-147`). ADR 0005 already decided the question for this class of knob: @@ -87,7 +87,7 @@ leaves beside the keys that already live in those sections: One function reads them and nothing else does — the daemon and the test share it, so they cannot drift: "pub fn from_settings(settings: &Settings) -> Self {" -(`crates/nostream-relay/src/relay.rs:108`). The daemon calls it once and keeps its one environment +(`crates/nostream-relay/src/relay.rs:133`). The daemon calls it once and keeps its one environment read: "let mut relay_config = RelayConfig::from_settings(&settings);" (`crates/nostreamd/src/main.rs:110`). `RelayConfig::default()` remains the single source of the two numbers — the document mirrors it, and a test in `crates/nostream-relay/tests/config_defaults.rs` @@ -112,7 +112,7 @@ them). Both bounds close the socket with `CloseCode::Again` (1013) written straight to the transport rather than through the sink, bounded by `config.write_timeout` exactly as the existing slow-consumer close -is (`Relay::close_frame`, `crates/nostream-relay/src/relay.rs:530-541`). Three fixed reason strings +is (`Relay::close_frame`, `crates/nostream-relay/src/relay.rs:607-618`). Three fixed reason strings now exist, each lowercase and resource-stating, per the convention `SLOW_CONSUMER_REASON` set (`crates/nostream-relay/src/transport.rs:37-47`): @@ -163,8 +163,8 @@ the queue bound has already latched does not even pay that, because `send` retur The read does not finish there: it waits on the read's cancellation watch and then calls `end_read`. **On the refusal path that wait is never resolved.** Every exit of the connection loop is a `break`, -`handlers.abort_all()` runs before `connection.close()` (`crates/nostream-relay/src/relay.rs:494-495`), -and a task aborted by the `JoinSet` it was spawned into (`crates/nostream-relay/src/relay.rs:377`, +`handlers.abort_all()` runs before `connection.close()` (`crates/nostream-relay/src/relay.rs:571-572`), +and a task aborted by the `JoinSet` it was spawned into (`crates/nostream-relay/src/relay.rs:454`, `:602-603`) is dropped without being polled again: the parked read is dropped inside the watch, the `end_read` below it never runs, and the store stream goes with it. What retires a refused read in production is therefore the abort, not the watch — the watch is the path a client can still take, by @@ -233,7 +233,7 @@ send holds a sink and therefore cannot be in flight or begin once that drop star the refund **exactly this socket's bytes, once**, whatever the connection loop did on the way out. **The rejected shape: a decrement at each of the loop's exits.** Today `release` has exactly one -caller, the drain arm (`crates/nostream-relay/src/relay.rs:407`), and every other exit leaves whatever +caller, the drain arm (`crates/nostream-relay/src/relay.rs:486`), and every other exit leaves whatever was still queued counted: the drain itself (`:384-385`), the refusal close (`:391-394`), the `None` receiver (`:398`), a failed write (`:409-411`), the inbound arms (`:414`, `:433-435`, `:440`, `:443-445`), the closed hub (`:460`), the liveness arms (`:463-469`, `:479-481`) and the final @@ -279,10 +279,10 @@ checked in `handle_socket` **after** `is_connection_rate_limited` and **before** - before `run_connection`, because `Connection::accept`'s first act is to send the NIP-42 challenge as a text frame (`crates/nostream-relay/src/connection.rs:109-115`) — a client refused afterwards would already have received a text frame, which the policy forbids. The window is - `crates/nostream-relay/src/relay.rs:292-315`. + `crates/nostream-relay/src/relay.rs:369-393`. Acquisition returns a `ConnectionSlot` held for the rest of `handle_socket` -(`crates/nostream-relay/src/relay.rs:299`), so the normal exit, the registration failure inside +(`crates/nostream-relay/src/relay.rs:376`), so the normal exit, the registration failure inside `run_connection`, and any early return added later all release it through `Drop` (`crates/nostream-relay/src/connection_cap.rs:101-116`). A count that leaks makes the cap permanent for that address, which is a defect and not a memory leak. The map entry is removed when its count @@ -308,13 +308,13 @@ prefixes, and it is not this job's. ## What an operator can see today Two accessors in the `hidden_content()` style, `Relay::frame_budget()` and -`Relay::connection_cap()` (`crates/nostream-relay/src/relay.rs:175-182`), and the counters behind +`Relay::connection_cap()` (`crates/nostream-relay/src/relay.rs:257-259`), and the counters behind them: `accounted()`, `limit()`, `refusals()` on the budget; `live(ip)`, `max_per_ip()`, `refusals()`, `tracked_addresses()` on the cap. Plus two structured log lines, one per refusal — a `warn!` carrying `budget_bytes` and `accounted_bytes` from the sink (`crates/nostream-relay/src/sink.rs:321-325`), and a `debug!` carrying `peer`, `max_connections_per_ip` and `live` from the connection task -(`crates/nostream-relay/src/relay.rs:300-305`). +(`crates/nostream-relay/src/relay.rs:377-382`). **Those two lines are not visible at the same level, and the daemon ships `info`.** The filter is `--log-level`, then `LOG_LEVEL`, then `RUST_LOG`, else `info` @@ -382,12 +382,12 @@ strictly worse than the default it replaced, which is the wrong direction for a The constructor implements exactly that, and nothing else: - the budget is taken only when the leaf is present, convertible, positive and within the - ceiling — " value > 0 && value <= MAX_SETTING_BOUND" (`crates/nostream-relay/src/relay.rs:109`), + ceiling — " value > 0 && value <= MAX_SETTING_BOUND as u64" (`crates/nostream-relay/src/relay.rs:115`), applied to the leaf at " if let Some(bytes) = settings.network.frame_memory_budget_bytes" - (`crates/nostream-relay/src/relay.rs:130`); + (`crates/nostream-relay/src/relay.rs:136`); - the cap likewise, through the two optional levels the typed schema forces — " .and_then(|connection| connection.max_connections_per_ip)" - (`crates/nostream-relay/src/relay.rs:140`); + (`crates/nostream-relay/src/relay.rs:157`); - otherwise the field keeps `RelayConfig::default()`'s value, so a zero, an absent key — absent leaf, absent `limits`, absent `limits.connection` — and a number above the ceiling all behave as "the shipped default". @@ -401,10 +401,37 @@ passes it through, and the budget then stops bounding anything: the fit guard su debug build and wraps in a release one, and a wrapped counter refills the budget it was meant to exhaust. The ceiling is what keeps an operator's number out of that arithmetic: "pub const MAX_SETTING_BOUND: usize = 1024 * 1024 * 1024 * 1024;" -(`crates/nostream-relay/src/relay.rs:105`). A TiB of queued frames and a million sockets from one -address are far above any deployment; a too-large value falls back exactly as a zero does, and a -value at the ceiling is accepted. **The unchecked add itself is not this job's** — it predates the -bounds and is unreachable through settings now — so it is reported rather than changed here. +(`crates/nostream-relay/src/relay.rs:108`). One number serves both keys because one predicate reads +them, and each reads it in its own unit: 1 099 511 627 776 **bytes** is 1 TiB of queued frames for +`network.frameMemoryBudgetBytes`, and 1 099 511 627 776 **sockets** for +`limits.connection.maxConnectionsPerIp` — a count no deployment reaches, and the reason the cap's +share of the constant is generous rather than tuned. Both are far below `usize::MAX`; a too-large +value falls back exactly as a zero does, and a value at the ceiling is accepted. **The unchecked +add itself is not this job's** — it predates the bounds and is unreachable through settings now — +so it is reported rather than changed here. + +**The fallback is not silent any more, and it used to be.** Measured before this fix, `--check` +printed `settings valid: 0 issues` for `1099511627776` and for `1099511627777`, whose enforced +budgets differ by 4000× (1024 GiB against 268 435 456), and said nothing about the second. A +value-level fallback is this repository's policy — `network.maxPayloadSize` behaves the same and +`--check` reports schema faults, not range — so no validator rule is added here. Instead both +places an operator looks now say it: at start-up the constructor warns — +` "setting out of range for a bound; using the default"` +(`crates/nostream-relay/src/relay.rs:148`) — naming the setting, the value, the ceiling and the +default it fell back to, and `--check` reports the same verdict before a relay exists: +`eprintln!` in the gate, using the same `in_range` the constructor uses — +"pub fn in_range(value: u64) -> bool {" (`crates/nostream-relay/src/relay.rs:114`). `warn!` rather +than `debug!` because the start-up line is a one-off statement about a configuration that is +wrong, not a per-request event, so it is not log-amplifiable by a client; `--check` writes outside +the `tracing` filter by design (`crates/nostreamd/src/settings_gate.rs:1-18`), so its report is +reached at the shipped level too. What is **not** fixed, deliberately: `--check` still exits 0 on +a value it will replace. That is the existing policy for a valid key with an out-of-range value, +and changing it means a validator rule, which is out of this job. + +**The pair's real ceiling is not the cap's.** The budget is process-wide and the cap is per +address, so what the two bound together is `min(budget, addresses × cap × frame_queue_bytes)` — the +cap alone cannot be read as a memory ceiling, and the shipped `64 × 8 MiB` is one address's share, +not the process's. **The guided catalogue does not bound these two, and no validator in the repository does.** The catalogue has two validators — " SafeNonNegativeInteger," (`crates/nostream-config/src/guided.rs:25`) @@ -414,8 +441,12 @@ and `NonEmpty` — and `SafeNonNegativeInteger`'s ceiling is the capture's maxim (`src/utils/settings-guided-schema.ts` has one), so both entries carry it as its precedent `network.maxPayloadSize` does, and an editor accepts about 9 exabytes of budget and about 9 quadrillion sockets — values the constructor then treats as the shipped default, at the operator's -console. A bounded-integer validator is the fix and is **not** in this job: it is a new validator -kind with its own messages, and it belongs with the guided catalogue's unimplemented consumers. +console. A bounded-integer validator is the fix and is **not** in this job: the capture composes +its ceiling into the same primitive — +" .pipe(z.number().int().min(0).max(Number.MAX_SAFE_INTEGER, 'Value must be a safe integer'))" +(`src/utils/settings-guided-schema.ts:31`) — so the change is a bound on one rule, with messages of +its own, and it belongs with the guided catalogue's unimplemented consumers rather than with this +job. Both entries say what the number accepts and what a zero means in their labels — "Frame memory budget (bytes, 0 means the default)" (`crates/nostream-config/src/guided.rs:210`) — so the editor at least does not mislabel the value. diff --git a/resources/default-settings.yaml b/resources/default-settings.yaml index 5d0d38a2..93132151 100755 --- a/resources/default-settings.yaml +++ b/resources/default-settings.yaml @@ -219,7 +219,7 @@ limits: connection: # The live sockets one source address may hold at once. Unit: sockets; the guided editor calls # it "Max connections per IP (0 means the default)". Defaults to 64; 0 — or anything above - # 1099511627776 — means the default (ADR 0008). + # 1099511627776 (about a million times any deployment) — means the default (ADR 0008). maxConnectionsPerIp: 64 rateLimits: - period: 1000 From 593eac8206f230514f5e1482cd5146b0c5d760a4 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 03:06:51 -0400 Subject: [PATCH 20/26] fix(daemon): tell a zero bound from one above the ceiling, on both read-only doors --- Cargo.lock | 1 + crates/nostream-cli/Cargo.toml | 1 + crates/nostream-cli/src/config.rs | 27 +++++++- crates/nostream-cli/tests/config_cli.rs | 27 ++++++++ crates/nostream-relay/src/relay.rs | 83 +++++++++++++++++++++---- crates/nostreamd/src/settings_gate.rs | 47 ++++++-------- crates/nostreamd/tests/check_flag.rs | 34 +++++++++- scripts/citation-meaning.py | 63 +++++++++++++++++++ 8 files changed, 241 insertions(+), 42 deletions(-) create mode 100755 scripts/citation-meaning.py diff --git a/Cargo.lock b/Cargo.lock index e68fdad8..53251089 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1151,6 +1151,7 @@ dependencies = [ "clap", "nostream-config", "nostream-core", + "nostream-relay", "serde_json", "serde_yaml", "tempfile", diff --git a/crates/nostream-cli/Cargo.toml b/crates/nostream-cli/Cargo.toml index 1eceabdc..05ec9439 100644 --- a/crates/nostream-cli/Cargo.toml +++ b/crates/nostream-cli/Cargo.toml @@ -15,6 +15,7 @@ path = "src/main.rs" clap.workspace = true anyhow = { workspace = true } nostream-config.workspace = true +nostream-relay.workspace = true nostream-core.workspace = true serde_json.workspace = true serde_yaml.workspace = true diff --git a/crates/nostream-cli/src/config.rs b/crates/nostream-cli/src/config.rs index 33becf63..a85cd876 100644 --- a/crates/nostream-cli/src/config.rs +++ b/crates/nostream-cli/src/config.rs @@ -11,6 +11,7 @@ use nostream_config::{ SettingsPaths, fit, load, load_defaults, load_document, load_user_settings, merge, validate_document, }; +use nostream_relay::relay::{BoundRefusal, refused_bounds}; use serde_yaml::Value; use crate::error::CliError; @@ -67,8 +68,32 @@ pub fn validate( } } if issues.is_empty() { + // The daemon bounds two settings at run time and ignores a value it cannot use. That is + // not a schema fault, so it is not an issue and not fatal — but this door and the + // daemon's `--check` are the two read-only ways an operator looks, so the verdict says + // it here too, in the same words the daemon's gate uses (ADR 0008). + let refused = + match serde_yaml::from_value::(merged.clone()) { + Ok(settings) => refused_bounds(&settings), + // The document already passed `fit`, so this cannot fail; with no typed value there + // is nothing to report rather than a reason to refuse a config the daemon can load. + Err(_) => Vec::new(), + }; + + let mut verdict = "settings valid: 0 issues".to_owned(); + for (setting, reason) in refused { + match reason { + BoundRefusal::Zero => verdict.push_str(&format!( + "; {setting}: 0 means the default, and the relay runs with it" + )), + BoundRefusal::AboveCeiling(ceiling) => verdict.push_str(&format!( + "; {setting}: above the {ceiling} a bound accepts; \ + the key's value is ignored" + )), + } + } return Ok(CommandOutput { - lines: vec!["settings valid: 0 issues".to_owned()], + lines: vec![verdict], exit_code: 0, }); } diff --git a/crates/nostream-cli/tests/config_cli.rs b/crates/nostream-cli/tests/config_cli.rs index 0cfcadfe..aba6453b 100644 --- a/crates/nostream-cli/tests/config_cli.rs +++ b/crates/nostream-cli/tests/config_cli.rs @@ -66,3 +66,30 @@ fn the_paths_resolver_honours_the_config_dir() { ); assert_eq!(paths.config_base_dir, directory.path().to_path_buf()); } + +#[test] +fn config_validate_says_which_bound_setting_the_relay_will_not_use() { + // The CLI door and `nostreamd --check` are the two read-only ways onto the same validator; + // a bound the relay replaces must not be silent on one of them. Zero and an over-ceiling + // value are different facts and get different sentences. + let directory = tempfile::tempdir().expect("tempdir"); + let path = directory.path().join("settings.yaml"); + fs::write( + &path, + "network:\n frameMemoryBudgetBytes: 0\nlimits:\n connection:\n maxConnectionsPerIp: 1099511627777\n", + ) + .expect("write"); + + let output = + config::validate(&workspace_root(), Some(directory.path()), Some(&path)).expect("validate"); + assert_eq!(output.exit_code, 0); + let verdict = output.lines.join("\n"); + assert!( + verdict.contains("network.frameMemoryBudgetBytes: 0 means the default"), + "verdict: {verdict}" + ); + assert!( + verdict.contains("limits.connection.maxConnectionsPerIp: above the 1099511627776"), + "verdict: {verdict}" + ); +} diff --git a/crates/nostream-relay/src/relay.rs b/crates/nostream-relay/src/relay.rs index 44a30963..6ea3f7a0 100644 --- a/crates/nostream-relay/src/relay.rs +++ b/crates/nostream-relay/src/relay.rs @@ -115,6 +115,69 @@ pub fn in_range(value: u64) -> bool { value > 0 && value <= MAX_SETTING_BOUND as u64 } +/// Why a value was not taken from a bound's setting: the question [`in_range`] answers, said out +/// loud, so the report can distinguish the two cases the predicate collapses — a zero, which the +/// document says means the default, from a value above the ceiling, which it does not. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum BoundRefusal { + /// The operator wrote `0`, which the shipped document documents as "the default". + Zero, + /// The value is above [`MAX_SETTING_BOUND`]. + AboveCeiling(u64), +} + +/// The two settings whose value is a bound, each with the value the operator wrote when that value +/// is not taken. This is the only walk of the two leaves outside [`RelayConfig::from_settings`]: +/// the daemon's gate and the CLI's `config validate` both report from it, so a key added to one +/// surface cannot be reported by one door and ignored by another (ADR 0008). +pub fn refused_bounds(settings: &Settings) -> Vec<(&'static str, BoundRefusal)> { + let mut refused = Vec::new(); + if let Some(bytes) = settings.network.frame_memory_budget_bytes + && let Some(reason) = bound_refusal(bytes) + { + refused.push(("network.frameMemoryBudgetBytes", reason)); + } + if let Some(cap) = settings + .limits + .as_ref() + .and_then(|limits| limits.connection.as_ref()) + .and_then(|connection| connection.max_connections_per_ip) + && let Some(reason) = bound_refusal(cap) + { + refused.push(("limits.connection.maxConnectionsPerIp", reason)); + } + refused +} + +/// The two ways a bound's setting is not taken, or `None` when it is. +pub fn bound_refusal(value: u64) -> Option { + if value == 0 { + Some(BoundRefusal::Zero) + } else if value > MAX_SETTING_BOUND as u64 { + Some(BoundRefusal::AboveCeiling(MAX_SETTING_BOUND as u64)) + } else { + None + } +} + +/// Reports one bound's setting that the relay will not run with, saying which of the two reasons +/// it was: a zero (documented as the default) and a value above the ceiling are different facts. +fn warn_refused_bound(setting: &'static str, value: u64, default: usize) { + match bound_refusal(value) { + Some(BoundRefusal::Zero) => { + tracing::warn!(setting, default, "a bound set to 0 runs with its default") + } + Some(BoundRefusal::AboveCeiling(ceiling)) => tracing::warn!( + setting, + value, + ceiling, + default, + "a bound above its ceiling runs with its default" + ), + None => {} + } +} + impl RelayConfig { /// The configuration a relay built from `settings` runs with: [`RelayConfig::default`] for /// every knob the capture fixes in code, with the two bounds an operator sets read from @@ -140,12 +203,10 @@ impl RelayConfig { // `warn!`, not `debug!`: the value passes `--check` and is then quietly // replaced, so this line and the gate's report are the only places an operator // learns that the number they wrote is not the number the relay runs with. - tracing::warn!( - setting = "network.frameMemoryBudgetBytes", - value = bytes, - ceiling = MAX_SETTING_BOUND, - default = config.frame_memory_budget_bytes, - "setting out of range for a bound; using the default" + warn_refused_bound( + "network.frameMemoryBudgetBytes", + bytes, + config.frame_memory_budget_bytes, ); } } @@ -159,12 +220,10 @@ impl RelayConfig { if in_range(cap) { config.max_connections_per_ip = cap as usize; } else { - tracing::warn!( - setting = "limits.connection.maxConnectionsPerIp", - value = cap, - ceiling = MAX_SETTING_BOUND, - default = config.max_connections_per_ip, - "setting out of range for a bound; using the default" + warn_refused_bound( + "limits.connection.maxConnectionsPerIp", + cap, + config.max_connections_per_ip, ); } } diff --git a/crates/nostreamd/src/settings_gate.rs b/crates/nostreamd/src/settings_gate.rs index c82a2390..ef62d492 100644 --- a/crates/nostreamd/src/settings_gate.rs +++ b/crates/nostreamd/src/settings_gate.rs @@ -17,42 +17,35 @@ //! request, per message or per streamed record. use nostream_config::LoadOutcome; -use nostream_relay::relay::{MAX_SETTING_BOUND, in_range}; +use nostream_relay::relay::{BoundRefusal, refused_bounds}; use crate::error::DaemonError; -/// The two settings whose value the relay bounds at run time, with the default it falls back to. -/// A value out of range is not a schema fault — the key is valid and the number is a number — so -/// it is neither an issue nor fatal; but `--check` is where an operator looks before starting, so -/// a value about to be replaced is reported here rather than only at start-up (ADR 0008). -fn out_of_range_bounds(settings: &nostream_config::settings::Settings) -> Vec<(&'static str, u64)> { - let mut dropped = Vec::new(); - if let Some(bytes) = settings.network.frame_memory_budget_bytes - && !in_range(bytes) - { - dropped.push(("network.frameMemoryBudgetBytes", bytes)); - } - if let Some(cap) = settings - .limits - .as_ref() - .and_then(|limits| limits.connection.as_ref()) - .and_then(|connection| connection.max_connections_per_ip) - && !in_range(cap) - { - dropped.push(("limits.connection.maxConnectionsPerIp", cap)); +/// Prints one refused bound. A zero is the documented "use the default", so its line says only +/// that; an over-ceiling value names the ceiling. Neither says the relay falls back to *this key's* +/// default on top of a schema fault, because it would not: a rejected document is replaced whole — +/// " settings: defaults_settings," (`crates/nostream-config/src/load.rs:421`). +fn report_refused_bound(setting: &str, reason: BoundRefusal) { + match reason { + BoundRefusal::Zero => { + eprintln!("{setting}: 0 means the default, and the relay runs with it"); + } + BoundRefusal::AboveCeiling(ceiling) => { + eprintln!("{setting}: above the {ceiling} a bound accepts; the key's value is ignored") + } } - dropped } /// Applies the gate to one load outcome: report the findings, decide whether they are fatal. pub fn apply_settings_gate(outcome: &LoadOutcome, check: bool) -> Result<(), DaemonError> { + // Independent of the schema verdict: an unrelated typo must not hide a range report, and a + // rejected document has already been replaced whole, so the report says what happened to the + // number rather than what the relay runs with. + for (setting, reason) in refused_bounds(&outcome.settings) { + report_refused_bound(setting, reason); + } + if outcome.issues.is_empty() { - for (setting, value) in out_of_range_bounds(&outcome.settings) { - eprintln!( - "{setting}: {value} is above the {MAX_SETTING_BOUND} a bound accepts; \ - the relay runs with its default" - ); - } if check { println!("settings valid: 0 issues"); } diff --git a/crates/nostreamd/tests/check_flag.rs b/crates/nostreamd/tests/check_flag.rs index deb48a49..e31b5729 100644 --- a/crates/nostreamd/tests/check_flag.rs +++ b/crates/nostreamd/tests/check_flag.rs @@ -170,11 +170,41 @@ fn check_reports_a_bound_it_would_replace_with_the_default() { assert_eq!(code, Some(0), "exit code"); assert_eq!(stdout.trim(), "settings valid: 0 issues"); assert!( - stderr.contains("network.frameMemoryBudgetBytes: 1099511627777"), + stderr.contains("network.frameMemoryBudgetBytes: above the 1099511627776 a bound accepts"), "stderr: {stderr}" ); assert!( - stderr.contains("limits.connection.maxConnectionsPerIp: 1099511627777"), + stderr.contains( + "limits.connection.maxConnectionsPerIp: above the 1099511627776 a bound accepts" + ), "stderr: {stderr}" ); } + +#[test] +fn check_reports_a_zero_bound_as_the_documented_default() { + // Zero is not "out of range": both the defaults document and `--check` document it as the + // default, so the line must not call it a violation. + let directory = tempfile::tempdir().expect("tempdir"); + std::fs::write( + directory.path().join("settings.yaml"), + "network:\n frameMemoryBudgetBytes: 0\nlimits:\n connection:\n maxConnectionsPerIp: 0\n", + ) + .expect("write"); + + let config_dir = directory.path().to_string_lossy().into_owned(); + let (code, stdout, stderr) = run_check( + Duration::from_secs(30), + &["--check", "--config-dir", &config_dir], + ); + assert_eq!(code, Some(0), "exit code"); + assert_eq!(stdout.trim(), "settings valid: 0 issues"); + assert!( + stderr.contains("network.frameMemoryBudgetBytes: 0 means the default"), + "stderr: {stderr}" + ); + assert!( + !stderr.contains("above the"), + "a zero must not be reported as above the ceiling: {stderr}" + ); +} diff --git a/scripts/citation-meaning.py b/scripts/citation-meaning.py new file mode 100755 index 00000000..0d7e2f1c --- /dev/null +++ b/scripts/citation-meaning.py @@ -0,0 +1,63 @@ +#!/usr/bin/env python3 +"""citation-meaning — print each cited line beside the sentence that cites it. + +The factory's `citation-oracle.ts` proves a `path:line` resolves and is in bounds. It does not +read the line, so a citation that drifted into a different function passes. This script closes +that gap the only honest way a machine can: it prints both texts, adjacent, so a reader (or a +later pass) compares meaning instead of bounds. + + python3 scripts/citation-meaning.py [ ...] + +For every `path:line` / `path:a-b` it prints: + + L :(-) EXISTS|MISSING|OUT + cites: + says: + +Exit 1 when any citation is MISSING or OUT, so it also works as a check; 0 otherwise. +""" +import os +import re +import sys + +REF = re.compile(r"((?:[\w.\-]+/)*[\w.\-]+\.[A-Za-z0-9]{1,8}):(\d+)(?:-(\d+))?") + + +def resolve(path: str) -> str | None: + candidate = path + while True: + if os.path.isfile(candidate): + return candidate + if "/" not in candidate: + return None + candidate = candidate.split("/", 1)[1] + + +def main(docs: list[str]) -> int: + bad = 0 + for doc in docs: + print(f"== {doc}") + for number, line in enumerate(open(doc, errors="replace"), 1): + for match in REF.finditer(line): + path, first, last = match.group(1), int(match.group(2)), match.group(3) + end = int(last) if last else first + real = resolve(path) + if real is None: + print(f" L{number} {match.group(0)} MISSING") + bad += 1 + continue + body = open(real, errors="replace").read().split("\n") + if not (1 <= first <= len(body)) or end > len(body): + print(f" L{number} {match.group(0)} OUT ({len(body)} lines)") + bad += 1 + continue + print(f" L{number} {match.group(0)} EXISTS") + print(f" cites: {line.strip()[:150]}") + print(f" says: {body[first - 1].strip()[:150]}") + if end != first: + print(f" ends: {body[end - 1].strip()[:150]}") + return 1 if bad else 0 + + +if __name__ == "__main__": + sys.exit(main(sys.argv[1:])) From fdc9b22a9371105ccd103aa31744c6d247fbfa1d Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 03:08:11 -0400 Subject: [PATCH 21/26] feat(config): name the cap's unit in its guided label --- crates/nostream-config/src/guided.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/nostream-config/src/guided.rs b/crates/nostream-config/src/guided.rs index ed0800bb..45e4be15 100644 --- a/crates/nostream-config/src/guided.rs +++ b/crates/nostream-config/src/guided.rs @@ -270,7 +270,7 @@ fn build_categories() -> Vec { SafeNonNegativeInteger, ), validated( - "Max connections per IP (0 means the default)", + "Max connections per IP (sockets, 0 means the default)", "limits.connection.maxConnectionsPerIp", Number, SafeNonNegativeInteger, From 78ef4cf5d025f456fe2c4ef1832ab7999526aa64 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 03:11:28 -0400 Subject: [PATCH 22/26] docs(rewrite): say what the skip does, and re-anchor the citations it moved --- .../decisions/0005-connection-concurrency.md | 7 +- .../decisions/0008-frame-memory-budget.md | 109 +++++++++++------- 2 files changed, 74 insertions(+), 42 deletions(-) diff --git a/docs/rewrite/decisions/0005-connection-concurrency.md b/docs/rewrite/decisions/0005-connection-concurrency.md index df9efd0e..beb00391 100644 --- a/docs/rewrite/decisions/0005-connection-concurrency.md +++ b/docs/rewrite/decisions/0005-connection-concurrency.md @@ -115,10 +115,11 @@ need — and these two port-only policies, which every test so far runs at their argument against porting a *captured* key, and these two are not captured knobs. They are the port's own denial-of-service bounds, and a bound an operator cannot set in production is a policy constant wearing a config field's name — the state #989 was filed against. Both are now settings -(`network.frameMemoryBudgetBytes` and `limits.connection.maxConnectionsPerIp`), read into -`RelayConfig` in one place, so the compiled default and the document cannot disagree: +(`network.frameMemoryBudgetBytes` and `limits.connection.maxConnectionsPerIp`), interpreted in one +place — `from_settings` is the only answer the relay runs with, so the compiled default and the +document cannot disagree: "pub fn from_settings(settings: &Settings) -> Self {" -(`crates/nostream-relay/src/relay.rs:133`), from leaves the document carries: +(`crates/nostream-relay/src/relay.rs:196`), from leaves the document carries: "pub frame_memory_budget_bytes: Option," (`crates/nostream-config/src/settings.rs:71`). The write timeout and the per-socket queue bound stay `RelayConfig`-only, as this section decided; ADR 0008 carries what a zero means for the two that diff --git a/docs/rewrite/decisions/0008-frame-memory-budget.md b/docs/rewrite/decisions/0008-frame-memory-budget.md index ceeaa08c..99162fee 100644 --- a/docs/rewrite/decisions/0008-frame-memory-budget.md +++ b/docs/rewrite/decisions/0008-frame-memory-budget.md @@ -12,7 +12,7 @@ The outbound frame queue was bounded per socket and nothing bounded their sum. (`crates/nostream-relay/src/sink.rs:260-334`), and `Relay::serve` accepted every connection with no count at all. The only per-address bookkeeping was the **attempt** limiter, which throttles how fast connections open, not how many are held (`crates/nostream-relay/src/limits.rs`, windows -consumed at `crates/nostream-relay/src/relay.rs:404`). +consumed at `crates/nostream-relay/src/relay.rs:463`). The operator's policy comment on #959 chose the numbers and the refusal and asked for two consequences a reviewer can hold the change to: both bounds refuse the same way, and the budget is @@ -20,14 +20,14 @@ released when a socket closes ("a leak here is worse than no bound"). **What the change is worth, measured.** One address that opens sockets and then stops reading held `N × 8 MiB`. With the shipped attempt windows — 12 per 1 000 ms and 48 per 60 000 ms -(`resources/default-settings.yaml:215-224`) — one address gets 48 sockets a minute indefinitely, so +(`resources/default-settings.yaml:219-229`) — one address gets 48 sockets a minute indefinitely, so the pre-change worst case is **384 MiB**, and that figure is **arithmetic from named constants, not a measurement**; it is the same derivation the performance lens made, marked there as arithmetic. **The failure the counter exists to avoid was measured on the wire.** Twenty sockets opened back to back on a fresh daemon: the first twelve received `HTTP/1.1 101 Switching Protocols` and then a text frame starting `["AUTH","…`, and the remaining eight were upgraded and dropped with **no websocket close frame at all** — the attempt limiter's silent return -(`crates/nostream-relay/src/relay.rs:364-367`). A refusal indistinguishable from a network fault is +(`crates/nostream-relay/src/relay.rs:423-426`). A refusal indistinguishable from a network fault is exactly what the new bounds are not allowed to be, so each of them closes with a code, a fixed reason and its own counter. @@ -67,7 +67,7 @@ and folding them in would make one knob mean two things. ## The config surface — `RelayConfig`, not settings Both fields are `RelayConfig` fields, `frame_memory_budget_bytes` at -`crates/nostream-relay/src/relay.rs:64` and `max_connections_per_ip` at `:67`, built into the relay's +`crates/nostream-relay/src/relay.rs:66` and `max_connections_per_ip` at `:70`, built into the relay's two shared values in `Relay::new` (`:146-147`). ADR 0005 already decided the question for this class of knob: @@ -75,7 +75,7 @@ of knob: > counterpart for either … so a settings key would be an operator-facing surface mirroring nothing. > `RelayConfig` is where this branch already keeps the knobs the capture fixes in code … -— `docs/rewrite/decisions/0005-connection-concurrency.md:104-110` +— `docs/rewrite/decisions/0005-connection-concurrency.md:105-111` **The honest cost, as this job left it:** an operator could not change either number without a rebuild, because "configurable" meant readable and settable through a `RelayConfig` literal rather @@ -85,16 +85,21 @@ in a second document.** Both numbers are now operator settings, `network.frameMe leaves beside the keys that already live in those sections: "pub max_connections_per_ip: Option," (`crates/nostream-config/src/settings.rs:312`). -One function reads them and nothing else does — the daemon and the test share it, so they cannot -drift: "pub fn from_settings(settings: &Settings) -> Self {" -(`crates/nostream-relay/src/relay.rs:133`). The daemon calls it once and keeps its one environment +`from_settings` is the one place either setting is *interpreted* — the daemon and the test share +it, so they cannot drift: "pub fn from_settings(settings: &Settings) -> Self {" +(`crates/nostream-relay/src/relay.rs:196`). The daemon calls it once and keeps its one environment read: "let mut relay_config = RelayConfig::from_settings(&settings);" -(`crates/nostreamd/src/main.rs:110`). `RelayConfig::default()` remains the single source of the two +(`crates/nostreamd/src/main.rs:110`). One other function walks the same two leaves — `refused_bounds`, +so the gate and the CLI can report a value the constructor will not take — and it shares +`bound_refusal` with the constructor, so the two cannot disagree about which values those are: +"pub fn refused_bounds(settings: &Settings) -> Vec<(&'static str, BoundRefusal)> {" +(`crates/nostream-relay/src/relay.rs:133`). It never produces a config; +`from_settings` is the only reader whose answer the relay runs with. `RelayConfig::default()` remains the single source of the two numbers — the document mirrors it, and a test in `crates/nostream-relay/tests/config_defaults.rs` fails when they part — because a missing key must still mean a working relay, not a relay that needs the document at start-up. The compiled values are unchanged: -"frame_memory_budget_bytes: 256 * 1024 * 1024," (`crates/nostream-relay/src/relay.rs:90`) and -"max_connections_per_ip: 64," (`crates/nostream-relay/src/relay.rs:94`). +"frame_memory_budget_bytes: 256 * 1024 * 1024," (`crates/nostream-relay/src/relay.rs:92`) and +"max_connections_per_ip: 64," (`crates/nostream-relay/src/relay.rs:97`). Two consequences a reader should have in one place. The document gains its first key whose name states a byte unit, `frameMemoryBudgetBytes`; the convention it breaks is that the unit lives in @@ -112,7 +117,7 @@ them). Both bounds close the socket with `CloseCode::Again` (1013) written straight to the transport rather than through the sink, bounded by `config.write_timeout` exactly as the existing slow-consumer close -is (`Relay::close_frame`, `crates/nostream-relay/src/relay.rs:607-618`). Three fixed reason strings +is (`Relay::close_frame`, `crates/nostream-relay/src/relay.rs:666-677`). Three fixed reason strings now exist, each lowercase and resource-stating, per the convention `SLOW_CONSUMER_REASON` set (`crates/nostream-relay/src/transport.rs:37-47`): @@ -163,8 +168,8 @@ the queue bound has already latched does not even pay that, because `send` retur The read does not finish there: it waits on the read's cancellation watch and then calls `end_read`. **On the refusal path that wait is never resolved.** Every exit of the connection loop is a `break`, -`handlers.abort_all()` runs before `connection.close()` (`crates/nostream-relay/src/relay.rs:571-572`), -and a task aborted by the `JoinSet` it was spawned into (`crates/nostream-relay/src/relay.rs:454`, +`handlers.abort_all()` runs before `connection.close()` (`crates/nostream-relay/src/relay.rs:630-631`), +and a task aborted by the `JoinSet` it was spawned into (`crates/nostream-relay/src/relay.rs:513`, `:602-603`) is dropped without being polled again: the parked read is dropped inside the watch, the `end_read` below it never runs, and the store stream goes with it. What retires a refused read in production is therefore the abort, not the watch — the watch is the path a client can still take, by @@ -233,11 +238,13 @@ send holds a sink and therefore cannot be in flight or begin once that drop star the refund **exactly this socket's bytes, once**, whatever the connection loop did on the way out. **The rejected shape: a decrement at each of the loop's exits.** Today `release` has exactly one -caller, the drain arm (`crates/nostream-relay/src/relay.rs:486`), and every other exit leaves whatever -was still queued counted: the drain itself (`:384-385`), the refusal close (`:391-394`), the `None` -receiver (`:398`), a failed write (`:409-411`), the inbound arms (`:414`, `:433-435`, `:440`, -`:443-445`), the closed hub (`:460`), the liveness arms (`:463-469`, `:479-481`) and the final -cleanup (`:494-495`); the lagged hub (`:455-457`) warns and does not leave. Per socket that leak is +caller, the release after a frame is written (`crates/nostream-relay/src/relay.rs:543`), and every +other exit leaves whatever was still queued counted: the drain entry (`:519-521`), the refusal +close (`:527-529`), the closed frame queue (`:534`), a failed write (`:545-547`), the closed +inbound stream (`:550`), an inbound `CLOSE` (`:576`), a failed read (`:578-580`), a failed pong +write (`:568-570`), the closed hub (`:596`), the pong timeout (`:603`) and a failed ping write +(`:611-615`) and the final cleanup (`:630-631`); the lagged hub (`:591-593`) warns and does not +leave. Per socket that leak is invisible because the counter dies with the sink; a process-wide budget makes it a leak of up to 8 MiB on every disconnect. Writing the decrement at each of those sites is the change that looks right and rots: @@ -253,7 +260,7 @@ written. **The parked read holds its store stream, and that is what must change when a store is wired.** Stopping at the refusal without leaving the loop keeps the stream from `find_by_filters` alive until the loop aborts the read, a close frame later; the version before this one dropped it at the refusal. -Latent today — the daemon wires `unavailable::stores()` (`crates/nostreamd/src/main.rs:113`), whose +Latent today — the daemon wires `unavailable::stores()` (`crates/nostreamd/src/main.rs:118`), whose reads are stubs — but not for ever: sqlx's `&Pool` executor draws a pooled connection for as long as a query stream is alive. The size that matters is the deployed one, not the code's: `DB_MAX_POOL_SIZE` defaults to **3** (`crates/nostream-storage/src/postgres/mod.rs:362`) but @@ -279,10 +286,10 @@ checked in `handle_socket` **after** `is_connection_rate_limited` and **before** - before `run_connection`, because `Connection::accept`'s first act is to send the NIP-42 challenge as a text frame (`crates/nostream-relay/src/connection.rs:109-115`) — a client refused afterwards would already have received a text frame, which the policy forbids. The window is - `crates/nostream-relay/src/relay.rs:369-393`. + `crates/nostream-relay/src/relay.rs:428-452`. Acquisition returns a `ConnectionSlot` held for the rest of `handle_socket` -(`crates/nostream-relay/src/relay.rs:376`), so the normal exit, the registration failure inside +(`crates/nostream-relay/src/relay.rs:435`), so the normal exit, the registration failure inside `run_connection`, and any early return added later all release it through `Drop` (`crates/nostream-relay/src/connection_cap.rs:101-116`). A count that leaks makes the cap permanent for that address, which is a defect and not a memory leak. The map entry is removed when its count @@ -308,13 +315,14 @@ prefixes, and it is not this job's. ## What an operator can see today Two accessors in the `hidden_content()` style, `Relay::frame_budget()` and -`Relay::connection_cap()` (`crates/nostream-relay/src/relay.rs:257-259`), and the counters behind +`Relay::frame_budget()` (`crates/nostream-relay/src/relay.rs:311-313`) and +`Relay::connection_cap()` (`crates/nostream-relay/src/relay.rs:316-318`), and the counters behind them: `accounted()`, `limit()`, `refusals()` on the budget; `live(ip)`, `max_per_ip()`, `refusals()`, `tracked_addresses()` on the cap. Plus two structured log lines, one per refusal — a `warn!` carrying `budget_bytes` and `accounted_bytes` from the sink (`crates/nostream-relay/src/sink.rs:321-325`), and a `debug!` carrying `peer`, `max_connections_per_ip` and `live` from the connection task -(`crates/nostream-relay/src/relay.rs:377-382`). +(`crates/nostream-relay/src/relay.rs:436-441`). **Those two lines are not visible at the same level, and the daemon ships `info`.** The filter is `--log-level`, then `LOG_LEVEL`, then `RUST_LOG`, else `info` @@ -412,21 +420,42 @@ so it is reported rather than changed here. **The fallback is not silent any more, and it used to be.** Measured before this fix, `--check` printed `settings valid: 0 issues` for `1099511627776` and for `1099511627777`, whose enforced -budgets differ by 4000× (1024 GiB against 268 435 456), and said nothing about the second. A +budgets differ by 4096× (1024 GiB against 268 435 456), and said nothing about the second. A value-level fallback is this repository's policy — `network.maxPayloadSize` behaves the same and -`--check` reports schema faults, not range — so no validator rule is added here. Instead both -places an operator looks now say it: at start-up the constructor warns — -` "setting out of range for a bound; using the default"` -(`crates/nostream-relay/src/relay.rs:148`) — naming the setting, the value, the ceiling and the -default it fell back to, and `--check` reports the same verdict before a relay exists: -`eprintln!` in the gate, using the same `in_range` the constructor uses — -"pub fn in_range(value: u64) -> bool {" (`crates/nostream-relay/src/relay.rs:114`). `warn!` rather -than `debug!` because the start-up line is a one-off statement about a configuration that is -wrong, not a per-request event, so it is not log-amplifiable by a client; `--check` writes outside -the `tracing` filter by design (`crates/nostreamd/src/settings_gate.rs:1-18`), so its report is -reached at the shipped level too. What is **not** fixed, deliberately: `--check` still exits 0 on -a value it will replace. That is the existing policy for a valid key with an out-of-range value, -and changing it means a validator rule, which is out of this job. +`--check` reports schema faults, not range — so no validator rule is added here. Instead the +three places an operator looks say it, in two different sentences, because **a zero and an +over-ceiling value are different facts**: zero is the documented "use the default", and the +ceiling is a violation. `in_range` stays the one range question; `bound_refusal` says why — +"pub fn bound_refusal(value: u64) -> Option {" +(`crates/nostream-relay/src/relay.rs:153`). + +At start-up the constructor warns, once per leaf — "a bound above its ceiling runs with its +default" (`crates/nostream-relay/src/relay.rs:175`) or, for a zero, +" tracing::warn!(setting, default, "a bound set to 0 runs with its default")" +(`crates/nostream-relay/src/relay.rs:168`) — naming the setting, the value where there is one, +the ceiling and the default. `warn!` rather than `debug!` because the start-up line is a one-off +statement about a configuration that is wrong, not a per-request event, so it is not +log-amplifiable by a client. + +`--check` reports the same through the gate — +" eprintln!("{setting}: 0 means the default, and the relay runs with it");" +(`crates/nostreamd/src/settings_gate.rs:31`) and +" eprintln!("{setting}: above the {ceiling} a bound accepts; the key's value is ignored")" +(`crates/nostreamd/src/settings_gate.rs:34`) — and `nostream-cli config validate`, the other +read-only door, appends the same two sentences to its verdict line +(`crates/nostream-cli/src/config.rs:86-93`), from the same `refused_bounds` walk. The gate is not a +`tracing` line because `--check` returns before the subscriber exists: the gate writes outside the +filter by design (`crates/nostreamd/src/settings_gate.rs:8-17`), so the report is reached at the +shipped level. + +**A rejected document changes what these lines can say.** `--check` prints the range report +whether or not the schema is clean, because an unrelated typo must not hide it; but when the +document is rejected it has already been replaced whole — +" settings: defaults_settings," (`crates/nostream-config/src/load.rs:421`) — so the +report then describes the operator's number, not the relay's behaviour that would have followed, +which is why neither sentence claims the relay is running on this key's default. What is +deliberately unchanged: `--check` still exits 0 on a value it will replace, since that is the +policy for a valid key with an out-of-range value and refusing it means a validator rule. **The pair's real ceiling is not the cap's.** The budget is process-wide and the cap is per address, so what the two bound together is `min(budget, addresses × cap × frame_queue_bytes)` — the @@ -448,7 +477,9 @@ its ceiling into the same primitive — its own, and it belongs with the guided catalogue's unimplemented consumers rather than with this job. Both entries say what the number accepts and what a zero means in their labels — "Frame memory -budget (bytes, 0 means the default)" (`crates/nostream-config/src/guided.rs:210`) — so the editor +budget (bytes, 0 means the default)" (`crates/nostream-config/src/guided.rs:210`) and +"Max connections per IP (sockets, 0 means the default)" +(`crates/nostream-config/src/guided.rs:273`) — so the editor at least does not mislabel the value. **The cost, stated:** an operator who wants no budget has no way to ask for one, and this record From 3257f018ec95567c0da61a646f6bbe95a93c0d46 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 03:37:20 -0400 Subject: [PATCH 23/26] fix(relay): derive the refusal from the range check and word it once --- crates/nostream-cli/src/config.rs | 68 ++++++++++--------- crates/nostream-cli/tests/config_cli.rs | 27 ++++++++ crates/nostream-relay/src/relay.rs | 28 ++++++-- .../nostream-relay/tests/config_defaults.rs | 64 ++++++++++++++++- crates/nostreamd/src/settings_gate.rs | 26 ++----- crates/nostreamd/tests/check_flag.rs | 26 +++++++ 6 files changed, 179 insertions(+), 60 deletions(-) diff --git a/crates/nostream-cli/src/config.rs b/crates/nostream-cli/src/config.rs index a85cd876..d3afb86c 100644 --- a/crates/nostream-cli/src/config.rs +++ b/crates/nostream-cli/src/config.rs @@ -11,7 +11,7 @@ use nostream_config::{ SettingsPaths, fit, load, load_defaults, load_document, load_user_settings, merge, validate_document, }; -use nostream_relay::relay::{BoundRefusal, refused_bounds}; +use nostream_relay::relay::{refusal_message, refused_bounds}; use serde_yaml::Value; use crate::error::CliError; @@ -58,39 +58,42 @@ pub fn validate( }; let mut issues = validate_document(&merged, &defaults); + // The typed value the document fits, kept rather than parsed again: it is what the daemon + // would load, and it is what the range report walks. + let mut fitted = None; if issues.is_empty() { // The document walk grades JavaScript types; the typed schema is stricter. A document // the walk approves but the daemon cannot load (a fractional integer, for one) is // refused here, with the field named, so `config validate` never blesses a config the // daemon will discard. - if let Err(issue) = fit(&merged, &defaults) { - issues.push(issue); + match fit(&merged, &defaults) { + Ok(settings) => fitted = Some(settings), + Err(issue) => issues.push(issue), } } - if issues.is_empty() { - // The daemon bounds two settings at run time and ignores a value it cannot use. That is - // not a schema fault, so it is not an issue and not fatal — but this door and the - // daemon's `--check` are the two read-only ways an operator looks, so the verdict says - // it here too, in the same words the daemon's gate uses (ADR 0008). - let refused = - match serde_yaml::from_value::(merged.clone()) { - Ok(settings) => refused_bounds(&settings), - // The document already passed `fit`, so this cannot fail; with no typed value there - // is nothing to report rather than a reason to refuse a config the daemon can load. - Err(_) => Vec::new(), - }; + // The daemon bounds two settings at run time and ignores a value it cannot use. That is not a + // schema fault, so it is not an issue and not fatal — but this door and the daemon's `--check` + // are the two read-only ways an operator looks, so the verdict says it here too, in the words + // `refusal_message` produces for both (ADR 0008). A document the walk refused has no typed + // value to walk — `fit` is what produces one, and it is not reached — so this door is silent + // about the bound on a rejected document, exactly as the daemon's gate is, and for the same + // reason the gate's comment gives. + let refused: Vec = fitted + .as_ref() + .map(|settings| { + refused_bounds(settings) + .into_iter() + .map(|(setting, reason)| refusal_message(setting, reason)) + .collect() + }) + .unwrap_or_default(); + + if issues.is_empty() { let mut verdict = "settings valid: 0 issues".to_owned(); - for (setting, reason) in refused { - match reason { - BoundRefusal::Zero => verdict.push_str(&format!( - "; {setting}: 0 means the default, and the relay runs with it" - )), - BoundRefusal::AboveCeiling(ceiling) => verdict.push_str(&format!( - "; {setting}: above the {ceiling} a bound accepts; \ - the key's value is ignored" - )), - } + for line in refused { + verdict.push_str("; "); + verdict.push_str(&line); } return Ok(CommandOutput { lines: vec![verdict], @@ -98,15 +101,14 @@ pub fn validate( }); } + // A refused document is reported too, and only when there is a typed value to walk (the gate's + // `settings_gate.rs:41-43` comment says the same, from the daemon's side). + let mut lines: Vec = issues.iter().map(|issue| issue.to_string()).collect(); + lines.extend(refused); + lines.push(format!("settings invalid: {} issues", issues.len())); + Ok(CommandOutput { - lines: issues - .iter() - .map(|issue| issue.to_string()) - .chain(std::iter::once(format!( - "settings invalid: {} issues", - issues.len() - ))) - .collect(), + lines, exit_code: 2, }) } diff --git a/crates/nostream-cli/tests/config_cli.rs b/crates/nostream-cli/tests/config_cli.rs index aba6453b..f27a9834 100644 --- a/crates/nostream-cli/tests/config_cli.rs +++ b/crates/nostream-cli/tests/config_cli.rs @@ -93,3 +93,30 @@ fn config_validate_says_which_bound_setting_the_relay_will_not_use() { "verdict: {verdict}" ); } + +#[test] +fn config_validate_reports_the_schema_fault_and_not_a_bound_it_cannot_read() { + // The same mixed document through the other door: the bound is not described, because a + // rejected document has no typed value to walk, and the exit code is the schema verdict. + let directory = tempfile::tempdir().expect("tempdir"); + let path = directory.path().join("settings.yaml"); + fs::write( + &path, + "network:\n frameMemoryBudgetBytes: 1099511627777\nlimits:\n rateLimiter:\n strategy: random\n", + ) + .expect("write"); + + let output = + config::validate(&workspace_root(), Some(directory.path()), Some(&path)).expect("validate"); + assert_eq!(output.exit_code, 2); + let text = output.lines.join("\n"); + assert!( + text.contains("strategy must be ewma or sliding_window"), + "{text}" + ); + assert!(!text.contains("above the"), "no range line: {text}"); + assert_eq!( + output.lines.last().map(String::as_str), + Some("settings invalid: 1 issues") + ); +} diff --git a/crates/nostream-relay/src/relay.rs b/crates/nostream-relay/src/relay.rs index 6ea3f7a0..7d9dd0b4 100644 --- a/crates/nostream-relay/src/relay.rs +++ b/crates/nostream-relay/src/relay.rs @@ -150,18 +150,34 @@ pub fn refused_bounds(settings: &Settings) -> Vec<(&'static str, BoundRefusal)> } /// The two ways a bound's setting is not taken, or `None` when it is. +/// +/// It asks [`in_range`] first, so the constructor and every report agree by construction rather +/// than by two comparisons that happen to match: a value `in_range` accepts is never refused here. pub fn bound_refusal(value: u64) -> Option { - if value == 0 { + if in_range(value) { + None + } else if value == 0 { Some(BoundRefusal::Zero) - } else if value > MAX_SETTING_BOUND as u64 { - Some(BoundRefusal::AboveCeiling(MAX_SETTING_BOUND as u64)) } else { - None + Some(BoundRefusal::AboveCeiling(MAX_SETTING_BOUND as u64)) + } +} + +/// The sentence one refused bound is reported with, for both doors: a zero (documented as the +/// default) and a value above the ceiling are different facts, and the words live beside the +/// decision so the daemon's gate and the CLI cannot phrase it differently. +pub fn refusal_message(setting: &str, reason: BoundRefusal) -> String { + match reason { + BoundRefusal::Zero => format!("{setting}: 0 means the default, and the relay runs with it"), + BoundRefusal::AboveCeiling(ceiling) => { + format!("{setting}: above the {ceiling} a bound accepts; the key's value is ignored") + } } } -/// Reports one bound's setting that the relay will not run with, saying which of the two reasons -/// it was: a zero (documented as the default) and a value above the ceiling are different facts. +/// Reports one bound's setting that the relay will not run with, as one structured line saying +/// which of the two reasons it was: a zero (documented as the default) and a value above the +/// ceiling are different facts. fn warn_refused_bound(setting: &'static str, value: u64, default: usize) { match bound_refusal(value) { Some(BoundRefusal::Zero) => { diff --git a/crates/nostream-relay/tests/config_defaults.rs b/crates/nostream-relay/tests/config_defaults.rs index bb61d38f..080ee0e6 100644 --- a/crates/nostream-relay/tests/config_defaults.rs +++ b/crates/nostream-relay/tests/config_defaults.rs @@ -11,7 +11,7 @@ use std::path::PathBuf; use nostream_config::settings::Settings; -use nostream_relay::relay::RelayConfig; +use nostream_relay::relay::{BoundRefusal, RelayConfig, bound_refusal, refusal_message}; fn defaults_settings() -> Settings { let path = @@ -182,3 +182,65 @@ fn an_absent_setting_falls_back_to_the_compiled_default() { defaults.max_connections_per_ip ); } + +#[test] +fn the_refusal_verdict_and_the_range_agree_at_every_boundary() { + // `bound_refusal` must derive its answer from `in_range`, not compare the ceiling again: with + // the comparison retyped as `>=`, the documented at-ceiling value would be reported as above + // the ceiling while the relay runs with it, and every other test here would still pass. + const CEILING: u64 = 1024 * 1024 * 1024 * 1024; + + assert_eq!(bound_refusal(0), Some(BoundRefusal::Zero)); + assert_eq!(bound_refusal(CEILING - 1), None); + assert_eq!( + bound_refusal(CEILING), + None, + "the ceiling itself is accepted" + ); + assert_eq!( + bound_refusal(CEILING + 1), + Some(BoundRefusal::AboveCeiling(CEILING)) + ); + + // And the same three cases through the constructor, by value. + let at_ceiling = + RelayConfig::from_settings(&settings_with_bounds(Some(CEILING), Some(CEILING))); + assert_eq!(at_ceiling.frame_memory_budget_bytes, CEILING as usize); + assert_eq!(at_ceiling.max_connections_per_ip, CEILING as usize); + + let over = + RelayConfig::from_settings(&settings_with_bounds(Some(CEILING + 1), Some(CEILING + 1))); + assert_eq!( + over.frame_memory_budget_bytes, + RelayConfig::default().frame_memory_budget_bytes + ); + assert_eq!( + over.max_connections_per_ip, + RelayConfig::default().max_connections_per_ip + ); + + let zero = RelayConfig::from_settings(&settings_with_bounds(Some(0), Some(0))); + assert_eq!( + zero.frame_memory_budget_bytes, + RelayConfig::default().frame_memory_budget_bytes + ); + assert_eq!( + zero.max_connections_per_ip, + RelayConfig::default().max_connections_per_ip + ); +} + +#[test] +fn a_refusal_is_reported_in_one_place_for_both_doors() { + assert_eq!( + refusal_message("network.frameMemoryBudgetBytes", BoundRefusal::Zero), + "network.frameMemoryBudgetBytes: 0 means the default, and the relay runs with it" + ); + assert_eq!( + refusal_message( + "limits.connection.maxConnectionsPerIp", + BoundRefusal::AboveCeiling(1024) + ), + "limits.connection.maxConnectionsPerIp: above the 1024 a bound accepts; the key's value is ignored" + ); +} diff --git a/crates/nostreamd/src/settings_gate.rs b/crates/nostreamd/src/settings_gate.rs index ef62d492..843d4fea 100644 --- a/crates/nostreamd/src/settings_gate.rs +++ b/crates/nostreamd/src/settings_gate.rs @@ -17,32 +17,18 @@ //! request, per message or per streamed record. use nostream_config::LoadOutcome; -use nostream_relay::relay::{BoundRefusal, refused_bounds}; +use nostream_relay::relay::{refusal_message, refused_bounds}; use crate::error::DaemonError; -/// Prints one refused bound. A zero is the documented "use the default", so its line says only -/// that; an over-ceiling value names the ceiling. Neither says the relay falls back to *this key's* -/// default on top of a schema fault, because it would not: a rejected document is replaced whole — -/// " settings: defaults_settings," (`crates/nostream-config/src/load.rs:421`). -fn report_refused_bound(setting: &str, reason: BoundRefusal) { - match reason { - BoundRefusal::Zero => { - eprintln!("{setting}: 0 means the default, and the relay runs with it"); - } - BoundRefusal::AboveCeiling(ceiling) => { - eprintln!("{setting}: above the {ceiling} a bound accepts; the key's value is ignored") - } - } -} - /// Applies the gate to one load outcome: report the findings, decide whether they are fatal. pub fn apply_settings_gate(outcome: &LoadOutcome, check: bool) -> Result<(), DaemonError> { - // Independent of the schema verdict: an unrelated typo must not hide a range report, and a - // rejected document has already been replaced whole, so the report says what happened to the - // number rather than what the relay runs with. + // Unconditional here, but there is only something to say when the outcome carries a typed + // value: on a rejected document `load` has already substituted the shipped defaults — + // " settings: defaults_settings," (`crates/nostream-config/src/load.rs:421`) — so + // the walk finds nothing and the schema fault is the whole report. for (setting, reason) in refused_bounds(&outcome.settings) { - report_refused_bound(setting, reason); + eprintln!("{}", refusal_message(setting, reason)); } if outcome.issues.is_empty() { diff --git a/crates/nostreamd/tests/check_flag.rs b/crates/nostreamd/tests/check_flag.rs index e31b5729..2d7303b7 100644 --- a/crates/nostreamd/tests/check_flag.rs +++ b/crates/nostreamd/tests/check_flag.rs @@ -208,3 +208,29 @@ fn check_reports_a_zero_bound_as_the_documented_default() { "a zero must not be reported as above the ceiling: {stderr}" ); } + +#[test] +fn check_reports_the_schema_fault_and_not_a_bound_it_cannot_read() { + // On a rejected document `load` substitutes the shipped defaults, so there is no operator + // number left to describe: the gate is unconditional but has nothing to walk. This pins that + // it does not invent a range line, and that the exit code stays the schema verdict (2). + let directory = tempfile::tempdir().expect("tempdir"); + std::fs::write( + directory.path().join("settings.yaml"), + "network:\n frameMemoryBudgetBytes: 1099511627777\nlimits:\n rateLimiter:\n strategy: random\n", + ) + .expect("write"); + + let config_dir = directory.path().to_string_lossy().into_owned(); + let (code, stdout, stderr) = run_check( + Duration::from_secs(30), + &["--check", "--config-dir", &config_dir], + ); + assert_eq!(code, Some(2), "exit code"); + assert!(!stdout.contains("settings valid"), "stdout: {stdout}"); + assert!( + stderr.contains("strategy must be ewma or sliding_window"), + "stderr: {stderr}" + ); + assert!(!stderr.contains("above the"), "no range line: {stderr}"); +} From 0679ea1e4bef5d112b8c16734f63f82760490e24 Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 03:39:47 -0400 Subject: [PATCH 24/26] docs(rewrite): say what a rejected document leaves to report, and re-anchor --- .changeset/relay-frame-budget-settings.md | 12 ++- .../decisions/0005-connection-concurrency.md | 2 +- .../decisions/0008-frame-memory-budget.md | 97 +++++++++++-------- 3 files changed, 67 insertions(+), 44 deletions(-) diff --git a/.changeset/relay-frame-budget-settings.md b/.changeset/relay-frame-budget-settings.md index c5fa2cba..ced2608f 100644 --- a/.changeset/relay-frame-budget-settings.md +++ b/.changeset/relay-frame-budget-settings.md @@ -4,8 +4,8 @@ feat(config): expose the frame budget and the per-IP cap as settings keys The two bounds #959 added were `RelayConfig` fields only: -"pub frame_memory_budget_bytes: usize," (`crates/nostream-relay/src/relay.rs:65`) and -"pub max_connections_per_ip: usize," (`crates/nostream-relay/src/relay.rs:69`). A running +"pub frame_memory_budget_bytes: usize," (`crates/nostream-relay/src/relay.rs:66`) and +"pub max_connections_per_ip: usize," (`crates/nostream-relay/src/relay.rs:70`). A running daemon could not change either number without a rebuild, which is what #989 is about. Both are now settings the operator writes: @@ -21,9 +21,11 @@ operator's number out of the frame accounting: " value > 0 && value <= MAX_SETTING_BOUND as u64" (`crates/nostream-relay/src/relay.rs:115`). The daemon builds its config through it: "let mut relay_config = RelayConfig::from_settings(&settings);" (`crates/nostreamd/src/main.rs:110`), keeping its one environment read. A value it replaces is not -silent: the constructor warns with the setting, the value, the ceiling and the default -(`crates/nostream-relay/src/relay.rs:148`), and `nostreamd --check` reports the same before a relay -exists (`crates/nostreamd/src/settings_gate.rs:28-45`). +silent: the constructor warns with the setting, the value, the ceiling and the default — +" \"a bound above its ceiling runs with its default\"" (`crates/nostream-relay/src/relay.rs:191`) +— and `nostreamd --check` and `nostream-cli config validate` print the same sentences before a relay +exists, from one place: "pub fn refusal_message(setting: &str, reason: BoundRefusal) -> String {" +(`crates/nostream-relay/src/relay.rs:169`). The defaults document's values equal `RelayConfig::default()`'s, and a test in `crates/nostream-relay/tests/config_defaults.rs` holds the two together. No validator rule is diff --git a/docs/rewrite/decisions/0005-connection-concurrency.md b/docs/rewrite/decisions/0005-connection-concurrency.md index beb00391..f62bada2 100644 --- a/docs/rewrite/decisions/0005-connection-concurrency.md +++ b/docs/rewrite/decisions/0005-connection-concurrency.md @@ -119,7 +119,7 @@ constant wearing a config field's name — the state #989 was filed against. Bot place — `from_settings` is the only answer the relay runs with, so the compiled default and the document cannot disagree: "pub fn from_settings(settings: &Settings) -> Self {" -(`crates/nostream-relay/src/relay.rs:196`), from leaves the document carries: +(`crates/nostream-relay/src/relay.rs:212`), from leaves the document carries: "pub frame_memory_budget_bytes: Option," (`crates/nostream-config/src/settings.rs:71`). The write timeout and the per-socket queue bound stay `RelayConfig`-only, as this section decided; ADR 0008 carries what a zero means for the two that diff --git a/docs/rewrite/decisions/0008-frame-memory-budget.md b/docs/rewrite/decisions/0008-frame-memory-budget.md index 99162fee..d8f06c5d 100644 --- a/docs/rewrite/decisions/0008-frame-memory-budget.md +++ b/docs/rewrite/decisions/0008-frame-memory-budget.md @@ -12,7 +12,7 @@ The outbound frame queue was bounded per socket and nothing bounded their sum. (`crates/nostream-relay/src/sink.rs:260-334`), and `Relay::serve` accepted every connection with no count at all. The only per-address bookkeeping was the **attempt** limiter, which throttles how fast connections open, not how many are held (`crates/nostream-relay/src/limits.rs`, windows -consumed at `crates/nostream-relay/src/relay.rs:463`). +consumed at `crates/nostream-relay/src/relay.rs:479`). The operator's policy comment on #959 chose the numbers and the refusal and asked for two consequences a reviewer can hold the change to: both bounds refuse the same way, and the budget is @@ -27,7 +27,7 @@ the pre-change worst case is **384 MiB**, and that figure is **arithmetic from n back on a fresh daemon: the first twelve received `HTTP/1.1 101 Switching Protocols` and then a text frame starting `["AUTH","…`, and the remaining eight were upgraded and dropped with **no websocket close frame at all** — the attempt limiter's silent return -(`crates/nostream-relay/src/relay.rs:423-426`). A refusal indistinguishable from a network fault is +(`crates/nostream-relay/src/relay.rs:439-442`). A refusal indistinguishable from a network fault is exactly what the new bounds are not allowed to be, so each of them closes with a code, a fixed reason and its own counter. @@ -87,14 +87,20 @@ leaves beside the keys that already live in those sections: `from_settings` is the one place either setting is *interpreted* — the daemon and the test share it, so they cannot drift: "pub fn from_settings(settings: &Settings) -> Self {" -(`crates/nostream-relay/src/relay.rs:196`). The daemon calls it once and keeps its one environment +(`crates/nostream-relay/src/relay.rs:212`). The daemon calls it once and keeps its one environment read: "let mut relay_config = RelayConfig::from_settings(&settings);" (`crates/nostreamd/src/main.rs:110`). One other function walks the same two leaves — `refused_bounds`, -so the gate and the CLI can report a value the constructor will not take — and it shares -`bound_refusal` with the constructor, so the two cannot disagree about which values those are: +so the gate and the CLI can report a value the constructor will not take: "pub fn refused_bounds(settings: &Settings) -> Vec<(&'static str, BoundRefusal)> {" (`crates/nostream-relay/src/relay.rs:133`). It never produces a config; -`from_settings` is the only reader whose answer the relay runs with. `RelayConfig::default()` remains the single source of the two +`from_settings` is the only reader whose answer the relay runs with. + +The two cannot disagree because `bound_refusal` **asks `in_range` first** rather than comparing the +ceiling a second time — " if in_range(value) {" (`crates/nostream-relay/src/relay.rs:157`) — so a +value the constructor accepts is never reported as refused, and a typed edit that retyped the +comparison would fail `the_refusal_verdict_and_the_range_agree_at_every_boundary` +(`crates/nostream-relay/tests/config_defaults.rs`), which asserts the verdict at the ceiling, one +over it, and at zero. `RelayConfig::default()` remains the single source of the two numbers — the document mirrors it, and a test in `crates/nostream-relay/tests/config_defaults.rs` fails when they part — because a missing key must still mean a working relay, not a relay that needs the document at start-up. The compiled values are unchanged: @@ -117,7 +123,7 @@ them). Both bounds close the socket with `CloseCode::Again` (1013) written straight to the transport rather than through the sink, bounded by `config.write_timeout` exactly as the existing slow-consumer close -is (`Relay::close_frame`, `crates/nostream-relay/src/relay.rs:666-677`). Three fixed reason strings +is (`Relay::close_frame`, `crates/nostream-relay/src/relay.rs:682-693`). Three fixed reason strings now exist, each lowercase and resource-stating, per the convention `SLOW_CONSUMER_REASON` set (`crates/nostream-relay/src/transport.rs:37-47`): @@ -168,8 +174,8 @@ the queue bound has already latched does not even pay that, because `send` retur The read does not finish there: it waits on the read's cancellation watch and then calls `end_read`. **On the refusal path that wait is never resolved.** Every exit of the connection loop is a `break`, -`handlers.abort_all()` runs before `connection.close()` (`crates/nostream-relay/src/relay.rs:630-631`), -and a task aborted by the `JoinSet` it was spawned into (`crates/nostream-relay/src/relay.rs:513`, +`handlers.abort_all()` runs before `connection.close()` (`crates/nostream-relay/src/relay.rs:646-647`), +and a task aborted by the `JoinSet` it was spawned into (`crates/nostream-relay/src/relay.rs:529`, `:602-603`) is dropped without being polled again: the parked read is dropped inside the watch, the `end_read` below it never runs, and the store stream goes with it. What retires a refused read in production is therefore the abort, not the watch — the watch is the path a client can still take, by @@ -238,12 +244,12 @@ send holds a sink and therefore cannot be in flight or begin once that drop star the refund **exactly this socket's bytes, once**, whatever the connection loop did on the way out. **The rejected shape: a decrement at each of the loop's exits.** Today `release` has exactly one -caller, the release after a frame is written (`crates/nostream-relay/src/relay.rs:543`), and every -other exit leaves whatever was still queued counted: the drain entry (`:519-521`), the refusal -close (`:527-529`), the closed frame queue (`:534`), a failed write (`:545-547`), the closed -inbound stream (`:550`), an inbound `CLOSE` (`:576`), a failed read (`:578-580`), a failed pong -write (`:568-570`), the closed hub (`:596`), the pong timeout (`:603`) and a failed ping write -(`:611-615`) and the final cleanup (`:630-631`); the lagged hub (`:591-593`) warns and does not +caller, the release after a frame is written (`crates/nostream-relay/src/relay.rs:559`), and every +other exit leaves whatever was still queued counted: the drain entry (`:535-537`), the refusal +close (`:543-545`), the closed frame queue (`:550`), a failed write (`:561-563`), the closed +inbound stream (`:566`), an inbound `CLOSE` (`:592`), a failed read (`:594-596`), a failed pong +write (`:585-587`), the closed hub (`:612`), the pong timeout (`:619`) and a failed ping write +(`:631-633`) and the final cleanup (`:646-647`); the lagged hub (`:607-609`) warns and does not leave. Per socket that leak is invisible because the counter dies with the sink; a process-wide budget makes it a leak of up to 8 MiB on every disconnect. Writing the decrement at each of those sites is the change that looks @@ -286,10 +292,10 @@ checked in `handle_socket` **after** `is_connection_rate_limited` and **before** - before `run_connection`, because `Connection::accept`'s first act is to send the NIP-42 challenge as a text frame (`crates/nostream-relay/src/connection.rs:109-115`) — a client refused afterwards would already have received a text frame, which the policy forbids. The window is - `crates/nostream-relay/src/relay.rs:428-452`. + `crates/nostream-relay/src/relay.rs:444-468`. Acquisition returns a `ConnectionSlot` held for the rest of `handle_socket` -(`crates/nostream-relay/src/relay.rs:435`), so the normal exit, the registration failure inside +(`crates/nostream-relay/src/relay.rs:451`), so the normal exit, the registration failure inside `run_connection`, and any early return added later all release it through `Drop` (`crates/nostream-relay/src/connection_cap.rs:101-116`). A count that leaks makes the cap permanent for that address, which is a defect and not a memory leak. The map entry is removed when its count @@ -315,14 +321,14 @@ prefixes, and it is not this job's. ## What an operator can see today Two accessors in the `hidden_content()` style, `Relay::frame_budget()` and -`Relay::frame_budget()` (`crates/nostream-relay/src/relay.rs:311-313`) and -`Relay::connection_cap()` (`crates/nostream-relay/src/relay.rs:316-318`), and the counters behind +`Relay::frame_budget()` (`crates/nostream-relay/src/relay.rs:327-329`) and +`Relay::connection_cap()` (`crates/nostream-relay/src/relay.rs:332-334`), and the counters behind them: `accounted()`, `limit()`, `refusals()` on the budget; `live(ip)`, `max_per_ip()`, `refusals()`, `tracked_addresses()` on the cap. Plus two structured log lines, one per refusal — a `warn!` carrying `budget_bytes` and `accounted_bytes` from the sink (`crates/nostream-relay/src/sink.rs:321-325`), and a `debug!` carrying `peer`, `max_connections_per_ip` and `live` from the connection task -(`crates/nostream-relay/src/relay.rs:436-441`). +(`crates/nostream-relay/src/relay.rs:452-457`). **Those two lines are not visible at the same level, and the daemon ships `info`.** The filter is `--log-level`, then `LOG_LEVEL`, then `RUST_LOG`, else `info` @@ -427,35 +433,50 @@ three places an operator looks say it, in two different sentences, because **a z over-ceiling value are different facts**: zero is the documented "use the default", and the ceiling is a violation. `in_range` stays the one range question; `bound_refusal` says why — "pub fn bound_refusal(value: u64) -> Option {" -(`crates/nostream-relay/src/relay.rs:153`). +(`crates/nostream-relay/src/relay.rs:156`). At start-up the constructor warns, once per leaf — "a bound above its ceiling runs with its -default" (`crates/nostream-relay/src/relay.rs:175`) or, for a zero, +default" (`crates/nostream-relay/src/relay.rs:191`) or, for a zero, " tracing::warn!(setting, default, "a bound set to 0 runs with its default")" -(`crates/nostream-relay/src/relay.rs:168`) — naming the setting, the value where there is one, +(`crates/nostream-relay/src/relay.rs:184`) — naming the setting, the value where there is one, the ceiling and the default. `warn!` rather than `debug!` because the start-up line is a one-off statement about a configuration that is wrong, not a per-request event, so it is not log-amplifiable by a client. -`--check` reports the same through the gate — -" eprintln!("{setting}: 0 means the default, and the relay runs with it");" -(`crates/nostreamd/src/settings_gate.rs:31`) and -" eprintln!("{setting}: above the {ceiling} a bound accepts; the key's value is ignored")" -(`crates/nostreamd/src/settings_gate.rs:34`) — and `nostream-cli config validate`, the other -read-only door, appends the same two sentences to its verdict line -(`crates/nostream-cli/src/config.rs:86-93`), from the same `refused_bounds` walk. The gate is not a +**One place produces the words.** `refusal_message` sits beside the decision, so neither door can +phrase it differently: +"pub fn refusal_message(setting: &str, reason: BoundRefusal) -> String {" +(`crates/nostream-relay/src/relay.rs:169`). + +`--check` prints it through the gate — +" eprintln!("{}", refusal_message(setting, reason));" +(`crates/nostreamd/src/settings_gate.rs:31`) — and `nostream-cli config validate`, the other +read-only door, appends the same strings to its verdict +(`crates/nostream-cli/src/config.rs:84-98`), from the same `refused_bounds` walk. The gate is not a `tracing` line because `--check` returns before the subscriber exists: the gate writes outside the filter by design (`crates/nostreamd/src/settings_gate.rs:8-17`), so the report is reached at the shipped level. -**A rejected document changes what these lines can say.** `--check` prints the range report -whether or not the schema is clean, because an unrelated typo must not hide it; but when the -document is rejected it has already been replaced whole — -" settings: defaults_settings," (`crates/nostream-config/src/load.rs:421`) — so the -report then describes the operator's number, not the relay's behaviour that would have followed, -which is why neither sentence claims the relay is running on this key's default. What is -deliberately unchanged: `--check` still exits 0 on a value it will replace, since that is the -policy for a valid key with an out-of-range value and refusing it means a validator rule. +**On a rejected document neither door reports the bound, and that is not a gap to close here.** +The loop in the gate is unconditional, but `load` substitutes the shipped defaults for a document +with any issue — " settings: defaults_settings," +(`crates/nostream-config/src/load.rs:421`) — so there is no operator number left to describe and the +walk finds nothing. Measured on a document holding both an over-ceiling value and a bad rate-limiter +strategy: both doors print the schema fault, `settings invalid: 1 issues`, exit 2, and no range +line. The CLI notes the same in its own comment, and each door has a test pinning it +(`check_reports_the_schema_fault_and_not_a_bound_it_cannot_read`, +`config_validate_reports_the_schema_fault_and_not_a_bound_it_cannot_read`). What is deliberately +unchanged: `--check` still exits 0 on a value it will replace, since that is the policy for a valid +key with an out-of-range value and refusing it means a validator rule. + +**What putting this beside the arithmetic costs, measured.** `nostream-cli` depends on +`nostream-relay` to reach `refused_bounds` and `refusal_message`, and that edge drags the websocket +and storage graph into a settings-check binary: `cargo tree -p nostream-cli --edges normal` is 469 +lines with the edge and 177 without, and a cold `cargo build -p nostream-cli --all-features` took +51.3 s without it and 63 s with it on this host (single runs, shared machine — direction, not a +ratio). The alternative is to move the decision into `nostream-config`, which would invert the +dependency the bound's arithmetic lives behind (`RelayConfig` is `nostream-relay`'s), so it is +recorded rather than taken. **The pair's real ceiling is not the cap's.** The budget is process-wide and the cap is per address, so what the two bound together is `min(budget, addresses × cap × frame_queue_bytes)` — the From d3416ba3ecf5379d0284a8d9addf33d64d9a10ca Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 04:07:57 -0400 Subject: [PATCH 25/26] fix(daemon): never describe a substituted document as a bound violation --- crates/nostream-cli/tests/config_cli.rs | 8 +++++-- crates/nostreamd/src/settings_gate.rs | 16 ++++++++------ crates/nostreamd/tests/check_flag.rs | 28 ++++++++++++++++++++++--- 3 files changed, 41 insertions(+), 11 deletions(-) diff --git a/crates/nostream-cli/tests/config_cli.rs b/crates/nostream-cli/tests/config_cli.rs index f27a9834..0db0036e 100644 --- a/crates/nostream-cli/tests/config_cli.rs +++ b/crates/nostream-cli/tests/config_cli.rs @@ -85,11 +85,15 @@ fn config_validate_says_which_bound_setting_the_relay_will_not_use() { assert_eq!(output.exit_code, 0); let verdict = output.lines.join("\n"); assert!( - verdict.contains("network.frameMemoryBudgetBytes: 0 means the default"), + verdict.contains( + "network.frameMemoryBudgetBytes: 0 means the default, and the relay runs with it" + ), "verdict: {verdict}" ); assert!( - verdict.contains("limits.connection.maxConnectionsPerIp: above the 1099511627776"), + verdict.contains( + "limits.connection.maxConnectionsPerIp: above the 1099511627776 a bound accepts; the key's value is ignored" + ), "verdict: {verdict}" ); } diff --git a/crates/nostreamd/src/settings_gate.rs b/crates/nostreamd/src/settings_gate.rs index 843d4fea..8edfa34c 100644 --- a/crates/nostreamd/src/settings_gate.rs +++ b/crates/nostreamd/src/settings_gate.rs @@ -23,12 +23,16 @@ use crate::error::DaemonError; /// Applies the gate to one load outcome: report the findings, decide whether they are fatal. pub fn apply_settings_gate(outcome: &LoadOutcome, check: bool) -> Result<(), DaemonError> { - // Unconditional here, but there is only something to say when the outcome carries a typed - // value: on a rejected document `load` has already substituted the shipped defaults — - // " settings: defaults_settings," (`crates/nostream-config/src/load.rs:421`) — so - // the walk finds nothing and the schema fault is the whole report. - for (setting, reason) in refused_bounds(&outcome.settings) { - eprintln!("{}", refusal_message(setting, reason)); + // `settings` is what the relay would run with, so on a rejected document it is the shipped + // defaults (" format!(\"failed to load user settings: {error}\"),", + // `crates/nostream-config/src/load.rs:402`) and a walk of it would describe the document's own + // values rather than the operator's. The guard keeps a substitution from being read as a + // violation; it cannot recover the operator's leaf, which is why a rejected document reports + // the schema fault and nothing else. + if !outcome.used_defaults { + for (setting, reason) in refused_bounds(&outcome.settings) { + eprintln!("{}", refusal_message(setting, reason)); + } } if outcome.issues.is_empty() { diff --git a/crates/nostreamd/tests/check_flag.rs b/crates/nostreamd/tests/check_flag.rs index 2d7303b7..a842f83b 100644 --- a/crates/nostreamd/tests/check_flag.rs +++ b/crates/nostreamd/tests/check_flag.rs @@ -169,13 +169,16 @@ fn check_reports_a_bound_it_would_replace_with_the_default() { ); assert_eq!(code, Some(0), "exit code"); assert_eq!(stdout.trim(), "settings valid: 0 issues"); + // Whole lines, not prefixes: dropping the second clause must fail here. assert!( - stderr.contains("network.frameMemoryBudgetBytes: above the 1099511627776 a bound accepts"), + stderr.contains( + "network.frameMemoryBudgetBytes: above the 1099511627776 a bound accepts; the key's value is ignored" + ), "stderr: {stderr}" ); assert!( stderr.contains( - "limits.connection.maxConnectionsPerIp: above the 1099511627776 a bound accepts" + "limits.connection.maxConnectionsPerIp: above the 1099511627776 a bound accepts; the key's value is ignored" ), "stderr: {stderr}" ); @@ -200,7 +203,9 @@ fn check_reports_a_zero_bound_as_the_documented_default() { assert_eq!(code, Some(0), "exit code"); assert_eq!(stdout.trim(), "settings valid: 0 issues"); assert!( - stderr.contains("network.frameMemoryBudgetBytes: 0 means the default"), + stderr.contains( + "network.frameMemoryBudgetBytes: 0 means the default, and the relay runs with it" + ), "stderr: {stderr}" ); assert!( @@ -234,3 +239,20 @@ fn check_reports_the_schema_fault_and_not_a_bound_it_cannot_read() { ); assert!(!stderr.contains("above the"), "no range line: {stderr}"); } + +#[test] +fn check_reports_nothing_for_the_shipped_defaults() { + // With no user document, `settings` is the shipped defaults with `used_defaults` false, so a + // report that walked them would describe the document's own values as violations on a fresh + // install. The guard is what prevents it; without the guard this test sees two lines. + let directory = tempfile::tempdir().expect("tempdir"); + + let config_dir = directory.path().to_string_lossy().into_owned(); + let (code, stdout, stderr) = run_check( + Duration::from_secs(30), + &["--check", "--config-dir", &config_dir], + ); + assert_eq!(code, Some(0), "exit code"); + assert_eq!(stdout.trim(), "settings valid: 0 issues"); + assert_eq!(stderr, "", "the shipped defaults are not a violation"); +} From 60aa7e900f6ea7a23a7d3b66a1545c5dd29c015f Mon Sep 17 00:00:00 2001 From: chappie-daemon Date: Sun, 25 Oct 2026 04:08:02 -0400 Subject: [PATCH 26/26] docs(rewrite): state the two mechanisms a rejected document leaves, and re-anchor --- .../decisions/0008-frame-memory-budget.md | 56 ++++++++++++------- 1 file changed, 37 insertions(+), 19 deletions(-) diff --git a/docs/rewrite/decisions/0008-frame-memory-budget.md b/docs/rewrite/decisions/0008-frame-memory-budget.md index d8f06c5d..2981bc27 100644 --- a/docs/rewrite/decisions/0008-frame-memory-budget.md +++ b/docs/rewrite/decisions/0008-frame-memory-budget.md @@ -397,11 +397,11 @@ The constructor implements exactly that, and nothing else: - the budget is taken only when the leaf is present, convertible, positive and within the ceiling — " value > 0 && value <= MAX_SETTING_BOUND as u64" (`crates/nostream-relay/src/relay.rs:115`), - applied to the leaf at " if let Some(bytes) = settings.network.frame_memory_budget_bytes" - (`crates/nostream-relay/src/relay.rs:136`); + applied to the leaf at " if let Some(bytes) = settings.network.frame_memory_budget_bytes {" + (`crates/nostream-relay/src/relay.rs:215`); - the cap likewise, through the two optional levels the typed schema forces — " .and_then(|connection| connection.max_connections_per_ip)" - (`crates/nostream-relay/src/relay.rs:157`); + (`crates/nostream-relay/src/relay.rs:234`); - otherwise the field keeps `RelayConfig::default()`'s value, so a zero, an absent key — absent leaf, absent `limits`, absent `limits.connection` — and a number above the ceiling all behave as "the shipped default". @@ -450,33 +450,51 @@ phrase it differently: `--check` prints it through the gate — " eprintln!("{}", refusal_message(setting, reason));" -(`crates/nostreamd/src/settings_gate.rs:31`) — and `nostream-cli config validate`, the other +(`crates/nostreamd/src/settings_gate.rs:34`) — and `nostream-cli config validate`, the other read-only door, appends the same strings to its verdict (`crates/nostream-cli/src/config.rs:84-98`), from the same `refused_bounds` walk. The gate is not a `tracing` line because `--check` returns before the subscriber exists: the gate writes outside the filter by design (`crates/nostreamd/src/settings_gate.rs:8-17`), so the report is reached at the shipped level. -**On a rejected document neither door reports the bound, and that is not a gap to close here.** -The loop in the gate is unconditional, but `load` substitutes the shipped defaults for a document -with any issue — " settings: defaults_settings," -(`crates/nostream-config/src/load.rs:421`) — so there is no operator number left to describe and the -walk finds nothing. Measured on a document holding both an over-ceiling value and a bad rate-limiter -strategy: both doors print the schema fault, `settings invalid: 1 issues`, exit 2, and no range -line. The CLI notes the same in its own comment, and each door has a test pinning it +**What a rejected document leaves to report, in the two mechanisms that actually apply.** + +*Where `load` substitutes, there is no operator value left.* Four early returns replace the whole +outcome with the shipped defaults and set `used_defaults` — an unreadable user document, a document +the shape walk refuses, a typed document the fit refuses, and a read failure: +" /// Whether the user's document was rejected and the defaults were substituted." +(`crates/nostream-config/src/load.rs:386`). A document whose **schema** is rejected therefore reaches +both doors as the defaults, and the schema fault is the whole report. Measured on +`network.frameMemoryBudgetBytes: 0` with `admin.notifications.deliveryLogRetentionDays: 0`: both +doors print only `deliveryLogRetentionDays must be at least 1`, then `settings invalid: 1 issues`, +exit 2. The daemon's gate also refuses to walk a substitution at all: +" if !outcome.used_defaults {" (`crates/nostreamd/src/settings_gate.rs:32`), so a substituted +document can never be described in the operator's terms. Each door has a test pinning this (`check_reports_the_schema_fault_and_not_a_bound_it_cannot_read`, -`config_validate_reports_the_schema_fault_and_not_a_bound_it_cannot_read`). What is deliberately -unchanged: `--check` still exits 0 on a value it will replace, since that is the policy for a valid -key with an out-of-range value and refusing it means a validator rule. +`config_validate_reports_the_schema_fault_and_not_a_bound_it_cannot_read`). + +*Where nothing is substituted, there is a leaf to read and both doors read it.* With no user +document — a fresh install, a different `--config-dir` — `load` returns the shipped defaults with +`used_defaults` false, and with a valid document it returns the operator's typed settings. The +walk then reports the operator's values, and reports nothing when they are the shipped ones: +measured, `--check --config-dir ` prints only `settings valid: 0 issues`, exit 0, because +`268435456` and `64` are inside the range, while a file setting either bound to `0` or above the +ceiling prints its line on both doors. +`check_reports_nothing_for_the_shipped_defaults` pins the empty case. + +What is deliberately unchanged: `--check` still exits 0 on a value it will replace, since that is +the policy for a valid key with an out-of-range value and refusing it means a validator rule. **What putting this beside the arithmetic costs, measured.** `nostream-cli` depends on `nostream-relay` to reach `refused_bounds` and `refusal_message`, and that edge drags the websocket and storage graph into a settings-check binary: `cargo tree -p nostream-cli --edges normal` is 469 -lines with the edge and 177 without, and a cold `cargo build -p nostream-cli --all-features` took -51.3 s without it and 63 s with it on this host (single runs, shared machine — direction, not a -ratio). The alternative is to move the decision into `nostream-config`, which would invert the -dependency the bound's arithmetic lives behind (`RelayConfig` is `nostream-relay`'s), so it is -recorded rather than taken. +lines with the edge and 177 without (reproduced by two lenses), and a cold +`cargo build -p nostream-cli --all-features` took **32.4 s without it and 87.1 s with it** — the +performance lens's measurement, taken on this host (single runs, shared host — direction, not a +ratio; an independent measurement by the code lens read 39.9 s and 96.7 s, so the direction is +agreed and the ratio is not). The alternative is to move the decision into `nostream-config`, which +would invert the dependency the bound's arithmetic lives behind (`RelayConfig` is +`nostream-relay`'s), so it is recorded rather than taken. **The pair's real ceiling is not the cap's.** The budget is process-wide and the cap is per address, so what the two bound together is `min(budget, addresses × cap × frame_queue_bytes)` — the