From 69e8faef2e922e50b00e40a191ff38b3121215f4 Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 12 Oct 2026 06:21:51 -0500 Subject: [PATCH 1/3] transports: float caps and timeouts read by one strict reader (BACKLOG #3432) float_setting in transports/base.py refuses a bool, NaN, infinity, a non-number, and a value float() only rounds to zero ("1e-400"), naming the key and withholding the value. cap_setting's float path and poll_interval use it, and so do at least the timeout reads on MLLP, TCP, X12, DICOM, DATABASE, reference sync, HTTP reply, FHIR, REST, SOAP, DICOMweb, Email, Direct, REMOTEFILE, File, Timer, OAuth2, SMART. Range refusals in positive_cap, resolve_poll_ceiling, poll_interval, max_header_bytes, max_decompressed_members and the MLLP per-host and frame-seconds guards withhold the value too. Proposed PR title: a float cap or timeout refuses a bool, nan, inf or a value that only rounds to zero (BACKLOG #3432) Banner: BUILT -- engine branch fix-3432-float-caps, awaiting PR. --- changelog.d/3432.fixed.md | 24 ++ messagefoundry/config/wiring.py | 14 +- messagefoundry/pipeline/reference_sync.py | 21 +- messagefoundry/pipeline/wiring_runner.py | 3 +- messagefoundry/transports/base.py | 161 ++++++---- messagefoundry/transports/database.py | 19 +- messagefoundry/transports/dicom.py | 17 +- messagefoundry/transports/dicomweb.py | 5 +- messagefoundry/transports/direct.py | 5 +- messagefoundry/transports/email.py | 5 +- messagefoundry/transports/fhir.py | 11 +- messagefoundry/transports/file.py | 9 +- messagefoundry/transports/http_auth.py | 13 +- messagefoundry/transports/http_listener.py | 30 +- messagefoundry/transports/mllp.py | 26 +- messagefoundry/transports/remotefile.py | 13 +- messagefoundry/transports/rest.py | 5 +- messagefoundry/transports/smart.py | 14 +- messagefoundry/transports/soap.py | 5 +- messagefoundry/transports/tcp.py | 9 +- messagefoundry/transports/timer.py | 9 +- messagefoundry/transports/x12.py | 9 +- tests/test_cap_setting_spellings.py | 335 ++++++++++++++++++++- tests/test_connection_event_emit.py | 5 +- tests/test_dicom_scp_disposition.py | 2 +- tests/test_poll_seconds_validation.py | 9 +- tests/test_timer_source.py | 2 +- 27 files changed, 651 insertions(+), 129 deletions(-) create mode 100644 changelog.d/3432.fixed.md diff --git a/changelog.d/3432.fixed.md b/changelog.d/3432.fixed.md new file mode 100644 index 0000000000..2137869ed5 --- /dev/null +++ b/changelog.d/3432.fixed.md @@ -0,0 +1,24 @@ +- **A float cap or timeout refuses a bool, a NaN, an infinity, or text that only rounds to zero.** + Before this fix, a bare `float()` read `True` as one second and took `nan` and `inf`. A float cap + such as `receive_timeout`, `idle_timeout_seconds`, `max_frame_seconds` or a + `max_messages_per_second` rate read text such as `"1e-400"` as off, because `float()` rounds it to + `0.0`, so the guard it sets would not have run. Now one reader, `float_setting` in + `transports/base.py`, reads every float cap that `cap_setting` or `positive_cap` reads, and + `poll_seconds`. It also reads at least these former bare `float()` reads: `timeout_seconds` and + `connect_timeout` on the MLLP, TCP, X12 and DICOM connections, `timeout_seconds` on the FHIR, FHIR + lookup, REST, SOAP, DICOMweb, Email and Direct connections, `acquire_timeout` on the DATABASE + connections, the DATABASE lookup and a DATABASE reference source, the REMOTEFILE `connect_timeout`, + the File source's `min_age_seconds`, the Timer source's `interval_seconds`, the HTTP source's + `reply_timeout` and `reply_write_timeout`, and the OAuth2 and SMART token timeouts and expiry skews. + Each refusal names the key and withholds the value. A float cap is off only for an exact zero, such + as `0`, `"0"` or `"0.0"`; `"0e0"` is refused, as it already was on an integer cap. A + `connections.toml` literal `1e-400` still reads as zero, because the TOML parser turns it into + `0.0` before any reader sees it. The HTTP `reply_timeout` and `reply_write_timeout` now refuse + zero, as `docs/CONNECTIONS.md` already said; a numeric `0` used to take the 30-second default. + The `Http` factory's own check of those two keys refuses a bool, a NaN and an infinity too. The + other timeouts above are checked for type only: a zero or negative `timeout_seconds`, + `connect_timeout` or `acquire_timeout` is still not refused at build. + At least the range refusals in `positive_cap`, `resolve_poll_ceiling` and `poll_interval`, the + HTTP `max_header_bytes`, the File `max_decompressed_members` and the MLLP + `max_connections_per_host` and `max_frame_seconds` checks now withhold the value too. The reader's + docstring holds the full rule. (`BACKLOG #3432`) diff --git a/messagefoundry/config/wiring.py b/messagefoundry/config/wiring.py index 4dcd1a60b3..de4c9270b2 100644 --- a/messagefoundry/config/wiring.py +++ b/messagefoundry/config/wiring.py @@ -37,6 +37,7 @@ def handle(msg): import inspect import ipaddress import logging +import math import os import re import sys @@ -2494,10 +2495,17 @@ def _validate_sync_reply(settings: Mapping[str, Any]) -> None: for name in ("reply_timeout", "reply_write_timeout"): value = settings[name] - if not isinstance(value, int | float) or value <= 0: + # A bool is an int, and NaN and inf pass `<= 0`, so each is refused by name (BACKLOG #3432). + # The connector reads the same keys by the rule in transports/base.py float_setting. + if ( + isinstance(value, bool) + or not isinstance(value, int | float) + or not math.isfinite(value) + or value <= 0 + ): raise WiringError( - f"Http {name} must be a positive number of seconds — got {value!r}. It bounds a " - "blocked HTTP turn; an unbounded or zero budget is not a timeout" + f"Http {name} must be a finite positive number of seconds (value withheld). It " + "bounds a blocked HTTP turn; an unbounded or zero budget is not a timeout" ) if settings["reply_on_timeout"] not in ("504", "202"): diff --git a/messagefoundry/pipeline/reference_sync.py b/messagefoundry/pipeline/reference_sync.py index d4fc51ab18..4550e99b2c 100644 --- a/messagefoundry/pipeline/reference_sync.py +++ b/messagefoundry/pipeline/reference_sync.py @@ -53,7 +53,7 @@ from messagefoundry.pipeline.cluster import ClusterCoordinator, NullCoordinator from messagefoundry.store import Store from messagefoundry.store.base import DEFAULT_STORE_ACQUIRE_TIMEOUT, acquire_pooled -from messagefoundry.transports.base import count_setting +from messagefoundry.transports.base import count_setting, float_setting from messagefoundry.transports.egress import check_lookup_allowed __all__ = ["ReferenceSyncRunner", "ReferenceSyncError"] @@ -164,6 +164,18 @@ async def _load_database_source( dsn = database_source_dsn( settings, connection=None if name is None else reference_connection_name(name) ) + # BACKLOG #1052: bound the borrow. This pool is throwaway (closed in the finally below), but the + # acquire was unbounded, so an unresponsive server could hold the reference-sync runner's pass + # open indefinitely — and the runner is a single sequential loop over every declared set, so one + # wedged source would stall the sync of all the others. A StoreAcquireTimeout is an Exception, so + # the runner's existing per-set source-failure handler catches it, keeps the last-good snapshot + # and alerts, exactly as it does for any other source error. + # Read before the pool is made, so a refused value leaves no pool unclosed (BACKLOG #3432). + acquire_timeout = float_setting( + settings.get("acquire_timeout", DEFAULT_STORE_ACQUIRE_TIMEOUT), + knob="acquire_timeout", + transport="DATABASE reference source", + ) pool = await _make_pool( dsn, count_setting( @@ -172,13 +184,6 @@ async def _load_database_source( autocommit=True, login_timeout=_login_timeout(settings, "DATABASE reference source", dialect="sqlserver"), ) - # BACKLOG #1052: bound the borrow. This pool is throwaway (closed in the finally below), but the - # acquire was unbounded, so an unresponsive server could hold the reference-sync runner's pass - # open indefinitely — and the runner is a single sequential loop over every declared set, so one - # wedged source would stall the sync of all the others. A StoreAcquireTimeout is an Exception, so - # the runner's existing per-set source-failure handler catches it, keeps the last-good snapshot - # and alerts, exactly as it does for any other source error. - acquire_timeout = float(settings.get("acquire_timeout", DEFAULT_STORE_ACQUIRE_TIMEOUT)) try: conn = await acquire_pooled(pool, timeout=acquire_timeout, backend="reference:database") try: diff --git a/messagefoundry/pipeline/wiring_runner.py b/messagefoundry/pipeline/wiring_runner.py index d9d7a4523e..3056a8f0a7 100644 --- a/messagefoundry/pipeline/wiring_runner.py +++ b/messagefoundry/pipeline/wiring_runner.py @@ -218,6 +218,7 @@ ) from messagefoundry.transports.database import DatabaseLookupExecutor from messagefoundry.transports.fhir import FhirLookupExecutor +from messagefoundry.transports.http_listener import reply_seconds from messagefoundry.transports.mllp import build_ack __all__ = ["DrParkedError", "NotDeployedError", "RegistryRunner", "ShardLaneOwnershipError"] @@ -2071,7 +2072,7 @@ def _make_sync_reply_resolver(self, ic: InboundConnection) -> SyncReplyResolver self.store, self._reply_rendezvous, destination=str(settings["reply_from"]), - timeout=float(settings.get("reply_timeout") or 30.0), + timeout=reply_seconds(settings, "reply_timeout"), content_type=str(settings.get("reply_content_type") or "passthrough"), on_timeout=str(settings.get("reply_on_timeout") or "504"), metrics=metrics, diff --git a/messagefoundry/transports/base.py b/messagefoundry/transports/base.py index 1535fcb74d..e629241581 100644 --- a/messagefoundry/transports/base.py +++ b/messagefoundry/transports/base.py @@ -77,6 +77,8 @@ "DEFAULT_MAX_ITEMS_PER_POLL", "cap_setting", "count_setting", + "float_setting", + "positive_seconds", "positive_cap", "resolve_poll_ceiling", ] @@ -110,9 +112,10 @@ DEFAULT_MAX_ITEMS_PER_POLL = 500 -#: Text that spells zero in plain digits: ``"0"``, ``"00"``, ``"0.0"``, ``"-0.0"``, ``".0"``. An -#: ``int`` cap reads this as "off" without ``float()``, so text that ``float()`` merely rounds to -#: zero, such as ``"1e-400"``, reaches :func:`count_setting` and is refused there (BACKLOG #3431). +#: Text that spells zero in plain digits: ``"0"``, ``"00"``, ``"0.0"``, ``"-0.0"``, ``".0"``. A cap +#: reads this as "off" without ``float()``, so text that ``float()`` merely rounds to zero, such as +#: ``"1e-400"``, reaches :func:`count_setting` or :func:`float_setting` and is refused there +#: (BACKLOG #3431, #3432). _ZERO_TEXT = re.compile(r"[+-]?(?:0+(?:\.0*)?|\.0+)") @@ -142,8 +145,8 @@ def count_setting(value: Any, *, knob: str, transport: str) -> int: here already ``env()``-resolved, and a resolved value stays out of an error, as it does in ``_lookup_max_rows`` in ``transports/database.py`` (BACKLOG #1183). Every raise below sits outside the handler, so ``int()``'s own message, which quotes the value, is not left on ``__context__`` - (BACKLOG #1796). This covers the type refusals here and in :func:`cap_setting` only. At least the - range refusals in :func:`positive_cap` and :func:`resolve_poll_ceiling` still quote the value.""" + (BACKLOG #1796). The range refusals in :func:`positive_cap`, :func:`resolve_poll_ceiling` and + :func:`poll_interval` withhold it too (BACKLOG #3432).""" reason: str if isinstance(value, bool): reason = "a count is not a bool" @@ -171,6 +174,81 @@ def count_setting(value: Any, *, knob: str, transport: str) -> int: raise ValueError(f"{transport} {knob} is not a valid int value (value withheld): {reason}") +def float_setting(value: Any, *, knob: str, transport: str) -> float: + """Read one float setting, such as a timeout or a rate, as a finite number, or refuse it naming + the key (BACKLOG #3432). + + **This docstring holds the rule; the readers that use it point here.** They are at least every + ``float`` cap read through :func:`cap_setting`, :func:`poll_interval`, and the timeout reads that + call this directly. A bare ``float()`` turns ``True`` into 1.0, takes ``"nan"`` and ``"inf"``, + and rounds ``"1e-400"`` to ``0.0``. So a bool built a one-second timeout, a NaN built a timeout + that fires at once, and text that only rounds to zero could turn a guard off. So: + + - an ``int`` or a ``float`` passes, and so does another real number, such as a ``Decimal`` or a + ``Fraction``; + - a bool is refused, ``False`` as well as ``True``: a bool is not a number of seconds; + - a NaN or an infinity is refused, and so is a number too large for a float, such as ``1e400``; + - a value that ``float()`` only rounds to zero is refused, such as the text ``"1e-400"`` or + ``Decimal("1e-400")``. Text is zero only when it spells zero in plain digits, such as ``"0"``, + ``"0.0"`` or ``"-0"``, so ``"0e0"`` is refused too; + - other text is read by ``float()``, so ``" 2.5 "`` and ``"+2.5"`` pass; + - anything else is refused, whatever ``float()`` would have made of it. + + Zero and negatives come back as they are. What a zero means, and whether a negative is a typo, is + each caller's own question, answered with a message naming its own key. + + **A TOML literal is parsed before this rule sees it.** ``tomllib`` reads ``x = 1e-400`` as + ``0.0``, so in ``connections.toml`` that literal reads exactly as a written ``0.0`` does, which is + zero. That stays: the spelling is gone before any reader runs, and the alternative is a + ``parse_float`` hook on the whole file. Text from an uncast ``env()`` reference keeps its + spelling, so the rule above applies to it. + + The refusal names the transport, the key and the reason, and withholds the value, by the rule in + :func:`count_setting` (BACKLOG #1183, #1796).""" + reason: str + if isinstance(value, bool): + reason = "it is a bool" + elif isinstance(value, str | numbers.Number): + # numbers.Number promises no conversion to float; one without it (a complex) raises TypeError. + number: Any = value + try: + seconds = float(number) + except OverflowError: + reason = "it is too large for a float" + except (TypeError, ValueError): + reason = "it is not a real number" + else: + if not math.isfinite(seconds): + reason = "it is not a finite number" + elif seconds == 0 and not _spells_zero(value): + reason = "float() rounds it to zero, but it is not zero" + else: + return seconds + else: + reason = f"it is a {type(value).__name__}, not a number or text" + raise ValueError(f"{transport} {knob} is not a valid float value (value withheld): {reason}") + + +def positive_seconds(value: Any, *, knob: str, transport: str) -> float: + """:func:`float_setting`, then refuse a value that is not above zero, withholding it. + + For a number of seconds with no "off", where zero or a negative would make every wait end at + once: at least ``poll_seconds``, the HTTP reply timeouts and the Timer's ``interval_seconds``.""" + seconds = float_setting(value, knob=knob, transport=transport) + if seconds <= 0: + raise ValueError( + f"{transport} {knob} must be a number of seconds above zero (value withheld)" + ) + return seconds + + +def _spells_zero(value: Any) -> bool: + """Whether ``value`` is an exact zero: text in plain digits, or a number equal to zero.""" + if isinstance(value, str): + return _ZERO_TEXT.fullmatch(value.strip()) is not None + return bool(value == 0) + + def cap_setting[NumT: (int, float)]( value: Any, kind: type[NumT], *, knob: str, transport: str ) -> NumT | None: @@ -197,40 +275,27 @@ def cap_setting[NumT: (int, float)]( ``"0.0"`` or ``"-0.0"``, or when ``int()`` reads it as ``0``. Text is never tested with ``float()`` there, which would round ``"1e-400"`` to ``0.0`` and read it as off; that text goes to :func:`count_setting`, which refuses it, as it refuses ``"0e0"``. A number that equals zero, such - as ``0.0`` or ``-0.0``, is off too. On a ``float`` cap, any text ``float()`` reads as zero is off. - A ``float`` cap refuses a bool too, ``False`` included. + as ``0.0`` or ``-0.0``, is off too. + + **A ``float`` cap is read by :func:`float_setting`, and the same exact zeros are its only "off" + (BACKLOG #3432).** So ``"1e-400"`` and ``"0e0"`` are refused there rather than read as off, and a + bool, a NaN and an infinity are refused too. That function's docstring holds the rule, including + why a ``connections.toml`` literal ``1e-400`` still reads as zero. A negative value is returned as-is, for the callers that refuse one at build with a message naming their own key; :func:`positive_cap` is that caller for most sites. It is a different mistake with a different answer, and folding it into "off" here would swallow the typo. A value that is not a - number is refused here, naming the key and withholding the value, by the rule in - :func:`count_setting`. + number is refused here, naming the key and withholding the value. """ if value is None or value == "": return None + if isinstance(value, str) and _ZERO_TEXT.fullmatch(value.strip()): + return None if kind is int: - if isinstance(value, str) and _ZERO_TEXT.fullmatch(value.strip()): - return None count = count_setting(value, knob=knob, transport=transport) return None if count == 0 else count - if isinstance(value, str): - with contextlib.suppress(ValueError): - if float(value) == 0: # at least "0", " 0 ", "0.0" and "-0.0" - return None - if isinstance(value, bool): - # float(True) is 1.0, so a bool would build a one-second timeout or a rate of one per second. - raise ValueError( - f"{transport} {knob} is not a valid float value (value withheld): it is a bool" - ) - cap: NumT | None - try: - cap = kind(value) - except (TypeError, ValueError, OverflowError): - cap = None - if cap is None: - # Raised after the handler ends: float()'s own message quotes the value (BACKLOG #1796). - raise ValueError(f"{transport} {knob} is not a valid float value (value withheld)") - # A NaN is not zero, so it comes back for the caller's range check to refuse. + # kind is float here, so kind() converts nothing; it gives the result the NumT type. + cap = kind(float_setting(value, knob=knob, transport=transport)) return None if cap == 0 else cap @@ -244,20 +309,18 @@ def positive_cap[NumT: (int, float)]( ) -> NumT | None: """:func:`cap_setting`, then refuse at build whatever is left that is not above zero (BACKLOG #1872). - After zero has been read as "off", what reaches the check is a negative or a NaN. Each one is a - cap that refuses all traffic, or, for a timeout, closes every peer the moment it connects, and - nobody writes one on purpose. Refused here, the typo surfaces at wiring and in ``messagefoundry - check``, not as a connection that reports running and accepts nothing. Written ``not cap > 0`` - rather than ``cap <= 0`` so a NaN float cap is refused too: every comparison with NaN is false, so - ``cap_setting`` cannot read it as off either. An ``int`` cap is a count, so it is held to the rule - in :func:`count_setting` first. + After zero has been read as "off", what reaches the check is a negative. It is a cap that refuses + all traffic, or, for a timeout, closes every peer the moment it connects, and nobody writes one on + purpose. Refused here, the typo surfaces at wiring and in ``messagefoundry check``, not as a + connection that reports running and accepts nothing. :func:`cap_setting` has already refused a + NaN (BACKLOG #3432). The refusal withholds the value, by the rule in :func:`count_setting`. ``off_hint`` ends the not-above-zero refusal. The default tells the operator how to turn the cap off. A caller where ``None``/``0`` does NOT turn it off passes its own text, so the refusal does not point at an off switch that does not exist (the DICOM SCP's ``max_object_bytes``, BACKLOG #2103).""" cap = cap_setting(value, kind, knob=knob, transport=transport) - if cap is not None and not cap > 0: - raise ValueError(f"{transport} {knob}={value!r} must be above zero ({off_hint})") + if cap is not None and cap <= 0: + raise ValueError(f"{transport} {knob} must be above zero (value withheld; {off_hint})") return cap @@ -273,7 +336,7 @@ def resolve_poll_ceiling(value: Any, *, knob: str, transport: str) -> int | None Clamping it to 1 would instead silently ingest at a rate nobody asked for. The connector already raises a :class:`ValueError` for a bad ``after_read``, so a bad number surfaces the same way: at wiring / ``messagefoundry check``, before the connection ever starts. The count itself is read by - :func:`count_setting`.""" + :func:`count_setting`, and the refusal withholds the value by its rule.""" # ``value`` is typed Any rather than object because every call site reads it out of an untyped # settings mapping; object would need a `type: ignore` on this line, and a suppression a reader # has to decide whether to trust is worse than the honest Any. @@ -282,8 +345,8 @@ def resolve_poll_ceiling(value: Any, *, knob: str, transport: str) -> int | None return None if ceiling < 1: raise ValueError( - f"{transport} {knob}={value!r} must be a positive number of items per poll " - f"(or 0 for unlimited)" + f"{transport} {knob} must be a positive number of items per poll " + "(value withheld; or 0 for unlimited)" ) return ceiling @@ -300,18 +363,10 @@ def poll_interval(settings: Mapping[str, Any], *, default: float, transport: str ``interval_seconds``. Deliberately not :func:`positive_cap`: that helper reads ``0`` as "off", and an interval has no - off. Written ``not interval > 0`` so a NaN, for which every comparison is false, is refused by - the same test.""" - value = settings.get("poll_seconds", default) - try: - interval = float(value) - except (TypeError, ValueError, OverflowError) as exc: - raise ValueError(f"{transport} poll_seconds={value!r} is not a number of seconds") from exc - if not (math.isfinite(interval) and interval > 0): - raise ValueError( - f"{transport} poll_seconds={value!r} must be a finite number of seconds above zero" - ) - return interval + off. Read by :func:`positive_seconds` (BACKLOG #3432).""" + return positive_seconds( + settings.get("poll_seconds", default), knob="poll_seconds", transport=transport + ) # A source hands each inbound message (raw bytes, MLLP framing already stripped) to this diff --git a/messagefoundry/transports/database.py b/messagefoundry/transports/database.py index cd709bae06..92653285d7 100644 --- a/messagefoundry/transports/database.py +++ b/messagefoundry/transports/database.py @@ -88,6 +88,7 @@ NegativeAckError, SourceConnector, count_setting, + float_setting, intake_open, poll_interval, register_destination, @@ -1304,7 +1305,11 @@ def __init__(self, config: Destination) -> None: self._pool_max = count_setting( s.get("pool_max", 5), knob="pool_max", transport="DATABASE destination" ) - self._acquire_timeout = float(s.get("acquire_timeout", _DEFAULT_DB_ACQUIRE_TIMEOUT)) + self._acquire_timeout = float_setting( + s.get("acquire_timeout", _DEFAULT_DB_ACQUIRE_TIMEOUT), + knob="acquire_timeout", + transport="DATABASE destination", + ) self._pool: Any = None self._pool_lock = asyncio.Lock() # ADR 0013: capture the statement's result-set (its RETURNING/OUTPUT rows). Default False → @@ -1619,7 +1624,11 @@ def __init__(self, config: Source) -> None: self._pool_max = count_setting( s.get("pool_max", 5), knob="pool_max", transport="DATABASE source" ) - self._acquire_timeout = float(s.get("acquire_timeout", _DEFAULT_DB_ACQUIRE_TIMEOUT)) + self._acquire_timeout = float_setting( + s.get("acquire_timeout", _DEFAULT_DB_ACQUIRE_TIMEOUT), + knob="acquire_timeout", + transport="DATABASE source", + ) self._pool: Any = None self._pool_lock = asyncio.Lock() self._handler: InboundHandler | None = None @@ -2049,8 +2058,10 @@ def __init__( self._login_timeout[cname] = _login_timeout( s, f"DatabaseLookup {cname!r}", dialect="sqlserver" ) - self._acquire_timeout[cname] = float( - s.get("acquire_timeout", _DEFAULT_DB_ACQUIRE_TIMEOUT) + self._acquire_timeout[cname] = float_setting( + s.get("acquire_timeout", _DEFAULT_DB_ACQUIRE_TIMEOUT), + knob="acquire_timeout", + transport=f"DatabaseLookup {cname!r}", ) self._max_rows[cname] = _lookup_max_rows(s.get("max_rows"), cname) self._pools: dict[str, Any] = {} diff --git a/messagefoundry/transports/dicom.py b/messagefoundry/transports/dicom.py index ed44c5189c..5541bb2687 100644 --- a/messagefoundry/transports/dicom.py +++ b/messagefoundry/transports/dicom.py @@ -119,6 +119,7 @@ NegativeAckError, SourceConnector, count_setting, + float_setting, peer_ip_allowed, positive_cap, register_destination, @@ -572,7 +573,9 @@ def __init__(self, config: Source) -> None: self._max_pdu_size = count_setting( s.get("max_pdu_size", 16384), knob="max_pdu_size", transport="DICOM-SCP source" ) - self._timeout = float(s.get("timeout_seconds", 30.0)) + self._timeout = float_setting( + s.get("timeout_seconds", 30.0), knob="timeout_seconds", transport="DICOM-SCP source" + ) # Association-rate pacing (ASVS 2.4.1 / 15.2.2, BACKLOG #1114). The keys are NOT # max_messages_per_second/message_burst and are not read through mllp._pacing_settings, because # the UNIT here is an association, not a message — see _pace_association for why a message-rate @@ -1291,8 +1294,16 @@ def __init__(self, config: Destination) -> None: self._max_pdu_size = count_setting( s.get("max_pdu_size", 16384), knob="max_pdu_size", transport="DICOM-SCU destination" ) - self._timeout = float(s.get("timeout_seconds", 30.0)) - self._connect_timeout = float(s.get("connect_timeout", 10.0)) + self._timeout = float_setting( + s.get("timeout_seconds", 30.0), + knob="timeout_seconds", + transport="DICOM-SCU destination", + ) + self._connect_timeout = float_setting( + s.get("connect_timeout", 10.0), + knob="connect_timeout", + transport="DICOM-SCU destination", + ) # Build the client TLS context now so a bad cert/key fails at construction (check/dry-run), not # at the first delivery (like the SCP / MLLP / REST). #190 (ADR 0093): thread the instance [tls] # internal-CA trust-anchor policy so an internal hop that names no tls_ca_file of its own verifies diff --git a/messagefoundry/transports/dicomweb.py b/messagefoundry/transports/dicomweb.py index 24a8e7724c..d98b729d3b 100644 --- a/messagefoundry/transports/dicomweb.py +++ b/messagefoundry/transports/dicomweb.py @@ -52,6 +52,7 @@ DeliveryResponse, DestinationConnector, NegativeAckError, + float_setting, register_destination, ) from messagefoundry.transports.bounded_read import ( @@ -218,7 +219,9 @@ def __init__(self, config: Destination) -> None: # BACKLOG #1241: and the control-char screen alone is not enough for a value that becomes a # path SEGMENT — refuse at construction, where the operator sees it, not at the URL layer. _reject_non_uid(self.study_uid, "study_uid") - self.timeout: float = float(s.get("timeout_seconds", 30.0)) + self.timeout: float = float_setting( + s.get("timeout_seconds", 30.0), knob="timeout_seconds", transport="DICOMweb destination" + ) self.encoding: str = s.get("encoding", "utf-8") # ADR 0013: capture the STOW-RS dicom+json response. Default False → returns None, byte-identical. self.capture_response: bool = bool(s.get("capture_response", False)) diff --git a/messagefoundry/transports/direct.py b/messagefoundry/transports/direct.py index 124c6c4ffe..63bbb2b3eb 100644 --- a/messagefoundry/transports/direct.py +++ b/messagefoundry/transports/direct.py @@ -81,6 +81,7 @@ DestinationConnector, NegativeAckError, encode_wire_body, + float_setting, register_destination, ) from messagefoundry.transports.email import checked_envelope @@ -264,7 +265,9 @@ def __init__(self, config: Destination) -> None: tls_ca_file = s.get("tls_ca_file") self.tls_ca_file: str | None = str(tls_ca_file) if tls_ca_file else None self.tls_check_hostname = bool(s.get("tls_check_hostname", True)) - self.timeout: float = float(s.get("timeout_seconds", 30.0)) + self.timeout: float = float_setting( + s.get("timeout_seconds", 30.0), knob="timeout_seconds", transport="Direct destination" + ) self.encoding: str = str(s.get("encoding", "utf-8")) # S/MIME material — the whole point of Direct. Loaded and cross-checked at construction so a diff --git a/messagefoundry/transports/email.py b/messagefoundry/transports/email.py index b7153fb512..dd440e9767 100644 --- a/messagefoundry/transports/email.py +++ b/messagefoundry/transports/email.py @@ -80,6 +80,7 @@ DestinationConnector, NegativeAckError, encode_wire_body, + float_setting, register_destination, ) from messagefoundry.transports.mllp import InsecureHopGuard @@ -335,7 +336,9 @@ def __init__(self, config: Destination) -> None: tls_ca_file = s.get("tls_ca_file") self.tls_ca_file: str | None = str(tls_ca_file) if tls_ca_file else None self.tls_check_hostname = bool(s.get("tls_check_hostname", True)) - self.timeout: float = float(s.get("timeout_seconds", 30.0)) + self.timeout: float = float_setting( + s.get("timeout_seconds", 30.0), knob="timeout_seconds", transport="Email destination" + ) self.encoding: str = str(s.get("encoding", "utf-8")) # STARTTLS-by-default posture, mirroring RestDestination's verify_tls / diff --git a/messagefoundry/transports/fhir.py b/messagefoundry/transports/fhir.py index d4a6061bb0..b068864a4b 100644 --- a/messagefoundry/transports/fhir.py +++ b/messagefoundry/transports/fhir.py @@ -71,6 +71,7 @@ DestinationConnector, NegativeAckError, encode_wire_body, + float_setting, register_destination, ) from messagefoundry.transports.bounded_read import ( @@ -560,7 +561,9 @@ def __init__(self, config: Destination) -> None: "(ASVS 14.2.1); see docs/SECURITY-LOOSENING.md", config.name, ) - self.timeout: float = float(s.get("timeout_seconds", 30.0)) + self.timeout: float = float_setting( + s.get("timeout_seconds", 30.0), knob="timeout_seconds", transport="FHIR destination" + ) self.encoding: str = s.get("encoding", "utf-8") # ADR 0013: capture the FHIR server reply (assigned resource / ETag / OperationOutcome). Default # False → returns None, byte-identical. @@ -1309,7 +1312,11 @@ def __init__( ) refuse_url_credentials(url, f"FhirLookup {cname!r} 'url'") self._base[cname] = url - self._timeout[cname] = float(s.get("timeout_seconds", 30.0)) + self._timeout[cname] = float_setting( + s.get("timeout_seconds", 30.0), + knob="timeout_seconds", + transport=f"FhirLookup {cname!r}", + ) self._encoding[cname] = str(s.get("encoding", "utf-8")) # #200 (ADR 0092): the per-connection insecure-hop attestation keys the posture-keyed refusal. # Validated like the DB lookups': a flag written into this mutable dict without its diff --git a/messagefoundry/transports/file.py b/messagefoundry/transports/file.py index 726ee7092b..4589ef28bf 100644 --- a/messagefoundry/transports/file.py +++ b/messagefoundry/transports/file.py @@ -62,6 +62,7 @@ SourceConnector, SourceStartupError, encode_wire_body, + float_setting, intake_open, poll_interval, positive_cap, @@ -360,8 +361,8 @@ def _member_cap(value: object) -> int: ) if cap is None: # zero, in some spelling raise ValueError( - f"file source max_decompressed_members={value!r} must be above zero " - f"({_MEMBER_CAP_OFF_HINT})" + "file source max_decompressed_members must be above zero " + f"(value withheld; {_MEMBER_CAP_OFF_HINT})" ) return cap @@ -673,7 +674,9 @@ def __init__(self, config: Source) -> None: self._root_real = self.directory.resolve() self.pattern: str = s.get("pattern", "*") self.poll_seconds: float = poll_interval(s, default=1.0, transport="file source") - self.min_age_seconds: float = float(s.get("min_age_seconds", 0.0)) + self.min_age_seconds: float = float_setting( + s.get("min_age_seconds", 0.0), knob="min_age_seconds", transport="file source" + ) self.after_read: str = s.get("after_read", "move") # "move" | "delete" | "leave" (#142) if self.after_read not in ("move", "delete", "leave"): raise ValueError( diff --git a/messagefoundry/transports/http_auth.py b/messagefoundry/transports/http_auth.py index af8520c780..30258d5609 100644 --- a/messagefoundry/transports/http_auth.py +++ b/messagefoundry/transports/http_auth.py @@ -48,6 +48,7 @@ TrustAnchorPolicy, hop_name_prefix, ) +from messagefoundry.transports.base import float_setting from messagefoundry.transports.rest import ( HttpAuthError, ProxyConfig, @@ -265,8 +266,16 @@ def oauth2_cc_provider_from_settings( scope=(str(s["oauth2_scope"]) if s.get("oauth2_scope") else None), auth_style=str(s.get("oauth2_auth_style", "basic")), audience=(str(s["oauth2_audience"]) if s.get("oauth2_audience") else None), - expiry_skew_seconds=float(s.get("oauth2_expiry_skew_seconds", _DEFAULT_EXPIRY_SKEW)), - timeout_seconds=float(s.get("oauth2_timeout_seconds", _DEFAULT_TOKEN_TIMEOUT)), + expiry_skew_seconds=float_setting( + s.get("oauth2_expiry_skew_seconds", _DEFAULT_EXPIRY_SKEW), + knob="oauth2_expiry_skew_seconds", + transport="OAuth2", + ), + timeout_seconds=float_setting( + s.get("oauth2_timeout_seconds", _DEFAULT_TOKEN_TIMEOUT), + knob="oauth2_timeout_seconds", + transport="OAuth2", + ), # #200: the per-connection insecure-hop attestation keys the posture-keyed cleartext refusal in # __init__ (read from settings exactly as _dest_config / FhirLookup do). Default False → the hop # decides purely on posture. diff --git a/messagefoundry/transports/http_listener.py b/messagefoundry/transports/http_listener.py index 648765aebe..d767c580e8 100644 --- a/messagefoundry/transports/http_listener.py +++ b/messagefoundry/transports/http_listener.py @@ -56,6 +56,7 @@ ReplyOutcome, SourceConnector, positive_cap, + positive_seconds, register_source, wait_for_intake, ) @@ -75,6 +76,7 @@ "HttpRequest", "HttpRequestError", "HttpSource", + "reply_seconds", ] logger = logging.getLogger(__name__) @@ -627,11 +629,28 @@ def _header_cap(value: Any) -> int: ) if cap is None: # zero, in some spelling raise ValueError( - f"HTTP source max_header_bytes={value!r} must be above zero ({_HEADER_CAP_OFF_HINT})" + "HTTP source max_header_bytes must be above zero " + f"(value withheld; {_HEADER_CAP_OFF_HINT})" ) return cap +def reply_seconds(s: Mapping[str, Any], key: str) -> float: + """Read ``reply_timeout`` or ``reply_write_timeout``: seconds above zero, or 30 when unset. + + Unset, ``None`` and ``""`` take the 30-second default. Anything else is read by + :func:`~messagefoundry.transports.base.positive_seconds`, so it must be above zero, as + ``docs/CONNECTIONS.md`` says, because each bounds a blocked HTTP turn (BACKLOG #3432). The + ``float(s.get(key) or 30.0)`` this replaces read ``True`` as one second, took a NaN, and let + ``"0"`` through as a zero budget while a numeric ``0`` became the default. The runner reads + ``reply_timeout`` for its reply resolver through this function too, from the unresolved spec + settings; that agrees with this connector while the ``Http`` factory refuses a non-literal.""" + value = s.get(key) + if value is None or value == "": + return 30.0 + return positive_seconds(value, knob=key, transport="HTTP source") + + class HttpSource(SourceConnector): """Listen for inbound HTTP/1.1 requests, commit each POSTed body to the ingress stage via the pipeline handler, and return a ``202`` respond-with-receipt once it is durably committed (ADR 0023). @@ -729,11 +748,11 @@ def __init__(self, config: Source) -> None: # Synchronous captured-downstream reply (ADR 0154 D4). reply_from's presence is the mode # switch; absent, every path below is byte-identical to the shipped 202 slice (AC-8). self.reply_from: str | None = s.get("reply_from") or None - self.reply_timeout: float = float(s.get("reply_timeout") or 30.0) + self.reply_timeout: float = reply_seconds(s, "reply_timeout") self.reply_on_timeout: str = str(s.get("reply_on_timeout") or "504") self.reply_content_type: str = str(s.get("reply_content_type") or "passthrough") self.reply_on_empty: str = str(s.get("reply_on_empty") or "204") - self.reply_write_timeout: float = float(s.get("reply_write_timeout") or 30.0) + self.reply_write_timeout: float = reply_seconds(s, "reply_write_timeout") self._server: asyncio.Server | None = None self._handler: InboundHandler | None = None # vault BACKLOG #2606: the per-host cap SHIPS OFF here, unlike on the MLLP, raw-TCP and X12 @@ -1315,8 +1334,9 @@ def _drain_budget(self) -> float: defaults to 30 s against a 5 s shutdown grace, so ``stop()`` clamps this to its own sub-budget rather than letting one drain outlive the entire teardown by 6x. """ - if self.reply_from and self.reply_write_timeout: - return float(self.reply_write_timeout) + if self.reply_from: + # reply_seconds guarantees seconds above zero, so no falsy fallback is needed. + return self.reply_write_timeout return _CLIENT_SHUTDOWN_GRACE async def _write_safely(self, writer: asyncio.StreamWriter, data: bytes) -> None: diff --git a/messagefoundry/transports/mllp.py b/messagefoundry/transports/mllp.py index 72bcede590..737b28bb0f 100644 --- a/messagefoundry/transports/mllp.py +++ b/messagefoundry/transports/mllp.py @@ -87,6 +87,7 @@ InboundHandler, NegativeAckError, SourceConnector, + float_setting, intake_open, positive_cap, probe_tcp_reachable, @@ -750,8 +751,12 @@ def __init__(self, config: Destination) -> None: raise ValueError("MLLP destination requires a 'host' setting (the downstream peer)") self.host: str = host self.port: int = int(s["port"]) - self.timeout: float = float(s.get("timeout_seconds", 30.0)) - self.connect_timeout: float = float(s.get("connect_timeout", 10.0)) + self.timeout: float = float_setting( + s.get("timeout_seconds", 30.0), knob="timeout_seconds", transport="MLLP destination" + ) + self.connect_timeout: float = float_setting( + s.get("connect_timeout", 10.0), knob="connect_timeout", transport="MLLP destination" + ) self.encoding: str = s.get("encoding", "utf-8") # Per-outbound frame cap. This bounds ONLY the ACK-read decoder (the reply we read back); the # OUTGOING frame written by send() is deliberately UNCAPPED (frame() never truncates), so a @@ -1797,8 +1802,8 @@ def __init__(self, config: Source) -> None: # it: the unbounded table this control is not allowed to contain. Refuse at build, where # dry-run and `check` surface it, rather than at the first connection. raise ValueError( - "MLLP max_connections_per_host must be at least 1 (use None or 0 to disable the " - f"per-host cap), got {self.max_connections_per_host}" + "MLLP max_connections_per_host must be at least 1 (value withheld; use None or 0 to " + "disable the per-host cap)" ) self.receive_timeout: float | None = positive_cap( s.get("receive_timeout", DEFAULT_RECEIVE_TIMEOUT), @@ -1818,16 +1823,13 @@ def __init__(self, config: Source) -> None: knob="max_frame_seconds", transport="MLLP", ) - if self.max_frame_seconds is not None and not self.max_frame_seconds >= 0: - # Negative reaches here too, and would expire every frame the instant one opened — a + if self.max_frame_seconds is not None and self.max_frame_seconds < 0: + # Negative reaches here, and would expire every frame the instant one opened — a # listener that drops every sender, configured from a value the author meant as "off". - # Written `not >= 0` rather than `< 0` so NaN is refused as well: TOML can spell `nan`, - # `_cap_setting` cannot recognise it (`nan != 0`), and every comparison with it is false - # — so `min()` against the idle bound would silently drop the deadline and a bare - # `wait_for(..., nan)` would fire at once. + # `_cap_setting` has already refused a NaN (BACKLOG #3432). raise ValueError( - "MLLP max_frame_seconds must be a number of seconds, zero or more (use None or 0 to " - f"disable the frame deadline), got {self.max_frame_seconds}" + "MLLP max_frame_seconds must be a number of seconds, zero or more (value withheld; " + "use None or 0 to disable the frame deadline)" ) # BACKLOG #1725 act 3. Read through `positive_cap`, which also names the key when the value # is not a number; a negative one would let no frame be handled at all. diff --git a/messagefoundry/transports/remotefile.py b/messagefoundry/transports/remotefile.py index 6d8b7b1a8a..8bc5718bcf 100644 --- a/messagefoundry/transports/remotefile.py +++ b/messagefoundry/transports/remotefile.py @@ -114,6 +114,7 @@ SourceConnector, SourceStartupError, encode_wire_body, + float_setting, intake_open, poll_interval, positive_cap, @@ -935,7 +936,11 @@ def __init__( self._user = settings.get("username") self._password = settings.get("password") self._tls = tls - self._timeout = float(settings.get("connect_timeout", 30.0)) + self._timeout = float_setting( + settings.get("connect_timeout", 30.0), + knob="connect_timeout", + transport="REMOTEFILE ftp", + ) # Build the verifying TLS context once, fail-fast (build_check) — mirrors the SFTP host-key # posture: a verify-disabled ftps without the escape is refused here, not silently insecure. # #190 (ADR 0093): thread the instance [tls] internal-CA trust-anchor policy (verify path only). @@ -1604,7 +1609,11 @@ def __init__(self, settings: dict[str, Any]) -> None: # Read only to refuse it: a key_password never reaches paramiko (BACKLOG #2423). _refuse_sftp_key_wrap(self._private_key, settings.get("key_password")) self._known_hosts = settings.get("known_hosts") - self._timeout = float(settings.get("connect_timeout", 30.0)) + self._timeout = float_setting( + settings.get("connect_timeout", 30.0), + knob="connect_timeout", + transport="REMOTEFILE sftp", + ) # Fail fast at construction (build_check time): an unknown-host-key posture without the escape # must never silently weaken to auto-accept. #329: read the escape through the ADR-0092 clamp # (weakened_tls_escape_permitted_here consults the active construction posture, exactly like the diff --git a/messagefoundry/transports/rest.py b/messagefoundry/transports/rest.py index 910e07baf5..d96234da8b 100644 --- a/messagefoundry/transports/rest.py +++ b/messagefoundry/transports/rest.py @@ -76,6 +76,7 @@ DestinationConnector, NegativeAckError, encode_wire_body, + float_setting, register_destination, ) from messagefoundry.transports.bounded_read import ( @@ -1933,7 +1934,9 @@ def __init__(self, config: Destination) -> None: # BACKLOG #2060: names this hop in a bounded-read refusal, which never carries the URL. self._hop = hop_identity("REST", config.name) self.method: str = str(s.get("method", "POST")).upper() - self.timeout: float = float(s.get("timeout_seconds", 30.0)) + self.timeout: float = float_setting( + s.get("timeout_seconds", 30.0), knob="timeout_seconds", transport="REST destination" + ) self.encoding: str = s.get("encoding", "utf-8") # ADR 0013: capture the HTTP response body. Default False → returns None, byte-identical. A 2xx # with a body → outcome='accepted'; a 2xx with an empty body → outcome='no_reply' (a successful diff --git a/messagefoundry/transports/smart.py b/messagefoundry/transports/smart.py index 37531faeb9..4b11377f3a 100644 --- a/messagefoundry/transports/smart.py +++ b/messagefoundry/transports/smart.py @@ -60,7 +60,7 @@ hop_name_prefix, ) from messagefoundry.redaction import json_loads_or_refusal -from messagefoundry.transports.base import DeliveryError +from messagefoundry.transports.base import DeliveryError, float_setting from messagefoundry.transports.bounded_read import ( MAX_TOKEN_RESPONSE_BYTES, EgressReplyError, @@ -689,8 +689,16 @@ def token_provider_from_settings( private_key_password=( str(s["smart_private_key_password"]) if s.get("smart_private_key_password") else None ), - expiry_skew_seconds=float(s.get("smart_expiry_skew_seconds", _DEFAULT_EXPIRY_SKEW)), - timeout_seconds=float(s.get("smart_timeout_seconds", _DEFAULT_TOKEN_TIMEOUT)), + expiry_skew_seconds=float_setting( + s.get("smart_expiry_skew_seconds", _DEFAULT_EXPIRY_SKEW), + knob="smart_expiry_skew_seconds", + transport="SMART", + ), + timeout_seconds=float_setting( + s.get("smart_timeout_seconds", _DEFAULT_TOKEN_TIMEOUT), + knob="smart_timeout_seconds", + transport="SMART", + ), # #200: the per-connection insecure-hop attestation keys the posture-keyed cleartext refusal in # __init__ (read from settings exactly as _dest_config / the OAuth2 provider do). attested=attested, diff --git a/messagefoundry/transports/soap.py b/messagefoundry/transports/soap.py index 9d5a9ae402..7c24f667a0 100644 --- a/messagefoundry/transports/soap.py +++ b/messagefoundry/transports/soap.py @@ -80,6 +80,7 @@ NegativeAckError, count_setting, encode_wire_body, + float_setting, register_destination, ) from messagefoundry.transports.bounded_read import ( @@ -331,7 +332,9 @@ def __init__(self, config: Destination) -> None: self.url = url # BACKLOG #2060: names this hop in a bounded-read refusal, which never carries the URL. self._hop = hop_identity("SOAP", config.name) - self.timeout: float = float(s.get("timeout_seconds", 30.0)) + self.timeout: float = float_setting( + s.get("timeout_seconds", 30.0), knob="timeout_seconds", transport="SOAP destination" + ) self.encoding: str = s.get("encoding", "utf-8") self.version: str = str(s.get("soap_version", "1.1")) if self.version not in ("1.1", "1.2"): diff --git a/messagefoundry/transports/tcp.py b/messagefoundry/transports/tcp.py index 12b5c9dea6..f77d439bb7 100644 --- a/messagefoundry/transports/tcp.py +++ b/messagefoundry/transports/tcp.py @@ -38,6 +38,7 @@ DestinationConnector, InboundHandler, SourceConnector, + float_setting, positive_cap, probe_tcp_reachable, register_destination, @@ -152,8 +153,12 @@ def __init__(self, config: Destination) -> None: self.host: str = host self.port: int = int(s["port"]) self.codec = _codec_from_settings(s) - self.timeout: float = float(s.get("timeout_seconds", 30.0)) - self.connect_timeout: float = float(s.get("connect_timeout", 10.0)) + self.timeout: float = float_setting( + s.get("timeout_seconds", 30.0), knob="timeout_seconds", transport="TCP destination" + ) + self.connect_timeout: float = float_setting( + s.get("connect_timeout", 10.0), knob="connect_timeout", transport="TCP destination" + ) self.encoding: str = s.get("encoding", "utf-8") self.expect_reply: bool = bool(s.get("expect_reply", False)) self.max_frame_bytes: int | None = positive_cap( diff --git a/messagefoundry/transports/timer.py b/messagefoundry/transports/timer.py index f9b00e9894..435aa96cf2 100644 --- a/messagefoundry/transports/timer.py +++ b/messagefoundry/transports/timer.py @@ -29,6 +29,7 @@ InboundHandler, SourceConnector, intake_open, + positive_seconds, register_source, ) @@ -196,9 +197,11 @@ def __init__(self, config: Source) -> None: self._body_bytes = self.body.encode(self.encoding) self.run_once = bool(s.get("run_once", False)) iv = s.get("interval_seconds") - self.interval_seconds: float | None = float(iv) if iv is not None else None - if self.interval_seconds is not None and self.interval_seconds <= 0: - raise ValueError("timer source 'interval_seconds' must be > 0") + self.interval_seconds: float | None = ( + None + if iv is None + else positive_seconds(iv, knob="interval_seconds", transport="timer source") + ) # Cron / calendar schedule (ADR 0011 amendment, BACKLOG #160). Exclusive with interval/run_once; # DST is driven by the optional `timezone` (an IANA name → aware wall clock) or, by default, the # system-local wall clock (the OS handles local DST, no tzdata needed). diff --git a/messagefoundry/transports/x12.py b/messagefoundry/transports/x12.py index 687b48c13d..b3140747d2 100644 --- a/messagefoundry/transports/x12.py +++ b/messagefoundry/transports/x12.py @@ -49,6 +49,7 @@ NegativeAckError, SourceConnector, encode_wire_body, + float_setting, positive_cap, probe_tcp_reachable, register_destination, @@ -167,8 +168,12 @@ def __init__(self, config: Destination) -> None: self.host: str = host self.port: int = int(s["port"]) self.encoding: str = str(s.get("encoding", "utf-8")) - self.timeout: float = float(s.get("timeout_seconds", 30.0)) - self.connect_timeout: float = float(s.get("connect_timeout", 10.0)) + self.timeout: float = float_setting( + s.get("timeout_seconds", 30.0), knob="timeout_seconds", transport="X12 destination" + ) + self.connect_timeout: float = float_setting( + s.get("connect_timeout", 10.0), knob="connect_timeout", transport="X12 destination" + ) self.expect_reply: bool = bool(s.get("expect_reply", False)) # ADR 0016: capture the returned interchange (the 271/TA1) as a reply; ta1_required makes a # no-reply a retry. capture_response is forced True by reingress_to at wiring time (ADR 0013). diff --git a/tests/test_cap_setting_spellings.py b/tests/test_cap_setting_spellings.py index 5d58457ba7..ee54e61191 100644 --- a/tests/test_cap_setting_spellings.py +++ b/tests/test_cap_setting_spellings.py @@ -18,6 +18,7 @@ from __future__ import annotations import math +import operator from collections.abc import Callable from decimal import Decimal from fractions import Fraction @@ -34,21 +35,37 @@ ConnectionSpec, Database, DatabasePoll, + Ftp, Http, Sftp, Soap, + WiringError, env, resolve_env_settings, ) from messagefoundry.transports import build_destination, build_source, database, mllp -from messagefoundry.transports.base import cap_setting, positive_cap, resolve_poll_ceiling +from messagefoundry.transports.base import ( + cap_setting, + float_setting, + poll_interval, + positive_cap, + resolve_poll_ceiling, +) from messagefoundry.transports.database import DatabaseLookupExecutor from messagefoundry.transports.dicom import DicomScpSource, DicomScuDestination +from messagefoundry.transports.dicomweb import DicomWebDestination +from messagefoundry.transports.direct import DirectDestination +from messagefoundry.transports.email import EmailDestination +from messagefoundry.transports.fhir import FhirDestination, FhirLookupExecutor from messagefoundry.transports.file import FileSource +from messagefoundry.transports.http_auth import oauth2_cc_provider_from_settings from messagefoundry.transports.http_listener import DEFAULT_MAX_HEADER_BYTES, HttpSource from messagefoundry.transports.mllp import MLLPDestination, MLLPSource from messagefoundry.transports.remotefile import RemoteFileSource +from messagefoundry.transports.rest import RestDestination +from messagefoundry.transports.smart import token_provider_from_settings from messagefoundry.transports.tcp import TcpDestination, TcpSource +from messagefoundry.transports.timer import TimerSource from messagefoundry.transports.x12 import X12Destination, X12Source Build = Callable[[dict[str, Any]], Any] @@ -212,7 +229,7 @@ def test_a_string_zero_burst_defaults_to_the_rate_like_a_number_zero( assert getattr(string_zero, burst_attr) == 100.0 assert getattr(build({rate_key: 100, key: "7"}), burst_attr) == 7.0 # the control # The refusal names the connector too, so a listener that stops passing its own name shows up. - with pytest.raises(ValueError, match=f"{transport} {key}="): + with pytest.raises(ValueError, match=rf"{transport} {key} must be above zero \(value withheld"): build({rate_key: 100, key: -1}) @@ -327,13 +344,16 @@ def test_positive_cap_refuses_what_is_left_after_zero_reads_as_off() -> None: for bad in (0.5, math.nan, "1e3", "lots", [1], math.inf): with pytest.raises(ValueError, match=r"^t k is not a valid int value \(value withheld\)"): positive_cap(bad, int, knob="k", transport="t") - # A whole negative is a range refusal, which still quotes the value. + # A whole negative is a range refusal, which withholds the value too (BACKLOG #3432). for negative in (-1, "-1"): - with pytest.raises(ValueError, match="t k="): + with pytest.raises(ValueError, match=r"^t k must be above zero \(value withheld"): positive_cap(negative, int, knob="k", transport="t") - for bad_seconds in (-0.5, "-0.5", math.nan): - with pytest.raises(ValueError, match="t k="): + for bad_seconds in (-0.5, "-0.5"): + with pytest.raises(ValueError, match=r"^t k must be above zero \(value withheld"): positive_cap(bad_seconds, float, knob="k", transport="t") + # A NaN is not a number of seconds at all, so it is refused before the range check. + with pytest.raises(ValueError, match=r"^t k is not a valid float value \(value withheld\)"): + positive_cap(math.nan, float, knob="k", transport="t") # === an integer cap is a whole count (BACKLOG #3431) ============================================== @@ -595,8 +615,9 @@ def test_an_integer_cap_reads_only_an_exact_zero_as_off() -> None: for not_zero in ("1e-400", "0e0", "0x0", "zero"): with pytest.raises(ValueError, match=r"^t k is not a valid int value"): cap_setting(not_zero, int, knob="k", transport="t") - # A float cap is not a count, so text float() reads as zero is still "off" there. - assert cap_setting("0e0", float, knob="k", transport="t") is None + # A float cap takes the same exact zeros as "off", and refuses the same exponent text (#3432). + with pytest.raises(ValueError, match=r"^t k is not a valid float value"): + cap_setting("0e0", float, knob="k", transport="t") @pytest.mark.parametrize("kind", [int, float]) @@ -614,3 +635,301 @@ def test_a_refusal_withholds_the_value(kind: type[int] | type[float]) -> None: assert sentinel not in str(caught.value) assert caught.value.__cause__ is None assert caught.value.__context__ is None + + +# === a float cap or timeout is a finite number (BACKLOG #3432) ===================================== + + +def _timer(over: dict[str, Any]) -> TimerSource: + return TimerSource(Source(type=ConnectorType.TIMER, settings={"body": "x", **over})) + + +def _ftp(over: dict[str, Any]) -> RemoteFileSource: + settings = {**Ftp(host="ftp.example.com", remote_dir="/in").settings, **over} + src = build_source( + Source(type=ConnectorType.REMOTEFILE, settings=settings), + egress=EgressSettings(deny_by_default=False), + ) + assert isinstance(src, RemoteFileSource) + return src + + +def _url_dest(cls: type[Any], ctype: ConnectorType, url: str) -> Build: + return lambda over: cls(Destination(name="OB_TEST", type=ctype, settings={"url": url, **over})) + + +_EMAIL_BASE = { + "host": "smtp.example.com", + "sender": "a@example.com", + "recipients": ["b@example.com"], +} + + +def _email(over: dict[str, Any]) -> EmailDestination: + return EmailDestination( + Destination(name="OB", type=ConnectorType.EMAIL, settings={**_EMAIL_BASE, **over}) + ) + + +def _fhir_lookup(over: dict[str, Any]) -> SimpleNamespace: + executor = FhirLookupExecutor( + {"epic": {"url": "https://fhir.example.com/r4", **over}}, + egress=EgressSettings(deny_by_default=False), + ) + return SimpleNamespace(timeout_seconds=executor._timeout["epic"]) + + +def _db_lookup(over: dict[str, Any]) -> SimpleNamespace: + executor = DatabaseLookupExecutor( + {"clarity": {"server": "db.example", "database": "d", **over}}, + egress=EgressSettings(deny_by_default=False), + ) + return SimpleNamespace(acquire_timeout=executor._acquire_timeout["clarity"]) + + +def _oauth2(over: dict[str, Any]) -> Any: + return oauth2_cc_provider_from_settings( + { + "oauth2_token_url": "https://auth.example.com/token", + "oauth2_client_id": "c", + "oauth2_client_secret": "s", + **over, + } + ) + + +#: Float caps where ``None``/``0`` turns the cap off: (builder, key, attribute). At least one per +#: connector that reads one. +_FLOAT_OFF_BY_ZERO = [ + pytest.param(_MLLP_IN, "receive_timeout", "receive_timeout", id="mllp-in-receive_timeout"), + pytest.param(_MLLP_IN, "max_frame_seconds", "max_frame_seconds", id="mllp-in-frame_seconds"), + pytest.param(_MLLP_IN, _RATE, _RATE, id="mllp-rate"), + pytest.param(_MLLP_OUT, "idle_timeout_seconds", "idle_timeout_seconds", id="mllp-out-idle"), + pytest.param(_TCP_IN, "receive_timeout", "receive_timeout", id="tcp-in-receive_timeout"), + pytest.param(_TCP_IN, "max_frame_seconds", "max_frame_seconds", id="tcp-in-frame_seconds"), + pytest.param(_TCP_OUT, "idle_timeout_seconds", "idle_timeout_seconds", id="tcp-out-idle"), + pytest.param(_X12_IN, "receive_timeout", "receive_timeout", id="x12-in-receive_timeout"), + pytest.param(_X12_IN, "max_frame_seconds", "max_frame_seconds", id="x12-in-frame_seconds"), + pytest.param( + _X12_OUT, "max_connection_age_seconds", "max_connection_age_seconds", id="x12-out-age" + ), + pytest.param(_HTTP_IN, "receive_timeout", "receive_timeout", id="http-in-receive_timeout"), + pytest.param(_HTTP_IN, _RATE, _RATE, id="http-rate"), + pytest.param( + _SCP, + "max_associations_per_second", + "max_associations_per_second", + id="scp-association-rate", + ), +] + +#: Timeouts read by ``float_setting`` alone, where zero is not "off": (builder, key, attribute). +#: Each was a bare ``float()`` before #3432. +_TIMEOUTS = [ + pytest.param(_MLLP_OUT, "timeout_seconds", "timeout", id="mllp-out-timeout"), + pytest.param(_MLLP_OUT, "connect_timeout", "connect_timeout", id="mllp-out-connect"), + pytest.param(_TCP_OUT, "timeout_seconds", "timeout", id="tcp-out-timeout"), + pytest.param(_TCP_OUT, "connect_timeout", "connect_timeout", id="tcp-out-connect"), + pytest.param(_X12_OUT, "timeout_seconds", "timeout", id="x12-out-timeout"), + pytest.param(_X12_OUT, "connect_timeout", "connect_timeout", id="x12-out-connect"), + pytest.param(_SCP, "timeout_seconds", "_timeout", id="scp-timeout"), + pytest.param(_SCU, "timeout_seconds", "_timeout", id="scu-timeout"), + pytest.param(_SCU, "connect_timeout", "_connect_timeout", id="scu-connect"), + pytest.param(_database_out, "acquire_timeout", "_acquire_timeout", id="database-out-acquire"), + pytest.param(_database, "acquire_timeout", "_acquire_timeout", id="database-in-acquire"), + pytest.param(_db_lookup, "acquire_timeout", "acquire_timeout", id="database-lookup-acquire"), + pytest.param( + _url_dest(FhirDestination, ConnectorType.FHIR, "https://fhir.example.com/r4"), + "timeout_seconds", + "timeout", + id="fhir-timeout", + ), + pytest.param(_fhir_lookup, "timeout_seconds", "timeout_seconds", id="fhir-lookup-timeout"), + pytest.param( + _url_dest(RestDestination, ConnectorType.REST, "https://api.example.com/x"), + "timeout_seconds", + "timeout", + id="rest-timeout", + ), + pytest.param(_soap, "timeout_seconds", "timeout", id="soap-timeout"), + pytest.param( + _url_dest(DicomWebDestination, ConnectorType.DICOMWEB, "https://pacs.example.com/studies"), + "timeout_seconds", + "timeout", + id="dicomweb-timeout", + ), + pytest.param(_email, "timeout_seconds", "timeout", id="email-timeout"), + pytest.param(_remote, "connect_timeout", "_client._timeout", id="sftp-connect"), + pytest.param(_ftp, "connect_timeout", "_client._timeout", id="ftp-connect"), + pytest.param(_file, "min_age_seconds", "min_age_seconds", id="file-min_age"), + pytest.param(_timer, "interval_seconds", "interval_seconds", id="timer-interval"), + pytest.param(_HTTP_IN, "reply_timeout", "reply_timeout", id="http-reply_timeout"), + pytest.param(_HTTP_IN, "reply_write_timeout", "reply_write_timeout", id="http-reply_write"), + pytest.param(_oauth2, "oauth2_timeout_seconds", "timeout_seconds", id="oauth2-timeout"), +] + +#: Each is refused by every float setting. ``"1e-400"`` is text ``float()`` only rounds to zero; +#: ``[2.5]`` is a type ``float()`` refuses anyway, refused here naming the key. +_NOT_SECONDS = [True, False, math.nan, math.inf, -math.inf, "nan", "inf", "1e400", "1e-400", [2.5]] + + +def _refusal(key: str) -> str: + return rf"{key} is not a valid float value \(value withheld\)" + + +@pytest.mark.parametrize(("build", "key", "attr"), _FLOAT_OFF_BY_ZERO + _TIMEOUTS) +@pytest.mark.parametrize("bad", _NOT_SECONDS) +def test_a_float_setting_refuses_a_bool_a_nan_an_infinity_or_a_rounded_zero( + build: Build, key: str, attr: str, bad: object +) -> None: + """``float(True)`` is 1.0, ``float("nan")`` is a timeout that fires at once, and + ``float("1e-400")`` is ``0.0``, which turned a float cap off. Each is refused, naming the key. + + Red mutation: put a bare ``float()`` back on any ``_TIMEOUTS`` site, or test a float cap's text + with ``float(value) == 0`` again. That site's cases build cleanly.""" + del attr + with pytest.raises(ValueError, match=_refusal(key)): + build({key: bad}) + + +@pytest.mark.parametrize(("build", "key", "attr"), _FLOAT_OFF_BY_ZERO + _TIMEOUTS) +def test_a_float_setting_accepts_a_number_or_its_text(build: Build, key: str, attr: str) -> None: + """The control for the refusal above: a number, its text from an uncast ``env()``, and a + ``Decimal`` all read as the number.""" + for good in (2.5, "2.5", " 2.5 ", Decimal("2.5")): + assert operator.attrgetter(attr)(build({key: good})) == 2.5, repr(good) + + +@pytest.mark.parametrize(("build", "key", "attr"), _FLOAT_OFF_BY_ZERO) +def test_a_float_cap_reads_only_an_exact_zero_as_off(build: Build, key: str, attr: str) -> None: + """``0``, ``"0"``, ``"0.0"`` and ``"-0"`` turn the cap off. ``"0e0"`` is exponent text, refused as + ``"1e-400"`` is, so no spelling that merely rounds to zero switches a guard off.""" + for zero in (0, 0.0, "0", " 0.0 ", "-0", None, ""): + assert getattr(build({key: zero}), attr) is None, repr(zero) + with pytest.raises(ValueError, match=_refusal(key)): + build({key: "0e0"}) + + +def test_the_direct_and_smart_timeouts_refuse_a_bool() -> None: + """The Direct destination and the SMART token provider load key material after reading the + timeout, so only the refusal is driven here; the reader is the one the tables above pin.""" + direct_base = { + **_EMAIL_BASE, + "signing_cert": "unused", + "signing_key": "unused", + "recipient_cert": "unused", + "trust_anchor": "unused", + } + for bad in (True, False, math.nan, "1e-400"): + with pytest.raises(ValueError, match=_refusal("timeout_seconds")): + DirectDestination( + Destination( + name="OB", + type=ConnectorType.DIRECT, + settings={**direct_base, "timeout_seconds": bad}, + ) + ) + with pytest.raises(ValueError, match=_refusal("smart_timeout_seconds")): + token_provider_from_settings( + { + "smart_token_url": "https://auth.example.com/token", + "smart_client_id": "c", + "smart_private_key": "unused", + "smart_timeout_seconds": bad, + } + ) + + +async def test_the_reference_sync_acquire_timeout_is_refused_before_a_pool_is_made( + monkeypatch: pytest.MonkeyPatch, +) -> None: + """Read after the pool was made, a refusal would have left that pool unclosed.""" + from messagefoundry.pipeline.reference_sync import _load_database_source + + async def _no_pool(*args: object, **kwargs: object) -> object: + raise AssertionError("the timeout should have been refused before a pool was made") + + monkeypatch.setattr(database, "_make_pool", _no_pool) + base = {"server": "db.example", "database": "d", "statement": "SELECT a", "key_column": "a"} + for bad in (True, False, math.nan, "inf", "1e-400"): + with pytest.raises(ValueError, match=_refusal("reference source acquire_timeout")): + await _load_database_source( + {**base, "acquire_timeout": bad}, EgressSettings(deny_by_default=False) + ) + + +def test_an_http_reply_timeout_must_be_above_zero() -> None: + """``docs/CONNECTIONS.md`` says each must be positive. A numeric ``0`` used to become the + default while ``"0"`` became a zero budget; both are refused now. Unset takes 30 seconds.""" + for key in ("reply_timeout", "reply_write_timeout"): + for bad in (0, "0", -1, "-0.5"): + with pytest.raises(ValueError, match=rf"HTTP source {key} must be a number of seconds"): + _HTTP_IN({key: bad}) + for unset in (None, ""): + assert getattr(_HTTP_IN({key: unset}), key) == 30.0 + assert getattr(_HTTP_IN({}), key) == 30.0 + + +_RANGE_REFUSALS = [ + pytest.param(lambda v: positive_cap(v, int, knob="k", transport="t"), -7, id="positive_cap"), + pytest.param(lambda v: positive_cap(v, float, knob="k", transport="t"), -7.5, id="float_cap"), + pytest.param(lambda v: resolve_poll_ceiling(v, knob="k", transport="t"), -7, id="poll_ceiling"), + pytest.param( + lambda v: poll_interval({"poll_seconds": v}, default=1.0, transport="t"), + -7.5, + id="poll_interval", + ), + pytest.param(lambda v: _HTTP_IN({"max_header_bytes": v}), "00", id="max_header_bytes"), + pytest.param(lambda v: _file({"max_decompressed_members": v}), "00", id="max_members"), + pytest.param(lambda v: _MLLP_IN({"max_connections_per_host": v}), -7, id="mllp-per_host"), + pytest.param(lambda v: _MLLP_IN({"max_frame_seconds": v}), -7.5, id="mllp-frame_seconds"), + pytest.param(lambda v: _HTTP_IN({"reply_timeout": v}), -7.5, id="http-reply_timeout"), +] + + +@pytest.mark.parametrize(("refuse", "value"), _RANGE_REFUSALS) +def test_a_range_refusal_withholds_the_value(refuse: Callable[[Any], Any], value: Any) -> None: + """A setting arrives ``env()``-resolved, so a range refusal keeps the value out of its message, + as the type refusals already did (BACKLOG #1183, #1796). + + Red mutation: put ``{value!r}`` back in any of these refusals.""" + with pytest.raises(ValueError) as caught: + refuse(value) + message = str(caught.value) + assert "value withheld" in message, message + assert str(value) not in message, message + + +def test_float_setting_states_one_rule() -> None: + """The reader itself, once: exact zeros come back as zero, and every value that is not a finite + real number is refused without its value.""" + for zero in (0, 0.0, -0.0, "0", "-0.0", Decimal(0)): + assert float_setting(zero, knob="k", transport="t") == 0.0, repr(zero) + assert float_setting(Fraction(5, 2), knob="k", transport="t") == 2.5 + assert float_setting(-3, knob="k", transport="t") == -3.0 + for bad in (Decimal("1e-400"), Decimal("NaN"), 10**400, complex(1, 0), b"2.5", None): + with pytest.raises(ValueError, match=r"^t k is not a valid float value \(value withheld\)"): + float_setting(bad, knob="k", transport="t") + with pytest.raises(ValueError) as caught: + float_setting("SENTINEL-3432", knob="k", transport="t") + assert "SENTINEL-3432" not in str(caught.value) + assert caught.value.__context__ is None + # A number too large for a float says so, rather than calling it not a number. + with pytest.raises(ValueError, match="too large for a float"): + float_setting(10**400, knob="k", transport="t") + + +@pytest.mark.parametrize("key", ["reply_timeout", "reply_write_timeout"]) +@pytest.mark.parametrize("bad", [True, math.nan, math.inf, -7.25, 0]) +def test_the_http_factory_refuses_what_the_connector_refuses(key: str, bad: object) -> None: + """The ``Http`` factory checks the two reply timeouts at wiring. ``isinstance(v, int | float)`` + let a bool through, and ``v <= 0`` let NaN and infinity through, so the factory and the connector + disagreed. The refusal also withholds the value now. + + Red mutation: drop the bool or the ``math.isfinite`` clause from the factory check.""" + overrides: dict[str, Any] = {key: bad} + with pytest.raises(WiringError) as caught: + Http(port=0, reply_from="OB_REPLY", **overrides) + message = str(caught.value) + assert f"Http {key} must be a finite positive number of seconds (value withheld)" in message + assert "-7.25" not in message diff --git a/tests/test_connection_event_emit.py b/tests/test_connection_event_emit.py index 99b5c4bc9b..b2eba010ca 100644 --- a/tests/test_connection_event_emit.py +++ b/tests/test_connection_event_emit.py @@ -671,9 +671,8 @@ def test_a_negative_cap_is_refused_at_build_not_discovered_at_the_first_connecti ): _mllp(max_frame_seconds=-1.0) # NaN is truthy and fails every comparison, so a `< 0` test let it through as a live deadline. - with pytest.raises( - ValueError, match="max_frame_seconds must be a number of seconds, zero or more" - ): + # The cap reader now refuses it before that guard runs (BACKLOG #3432). + with pytest.raises(ValueError, match=r"max_frame_seconds is not a valid float value"): _mllp(max_frame_seconds=float("nan")) # 0 stays the documented "off" spelling for both, and is NOT caught by the refusal above. assert _mllp(max_connections_per_host=0).max_connections_per_host is None diff --git a/tests/test_dicom_scp_disposition.py b/tests/test_dicom_scp_disposition.py index e574794b4b..5a43b9dafd 100644 --- a/tests/test_dicom_scp_disposition.py +++ b/tests/test_dicom_scp_disposition.py @@ -251,7 +251,7 @@ def test_a_negative_cap_is_refused_with_the_scps_own_text(bad: object) -> None: with pytest.raises(ValueError) as caught: _scp(bad) # type: ignore[arg-type] message = str(caught.value) - assert "DICOM-SCP source max_object_bytes=" in message + assert "DICOM-SCP source max_object_bytes must be above zero (value withheld" in message assert "disable" not in message.replace("does not disable", "") assert "does not disable" in message assert str(ingress_guards.INGRESS_MAX_BYTES) in message diff --git a/tests/test_poll_seconds_validation.py b/tests/test_poll_seconds_validation.py index 760c5bf568..d1ce22a39b 100644 --- a/tests/test_poll_seconds_validation.py +++ b/tests/test_poll_seconds_validation.py @@ -68,7 +68,7 @@ def _database(tmp_path: Path, monkeypatch: pytest.MonkeyPatch, value: Any) -> An @pytest.mark.parametrize("source", list(_SOURCES)) @pytest.mark.parametrize( "value", - [0, 0.0, -1, -0.5, math.nan, math.inf, "0", "nan", "soon"], + [0, 0.0, -1, -0.5, math.nan, math.inf, "0", "nan", "soon", True, False, "1e-400"], ids=[ "zero", "zero-float", @@ -79,13 +79,16 @@ def _database(tmp_path: Path, monkeypatch: pytest.MonkeyPatch, value: Any) -> An "zero-text", "nan-text", "not-a-number", + "true", + "false", + "rounds-to-zero", ], ) def test_a_poll_interval_that_is_not_a_finite_positive_number_is_refused( source: str, value: Any, tmp_path: Path, monkeypatch: pytest.MonkeyPatch ) -> None: build, _, _ = _SOURCES[source] - with pytest.raises(ValueError, match="poll_seconds="): + with pytest.raises(ValueError, match="poll_seconds "): build(tmp_path, monkeypatch, value) @@ -104,5 +107,5 @@ def test_the_refusal_names_the_source_and_says_what_is_wanted() -> None: with pytest.raises(ValueError) as caught: poll_interval({"poll_seconds": 0}, default=1.0, transport="file source") assert str(caught.value) == ( - "file source poll_seconds=0 must be a finite number of seconds above zero" + "file source poll_seconds must be a number of seconds above zero (value withheld)" ) diff --git a/tests/test_timer_source.py b/tests/test_timer_source.py index 6c7a4e140d..b3ef824062 100644 --- a/tests/test_timer_source.py +++ b/tests/test_timer_source.py @@ -254,7 +254,7 @@ def test_timer_requires_a_schedule() -> None: def test_timer_rejects_nonpositive_interval() -> None: - with pytest.raises(ValueError, match="must be > 0"): + with pytest.raises(ValueError, match="interval_seconds must be a number of seconds above zero"): TimerSource(Source(type=ConnectorType.TIMER, settings={"body": "X", "interval_seconds": 0})) From 064123114e7cc47babf66c2fb0125f57d8573bbd Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 12 Oct 2026 07:35:55 -0500 Subject: [PATCH 2/3] transports: float caps refuse by a true reason, check refuses a bad acquire_timeout (BACKLOG #3432) Review fixes on the float-cap reader: - The Http factory refuses an int too large for a float by name, instead of raising a raw OverflowError. - float_setting reads any exact zero as zero: text whose digits are all zeros, with no exponent ("0_0", a full-width zero). Its other zero-reading refusals now state a true reason. - DatabaseRef checks a literal acquire_timeout when declared, and build_check_registry reads an env() one once resolved, through the sync's own reader. So messagefoundry check refuses a bool or a NaN. - The changelog fragment scopes the zero-timeout sentence to a directly built HttpSource and splits its long sentence into a list. Proposed PR title: a float cap or timeout refuses a bool, nan, inf or a value that only rounds to zero (BACKLOG #3432) Banner: BUILT -- engine branch fix-3432-float-caps, awaiting PR. --- changelog.d/3432.fixed.md | 44 ++++++----- messagefoundry/config/wiring.py | 29 +++++++ messagefoundry/pipeline/reference_sync.py | 19 +++-- messagefoundry/pipeline/wiring_runner.py | 9 ++- messagefoundry/transports/base.py | 34 +++++--- tests/test_cap_setting_spellings.py | 96 +++++++++++++++++++++++ 6 files changed, 195 insertions(+), 36 deletions(-) diff --git a/changelog.d/3432.fixed.md b/changelog.d/3432.fixed.md index 2137869ed5..5f47b4ae32 100644 --- a/changelog.d/3432.fixed.md +++ b/changelog.d/3432.fixed.md @@ -4,21 +4,29 @@ `max_messages_per_second` rate read text such as `"1e-400"` as off, because `float()` rounds it to `0.0`, so the guard it sets would not have run. Now one reader, `float_setting` in `transports/base.py`, reads every float cap that `cap_setting` or `positive_cap` reads, and - `poll_seconds`. It also reads at least these former bare `float()` reads: `timeout_seconds` and - `connect_timeout` on the MLLP, TCP, X12 and DICOM connections, `timeout_seconds` on the FHIR, FHIR - lookup, REST, SOAP, DICOMweb, Email and Direct connections, `acquire_timeout` on the DATABASE - connections, the DATABASE lookup and a DATABASE reference source, the REMOTEFILE `connect_timeout`, - the File source's `min_age_seconds`, the Timer source's `interval_seconds`, the HTTP source's - `reply_timeout` and `reply_write_timeout`, and the OAuth2 and SMART token timeouts and expiry skews. - Each refusal names the key and withholds the value. A float cap is off only for an exact zero, such - as `0`, `"0"` or `"0.0"`; `"0e0"` is refused, as it already was on an integer cap. A - `connections.toml` literal `1e-400` still reads as zero, because the TOML parser turns it into - `0.0` before any reader sees it. The HTTP `reply_timeout` and `reply_write_timeout` now refuse - zero, as `docs/CONNECTIONS.md` already said; a numeric `0` used to take the 30-second default. - The `Http` factory's own check of those two keys refuses a bool, a NaN and an infinity too. The - other timeouts above are checked for type only: a zero or negative `timeout_seconds`, - `connect_timeout` or `acquire_timeout` is still not refused at build. - At least the range refusals in `positive_cap`, `resolve_poll_ceiling` and `poll_interval`, the - HTTP `max_header_bytes`, the File `max_decompressed_members` and the MLLP - `max_connections_per_host` and `max_frame_seconds` checks now withhold the value too. The reader's - docstring holds the full rule. (`BACKLOG #3432`) + `poll_seconds`. Each refusal names the key and withholds the value. The reader's docstring holds + the full rule. (`BACKLOG #3432`) + - A float cap is off only for an exact zero, such as `0`, `"0"`, `"0.0"` or `"0_0"`. `"0e0"` is + refused, as it already was on an integer cap. + - A `connections.toml` literal `1e-400` still reads as zero. The TOML parser turns it into `0.0` + before any reader sees it. + - The reader also replaces bare `float()` reads of `timeout_seconds` and `connect_timeout` on the + MLLP, TCP, X12 and DICOM connections. + - It reads `timeout_seconds` on the FHIR, FHIR lookup, REST, SOAP, DICOMweb, Email and Direct + connections. + - It reads `acquire_timeout` on the DATABASE connections, the DATABASE lookup and a DATABASE + reference source. `messagefoundry check` now refuses a reference source's `acquire_timeout` + that is not a number, from a literal or from `env()`. A zero or negative one still passes. + - It reads the REMOTEFILE `connect_timeout`, the File source's `min_age_seconds` and the Timer + source's `interval_seconds`. + - It reads the OAuth2 and SMART token timeouts and expiry skews. + - It reads the HTTP source's `reply_timeout` and `reply_write_timeout`. An `HttpSource` built + directly now refuses a zero there, where a numeric `0` took the 30-second default. `Http()` and + `connections.toml` already refused it. + - The `Http` factory's own check of those two keys now refuses a bool, a NaN, an infinity and an + integer too large for a float. + - The other timeouts above are checked for type only. A zero or negative `timeout_seconds`, + `connect_timeout` or `acquire_timeout` is still not refused at build. + - Range refusals withhold the value too. At least `positive_cap`, `resolve_poll_ceiling` and + `poll_interval` do, and so do the HTTP `max_header_bytes` and File `max_decompressed_members` + checks. So do the MLLP `max_connections_per_host` and `max_frame_seconds` checks. diff --git a/messagefoundry/config/wiring.py b/messagefoundry/config/wiring.py index de4c9270b2..019294cc75 100644 --- a/messagefoundry/config/wiring.py +++ b/messagefoundry/config/wiring.py @@ -876,6 +876,23 @@ def DatabaseRef( check_db_connect_timeout(connect_timeout, "DatabaseRef") except ValueError as exc: raise WiringError(str(exc)) from None + # The same holds for acquire_timeout: a bool or a NaN would otherwise be refused only at the first + # sync, after start, so `messagefoundry check` passed it (BACKLOG #3432). The sync reads it again, + # by the same rule, because an env() ref is resolved only then. + if not isinstance(acquire_timeout, EnvRef): + # Imported here: messagefoundry.transports imports this module at its top level. + from messagefoundry.transports.base import float_setting + + # The refusal is raised outside the handler, so it carries no exception chain. + refusal: str | None = None + try: + float_setting( + acquire_timeout, knob="acquire_timeout", transport="DATABASE reference source" + ) + except ValueError as exc: + refusal = str(exc) + if refusal is not None: + raise WiringError(refusal) return ReferenceSourceSpec( "database", { @@ -2497,6 +2514,18 @@ def _validate_sync_reply(settings: Mapping[str, Any]) -> None: value = settings[name] # A bool is an int, and NaN and inf pass `<= 0`, so each is refused by name (BACKLOG #3432). # The connector reads the same keys by the rule in transports/base.py float_setting. + # math.isfinite() raises OverflowError on an int too large for a float, such as 10**400 or a + # long TOML integer, so that int is refused by name here instead, outside the handler. + too_large = False + if isinstance(value, int) and not isinstance(value, bool): + try: + float(value) + except OverflowError: + too_large = True + if too_large: + raise WiringError( + f"Http {name} is not a valid float value (value withheld): it is too large for a float" + ) if ( isinstance(value, bool) or not isinstance(value, int | float) diff --git a/messagefoundry/pipeline/reference_sync.py b/messagefoundry/pipeline/reference_sync.py index 4550e99b2c..79cabcc226 100644 --- a/messagefoundry/pipeline/reference_sync.py +++ b/messagefoundry/pipeline/reference_sync.py @@ -111,6 +111,19 @@ def reference_connection_name(name: str) -> str: return f"reference:{name}" +def reference_acquire_timeout(settings: Mapping[str, Any]) -> float: + """Read a DATABASE reference source's ``acquire_timeout`` by the rule in + :func:`~messagefoundry.transports.base.float_setting`, or refuse it (BACKLOG #3432). + + One reader for the sync and for ``build_check_registry``, so an ``env()`` value the sync would + refuse fails ``messagefoundry check`` too. ``DatabaseRef`` reads a literal the same way.""" + return float_setting( + settings.get("acquire_timeout", DEFAULT_STORE_ACQUIRE_TIMEOUT), + knob="acquire_timeout", + transport="DATABASE reference source", + ) + + def database_source_dsn(settings: Mapping[str, Any], *, connection: str | None = None) -> str: """Build a DATABASE reference source's DSN, refusing weakened TLS under the AMBIENT hop posture. @@ -171,11 +184,7 @@ async def _load_database_source( # the runner's existing per-set source-failure handler catches it, keeps the last-good snapshot # and alerts, exactly as it does for any other source error. # Read before the pool is made, so a refused value leaves no pool unclosed (BACKLOG #3432). - acquire_timeout = float_setting( - settings.get("acquire_timeout", DEFAULT_STORE_ACQUIRE_TIMEOUT), - knob="acquire_timeout", - transport="DATABASE reference source", - ) + acquire_timeout = reference_acquire_timeout(settings) pool = await _make_pool( dsn, count_setting( diff --git a/messagefoundry/pipeline/wiring_runner.py b/messagefoundry/pipeline/wiring_runner.py index 3056a8f0a7..22d913a672 100644 --- a/messagefoundry/pipeline/wiring_runner.py +++ b/messagefoundry/pipeline/wiring_runner.py @@ -169,7 +169,11 @@ DeliveryPhaseTiming, delivery_phase_timing_enabled, ) -from messagefoundry.pipeline.reference_sync import database_source_dsn, reference_connection_name +from messagefoundry.pipeline.reference_sync import ( + database_source_dsn, + reference_acquire_timeout, + reference_connection_name, +) from messagefoundry.pipeline.reply_wait import ReplyRendezvous from messagefoundry.pipeline.sandbox import ( GraphShape, @@ -10849,6 +10853,9 @@ def _build_check_connectors( except WiringError: continue database_source_dsn(rsettings, connection=reference_connection_name(rname)) + # BACKLOG #3432: the sync reads acquire_timeout by this rule, so an env() value it would + # refuse fails check here rather than as a failed sync after start. + reference_acquire_timeout(rsettings) resolved_fhir_lookups: dict[str, dict[str, Any]] = {} for fname, fspec in registry.fhir_lookups.items(): resolved_fhir_lookups[fname] = _fhir_lookup_settings(fspec, env_values, egress) diff --git a/messagefoundry/transports/base.py b/messagefoundry/transports/base.py index e629241581..b57663e3f4 100644 --- a/messagefoundry/transports/base.py +++ b/messagefoundry/transports/base.py @@ -112,10 +112,11 @@ DEFAULT_MAX_ITEMS_PER_POLL = 500 -#: Text that spells zero in plain digits: ``"0"``, ``"00"``, ``"0.0"``, ``"-0.0"``, ``".0"``. A cap -#: reads this as "off" without ``float()``, so text that ``float()`` merely rounds to zero, such as +#: ASCII text that spells zero: ``"0"``, ``"00"``, ``"0.0"``, ``"-0.0"``, ``".0"``. A cap reads this +#: as "off" without ``float()``, so text that ``float()`` merely rounds to zero, such as #: ``"1e-400"``, reaches :func:`count_setting` or :func:`float_setting` and is refused there -#: (BACKLOG #3431, #3432). +#: (BACKLOG #3431, #3432). It is a shortcut, not the whole rule: ``int()`` and :func:`_spells_zero` +#: each read a few more exact zeros, such as ``"0_0"``. _ZERO_TEXT = re.compile(r"[+-]?(?:0+(?:\.0*)?|\.0+)") @@ -189,8 +190,9 @@ def float_setting(value: Any, *, knob: str, transport: str) -> float: - a bool is refused, ``False`` as well as ``True``: a bool is not a number of seconds; - a NaN or an infinity is refused, and so is a number too large for a float, such as ``1e400``; - a value that ``float()`` only rounds to zero is refused, such as the text ``"1e-400"`` or - ``Decimal("1e-400")``. Text is zero only when it spells zero in plain digits, such as ``"0"``, - ``"0.0"`` or ``"-0"``, so ``"0e0"`` is refused too; + ``Decimal("1e-400")``. Text is zero only when every digit in it is a zero and it has no + exponent. So ``"0"``, ``"0.0"``, ``"-0"``, ``"0_0"`` and the full-width ``"０"`` are zero, + as :func:`count_setting`'s ``int()`` reads the whole-number ones, and ``"0e0"`` is refused; - other text is read by ``float()``, so ``" 2.5 "`` and ``"+2.5"`` pass; - anything else is refused, whatever ``float()`` would have made of it. @@ -221,7 +223,7 @@ def float_setting(value: Any, *, knob: str, transport: str) -> float: if not math.isfinite(seconds): reason = "it is not a finite number" elif seconds == 0 and not _spells_zero(value): - reason = "float() rounds it to zero, but it is not zero" + reason = "float() reads it as zero, but it is not an exact zero in plain digits" else: return seconds else: @@ -243,9 +245,15 @@ def positive_seconds(value: Any, *, knob: str, transport: str) -> float: def _spells_zero(value: Any) -> bool: - """Whether ``value`` is an exact zero: text in plain digits, or a number equal to zero.""" + """Whether ``value``, which ``float()`` read as zero, is an exact zero: a number equal to zero, or + text whose digits are all zeros and which has no exponent. + + ``float()`` has already parsed the text, so without an exponent its value is its digits; when + every digit is a zero, the value is exactly zero. That takes ``"0_0"`` and the full-width + ``"０"``, which ``int()`` reads as ``0`` on an ``int`` cap too. An exponent is refused even on + zero digits, as ``"0e0"`` is on an ``int`` cap.""" if isinstance(value, str): - return _ZERO_TEXT.fullmatch(value.strip()) is not None + return not any(c in "eE" or (c.isdecimal() and int(c) != 0) for c in value) return bool(value == 0) @@ -277,10 +285,12 @@ def cap_setting[NumT: (int, float)]( :func:`count_setting`, which refuses it, as it refuses ``"0e0"``. A number that equals zero, such as ``0.0`` or ``-0.0``, is off too. - **A ``float`` cap is read by :func:`float_setting`, and the same exact zeros are its only "off" - (BACKLOG #3432).** So ``"1e-400"`` and ``"0e0"`` are refused there rather than read as off, and a - bool, a NaN and an infinity are refused too. That function's docstring holds the rule, including - why a ``connections.toml`` literal ``1e-400`` still reads as zero. + **A ``float`` cap is read by :func:`float_setting`, and only an exact zero is its "off" (BACKLOG + #3432).** Its exact zeros are a little wider than an ``int`` cap's: text whose digits are all + zeros with no exponent, so ``"0_0.0"`` is off on a ``float`` cap and refused on an ``int`` cap, + whose ``int()`` cannot read it. ``"1e-400"`` and ``"0e0"`` are refused on both, and a bool, a NaN + and an infinity are refused too. That function's docstring holds the rule, including why a + ``connections.toml`` literal ``1e-400`` still reads as zero. A negative value is returned as-is, for the callers that refuse one at build with a message naming their own key; :func:`positive_cap` is that caller for most sites. It is a different mistake with a diff --git a/tests/test_cap_setting_spellings.py b/tests/test_cap_setting_spellings.py index ee54e61191..ad3647a128 100644 --- a/tests/test_cap_setting_spellings.py +++ b/tests/test_cap_setting_spellings.py @@ -35,6 +35,7 @@ ConnectionSpec, Database, DatabasePoll, + DatabaseRef, Ftp, Http, Sftp, @@ -46,6 +47,7 @@ from messagefoundry.transports import build_destination, build_source, database, mllp from messagefoundry.transports.base import ( cap_setting, + count_setting, float_setting, poll_interval, positive_cap, @@ -933,3 +935,97 @@ def test_the_http_factory_refuses_what_the_connector_refuses(key: str, bad: obje message = str(caught.value) assert f"Http {key} must be a finite positive number of seconds (value withheld)" in message assert "-7.25" not in message + + +def test_the_http_factory_refuses_an_int_too_large_for_a_float_by_name() -> None: + """``math.isfinite(10**400)`` raises ``OverflowError``, so the factory raised that raw error, naming + no key. A long ``connections.toml`` integer reaches the same check through the desugar. + + Red mutation: drop the factory's ``float(value)`` overflow guard. ``OverflowError`` escapes.""" + for key in ("reply_timeout", "reply_write_timeout"): + overrides: dict[str, Any] = {key: 10**400} + with pytest.raises(WiringError, match=rf"^Http {key} .*too large for a float") as caught: + Http(port=0, reply_from="OB_REPLY", **overrides) + assert "0" * 20 not in str(caught.value) + + +#: Exact zeros that ``_ZERO_TEXT`` does not match. ``int()`` reads the whole-number ones as ``0``, so +#: an ``int`` cap already took them as off; a float setting called them "rounded to zero". +_ODD_ZERO_TEXT = ["0_0", "０", "-0_0.0_0", "０.０"] + + +def test_a_float_setting_reads_every_exact_zero_spelling_as_zero() -> None: + """Text whose digits are all zeros, with no exponent, is exactly zero, so it reads as zero and a + float cap reads it as off, as an ``int`` cap does. + + Red mutation: test text against ``_ZERO_TEXT`` alone in ``_spells_zero``. Each case is refused.""" + for zero in _ODD_ZERO_TEXT: + assert float_setting(zero, knob="k", transport="t") == 0.0, repr(zero) + assert cap_setting(zero, float, knob="k", transport="t") is None, repr(zero) + for zero in ("0_0", "０"): + assert count_setting(zero, knob="k", transport="t") == 0, repr(zero) + assert cap_setting(zero, int, knob="k", transport="t") is None, repr(zero) + + +def test_a_float_setting_refusal_of_a_zero_reading_states_a_true_reason() -> None: + """What ``float()`` reads as zero but is refused is either exponent text or not zero at all, such + as a plain decimal too small for a float. The reason says so, and claims no rounding of an exact + zero. + + Red mutation: restore "float() rounds it to zero, but it is not zero". The match fails.""" + tiny_decimal = "0." + "0" * 400 + "1" + for bad in ("0e0", "1e-400", tiny_decimal, Decimal("1e-400")): + with pytest.raises(ValueError, match="it is not an exact zero in plain digits$"): + float_setting(bad, knob="k", transport="t") + + +@pytest.mark.parametrize("bad", [True, False, math.nan, "inf", "1e-400", [2.5]]) +def test_database_ref_refuses_a_bad_acquire_timeout_when_declared(bad: object) -> None: + """A reference set is first synced after start, so a bool or a NaN refused only at sync passed + ``messagefoundry check``. ``DatabaseRef`` checks it when declared, as it checks ``connect_timeout``. + + Red mutation: drop the declaration check. ``DatabaseRef`` returns a spec.""" + overrides: dict[str, Any] = {"acquire_timeout": bad} + with pytest.raises(WiringError, match=_refusal("reference source acquire_timeout")): + DatabaseRef(server="s", database="d", statement="SELECT a", key_column="a", **overrides) + + +def test_database_ref_accepts_a_number_or_an_env_ref_for_acquire_timeout() -> None: + """The control: a number and its text are declared, and an ``env()`` ref is left for the sync.""" + for good in (12.5, "12.5", 0, env("MEFOR_TEST_3432_ACQUIRE")): + overrides: dict[str, Any] = {"acquire_timeout": good} + spec = DatabaseRef( + server="s", database="d", statement="SELECT a", key_column="a", **overrides + ) + assert spec.settings["acquire_timeout"] is good + + +def test_the_build_check_refuses_an_env_acquire_timeout_the_sync_would_refuse() -> None: + """``DatabaseRef`` cannot read an ``env()`` ref, so ``build_check_registry`` reads it once + resolved, by the sync's own reader. Before, ``messagefoundry check`` passed it. + + Red mutation: drop the ``reference_acquire_timeout`` call from ``build_check_registry``.""" + from messagefoundry.config.wiring import ReferenceSpec, Registry + from messagefoundry.pipeline.wiring_runner import build_check_registry + + registry = Registry() + source = DatabaseRef( + server="db.example.org", + database="d", + statement="SELECT a", + key_column="a", + acquire_timeout=env("MEFOR_TEST_3432_ACQUIRE"), # type: ignore[arg-type] + ) + registry.references["codes"] = ReferenceSpec(name="codes", source=source) + + def check(value: str) -> None: + build_check_registry( + registry, + inbound_bind_host="127.0.0.1", + env_values={"mefor_test_3432_acquire": value}, + egress=EgressSettings(deny_by_default=False), + ) + + with pytest.raises(ValueError, match=_refusal("reference source acquire_timeout")): + check("nan") + check("12.5") # the control: a number passes From 67b55ee5323b6cf416b6d18b0b087e4582b4a23a Mon Sep 17 00:00:00 2001 From: wshallwshall Date: Mon, 12 Oct 2026 07:56:32 -0500 Subject: [PATCH 3/3] changelog: scope the 3432 acquire_timeout and bool sentences (BACKLOG #3432) The check refuses an env() acquire_timeout only when it resolves at check time, and refuses at least a bool, a NaN or "1e-400". A bool on the bare-float timeouts read as zero or one second and now fails at build; a cap_setting float cap already refused one (BACKLOG #3431). Proposed PR title: a float cap or timeout refuses a bool, nan, inf or a value that only rounds to zero (BACKLOG #3432) Banner: BUILT -- engine branch fix-3432-float-caps, awaiting PR. --- changelog.d/3432.fixed.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/changelog.d/3432.fixed.md b/changelog.d/3432.fixed.md index 5f47b4ae32..9dca86f851 100644 --- a/changelog.d/3432.fixed.md +++ b/changelog.d/3432.fixed.md @@ -14,9 +14,13 @@ MLLP, TCP, X12 and DICOM connections. - It reads `timeout_seconds` on the FHIR, FHIR lookup, REST, SOAP, DICOMweb, Email and Direct connections. + - On those timeouts, `false` used to read as a zero-second timeout and `true` as one second. + Both now fail at build. A float cap read through `cap_setting`, such as `receive_timeout`, + already refused a bool (`BACKLOG #3431`). - It reads `acquire_timeout` on the DATABASE connections, the DATABASE lookup and a DATABASE - reference source. `messagefoundry check` now refuses a reference source's `acquire_timeout` - that is not a number, from a literal or from `env()`. A zero or negative one still passes. + reference source. `messagefoundry check` now refuses at least a bool, a NaN or `"1e-400"` as + a reference source's `acquire_timeout`. That covers a literal, and an `env()` value when it + resolves at check time. A zero or negative one still passes. - It reads the REMOTEFILE `connect_timeout`, the File source's `min_age_seconds` and the Timer source's `interval_seconds`. - It reads the OAuth2 and SMART token timeouts and expiry skews.