Skip to content

feat(transports): give inbound message pacing a voice (BACKLOG #290) -- and the row is substantially stale - #1229

Open
wshallwshall wants to merge 1 commit into
mainfrom
claude/b290-data-plane-bounds
Open

wshallwshall wants to merge 1 commit into
mainfrom
claude/b290-data-plane-bounds

Conversation

@wshallwshall

Copy link
Copy Markdown
Collaborator

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. _MessagePacer in
messagefoundry/transports/mllp.py is a token-bucket per-connection message-rate
ceiling, shared verbatim by the raw-TCP, X12 and HTTP intakes (BACKLOG #1114) -- four
of the five bind_host families the row names. The knob is
max_messages_per_second / message_burst, reachable from the code-first factories
(config/wiring.py) and from connections.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:

Message-rate pacing ships OFF, and that is a DELIBERATE DEVIATION from this
module's "key absent -> secure default" convention, ruled 2026-08-11 (ASVS 2.4.1 /
15.2.2). A rate limit on a clinical interface is only safe at a number derived from
a real feed profile, and this project has no site data to derive one from --
shipping a guessed default would throttle real traffic, which is a worse failure
than the unbounded intake it would be guarding. So the mechanism exists and an
operator opts in with their own number. The cell stays partial on the shipped
default and the record says why; that is the honest outcome, not a disappointing one.

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.py already pins that default with
test_the_shipped_default_constant_is_still_off, so the flip would also red an
existing 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:117
already 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 drawn
from it is not: the ceiling is per-connection, so it lives in config/wiring.py
and transports/, never in the global [inbound] settings section. The instrument
answered an adjacent question (SDS-3.8).

What is genuinely still unbuilt

Limb State at HEAD
Per-connection rate ceiling BUILT (MLLP, TCP, X12, HTTP). Ships OFF by the 2026-08-11 ruling.
DICOM C-STORE bound Scoped out at transports/dicom.py:117 under that same ruling.
Pacing observability Was unbuilt. This PR.
Store-global staged-backlog depth bound Unbuilt. pending_depth / in_pipeline_depth queries already exist in store/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 floor Unbuilt. max_db_mb is advisory-only and defaults 0/off.
New ingest-side AlertSink signal Unbuilt as an AlertSink signal. See note below.
F-2 runbook proxy rate-limiter directives Not attempted; coordinated through #293's proxy-fence ownership.

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.

_MessagePacer now tallies each APPLIED read delay and reports it at WARNING, at most
once 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 -- and
deliberately not in charge(), which both route through and which deficit()
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) so
a 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. AlertSink is a closed Protocol
with two concrete implementers plus an [alerts] settings surface and docs rows; a
new 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_KINDS entry. docs/PHI.md untouched.

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-receipt
and count-and-log seams in pipeline/wiring_runner.py are not touched -- the diff
contains 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_message already pins
the 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 early return, leaving
the 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 == 1 on the throttle test), and the negative and positive controls still
pass. Restored afterwards; the branch carries the working version.

Included deliberately:

  • a NEGATIVE control -- a pacer that is not engaging must say nothing, which a report
    emitted unconditionally on every read would violate while still passing the others;
  • a POSITIVE control -- for_rate still returns None when pacing is off, without
    which 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. 22
    errors remain in auth/webauthn.py, transports/dicom.py, parsing/dicom/_deps.py
    and parsing/fhir/_deps.py, all import-not-found for optional extras absent from
    this venv, none touched here.
  • pytest over a census enumerated by grepping tests/ for the symbols this PR
    changes (_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, webauthn and vault extras, and
those 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.py or tests/test_dicom_association_intake_bound.py
even 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

  • PR 1212 (#1729, stream_inflight budget) -- I stayed out of it. It edits
    config/settings.py and pipeline/wiring_runner.py; this PR touches neither
    file.
    No overlap.
  • PR 1178 and PR 1189 -- both pipeline/wiring_runner.py only. No overlap.
  • The #1659 egress region of 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 main 2026-07-24, 36/37 CONFIRMED_PASS. Its
15.1.3 section is present at HEAD in docs/SECURITY.md (the resource-demanding
functionality 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

  1. Should backlog: file #1103, the connscale API port range is unverified past its base #290 be closed as superseded? Its headline claim is false at HEAD, its
    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.
  2. Does the 2026-08-11 ruling also govern the storage floor and the backlog depth
    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.
  3. Is the ASVS 2.4.1 / 15.2.2 disposition already settled? The ruling says the cell
    stays partial on the shipped default "and the record says why". If the scorecard
    already 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=enforce and the low-disk refusal floor, the
AlertSink 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).

…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.
@wshallwshall

Copy link
Copy Markdown
Collaborator Author

LANDER INSPECTION -- labelled self-review, not a peer review

Posted under the korus LANDER.md line 364 obligation. I did not author this change.

Observability only, which is what makes it compatible with the ruling it cites

No raise, no NAK, no drop, no skipped read, no default moved. pace() gains one _note_paced(wait)
before the unchanged await asyncio.sleep(wait); deficit() computes owed, reports, and returns it
unchanged. The only early return suppresses a log line, never a message.

That matters because the 2026-08-11 owner ruling quoted at DEFAULT_MAX_MESSAGES_PER_SECOND says a
clinical-interface rate limit is only safe at a number from a real feed profile, which this project
does not have. A refusal path would have contradicted it. This adds none, and the shipped default
stays None.

The gap it closes is real: pacing never drops, NAKs or errors, so a paced interface was
indistinguishable from a slow one
and nothing was logged at any level.

PHI-safe, checked rather than assumed

The log interpolates four things: a config-supplied connection name, an int, a float, and the
configured rate. No frame, no peer address, no body, no patient field. self._name traces to
Source.name, and connection=config.name is established operator-facing practice across roughly ten
transports.

One real gap: DICOM is the FIFTH consumer and is not plumbed

Every _MessagePacer.for_rate call site on origin/main:

transports/dicom.py:220          <== NOT in this PR's file list
transports/http_listener.py:473
transports/mllp.py:1632
transports/tcp.py:571
transports/x12.py:595

dicom.py shares _MessagePacer and calls deficit() under its pacer lock. Since deficit() now
reports, an operator who opts DICOM association pacing in will get:

inbound message pacing engaged on <unnamed inbound>: 3 read delay(s) totalling 4.000s (max_messages_per_second=1)

Wrong twice on that path. It names no connection, which is the entire purpose of the change; and
it labels an association rate as max_messages_per_second and associations as "read delay(s)".
config.name is already available in that constructor and used elsewhere in the same file.

Not a blocker: DICOM pacing also ships OFF, so only an opted-in site sees it, and nothing is unsafe.
But the PR counts "four of the five families" in its own body, so the fifth was known and the report
was not followed through to it. One line of plumbing plus a wording decision -- parameterise the unit,
or scope DICOM out of the report explicitly.

Verdict

Verdict: merge. The DICOM plumbing is a follow-up, recorded here so it is not lost.

@wshallwshall
wshallwshall added this pull request to the merge queue Sep 17, 2026
@github-actions github-actions Bot added the ci-red A required check went red. Attribute it before retrying. label Sep 17, 2026
@github-actions

Copy link
Copy Markdown

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

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Sep 17, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci-red A required check went red. Attribute it before retrying.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant