diff --git a/.changeset/relay-frame-budget-settings.md b/.changeset/relay-frame-budget-settings.md new file mode 100644 index 000000000..ced2608fd --- /dev/null +++ b/.changeset/relay-frame-budget-settings.md @@ -0,0 +1,36 @@ +--- +--- + +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: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: + +- `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, zero, or above the ceiling that keeps an +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 — +" \"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 +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 0cdd83603..03716e0e0 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. 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. | @@ -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. 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/Cargo.lock b/Cargo.lock index eb3ccdf0a..53251089b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1151,6 +1151,7 @@ dependencies = [ "clap", "nostream-config", "nostream-core", + "nostream-relay", "serde_json", "serde_yaml", "tempfile", @@ -1203,6 +1204,7 @@ dependencies = [ "nostream-core", "nostream-storage", "serde_json", + "serde_yaml", "sha2 0.11.1", "thiserror", "tokio", diff --git a/crates/nostream-cli/Cargo.toml b/crates/nostream-cli/Cargo.toml index 1eceabdc2..05ec94392 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 33becf63c..d3afb86c9 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::{refusal_message, refused_bounds}; use serde_yaml::Value; use crate::error::CliError; @@ -57,31 +58,57 @@ 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), } } + + // 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 line in refused { + verdict.push_str("; "); + verdict.push_str(&line); + } return Ok(CommandOutput { - lines: vec!["settings valid: 0 issues".to_owned()], + lines: vec![verdict], exit_code: 0, }); } + // 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 0cfcadfee..0db0036ea 100644 --- a/crates/nostream-cli/tests/config_cli.rs +++ b/crates/nostream-cli/tests/config_cli.rs @@ -66,3 +66,61 @@ 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, and the relay runs with it" + ), + "verdict: {verdict}" + ); + assert!( + verdict.contains( + "limits.connection.maxConnectionsPerIp: above the 1099511627776 a bound accepts; the key's value is ignored" + ), + "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-config/src/guided.rs b/crates/nostream-config/src/guided.rs index 5402a2a35..45e4be157 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, 0 means the default)", + "network.frameMemoryBudgetBytes", + Number, + SafeNonNegativeInteger, + ), validated( "Worker count", "workers.count", @@ -263,6 +269,12 @@ fn build_categories() -> Vec { Number, SafeNonNegativeInteger, ), + validated( + "Max connections per IP (sockets, 0 means the default)", + "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 d76a7290e..c551cdba0 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 3dd1371fb..d153dd905 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 2dcd1f3b8..7d9dd0b45 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), @@ -88,14 +90,164 @@ 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 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`]. 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 +} + +/// 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. +/// +/// 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 in_range(value) { + None + } else if value == 0 { + Some(BoundRefusal::Zero) + } else { + 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, 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) => { + 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 + /// 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. + /// + /// 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 { + 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. + warn_refused_bound( + "network.frameMemoryBudgetBytes", + bytes, + config.frame_memory_budget_bytes, + ); + } + } + + if let Some(cap) = settings + .limits + .as_ref() + .and_then(|limits| limits.connection.as_ref()) + .and_then(|connection| connection.max_connections_per_ip) + { + if in_range(cap) { + config.max_connections_per_ip = cap as usize; + } else { + warn_refused_bound( + "limits.connection.maxConnectionsPerIp", + cap, + config.max_connections_per_ip, + ); + } + } + + 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 000000000..080ee0e65 --- /dev/null +++ b/crates/nostream-relay/tests/config_defaults.rs @@ -0,0 +1,246 @@ +//! `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::{BoundRefusal, RelayConfig, bound_refusal, refusal_message}; + +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 + ); +} + +/// 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_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 + // 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 + ); +} + +#[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/nostream-relay/tests/handlers.rs b/crates/nostream-relay/tests/handlers.rs index ffaec0c62..695a67d69 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 f04311759..b6ed1eceb 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 a5937dd18..987a5a17a 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/src/settings_gate.rs b/crates/nostreamd/src/settings_gate.rs index 521362715..8edfa34c5 100644 --- a/crates/nostreamd/src/settings_gate.rs +++ b/crates/nostreamd/src/settings_gate.rs @@ -17,11 +17,24 @@ //! request, per message or per streamed record. use nostream_config::LoadOutcome; +use nostream_relay::relay::{refusal_message, refused_bounds}; 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> { + // `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() { if check { println!("settings valid: 0 issues"); diff --git a/crates/nostreamd/tests/check_flag.rs b/crates/nostreamd/tests/check_flag.rs index 2297377f5..a842f83ba 100644 --- a/crates/nostreamd/tests/check_flag.rs +++ b/crates/nostreamd/tests/check_flag.rs @@ -129,3 +129,130 @@ 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"); +} + +#[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"); + // Whole lines, not prefixes: dropping the second clause must fail here. + assert!( + 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; the key's value is ignored" + ), + "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, and the relay runs with it" + ), + "stderr: {stderr}" + ); + assert!( + !stderr.contains("above the"), + "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}"); +} + +#[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"); +} diff --git a/docs/rewrite/decisions/0005-connection-concurrency.md b/docs/rewrite/decisions/0005-connection-concurrency.md index 4a4b5240d..f62bada2a 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,21 @@ 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`), 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: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 +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 962b8814f..2981bc27c 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: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 @@ -20,21 +20,21 @@ 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:287-290`). 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. ## 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 @@ -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: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,23 +75,55 @@ 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` - -**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. +— `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 +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`). + +`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: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: +"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. + +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: +"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 +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 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: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`): @@ -142,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: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: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 @@ -212,11 +244,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:407`), 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: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 right and rots: @@ -232,7 +266,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 @@ -258,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:292-315`. + `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:299`), 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 @@ -287,13 +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::connection_cap()` (`crates/nostream-relay/src/relay.rs:175-182`), 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:300-305`). +(`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` @@ -306,7 +341,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 +354,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 +366,163 @@ 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, 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: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: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". + +**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: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 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 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: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:191`) or, for a zero, +" tracing::warn!(setting, default, "a bound set to 0 runs with its default")" +(`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. + +**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: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. + +**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`). + +*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 (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 +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`) +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: 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`) 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 +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 115ea8454..931321512 100755 --- a/resources/default-settings.yaml +++ b/resources/default-settings.yaml @@ -150,6 +150,10 @@ nip56: hideActionableReports: false network: maxPayloadSize: 524288 + # 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 # remoteIpHeader: cf-connecting-ip @@ -213,6 +217,10 @@ limits: - "10.10.10.1" - "::ffff:10.10.10.1" 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 (about a million times any deployment) — means the default (ADR 0008). + maxConnectionsPerIp: 64 rateLimits: - period: 1000 rate: 12 diff --git a/scripts/citation-meaning.py b/scripts/citation-meaning.py new file mode 100755 index 000000000..0d7e2f1c2 --- /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:]))