Skip to content

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

Open
chappie-daemon wants to merge 8 commits into
factory/017-frame-budget-settingsfrom
factory/018-zero-is-invalid
Open

chappie-daemon wants to merge 8 commits into
factory/017-frame-budget-settingsfrom
factory/018-zero-is-invalid

Conversation

@chappie-daemon

@chappie-daemon chappie-daemon commented Oct 25, 2026 •

Copy link
Copy Markdown
Collaborator

#989 gave 0 a new meaning — use the compiled default — and enforced it with a test. That was the wrong answer, and this PR reverses it: a stated 0 is a validator issue on both keys, and absent is the only route to the default.

The record says so itself. The ADR's own predecessor text is "A zero budget or a zero cap is a literal the operator wrote; policing it is a settings decision this job does not own" — a sentence written against the default reading, and a deferral this PR spends.

Closes #996. Stacked: base is #995's branch, which bases on #990 — see the end.

Why rejection, not fallback

A 0 meaning default is a fourth meaning for a numeral that already carries two here: disabled (limits.client.subscription, if max_subscriptions > 0) and invalid (the adaptive-pow numbers). These two bounds belong to neither — a zero budget or a zero cap is a relay that cannot serve.

And rejection is the only option whose failure is loud and early. The app names the key and refuses the file:

network.frameMemoryBudgetBytes: frameMemoryBudgetBytes must be greater than 0
settings invalid: 1 issues                                    exit 2

A fallback turns a typo — 0 where 1048576 was meant — into a relay that budgets 256 MiB and says nothing, leaving the operator to infer it from a reading of a file they did not write.

What changed

from_settings the > 0 guard goes; a present value is used as written, None is the compiled default
validate_document two rules in the periodMs / targetEventsPerSecond shape, one per key
BoundRefusal::Zero deleted with its three arms; the ceiling and AboveCeiling are byte-identical to base
tests one per key, each failing alone when its rule is removed; the four non-number shapes pinned to the shape message alone
documents the ADR's ## What 0 means, both CONFIGURATION.md rows, the two YAML comments, the guided labels

One subtlety worth the reviewer's time: the first attempt guarded the new rules on to_number, which maps null, false and "0" to 0.0 — so those documents printed two issues for one key. The rule now enters only on a JSON number, and a string, a null, abc and a boolean each print the shape message alone. Both doors agree.

The three surfaces, measured

  • --check: the issue, then settings invalid: N issues, exit 2.
  • a serving start: the issue, then continuing on the shipped defaults (N issue(s) in the user settings), exit 0 — an operator is told their relay is up and on which numbers.
  • config validate: the issue, then the invalid verdict, exit 2.

These are the sentences the ADR carries, one bullet each, anchored to the lines that print them.

Verification

All five repository commands green; validator 59, config_defaults 8, check_flag 9, settings_gate 3, config_cli 7; workspace 940 tests over 57 targets, 0 failures. Per-key mutation: each rule removed alone fails only its own test. BoundRefusal::Zero and 0 means the default are gone from the tree. Nine protected paths — sink.rs, connection_cap.rs, settings.rs, ts_parity.rs, ts-issues.json, nostreamd/src/main.rs, acceptor.rs, settings_gate.rs, nostream-cli/src/config.rs — have an empty git diff --stat against the base.

Two disclosures

The ADR quotes a base that no longer exists here, and says so. Reason 1 quotes 0008:369-371 as superseded — those are the base tree's lines, resolved with git show 60aa7e9:<path>, and the text is marked as the position this PR replaces. The sentence being restored is quoted from 7f16351:228-229, where the ADR first wrote it. Both were read back verbatim against their trees by hand; the audit tools resolve line numbers, not meaning.

scripts/citation-meaning.py — added by #995 — cannot resolve a bare filename, so five citations in this document needed a path prefix to keep it green. That is a tool limitation recorded rather than fixed here; the prefix is correct for a reader anyway.

What this $1.03 is the price of

This job exists because a policy answer arrived after #989's build had started: 017 shipped
0 as use the compiled default and enforced it with a test, and the operator rejected that
during review. The change here is 60 lines and one ADR paragraph; the cost is a sequencing
failure, not a large change. The rule recorded for both sides of the queue: a manager checks the
queue for an existing answer before asking, and an operator checks the job's stage before
answering a question it has already moved past — a policy answer that lands after the build is a
new job.

Base

This PR bases on factory/017-frame-budget-settings (#995), which bases on factory/016-frame-memory-budget (#990). Merge order is #990 → #995 → this, and nothing retargets automatically — GitHub moves a pull request only when its base branch is deleted:

gh api -X PATCH repos/cameri/nostream/pulls/<n> -f base=factory/017-frame-budget-settings   # after 995 merges
gh api -X PATCH repos/cameri/nostream/pulls/<n> -f base=rewrite/main                        # after 995's own retarget

Each step: retarget, then rebase if the merge added anything.

@changeset-bot

changeset-bot Bot commented Oct 25, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: d0d8a3f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 0 packages

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@greptile-apps

greptile-apps Bot commented Oct 25, 2026 •

Copy link
Copy Markdown

RetriggerView in GreptileConfidence Score: 5/5

[Medium impact] The PR appears safe to merge; no actionable issues were found.

Summary

Rejects zero for both relay bounds while preserving defaults for absent keys and values above the ceiling.

  • A written zero now fails validation for both relay bounds.
  • The relay constructor now keeps a present zero instead of using defaults.

Scope note: chappie-daemon explicitly defers bounded checks in the guided editor in ADR 0008.

Reviews (1) · Last reviewed commit: "docs(rewrite): cite reason 1 by full pat..." · Reviewed by Greptile

@coveralls

Copy link
Copy Markdown
Collaborator

Coverage Report for CI Build 40183229062

Warning

No base build found for commit 60aa7e9 on factory/017-frame-budget-settings.
Coverage changes can't be calculated without a base build.
If a base build is processing, this comment will update automatically when it completes.

Coverage: 73.979%

Details

  • Patch coverage: No coverable lines changed in this PR.

Uncovered Changes

No uncovered changes found.

Coverage Regressions

Requires a base build to compare against. How to fix this →


Coverage Stats

Coverage Status
Relevant Lines: 10291
Covered Lines: 7992
Line Coverage: 77.66%
Relevant Branches: 5362
Covered Branches: 3588
Branch Coverage: 66.92%
Branches in Coverage %: Yes
Coverage Strength: 868.55 hits per line

💛 - Coveralls

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants