Repository navigation
fix(config): reject a zero frame-memory budget or per-IP cap (#996) - #997
Merged
Merged
Conversation
…s may hold (#959) The outbound queue was bounded per socket and their sum was bounded by nothing: one address could hold 8 MiB for every socket it kept, and a closed socket's queued bytes were never given back. Every socket now counts its queued payload against one process-wide FrameBudget (256 MiB, RelayConfig::frame_memory_budget_bytes), refunded when the socket's last sink handle drops, and one address may hold at most max_connections_per_ip live sockets (64) as a gauge beside the attempt limiter. Both refuse with an immediate 1013 close, a fixed reason and their own counter. See docs/rewrite/decisions/0008-frame-memory-budget.md and #959.
… 0008) Both numbers are deviations from the capture, with their arithmetic, the rejected accept-time shape, the refund on the last handle's Drop, and what an operator can see today (#959).
…the budget in the sink doc The RelayConfig doc enumerates the port-only policies, so it has to name all four; the sink's module doc is where a reader learns what the sum is bounded by.
…ames it can queue Three static-review findings on the frame-memory budget, in one commit because they meet in FrameSink::send: - A refusal recorded a flag but woke nothing. A shared-budget refusal can arrive while this socket's own queue is empty, the socket idle and its read parked, so the 1013 waited for the next liveness tick instead of being immediate. The first refusal now signals a tokio::Notify the account owns, and the connection loop selects on FrameSink::refused(); notify_one stores a permit, so a refusal recorded between the loop's check and its select is not lost. - The queue-full check ran after to_text(), so a latching socket kept serialising events nobody would read. The early return is back before the encoding, with the in-loop check kept for racing senders. - The racing-sender test's sampler stopped only at the expected total, so the accounting bug it exists to catch could spin forever inside thread::scope. It now stops when the last sender finishes, and fails with numbers. The new wire test holds the budget from a socket that never reads and loses a frame on a socket whose own queue is empty: it times out without the signal and closes in 1.3 s with it.
…t the record Review round 1: a performance finding and two documentation defects, plus four notes. - A budget refusal did not stop the encode. The read loop in fetch_and_send encoded one frame per event and discarded each refused one; the window is the connection loop's write plus close_frame (30 s each) and is the normal case, since the budget binds when queues are full and loops are parked in writes. The read now ends at the top of its loop when the sink has a refusal, before pulling or encoding the next record; the sink still does not latch a budget refusal, so AC6's re-admission holds. New test in subscribe.rs: with a 64-byte budget every event frame is refused and the EOSE would fit, so an empty queue proves the read stopped (without the check it holds the 12-byte EOSE). - Two ADR citations were stale by six lines and one named the wrong arm; every citation in the file was re-read against the final tree. - The ADR presented the cap's refusal line as operator-visible; it is a debug! line and the daemon ships the info filter, so the record now says so, names the switch, and calls the gap a gap. Notes: the two new refusals are separated by the reason string, not by 1013; the shared budget's blast radius (one address below its cap can refuse every other client, including a newcomer's AUTH) is recorded; the cap's key is an address, so a [::] bind keys per /128; and RelayConfig's doc no longer claims the policies never move from their defaults.
…ke-up is what closes Static round 2 showed the parked-read test passed with the refusal signal deleted: the read returned at the guard, its handler completed, and handlers.join_next() woke the loop. The two changes had cancelled each other's discriminating power, and the close's promptness came from a handler finishing rather than from the signal. A refused read now waits for the close that retires it (the cancellation watch, then end_read) instead of unwinding: the connection loop's abort_all/close stays the single owner of the teardown, and the close is prompt because the refusal woke the loop. The read's own test is restructured for the park — pending after the refusal, one record taken, no EOSE queued, retired by the connection's close — and the two tests now fail on their own mechanism: deleting the refused() arm makes the wire test time out at 2 s, and deleting the read's guard makes the unit test fail with the park message.
…that holds Round 2 (code, security, architecture) found one ADR paragraph that 5b23eeb had made false, the code comment copying it, and an ownership claim one clause too strong. - The read-stop paragraph said a refused read 'ends there'. It stops producing there and then waits for the close that retires it, so it now says that, names what a refused client therefore still holds, and no longer contradicts its own account of the two 30 s write windows. - The encode citation moved from the park comment to the send (subscribe.rs:325-330), and the guard is cited where it is (:284-288). - 'its read parked on the store' in the wake-up paragraph was the pre-1479c6c state; the relay.rs comment copied it. Both now say stopped and waiting for the close. - Section 7 records that the park holds the store stream to the close: latent today (unavailable::stores), but sqlx's &Pool executor holds a pooled connection per stream and DB_MAX_POOL_SIZE is 3, so whoever wires the store leaves the loop before parking. - The ownership comment claimed the loop's abort_all/close was the sole owner of teardown while end_read is reachable from six places. It now says what makes that safe: end_read removes the entry only while the generation still matches (connection.rs:290-297). Every line reference in the ADR was re-read from the final tree: 53 of them, all resolving.
Returning from the loop before parking does not put the close's promptness onto the refusal signal - it removes the signal's necessity, because a returned handler wakes the loop on completion. Say that, and that the wire test stops discriminating in that world.
Round 3 (code, architecture) measured the same clause independently: every connection-loop exit is a break, handlers.abort_all() runs before connection.close(), and a JoinSet-aborted task is dropped without being polled again. So on the refusal path the parked read dies inside the cancellation watch, its end_read never runs, and the store stream goes with the abort; the watch is the path a client CLOSE or a re-REQ takes, and the one the unit test exercises. Also: the refused frame is the only one encoded per read (and a queue-latched socket skips that too), the wire-test pointer no longer says 'below' with nothing below it, and section 7 gives the deployed pool size (prod compose 64, no replica, so held connections stall writes as well) beside the code default of 3. The two code comments that described the watch as the retirement now match.
…catalogue does not
…he number is typed
…it as the default
… the sentence it replaced
🦋 Changeset detectedLatest commit: 3c72dec The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen 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 |
 [Medium impact] The PR appears safe to merge; no new actionable issue was found.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Collaborator
Coverage Report for CI Build 41057474671Coverage remained the same at 73.973%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
Base automatically changed from
factory/017-frame-budget-settings
to
rewrite/main
October 30, 2026 12:39
cameri
approved these changes
Oct 30, 2026
rewrite/main squash-merged #995 (4a7e74c), which retargeted this PR onto rewrite/main and broke the stack. The branch already carries #995's commits and every change in the squash, so each of the 8 conflicting files resolves toward the branch; documented reasons and the ADR anchors come from this branch.
greptile-apps
Bot
dismissed
their stale review
October 30, 2026 12:43
Dismissed because a newer commit was pushed; Greptile will re-review the current head.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#989 gave
0a new meaning — use the compiled default — and enforced it with a test. That was the wrong answer, and this PR reverses it: a stated0is 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
0meaning 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:
A fallback turns a typo —
0where1048576was 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> 0guard goes; a present value is used as written,Noneis the compiled defaultvalidate_documentperiodMs/targetEventsPerSecondshape, one per keyBoundRefusal::ZeroAboveCeilingare byte-identical to base## What 0 means, bothCONFIGURATION.mdrows, the two YAML comments, the guided labelsOne subtlety worth the reviewer's time: the first attempt guarded the new rules on
to_number, which mapsnull,falseand"0"to0.0— so those documents printed two issues for one key. The rule now enters only on a JSON number, and a string, a null,abcand a boolean each print the shape message alone. Both doors agree.The three surfaces, measured
--check: the issue, thensettings invalid: N issues, exit 2.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;
validator59,config_defaults8,check_flag9,settings_gate3,config_cli7; workspace 940 tests over 57 targets, 0 failures. Per-key mutation: each rule removed alone fails only its own test.BoundRefusal::Zeroand0 means the defaultare 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 emptygit diff --statagainst the base.Two disclosures
The ADR quotes a base that no longer exists here, and says so. Reason 1 quotes
0008:369-371as superseded — those are the base tree's lines, resolved withgit show 60aa7e9:<path>, and the text is marked as the position this PR replaces. The sentence being restored is quoted from7f16351: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
0as use the compiled default and enforced it with a test, and the operator rejected thatduring 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 onfactory/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:Each step: retarget, then rebase if the merge added anything.