feat(transports): give inbound message pacing a voice (BACKLOG #290) -- and the row is substantially stale - #1229
wshallwshall wants to merge 1 commit into
Conversation
…eiling can be tuned (BACKLOG #290) Message-rate pacing ships OFF by the 2026-08-11 ruling, on the ground that a safe number can only come from a site's own feed profile. That posture needs the operator to be able to watch a number engage, and pacing was silent by construction: it never drops, NAKs, refuses or errors, so a paced interface was indistinguishable from a slow one and nothing was written anywhere. _MessagePacer now tallies each APPLIED read delay and reports it at WARNING, at most once per 60s. Instrumented at the two places a wait is acted on -- pace() for the stream pair and deficit() for the listener pair -- never in charge(), which both route through and which deficit() consults on every read; counting there would tally one outstanding debt once per consult. Carries the connection name through from Source.name so a report can be traced to a feed. Metadata only: name, count, duration, configured rate. No default changes and nothing paces differently.
LANDER INSPECTION -- labelled self-review, not a peer reviewPosted under the korus Observability only, which is what makes it compatible with the ruling it citesNo That matters because the 2026-08-11 owner ruling quoted at The gap it closes is real: pacing never drops, NAKs or errors, so a paced interface was PHI-safe, checked rather than assumedThe log interpolates four things: a config-supplied connection name, an int, a float, and the One real gap: DICOM is the FIFTH consumer and is not plumbedEvery
Wrong twice on that path. It names no connection, which is the entire purpose of the change; and Not a blocker: DICOM pacing also ships OFF, so only an opted-in site sees it, and nothing is unsafe. VerdictVerdict: merge. The DICOM plumbing is a follow-up, recorded here so it is not lost. |
|
CI failed while this pull request was in the merge queue, so the queue ejected it. Its own head can still be green: the queue revalidates the merge, and the path gates that skip on a pull request run there. Read the run before retrying. https://github.com/MEFORORG/MessageFoundry/actions/runs/35166186908 |
BACKLOG #290 -- the row is substantially stale, and its central instruction is superseded
Read this section before the diff. The dispatch asked for a build; what the tree
says is that most of #290 is already built, and that the one thing the row asks for
most insistently is forbidden by a ruling that postdates it.
The per-connection rate ceiling is BUILT, and ships OFF by a dated owner ruling
The row's Why paragraph reads: "Closes 2.4.1 - 15.2.2 -- both UNBUILT: no rate
ceiling, backlog bound, or storage floor exists in any configuration."
That is false at HEAD for the rate-ceiling half.
_MessagePacerinmessagefoundry/transports/mllp.pyis a token-bucket per-connection message-rateceiling, shared verbatim by the raw-TCP, X12 and HTTP intakes (BACKLOG #1114) -- four
of the five
bind_hostfamilies the row names. The knob ismax_messages_per_second/message_burst, reachable from the code-first factories(
config/wiring.py) and fromconnections.toml.It is also already the exact mechanism the row specifies. The row says
"Pause-reads backpressure only ... an unread byte was never received"; the pacer's
docstring says "It never drops, never NAKs and never refuses, and that is the whole
design ... Pacing the READ is the only option that satisfies the invariant by
construction."
It ships OFF, and that is a deliberate, dated ruling, quoted from
DEFAULT_MAX_MESSAGES_PER_SECOND:The row is dated 2026-07-22. The ruling is 2026-08-11 -- three weeks later. So the
row's core instruction (ship the ceiling enforced default-on) is superseded, and
building it would flip a default an owner ruled must stay off.
tests/test_mllp_message_pacing.pyalready pins that default withtest_the_shipped_default_constant_is_still_off, so the flip would also red anexisting guard.
I did not flip it. That is a scope ruling, not a build choice.
The Gate's DICOM C-STORE question is already answered in the code
The Gate names the DICOM C-STORE case -- association/PDU level, or an adjudicated
scope-out -- as the one owner item inside the row.
messagefoundry/transports/dicom.py:117already scopes DICOM out, citing the same 2026-08-11 ruling. So that question appears
to be disposed rather than open. Worth confirming before the register is updated, but
nothing here needs a fresh ruling to proceed.
Why the verifier's spot-check missed all of this
The dispatch reported "no per-connection rate ceiling ... anywhere in
messagefoundry/config/settings.py". That reading is correct and the conclusion drawnfrom it is not: the ceiling is per-connection, so it lives in
config/wiring.pyand
transports/, never in the global[inbound]settings section. The instrumentanswered an adjacent question (SDS-3.8).
What is genuinely still unbuilt
transports/dicom.py:117under that same ruling.pending_depth/in_pipeline_depthqueries already exist instore/base.py, so the row's "new per-connection backlog query on all three backends" is partly there already.[retention].storage_bound=enforce+ low-disk refusal floormax_db_mbis advisory-only and defaults 0/off.What this PR builds
One limb: pacing had no voice. The control never drops, NAKs, refuses or errors,
so a paced interface looked to an operator exactly like a slow one and nothing was
written anywhere, at any level. Under a posture where the operator is required to
supply their own number, a number nobody can watch engage cannot be tuned.
_MessagePacernow tallies each APPLIED read delay and reports it at WARNING, at mostonce per 60 seconds per pacer. Instrumented at the two places a wait is actually acted
on --
pace()for the stream pair,deficit()for the listener pair -- anddeliberately not in
charge(), which both route through and whichdeficit()consults on every read; counting there would tally one outstanding debt once per
consult and report a number that is not a count of anything.
The connection name is carried through from
Source.name(its documented purpose) soa report names the feed an operator has to go and look at. Metadata only: name, count,
duration, configured rate. Never a frame, a peer address, or a byte of the paced body.
No default changes, and nothing paces differently. This is observability over an
existing, ruled, opt-in control.
Why a log line and not a new AlertSink signal
The row asks for a new ingest-side AlertSink signal.
AlertSinkis a closed Protocolwith two concrete implementers plus an
[alerts]settings surface and docs rows; anew signal is a vocabulary change. Given the finding above -- that the owner may well
choose to close #290 as superseded rather than extend it -- committing the vocabulary
seemed like the wrong order. The log line is the honest minimal form and is
independently correct. If the signal is still wanted, it layers cleanly on this.
No new AlertSink signal. No new
MESSAGE_EVENT_KINDSentry.docs/PHI.mduntouched.Invariants
No message can be accepted-then-dropped by this change, and the argument is
structural rather than empirical: this PR adds no refusal path at all. It adds a
tally and a throttled log line beside an existing
asyncio.sleep. The ACK-on-receiptand count-and-log seams in
pipeline/wiring_runner.pyare not touched -- the diffcontains no pipeline file.
The pre-existing pacer satisfies both invariants by construction, and this PR does not
change how it paces: the excess is never read off the socket, so it never becomes a
received message that count-and-log would oblige us to account for.
tests/test_mllp_message_pacing.py::test_pacing_never_drops_a_messagealready pinsthe zero-loss property and still passes.
Tests, and that they fail without the change
Six added to
tests/test_mllp_message_pacing.py.Control run: I neutered only
_note_paced's body with an earlyreturn, leavingthe constructor change in place, so the discriminator is the report itself and not a
missing keyword argument. Result: the three report tests fail on their assertions
(
assert 0 == 1on the throttle test), and the negative and positive controls stillpass. Restored afterwards; the branch carries the working version.
Included deliberately:
emitted unconditionally on every read would violate while still passing the others;
for_ratestill returnsNonewhen pacing is off, withoutwhich every pacing test in the file would pass with pacing silently on for everyone.
Checks
ruff format --check .-- pass (1308 files).ruff check .-- pass.mypy messagefoundry(strict) -- zero errors in every file this PR touches. 22errors remain in
auth/webauthn.py,transports/dicom.py,parsing/dicom/_deps.pyand
parsing/fhir/_deps.py, allimport-not-foundfor optional extras absent fromthis venv, none touched here.
pytestover a census enumerated by greppingtests/for the symbols this PRchanges (
_MessagePacer,for_rate,_pacing_settings,max_messages_per_second,and the four source classes) -- 23 modules, 534 passed, 5 skipped.
Hosted legs that must be read
This venv lacks the
x12,dicom,fhir,xml,webauthnandvaultextras, andthose modules remove themselves at COLLECTION time. So my local run does not cover
tests/test_x12_transport.py,tests/test_x12_rte.py,tests/test_x12_source_ip_allowlist.pyortests/test_dicom_association_intake_bound.pyeven though they are in the census above. X12 is a file this PR edits, so that leg
is load-bearing, not incidental. Please read the full hosted suite.
Collisions
#1729, stream_inflight budget) -- I stayed out of it. It editsconfig/settings.pyandpipeline/wiring_runner.py; this PR touches neitherfile. No overlap.
pipeline/wiring_runner.pyonly. No overlap.wiring_runner.py-- not entered.pipeline/dr_backup.py-- not touched.Files here:
transports/mllp.py,transports/tcp.py,transports/x12.py,transports/http_listener.py,tests/test_mllp_message_pacing.py. Nothing else.The BLOCKED-BY-#281 edge was stale, and stays retired
Confirmed before building, so nobody re-applies it. #281 reads Done in the vault
ledger -- all 17 cells verified Pass on
main2026-07-24, 36/37 CONFIRMED_PASS. Its15.1.3 section is present at HEAD in
docs/SECURITY.md(the resource-demandingfunctionality block, around :1915-1917). The edge was recorded while #281's banner
still read Open and nobody re-derived it after the restamp.
Open questions I could not resolve
central instruction is forbidden by a later ruling, and its one owner-gated question
is already disposed. What remains is three limbs (backlog depth bound, storage floor
enforce, AlertSink signal) that would be clearer as their own rows than as the
residue of this one. I could not make that call.
bound? Its reasoning -- that a guessed default on a clinical interface is worse
than the unbounded intake it guards -- is about rate. It may or may not extend to
refusing ingest on low disk, which is a different kind of bound with a different
failure mode. This needs an owner, and it is the gate on the two remaining build
limbs.
stays
partialon the shipped default "and the record says why". If the scorecardalready records that, backlog: file #1103, the connscale API port range is unverified past its base #290's premise of driving those cells to green may itself be
the stale part.
Not built, and named so the PR does not read as closing the row: the staged-backlog
depth bound,
[retention].storage_bound=enforceand the low-disk refusal floor, theAlertSink signal, the F-2 proxy directives, and DICOM C-STORE, which is out pending
the Gate's scope ruling (and appears already scoped out in code).