fix(config): reject a zero frame-memory budget or per-IP cap (#996)

.changeset/relay-zero-bound-invalid.md

@@ -0,0 +1,35 @@
+---
+---
+
+fix(config): refuse a zero frame-memory budget or per-IP cap in the validator
+
+#989 gave `network.frameMemoryBudgetBytes` and `limits.connection.maxConnectionsPerIp` a
+meaning for `0` — *use the compiled default* — and ADR 0008 at this branch's base states it as
+policy: "- **Any validation of the two numbers.** A zero budget or a zero cap is still not
+refused — it is" (`docs/rewrite/decisions/0008-frame-memory-budget.md:369`). This change is the
+one that supersedes those lines. It is also a fourth meaning for the same numeral in one
+repository, where `limits.client.subscription` reads a zero as *disabled* and
+`limits.event.pow.targetEventsPerSecond` reads it as *invalid*.
+
+A stated `0` is now an issue, in the shape the validator already uses for the adaptive-pow
+numbers: `network.frameMemoryBudgetBytes: frameMemoryBudgetBytes must be greater than 0` and
+`limits.connection.maxConnectionsPerIp: maxConnectionsPerIp must be greater than 0` —
+"            \"maxConnectionsPerIp must be greater than 0\""
+(`crates/nostream-config/src/validate.rs:95`). Both verdict surfaces report it and exit 2 —
+`nostreamd --check` through "    if check {" (`crates/nostreamd/src/settings_gate.rs:48`) — while
+a start reports it and keeps serving, saying so with
+"        \"nostreamd: continuing on the shipped defaults ({} issue(s) in the user settings)\""
+(`crates/nostreamd/src/settings_gate.rs:54`). Either way the operator learns which key to fix,
+where a fallback turned the typo into a relay that silently budgeted 256 MiB. A non-number is not
+a zero: that document keeps the shape walk's single finding, as before.
+
+`RelayConfig::from_settings` therefore takes a present leaf as written and keeps `in_range` as
+the ceiling question alone, "pub fn in_range(value: u64) -> bool {"
+(`crates/nostream-relay/src/relay.rs:114`), which now spells
+"    value <= MAX_SETTING_BOUND as u64" (`crates/nostream-relay/src/relay.rs:115`). The zero
+variant of `BoundRefusal`, its refusal sentence and its start-up `warn!` arm are gone with it;
+an absent leaf still means the compiled default and an over-ceiling value still falls back with
+the same two sentences as before.
+
+The TypeScript tree is untouched: no `src/` or `test/` file changes, so the published package
+has no user-visible change.

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`, `#<tag>`) 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.connection.maxConnectionsPerIp      | Maximum number of live sockets one source address may hold at once. Unit: sockets. Defaults to 64. Zero is invalid; a value above 1099511627776 keeps 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, or a value above 1099511627776 (1 TiB), means the default. |
+| network.frameMemoryBudgetBytes              | The queued outbound payload bytes every socket of this relay shares. Unit: bytes. Defaults to 268435456 (256 MiB). Zero is invalid; a value above 1099511627776 (1 TiB) keeps 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. |

crates/nostream-cli/tests/config_cli.rs

@@ -68,33 +68,27 @@ fn the_paths_resolver_honours_the_config_dir() {
 }
 
 #[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.
+fn config_validate_refuses_a_zero_bound() {
+    // The CLI door and `nostreamd --check` are the two read-only ways onto the same validator; a
+    // zero is an issue on both, and the document that carries one is not blessed (#996, ADR 0008).
     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",
+        "network:\n  frameMemoryBudgetBytes: 0\nlimits:\n  connection:\n    maxConnectionsPerIp: 0\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}"
+    assert_eq!(output.exit_code, 2);
+    assert_eq!(
+        output.lines,
+        [
+            "network.frameMemoryBudgetBytes: frameMemoryBudgetBytes must be greater than 0",
+            "limits.connection.maxConnectionsPerIp: maxConnectionsPerIp must be greater than 0",
+            "settings invalid: 2 issues",
+        ]
     );
 }
 

crates/nostream-config/src/guided.rs

@@ -207,7 +207,7 @@ fn build_categories() -> Vec<GuidedSettingCategory> {
                     SafeNonNegativeInteger,
                 ),
                 validated(
-                    "Frame memory budget (bytes, 0 means the default)",
+                    "Frame memory budget (bytes)",
                     "network.frameMemoryBudgetBytes",
                     Number,
                     SafeNonNegativeInteger,
@@ -270,7 +270,7 @@ fn build_categories() -> Vec<GuidedSettingCategory> {
                     SafeNonNegativeInteger,
                 ),
                 validated(
-                    "Max connections per IP (sockets, 0 means the default)",
+                    "Max connections per IP (sockets)",
                     "limits.connection.maxConnectionsPerIp",
                     Number,
                     SafeNonNegativeInteger,

crates/nostream-config/src/validate.rs

@@ -70,6 +70,32 @@ pub fn validate_document(candidate: &Value, defaults: &Value) -> Vec<Issue> {
         ));
     }
 
+    // The two bounds the relay reads at run time are numerical settings before they are anything
+    // else, so a zero written for one is refused here, by name, exactly as the adaptive-pow
+    // numbers below are: absent still means the compiled default, and a stated zero is a literal
+    // the operator wrote (ADR 0008). Only a JSON number takes this rule: `to_number` reads a null,
+    // a boolean and a numeric string as numbers too, and a wrong type is the shape walk's one
+    // finding to make — two issues for one key would leave the operator to guess which to fix.
+    if let Some(budget @ Value::Number(_)) =
+        key_path(candidate, &["network", "frameMemoryBudgetBytes"])
+        && !to_number_is_positive(Some(budget))
+    {
+        issues.push(Issue::new(
+            "network.frameMemoryBudgetBytes",
+            "frameMemoryBudgetBytes must be greater than 0",
+        ));
+    }
+
+    if let Some(cap @ Value::Number(_)) =
+        key_path(candidate, &["limits", "connection", "maxConnectionsPerIp"])
+        && !to_number_is_positive(Some(cap))
+    {
+        issues.push(Issue::new(
+            "limits.connection.maxConnectionsPerIp",
+            "maxConnectionsPerIp must be greater than 0",
+        ));
+    }
+
     if let Some(pow) = key_path(candidate, &["limits", "event", "pow"])
         && truthy_opt(key(pow, "enabled"))
     {

crates/nostream-config/tests/validator.rs

@@ -194,6 +194,90 @@ fn a_non_array_rate_limits_value_is_left_to_the_shape_validator() {
     );
 }
 
+// --- the two runtime bounds -------------------------------------------------------------
+
+#[test]
+fn rejects_a_zero_frame_memory_budget() {
+    // A zero here is the operator's literal, and the relay must not read it as the default
+    // (#996, ADR 0008). Absent still means the compiled default, which is why the document is
+    // the shipped one with this leaf changed and no other issue can appear.
+    let document = set_by_path(&defaults(), "network.frameMemoryBudgetBytes", number(0))
+        .expect("frameMemoryBudgetBytes is writable");
+    assert_eq!(
+        issues(&document),
+        vec![
+            "network.frameMemoryBudgetBytes: frameMemoryBudgetBytes must be greater than 0"
+                .to_owned()
+        ]
+    );
+}
+
+#[test]
+fn rejects_a_zero_max_connections_per_ip() {
+    let document = set_by_path(
+        &defaults(),
+        "limits.connection.maxConnectionsPerIp",
+        number(0),
+    )
+    .expect("maxConnectionsPerIp is writable");
+    assert_eq!(
+        issues(&document),
+        vec![
+            "limits.connection.maxConnectionsPerIp: maxConnectionsPerIp must be greater than 0"
+                .to_owned()
+        ]
+    );
+}
+
+/// The shape walk's one finding for a wrong type, which is all such a document gets.
+fn shape_only(path: &str, got: &str) -> Vec<String> {
+    vec![format!("{path}: Expected number, got {got}")]
+}
+
+#[test]
+fn a_non_number_frame_memory_budget_is_only_a_shape_fault() {
+    // `to_number` reads a null, a boolean and a numeric string as numbers, so a rule written on
+    // the number alone reports twice for one key: two things for the operator to act on, and no
+    // way to tell which one is the mistake. The zero rule is the shape walk's neighbour, not its
+    // replacement (#996).
+    for (value, got) in [
+        (Value::String("0".to_owned()), "string"),
+        (Value::Null, "null"),
+        (Value::String("abc".to_owned()), "string"),
+        (Value::Bool(true), "boolean"),
+    ] {
+        let document = set_by_path(&defaults(), "network.frameMemoryBudgetBytes", value.clone())
+            .expect("frameMemoryBudgetBytes is writable");
+        assert_eq!(
+            issues(&document),
+            shape_only("network.frameMemoryBudgetBytes", got),
+            "for {value:?}"
+        );
+    }
+}
+
+#[test]
+fn a_non_number_max_connections_per_ip_is_only_a_shape_fault() {
+    for (value, got) in [
+        (Value::String("0".to_owned()), "string"),
+        (Value::Null, "null"),
+        (Value::String("abc".to_owned()), "string"),
+        (Value::Bool(false), "boolean"),
+    ] {
+        let document = set_by_path(
+            &defaults(),
+            "limits.connection.maxConnectionsPerIp",
+            value.clone(),
+        )
+        .expect("maxConnectionsPerIp is writable");
+        assert_eq!(
+            issues(&document),
+            shape_only("limits.connection.maxConnectionsPerIp", got),
+            "for {value:?}"
+        );
+    }
+}
+
 // --- adaptive pow -----------------------------------------------------------------------
 
 fn pow_with(overrides: &[(&str, Value)]) -> Value {

crates/nostream-relay/src/relay.rs

@@ -103,25 +103,22 @@ impl Default for RelayConfig {
 /// 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).
+/// [`FrameBudget::admit`]'s arithmetic. A value above it keeps the compiled default (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
+/// Whether an operator's value for a bound is one the relay can run with: 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).
+/// disagreeing (ADR 0008). Zero is in range: the validator refuses it, so a value that reaches
+/// here is one the relay runs with as written.
 pub fn in_range(value: u64) -> bool {
-    value > 0 && value <= MAX_SETTING_BOUND as u64
+    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.
+/// loud, so the report can name the ceiling the value crossed.
 #[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),
 }
@@ -149,40 +146,32 @@ pub fn refused_bounds(settings: &Settings) -> Vec<(&'static str, BoundRefusal)>
     refused
 }
 
-/// The two ways a bound's setting is not taken, or `None` when it is.
+/// Whether 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<BoundRefusal> {
     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
+/// The sentence one refused bound is reported with, for both doors; 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.
+/// the ceiling it crossed.
 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,
@@ -198,17 +187,19 @@ 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.
+    /// `limits.connection.maxConnectionsPerIp` — when present.
     ///
     /// 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.
+    /// A leaf is taken as written whenever it is at most [`MAX_SETTING_BOUND`]; an absent leaf
+    /// keeps the default, and so does a value above the ceiling. The ceiling is not cosmetic:
+    /// `usize::MAX` fits a `usize`, so it would pass a bare range 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. A zero never
+    /// reaches this constructor from a loaded document: the validator refuses it by name
+    /// (ADR 0008).
     pub fn from_settings(settings: &Settings) -> Self {
         let mut config = Self::default();