Repository navigation
Conversation
…past the queue and says so; a sender paces to the budget A receiver bounds what one session's records cost it (8 a second after 32; 128 KiB/s after 1.5 MiB; 32/s and 1 MiB/s before verification). A record past either lane was dropped, unevaluated, while its sender reported it dispatched: 49 directed messages in a burst arrived as 32-38, 12 x 200 KB as 7, 120 held records re-offered after a relay drop as 38. Receiver (lib/flow-control.js RecordAdmission, frame-handler): every record of a session is admitted only when both lanes hold it and nothing of the session waits ahead of it, else it waits in the session's queue, in order (64 s of the budget: 512 records, 8 Mi characters; 4096 and 32 Mi in all). Past the queue it is refused and the sender told in a sealed `cmb-refused` (assertionIds, retryAfterMs), at most once a second per peer. A peer that, told, keeps sending past its own queue before it drains is closed and not admitted for 60 s. A stopping node refuses and notifies what waits. Each admitted session is sent `cmb-budget` (rate, burst, chars rate, chars burst, queue bound). Sender (node.js): every record to a peer goes through its held list and is written only within the budget the peer advertised (0.14.2 peers: 6/s after 24, a margin under their 8/32, no size budget). Dispatched means written; delivery.queued counts what is held. A refused record goes back in its place and waits for the retry-after on any session; after 8 refusals or 23 h it is given up (cmb-undelivered, the 'undelivered' event, undelivered()). The held list refuses past its bound instead of pushing out what it holds.
…-peer directions) and every new guard tests/flow-control.test.js: 49 messages, 12 x 200 KB and 40 x 50 KB over the LAN, 49 messages over a relay, 120 records across a relay drop and across a LAN drop, new sender to a 0.14.2-like receiver and a 0.14.2-like sender to a new receiver: all delivered once, the sender's counts true. tests/flow-control-bounds.test.js: the queue's bounds (per session and in all, by count and size), the door and penalties for what waits, arrival order, the cut and its episodes, the pre-verification lane for copies, the notice rate, a stopping receiver, the advertisement and malformed frames, legacy pacing, a late advertisement, anchors, refusals (in place, back-off, any session, given up, room records to one peer only, an unmatched or late refusal, one refused in the relay pacer), the age bound, the held list's bounds, the buckets' clock. The R2-3 and record-flood tests and the held-list bound test now pin waiting, not dropping.
…ng, the cut, old peers), and the budget no longer drops The 0.14.3 header no longer says the wire is unchanged: two sealed control frames, which 0.14.2 ignores. "Known, not fixed here" loses the dropping receiver and gains what flow control does not keep (order across a refusal). The R3-4 held list refuses past its bound instead of pushing out the oldest. SECURITY.md's record bounds name the queue, the refusal notice and the cut.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3259a4414e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const cats = cmb && typeof cmb === 'object' ? cmb.categories : null; | ||
| if (cats && typeof cats === 'object') { | ||
| for (const v of Object.values(cats)) { | ||
| if (typeof v === 'string') n += v.length; | ||
| else if (v && typeof v === 'object' && typeof v.text === 'string') n += v.text.length; | ||
| } | ||
| } | ||
| const md = cmb && typeof cmb === 'object' ? cmb.metadata : null; | ||
| if (md && typeof md === 'object') { | ||
| if (md.application && typeof md.application === 'object' && typeof md.application.data === 'string') n += md.application.data.length; | ||
| if (typeof md.createdBy === 'string') n += md.createdBy.length; |
There was a problem hiding this comment.
recordChars() counts only recognized text fields, but schema validation happens after admission, so an authenticated peer can place a near-720 KiB malformed record in an uncounted category/metadata member while this function charges only 512 characters. Such records can fill the 512-entry per-session queue with roughly 360 MiB, and multiple sessions can approach several GiB despite QUEUE_TOTAL_CHARS, creating a remotely triggerable memory-exhaustion path; bound the queue using the complete encoded record size or validate its shape before retaining it.
Useful? React with 👍 / 👎.
| const ids = n.ids.splice(0, REFUSAL_IDS_MAX); | ||
| const retryAfterMs = this._retryAfter(session, n.othersFull); | ||
| let sent = false; | ||
| try { sent = session.trySend({ type: REFUSED_FRAME, assertionIds: ids, retryAfterMs }).ok === true; } catch { sent = false; } | ||
| if (!sent) { | ||
| this.stats.unsent += ids.length; | ||
| this._o.metric({ type: 'cmb-refusal-unsent', peer: peerId, count: ids.length, reason: 'send-failed' }); | ||
| break; |
There was a problem hiding this comment.
When the connection closes or its transport rejects cmb-refused, the IDs have already been removed with splice() and are merely counted as unsent. A subsequent session therefore has nothing for flushFor() to send, while the original sender still considers those receiver-discarded records dispatched, causing permanent record loss exactly during the reconnect scenario this flow control is intended to recover from; restore the IDs to the pending notice on failure.
Useful? React with 👍 / 👎.
| const e = this._shift(session, q); | ||
| this.stats.admittedLate++; | ||
| try { this._o.run(e); } catch (err) { this._o.fail(e, err); } |
There was a problem hiding this comment.
run() can return the asynchronous SVAF/admission promise, but _drain() immediately starts subsequent entries without awaiting it, so a later small record can surface before an earlier large record. Running node --test --test-name-pattern='arrival order' tests/flow-control-bounds.test.js reproduces this consistently, with order small messages interleaved before order big 11; serialize completion if the promised arrival-order behavior is required.
Useful? React with 👍 / 👎.
…k; the stop test waits until the receiver holds every record CI (Linux) caught both. The two final-review tests counted cmb-over-verify-budget, which flow control retires: past the pre-verification lane a record now waits in the session's queue (or, past it, is refused unchecked), and 3b also pins that only the lane's burst is checked at once. On a slower host part of the stop test's burst was still unread in the receiver's socket when it stopped: that is the transport residual (CHANGELOG now names it), not the queue, so the test now stops the receiver once every record has reached it.
…none), not a surfaced total a slower host drains past; two timing windows widened
… surface events, which a large record's asynchronous evaluation reorders
Flow control for directed and room records (0.14.3)
The defect (independent review of release/0.14.3 head 9f6847c, and 0.14.2): a receiver applies a per-session record budget (8/s after 32; 128 KiB/s after 1.5 MiB; 32/s and 1 MiB/s before verification) and DROPPED what was past it, unevaluated, while the sender reported it dispatched and nothing offered it again. Measured on 9f6847c with real nodes: 49 directed messages in a burst -> 32 delivered (LAN), 38 (relay); 12 x 200 KB -> 7; 40 x 50 KB -> 30; 120 records held across a relay drop and re-offered -> 38, the sender reporting 115 redelivered.
The design (lib/flow-control.js):
{ type: 'cmb-refused', assertionIds: [<=256], retryAfterMs }(drain time of the queue, 1-64 s; at most one notice a second per peer). A peer that, told, sends more than 512 past its own queue before it drains is cut (closed, not admitted for 60 s). Refused only because others filled the queue never counts. A stopping node refuses and notifies what waits.{ type: 'cmb-budget', recordsPerSecond, recordsBurst, charsPerSecond, charsBurst, queueRecords, queueChars }.delivery.queuedcounts held copies. Refused records go back in place, wait for the retry-after on any session, are retried to that peer only; after 8 refusals or 23 h they are given up (cmb-undelivered, theundeliveredevent,undelivered()).status().flowControlandpendingDeliveries()(with state) report it all.Known limits (in CHANGELOG): order across a refusal is not kept (records written before the notice arrives can be taken ahead of a refused one; keeping it would mean refusing inside the bound, which costs 0.14.2 senders); the residual of a frame lost after a socket wrote it stays.
Tests, measurements, mutation, suite/gate/audit/docker: see the CHANGELOG 0.14.3 "Flow control" part and "Tests".
Spec text is not changed here (to be updated after the implementation is fully tested).