fix(mllp): bound one frame's life and one peer's connection count (BACKLOG #1725) - #1309
Open
wshallwshall wants to merge 8 commits into
Open
wshallwshall wants to merge 8 commits into
wshallwshall wants to merge 8 commits into
Conversation
added 5 commits
September 19, 2026 13:28
… #1725) Work in progress, committed early so an interruption cannot lose it. Checks and review still to run; this message is amended before the push. `receive_timeout` is applied per read, so it resets on every byte received: it bounds a silent socket and says nothing about how long one frame may take to arrive. `max_connections` counts sockets rather than hosts, and `source_ip_allowlist` ships off, so a default listener carried no peer-scoped term at all. Items 1, 2 and 4 of the row. Item 3 (a per-listener in-flight ingress semaphore) is written as "Consider" and is deliberately left out of scope.
…tants `docs/CONNECTIONS.md` is the operator reference for connector settings, so a shipped-on cap that is not in it is a cap nobody can find or tune. Three places: the MLLP settings table gets a row each for `max_connections_per_host` and `max_frame_seconds`; the resource-management prose gains two bullets saying what `receive_timeout` does NOT bound and that `max_connections` counts sockets rather than hosts; Table A and Table B's MLLP listener rows name the new bounds beside the old ones. Both new bullets say MLLP-listener-only in words. The raw-TCP, X12, HTTP and DICOM intakes carry neither bound, and the tables sit close enough together that a reader could otherwise take these as general listener behaviour. `tests/test_communications_inventory.py` pins each stated default to the module constant it came from (`_import_code_defaults`), the same way the existing `max_connections` and `receive_timeout` rows are pinned. Changing a constant without the doc now reds instead of shipping a listener whose documented default is not the one it runs. Items 1, 2 and 4 of BACKLOG #1725. Item 3 (a per-listener in-flight ingress semaphore) is written as "Consider" and is deliberately left out of scope.
Review round 1, and the finding was reproduced rather than argued. A pipelined sender's reads almost never end on a frame boundary, so `decoder.in_frame` stays True read after read across DIFFERENT frames. Stamping only when nothing was being timed therefore measured every later frame from the FIRST frame's start byte: a healthy high-volume feed was reset once per `max_frame_seconds`, forever. At the shipped 60 s default that is roughly once a minute on any feed that keeps the socket continuously in-frame. `decoded` is what makes the stamp per frame. Any frame that was being timed is finished once a read completes one, so an open frame after that is a new one and its clock starts there. The existing control arm could not see this: it sends ONE frame, so `in_frame` goes False and the clock never runs across frames. The new arm keeps the decoder continuously in-frame by ending every write with the next frame's start byte, which is deterministic rather than dependent on where a 4096-byte read happens to land. A first version of it wrote all the frames at once and passed against the defect, because two reads drained the lot. Two further fixes in the same layer: - Paced time is no longer charged to the frame's budget. `_MessagePacer` promises it "never drops, never NAKs and never refuses"; without this the engine's own back-pressure could close a connection holding a partial frame, which is that promise broken and a partial frame discarded outside the count-and-log boundary. - `_admit()`'s slot is now released on a cancellation during the `established` emit. That emit is fail-soft against `Exception` but not `CancelledError`, and `stop()` cancels straggling client tasks. It leaked an `_active` count before; with a per-host table it would leak an entry nothing decrements, locking that address out until restart. The trickle test now accepts an abortive RST as well as a clean EOF. Windows sends RST when a socket is closed with bytes still unread, and a peer that is still writing when the listener closes sits in exactly that window; asserting `b""` alone flaked on which the OS chose. Items 1, 2 and 4 of BACKLOG #1725. Item 3 (a per-listener in-flight ingress semaphore) is written as "Consider" and is deliberately left out of scope.
…three claims
The rest of review round 1. The critical per-frame clock fix landed separately in
the commit before this one.
Behaviour:
- The per-host refusal now logs once per host per episode instead of once per
attempt. A peer looping connect() against a budget it has already filled is
refused as fast as it can open sockets, and NSSM captures the service's stdout
to files, so a line per refusal let an unauthenticated peer fill the log volume
-- the defense amplifying the flood it exists to stop. The global
`max_connections` refusal logs nothing at all; one line per episode is the
middle ground, and the `at_capacity` event still records every refusal. The
warned set is keyed and cleared exactly like `_per_host`, so it adds no second
population to leak, and it clears at zero rather than at the cap so a peer
oscillating on the boundary cannot earn a line per cycle.
- `_read_budget` drops an unreachable `max(0.0, ...)` clamp. No input could reach
it: `receive_timeout` is None rather than falsy when off, and the caller has
already broken out of the loop on a spent frame budget. Dead defensive code in
a bound is a claim nobody can check.
Three claims that were false or overstated, all found by review:
- `docs/PHI.md` said the `closed` reason is `eof` or `idle_timeout` and that "no
path produces any other". This change adds `frame_deadline`, and nothing in CI
pinned the sentence, so the false enumeration would have shipped in the record
an assessor reads. Now states the vocabulary without the closed claim.
`test_connection_event_row_names_exactly_the_shipped_kinds` reads every
backticked token in that cell as an event KIND, so `frame_deadline` joins `eof`
in the close-reason exemption -- verified still red on a planted bad kind.
- The `_per_host` comment implied the cap bounds a peer cycling source addresses.
It does not: it keys on the address, so eight addresses restore the full 256.
What the key-dropping bounds is the TABLE, which is keyed by an attacker-chosen
value. The constant now says plainly what the cap does not cover.
- `DEFAULT_MAX_CONNECTIONS_PER_HOST` claimed thirty-two sockets from one address
"is not a shape ordinary NAT'd traffic produces". False behind a source-NAT
proxy, where every partner arrives as one peer and 32 becomes the effective
listener capacity while the operator reads 256 in their config. Recorded as a
real cost with the two things that make it survivable: the refusal names itself,
and `None`/`0` is the documented setting for that topology.
Also: the 60 s deadline does not derive from the 16 MiB byte cap, and the pair
needs about 2.2 Mbps to be satisfiable. Both are now documented as one bound in
two units -- raise the seconds when you raise the bytes. A start-time warning is
deliberately not built: the honest threshold is a link speed the engine cannot
see. The review cited two samples as evidence here; both are OUTBOUND, where
`max_frame_seconds` does not apply, so the tension is real but that support is not.
Two enumerations that contradicted each other ("four resource caps" in the
connector, "five keys" in the factory) are gone rather than reconciled, per
SDS-3.6, and the repeated explanation now sits once on each constant with the
other sites pointing at it (SDS-3.5). The ratio pin moves from `* 4` to `* 8`,
which is the eighth both the constant and the docs commit to; at `* 4` a per-host
default of 64 would have passed while falsifying all three statements.
KNOWN AND OUT OF SCOPE: `transports/tcp.py` carries the same read loop and the
same unbounded-frame defect, and the raw-TCP/X12/HTTP/DICOM intakes have no
frame-life bound. `docs/CONNECTIONS.md` says so in words. Lifting the three
helpers into a shared place would fix all four at once and is worth its own row;
`_MessagePacer` is the precedent for sharing this shape rather than forking it.
Items 1, 2 and 4 of BACKLOG #1725. Item 3 (a per-listener in-flight ingress
semaphore) is written as "Consider" and is deliberately left out of scope.
…two claims Review round 2 of two, the last. Everything confirmed here is applied; what was declined is named below with the reason. Behaviour: - A negative `max_connections_per_host` or `max_frame_seconds` is now refused at build. Negative is TRUTHY, so it survived the `if value` that reads 0 as "off". At -1 the per-host cap refused every peer including one holding no connections (`0 >= -1`), and because nothing was ever admitted nothing ever cleared `_host_capacity_warned` -- one attacker-chosen key per source address, the unbounded table this control is not allowed to contain. Caught at dry-run and `messagefoundry check` rather than at the first connection. - The TimeoutError arm now names the bound that ARMED the wait instead of re-measuring after it fired. Re-measuring loses at the boundary: with the two budgets close, the frame's remaining life reads as a small positive number and a peer that was never idle closes as `idle_timeout` -- the exact misdiagnosis the reason exists to prevent. - `stop()` clears the per-host tables. A client task cancelled past its grace, or one the runner ABANDONS when a stop overruns and then reuses the same instance at the next promotion, never runs its `finally`; a stale count would survive into the restarted listener and lock that address out permanently. `_active` keeps its existing behaviour, being the pre-existing global cap's counter. Two claims corrected, both mine: - `docs/SECURITY.md`'s ingest row said no per-peer bound exists and enumerated the caps that ship on. This change falsified both. It now distinguishes the per-peer MESSAGE-RATE bound (still absent, still for the NAT reason) from the per-peer CONNECTION bound this row adds, and the enumeration is prefixed "at least". CONNECTIONS.md and PHI.md were updated last round and this one was not, which is the restatement cost landing on its first edit. - `DEFAULT_MAX_FRAME_SECONDS` implied it bounds any slot-holding trickle. It does not: the clock starts at a START byte, and inter-frame noise is discarded without opening a frame, so a peer trickling non-MLLP bytes is neither idle nor in-frame. It buffers nothing and `max_connections_per_host` bounds how many slots one address holds that way, but the key does not cover it and now says so. Closing it needs a bound on connected time without a completed message, which is a different unit and a separate decision. Also: the top-of-loop deadline comment claimed the trickle case, which the TimeoutError arm actually handles; the duplicated final assertion in the pipelined test is gone; the pipelined test no longer leaves a dangling open frame racing the ACK drain, which could have fired `frame_deadline` and failed the test pointing at a defect that was not there; the NAT guidance is trimmed to one statement plus pointers (SDS-3.5); and the inventory docstring's count of pinned defaults is gone rather than corrected (SDS-3.6). DECLINED, with reasons: - `transports/tcp.py` carries the same read loop, the same trickle exposure, no per-host term, and the same cancellation window this row closed for MLLP. Out of scope by the brief, and `docs/CONNECTIONS.md` says in words that the raw-TCP, X12, HTTP and DICOM intakes have no frame-life bound. Lifting the helpers to a shared layer fixes all four at once and is worth its own row; `_MessagePacer` is the precedent for sharing this shape rather than forking it. - Both inbound-only caps are written into the settings dict for outbound MLLP too, where nothing reads them. That shape predates this row for `max_connections` and `receive_timeout`; closing it is a wiring refusal, which is a behaviour change wider than this row. - `_log_frame_deadline` stays unthrottled. Reaching it costs a connection held for `max_frame_seconds`, so the caps already bound it to about 32 lines per minute per address; the throttled path could be driven as fast as `connect()` returns. Same shape as the `frame_oversize` warning beside it. PROPOSED PR TITLE: fix(mllp): bound an open frame's life and one peer's connections (BACKLOG #1725) PROPOSED LEDGER BANNER: BUILT 2026-09-19 -- the MLLP listener now bounds an open frame start-byte to end-byte (`max_frame_seconds`, 60 s) and concurrent connections from one peer address (`max_connections_per_host`, 32, an eighth of the socket cap). Items 1, 2 and 4 only. ITEM 3 -- a per-listener in-flight ingress semaphore -- was written as "Consider" and is DELIBERATELY LEFT OUT OF SCOPE; it is a design question about making the aggregate handling peak a setting rather than a product of two others, and it wants its own row. `receive_timeout` is unchanged and still fires independently on an idle socket: it resets on every byte received, so it bounds silence and the new deadline bounds the frame, and both apply. The deadline restarts per frame, so a pipelined sender is unaffected. Two limits are recorded at the constants rather than left implied: the per-host cap keys on source address, so it raises the floor on a single-address peer and does not bound a distributed one, and behind a source-NAT proxy it becomes the effective listener capacity (set it to `None`/`0` there); and the frame deadline starts at a START byte, so a peer trickling non-MLLP noise is outside it. `transports/tcp.py` has the same defects and is untouched.
Collaborator
Author
|
SECURITY-RECORD REWORDING VERIFIED FOR ACCURACY, not merely for consistency. Read by the Lander at
Three things it does right that a merely-consistent edit would have got wrong:
It also adds
Not enqueued yet -- the merge queue is full at five. Nothing here blocks it. |
github-actions
Bot
added
the
ci-red
…dline together PR 1229 (BACKLOG #290, merged 21:22:47Z) named the pacer so its throttled WARNING can say which inbound is being held back. This branch (BACKLOG #1725) adds a per-frame deadline in the same MLLP read loop. One conflicted hunk, in MLLPSource._on_client where the pacer is built: - took main's _MessagePacer.for_rate(..., name=self._pacing_name); - kept this branch's frame_opened_at stamp directly after it. Both bounds stay. The loop still pushes frame_opened_at forward by the time pace() withheld, so pacing never spends the peer's frame budget. pace() now also tallies and logs through _note_paced before its sleep, which changes no timing. docs/PHI.md is byte-identical to this branch's head; docs/SECURITY.md auto-merged and keeps the per-peer MESSAGE-RATE narrowing and the max_connections_per_host NAT sentence.
Collaborator
Author
|
Merged origin/main (a087dea) in as c3705f2, a plain merge commit, no rebase or force-push. Resolution: one conflicted hunk, in Checks run before the commit:
Skipped: the full suite and every hosted-only leg. Read CI for those. |
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
1 participant
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.
Bounds the life of one MLLP frame and the number of concurrent connections from one peer.
Scope: the row's items 1, 2 and 4. Item 3 -- the per-listener in-flight ingress semaphore -- is written as "Consider" in the row and is deliberately out of scope here.
The defect
messagefoundry/transports/mllp.py,_on_client, appliedreceive_timeoutper read, so the timer reset on every byte received. A peer trickling one byte at a time held a frame open indefinitely while never being idle.A search of
messagefoundry/formax_connections_per_host,per_host,frame_deadlineandmax_frame_secondsreturned zero hits on every needle. Neither a per-connection frame deadline nor a per-host slot cap existed.What ships
max_frame_seconds(default 60.0) bounds one frame start-byte to end-byte.max_connections_per_host(default 32) bounds concurrent sockets from one peer address. Both reach the connector through theMLLP()factory, which is the schemaconnections.tomldecodes against, so there is no second configuration path.receive_timeoutis unchanged and still bounds an idle socket. The two bounds are independent and both are needed; the deadline sits beside the timeout rather than replacing it.The default of 32, and its limits
An eighth of the existing 256 socket cap. A per-host term set near the global one bounds nothing; at an eighth it takes at least eight distinct addresses to fill a listener, while staying far above a real partner's need -- MLLP is request/response over one socket per sending channel.
Two limits are recorded at the constant rather than left implied:
Noneor0as the setting for that topology, and the refusal names itself in both the log and the connection event so the diagnosis is a log line rather than an investigation.The idle bound still fires independently, and it was proved by mutation
test_an_idle_peer_still_hits_receive_timeout_while_a_frame_is_opensetsreceive_timeout=0.1againstmax_frame_seconds=5.0, opens a frame then goes silent, and asserts the close reason isidle_timeout. The pre-existingtest_idle_timeout_close_reasoncovers the same bound with no frame open.Confirmed in both directions: forcing the deadline always-spent reds that arm plus the slow-sender arm, while disabling the deadline reds only the trickle arm. Disjoint reds.
Review found a critical bug in the change itself
Round one, at xhigh, reproduced a real defect in the new code: the frame clock was stamped per connection rather than per frame, so a pipelined sender was reset every 60 seconds. Fixed, and pinned by an arm that holds the decoder continuously in-frame deterministically.
The builder's own first version of that test passed against the defect, and it was caught only by running the mutation. That is worth a reviewer's attention: the guard was written before it could fail.
Also fixed in round one: paced time being charged to the frame budget, which broke
_MessagePacer's "never drops" promise; a cancellation window that leaked a per-host slot permanently; and per-refusal log amplification.Round two found a negative-value hole (
-1is truthy, so it refused every peer while growing an attacker-keyed set), a boundary case mislabellingframe_deadlineasidle_timeout, and stale state survivingstop(). All applied.Three findings were declined with reasons in the commit message, chiefly that
messagefoundry/transports/tcp.pycarries the same defects and is outside this row. That is a real finding about tcp.py and it should be filed rather than lost here.Why the diff reaches eight files
tests/test_connection_event_emit.pyat_capacitywith amax_connections_per_hostreason, the deadline asclosedwith reasonframe_deadline.docs/CONNECTIONS.mdtests/test_communications_inventory.pymax_connectionsandreceive_timeoutalready were.docs/PHI.md,docs/SECURITY.md,tests/test_phi_logging_inventory.pyclosedreasons as "no path produces any other"; SECURITY.md said no per-peer bound exists and listed the caps that ship on.The last row is the one to check. Those are security records, and a reviewer should confirm the rewording is accurate rather than merely consistent.
One judgement made without asking
The per-host refusal reuses the existing
at_capacityevent kind with a distinguishingreason, rather than adding a new kind. A new kind would have forced an edit to the web console's filter tuple, which CI asserts equals the engine's emit sites -- widening the diff into a package another builder might be holding, for information thereasonfield already carries.Severity
No live exposure. MessageFoundry has zero deployments. Conditional only: a deploying site would have a slow-loris peer hold listener slots open indefinitely, and would have no per-peer bound at all since the source allowlist ships off and
max_connectionscounts sockets rather than hosts.Checks
Ran:
ruff check .,ruff format --check .,mypy messagefoundrystrict, all clean. Pytest over every module importingtransports.mllp, found by grep rather than guessed: 976 passed, 2 skipped. Plus the config, schema and doc-drift guards: 627 passed. The new test module was run three times to check for flakes.Not run, and these need reading on this pull request: the full pytest suite and every hosted-runner-only leg, by name
windows-service-smoke, the SQL Server and Postgres store suites, and the load legs.Ledger
docs/BACKLOG.mdis untouched. It is a stub in this repository; the ledger lives in the private repository. The claim on 1725 is still held by the build worktree and is released at landing, per step 14.This branch was interrupted three times by an antivirus process kill and a session archive, and was resumed from its own transcript each time. That cost wall-clock only; every commit here was re-verified against the tree afterwards.