Repository navigation
feat(relay): bound frame memory globally and connections per IP (#959) - #990
Open
chappie-daemon wants to merge 12 commits into
Open
chappie-daemon wants to merge 12 commits into
chappie-daemon wants to merge 12 commits into
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.
🦋 Changeset detectedLatest commit: 04186d4 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 with its documented policy choices.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
This was referenced Oct 25, 2026
This branch has not been deployed
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.
The relay's memory is bounded per socket — 8 MiB of queued frames each — and by nothing in aggregate. At the shipped limits one address can hold 48 sockets (the attempt limiter's window) or 64 (this PR's cap), so the arithmetic on configured counts tops out around 384 MiB of counted payload with a worse resident factor. This bounds the thing that actually grows.
Closes #959.
What it does
Two bounds, both
RelayConfigfields with the operator's numbers as defaults:frame_memory_budget_bytesRelay, shared by every connection and every listenermax_connections_per_ipA connection that crosses either bound is closed immediately with
1013and no text frame. The per-IP check runs after the attempt limiter and beforeConnection::accept, because that call's first act writes the AUTH challenge as a text frame — a refusal that wrote it would be a refusal that talked first. The budget is admitted per frame insideFrameSink::send, sharing the CAS accounting already there, so there is no lock on the hot path.256 MiB / 8 MiB = 32 full sockets, which binds before both the 48-socket rate outcome and the 64-socket cap. That is the point of the shape: the cap is not a memory bound, and the budget is.The close is prompt because it is signalled, not polled. The first refusal stores a
Notifypermit; the connection loop selects on it beside its other arms. Without that, a socket whose queue is empty waits for the next liveness tick — up to 120 s — for a close the policy calls immediate. A test pins it: one socket holds most of the budget with a blocked write, a second is refused from a parked read, and the close must arrive inside two seconds.The refund is a
Drop, not a code path.FrameSinkholds anArc<Account>; the last handle's drop returns the socket's outstanding bytes. The existingreleasehad exactly one caller, so every other loop exit — including the1013overflow close — would have leaked the budget. The test that guards it drops one of two sinks without callingreleaseand asserts exactly its own bytes come back; it fails withleft: 42, right: 21when the refund is emptied.Two refusals, distinguishable where it matters. Both close
1013; what tells them apart is the reason string on the close frame, the counter, and the log fields. Refused frames are counted and logged per cause, so a refusal does not read as a network fault.The record
docs/rewrite/decisions/0008-frame-memory-budget.mdrecords both bounds as deviations from the capture, which has neither, with the numbers, the rejected shapes (accept-time reservation, a latched budget refusal), what an operator can see today — including that the cap's line isdebug!and therefore invisible at the shippedinfofilter, which is a gap this record owns rather than covers — and what it does not decide.Two things it states that are worth knowing before reading the code:
[::]bind keys per /128, and one host with a /64 would bound nothing.Known limitation, filed
The blast radius §6 of the ADR describes — one address below its own 64-socket cap exhausting the shared 256 MiB budget and refusing every other client's next frame, including a newcomer's AUTH challenge — is tracked as #992, with the fix shape (a per-address share of the budget rather than a second charge on legitimate traffic) and with the same measured caveat the ADR states: 256 MiB of counted payload is 1.04×–2.83× resident, so it is not an RSS ceiling. I left the ADR's green head alone rather than adding a link commit; this paragraph is the link, and it is the only edit this PR has had since approval.
Latent, recorded, not fixed here
The refused read stops producing at its next loop iteration and then waits for the close that retires it — so, while it waits, it holds its store stream. Today that costs nothing: the daemon wires
unavailable::stores(). When a real store lands, sqlx holds a pooled connection per live stream andDB_MAX_POOL_SIZEis 3 (64 in the deployed compose, with no replica), so whoever wires it must leave the loop before parking. §7 of the record says so with the arithmetic.Verification
cargo test --workspace --all-features(56ok, 0 failed), both clippy passes,fmt, release build — all clean at head; relay crate 249 tests across 9 targets, from 238 at base. Two mutations prove the two tests that matter: empty the refundDrop→ the release test fails with numbers; delete the refusal select arm → the prompt-close test fails on its 2 s bound.One deliberate deviation, recorded in the job's
build.md:tests/limits.rsgains exactly one argument (channel(1024 * 1024)→channel(1024 * 1024, None)) becauseFrameSink::channeltakes the budget. The limiter's own tests, windows and assertions are untouched;src/limits.rsis byte-identical.Notes
.husky/_/husky.shis absent and the hook drives the legacy pnpm checks, so commits usedHUSKY=0 git commit --no-verify.