From a8ee7730612faf246a642b2e8c7a756f7b39aa4a Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Wed, 28 Oct 2026 02:06:52 -0500 Subject: [PATCH 01/15] Sandbox mode isolated: worker checks read is-not-OFF, and the parent brokers db_lookup/fhir_lookup (BACKLOG #3131) Adds [sandbox].mode = "isolated" (ADR 0147). Every check that means "runs in a sandbox worker" now reads `is not SandboxMode.OFF`: dryrun route_only, transform_one and _accepted, and the wiring_runner fusion gate. A graph with no config dir raises under isolated instead of running in-process with unbrokered lookups. The worker asks the parent for a Handler's lookup with a `lookup` frame in the closed tag set, bound to the dispatch's (id, phase, name) and a per-dispatch seq. The parent answers on the dispatch thread through the same db_lookup/fhir_lookup accessor the in-process path uses, refuses a non-Handler phase, and refuses an empty [egress].allowed_db or allowed_http even under block_unlisted_outbound = false. The codec gains a Decimal value tag for DB rows. Other modes still refuse a lookup frame. --- messagefoundry/config/settings.py | 13 +- messagefoundry/pipeline/_sandbox_codec.py | 274 +++++++++- messagefoundry/pipeline/_sandbox_worker.py | 146 +++++- messagefoundry/pipeline/dryrun.py | 6 +- messagefoundry/pipeline/engine.py | 18 +- messagefoundry/pipeline/sandbox.py | 244 ++++++++- messagefoundry/pipeline/wiring_runner.py | 25 +- tests/test_checks.py | 2 +- tests/test_sandbox_isolated.py | 569 +++++++++++++++++++++ 9 files changed, 1230 insertions(+), 67 deletions(-) create mode 100644 tests/test_sandbox_isolated.py diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index 82c7b439f4..9f215e0c3b 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -2402,7 +2402,11 @@ class SandboxSettings(_Section): * **Live enrichment is refused.** ``db_lookup``/``fhir_lookup`` re-enter the event loop, which a process boundary breaks, so they fail closed inside the child. **A Handler needing either must - run ``mode="off"``** — that escape is supported and is not going away. ``[sandbox]`` is a + run ``mode="off"``, or ``mode="isolated"``** — the off escape is supported and is not going + away. ``mode="isolated"`` (ADR 0147, vault BACKLOG #3131) is the same worker with both lookups + brokered through the engine, refused while ``[egress].allowed_db`` (``allowed_http`` for + ``fhir_lookup``) is empty even under the ``block_unlisted_outbound`` opt-out. It adds no + OS-level confinement yet: ``docs/CONFIGURATION.md``, section ``[sandbox]``, says what it does. ``[sandbox]`` is a single **engine-wide** section: :meth:`messagefoundry.pipeline.engine.Engine.add_registry` renders ONE ``SandboxPolicy`` for the whole graph and a connection carries no per-connection sandbox field, so ``mode="off"`` set for one Handler runs **every** Router and Handler in the @@ -2427,9 +2431,10 @@ class SandboxSettings(_Section): **restart to change**, exactly like ``claim_mode``).""" # off (default, byte-identical, no subprocess — and the supported escape for a Handler needing - # live enrichment) | subprocess (persistent per-inbound worker child). ENGINE-WIDE, not per - # connection: one policy is rendered for the whole graph. - mode: Literal["off", "subprocess"] = Field(default="off") + # live enrichment) | subprocess (persistent per-inbound worker child) | isolated (that worker, + # with db_lookup/fhir_lookup brokered through the engine; ADR 0147). Any other value refuses to + # load. ENGINE-WIDE, not per connection: one policy is rendered for the whole graph. + mode: Literal["off", "subprocess", "isolated"] = Field(default="off") # Authoritative wall-clock cap (seconds) per Router/Handler call on EVERY platform: the parent # kills a worker that overruns it, so a pathological busy-loop can never wedge intake. Floor > 0. # There is no RLIMIT_CPU, so CPU spent outside a call has no bound (ADR 0087, 2026-10-07). diff --git a/messagefoundry/pipeline/_sandbox_codec.py b/messagefoundry/pipeline/_sandbox_codec.py index 48607c82c7..e0d4b6aca8 100644 --- a/messagefoundry/pipeline/_sandbox_codec.py +++ b/messagefoundry/pipeline/_sandbox_codec.py @@ -55,6 +55,7 @@ import struct from collections.abc import Mapping, Sequence from dataclasses import dataclass +from decimal import Decimal, InvalidOperation from pathlib import Path from typing import Any, Final @@ -71,6 +72,7 @@ handler_item_fault, handler_result_items, ) +from messagefoundry.fhirsearch import FhirRaw, FhirToken from messagefoundry.parsing.message import Message, RawMessage from messagefoundry.redaction import safe_exc @@ -79,18 +81,26 @@ "SandboxError", "SandboxCodecError", "GraphShape", + "LOOKUP_DB", + "LOOKUP_FHIR", + "LookupRequest", "Request", "Response", "build_payload", "decode_boot", + "decode_answer", "decode_boot_reply", "decode_frame", + "decode_lookup_reply", "decode_request", "decode_response", "encode_bootfail", "encode_boot", "encode_fail", "encode_frame", + "encode_lookup", + "encode_lookup_fail", + "encode_lookup_ok", "encode_ok", "encode_ready", "encode_request", @@ -301,6 +311,11 @@ def enc_value(value: object, blobs: _Blobs, _depth: int = 0) -> Any: return blobs.ref(value) if isinstance(value, bytes | bytearray): return {"b": base64.b64encode(bytes(value)).decode("ascii")} + if isinstance(value, Decimal): + # A NUMERIC/DECIMAL/MONEY column comes back from the driver as a Decimal, so a brokered + # db_lookup row carries one wherever mode=off would hand the Handler one (ADR 0147). Its str() + # round-trips exactly, NaN and Infinity included. + return {"N": str(value)} # datetime BEFORE date — datetime is a date subclass, and tomllib produces all three natively, so a # .toml code set with a date value is a legitimate shape plain JSON refuses. if isinstance(value, _dt.datetime): @@ -368,6 +383,13 @@ def dec_value(node: Any, reader: _Reader, _depth: int = 0) -> Any: return base64.b64decode(arg, validate=True) except (ValueError, TypeError) as exc: raise SandboxCodecError(f"sandbox bytes tag is not valid base64: {exc}") from exc + if tag == "N": + if not isinstance(arg, str): + raise SandboxCodecError("sandbox decimal tag must carry a string") + try: + return Decimal(arg) + except InvalidOperation as exc: + raise SandboxCodecError("sandbox decimal tag is not a decimal number") from exc if tag == "f": if not isinstance(arg, str) or arg not in _FLOAT_TAGS: raise SandboxCodecError(f"unknown sandbox float tag {arg!r}") @@ -1032,6 +1054,7 @@ def encode_boot( forbidden: Sequence[str], mem_mb: int | None, code_sets: object, + broker: bool = False, ) -> bytes: """The one-per-spawn bootstrap frame. @@ -1039,7 +1062,12 @@ def encode_boot( from those bytes and reads nothing from disk (vault BACKLOG #2596). ``env`` is deliberately NOT marshalled: the worker's load takes only the snapshot and the worker - never read it — a dead field on the wire is a field nobody validates.""" + never read it — a dead field on the wire is a field nobody validates. + + ``broker`` tells the worker to ask the parent for a Handler's ``db_lookup``/``fhir_lookup`` + (``mode=isolated``, ADR 0147) rather than refuse it in the child. It is a request for a protocol, + not a grant: the parent answers a ``lookup`` frame only on a session whose own policy is + isolated, so a worker that claims it on any other session is dropped.""" blobs = _Blobs() header = _envelope( { @@ -1049,6 +1077,7 @@ def encode_boot( "forbidden": list(forbidden), "mem_mb": None if mem_mb is None else int(mem_mb), "code_sets": enc_code_sets(code_sets, blobs), + "broker": bool(broker), }, "boot", ) @@ -1063,6 +1092,7 @@ class Boot: forbidden: tuple[str, ...] mem_mb: int | None code_sets: dict[str, CodeSet] | None + broker: bool = False def decode_boot(body: bytes) -> Boot: @@ -1074,12 +1104,14 @@ def decode_boot(body: bytes) -> Boot: ) mem_mb = None if header.get("mem_mb") is None else _req_int(header.get("mem_mb"), "mem_mb") code_sets = dec_code_sets(header.get("code_sets"), reader) + broker = _req_bool(header.get("broker"), "boot broker") reader.finish() return Boot( snapshot=snapshot, forbidden=forbidden, mem_mb=mem_mb, code_sets=code_sets, + broker=broker, ) @@ -1267,14 +1299,7 @@ def decode_response(body: bytes, *, request_id: str, phase: str, name: str) -> R :meth:`sandbox.SandboxSession.dispatch` supplies the other half — an unsolicited frame queued before or after a dispatch is fatal to the worker — so a pre-staged answer has no window at all.""" header, reader, kind = _open(body, ("ok", "fail")) - if _req_str(header.get("id"), "response id") != request_id: - raise SandboxCodecError( - f"sandbox response correlation mismatch (expected request {request_id!r})" - ) - if _req_str(header.get("phase"), "response phase") != phase: - raise SandboxCodecError(f"sandbox response phase mismatch (expected {phase!r})") - if _req_str(header.get("name"), "response name") != name: - raise SandboxCodecError(f"sandbox response name mismatch (expected {name!r})") + _check_binding(header, request_id, phase, name) if kind == "ok": if "result" not in header: raise SandboxCodecError("sandbox 'ok' frame carries no result") @@ -1287,3 +1312,234 @@ def decode_response(body: bytes, *, request_id: str, phase: str, name: str) -> R error = reader.text(header.get("error")) reader.finish() return Response(ok=False, result=None, kind=fail_kind, error=error) + + +# --- the lookup broker (mode=isolated, ADR 0147) ------------------------------ +# While a Handler runs at mode=isolated, the worker may write ``lookup`` frames before its answer: one +# per db_lookup/fhir_lookup call, each answered by one ``lookupok``/``lookupfail`` frame from the +# parent before the worker writes anything else. Each carries the dispatch's ``(id, phase, name)`` +# triple, so it is bound to the dispatch in flight (ADR 0147 AC-6), and a ``seq`` that starts at 1 per +# dispatch and rises by one, so a reply cannot be taken for another call's. + +#: The two lookup kinds a ``lookup`` frame may name. +LOOKUP_DB: Final = "db" +LOOKUP_FHIR: Final = "fhir" + + +@dataclass(frozen=True) +class LookupRequest: + """One decoded ``lookup`` frame (parent side). ``statement`` is set for a db lookup and + ``query`` for a fhir one; ``params`` is rebuilt as the in-process call would have received it.""" + + seq: int + kind: str + connection: str + statement: str | None + query: str | None + params: Any + + +def _enc_fhir_params(params: object, blobs: _Blobs) -> Any: + """A ``fhir_lookup`` ``params`` mapping in its own small grammar: ``[key, is_list, items]`` per + key, each item ``{"s": str}``, ``{"k": [system, code]}`` or ``{"r": raw}``. Only the three kinds + :mod:`messagefoundry.fhirsearch` takes cross; the parent's own executor screens their content.""" + if params is None: + return None + if not isinstance(params, Mapping): + raise SandboxCodecError( + f"fhir_lookup params must be a mapping, not {type(params).__name__}" + ) + out: list[Any] = [] + for key, value in params.items(): + if not isinstance(key, str): + raise SandboxCodecError("a fhir_lookup params key must be a str") + is_list = isinstance(value, list) + items: list[Any] = [] + for one in value if is_list else [value]: + if isinstance(one, str): + items.append({"s": blobs.ref(one)}) + elif isinstance(one, FhirToken): + items.append({"k": [blobs.ref(one.system), blobs.ref(one.code)]}) + elif isinstance(one, FhirRaw): + items.append({"r": blobs.ref(one.value)}) + else: + # The words fhirsearch's own refusal uses; the value is withheld, as there. + raise SandboxCodecError( + f"FHIR search parameter {key!r}: a search value must be a str (data), a " + f"FhirToken(system, code), or a FhirRaw(...) author-syntax value -- got " + f"{type(one).__name__}. (The value is withheld: it may carry PHI.)" + ) + out.append([key, is_list, items]) + return out + + +def _dec_fhir_params(node: Any, reader: _Reader) -> dict[str, Any] | None: + if node is None: + return None + out: dict[str, Any] = {} + for entry in _req_list(node, "fhir_lookup params"): + if len(_req_list(entry, "fhir_lookup param")) != 3: + raise SandboxCodecError("a fhir_lookup param must be [key, is_list, items]") + key = _req_str(entry[0], "fhir_lookup param key") + if key in out: + raise SandboxCodecError(f"fhir_lookup param {key!r} appears twice") + is_list = _req_bool(entry[1], "fhir_lookup param is_list") + items: list[Any] = [] + for item in _req_list(entry[2], "fhir_lookup param items"): + obj = _req_obj(item, "fhir_lookup param item") + if len(obj) != 1: + raise SandboxCodecError("a fhir_lookup param item must carry exactly one tag") + ((tag, arg),) = obj.items() + if tag == "s": + items.append(reader.text(arg)) + elif tag == "k": + pair = _req_list(arg, "FhirToken") + if len(pair) != 2: + raise SandboxCodecError("a FhirToken must be [system, code]") + items.append(FhirToken(reader.text(pair[0]), reader.text(pair[1]))) + elif tag == "r": + items.append(FhirRaw(reader.text(arg))) + else: + raise SandboxCodecError(f"unknown fhir_lookup param tag {tag!r}") + if not is_list and len(items) != 1: + raise SandboxCodecError("a single fhir_lookup param must carry exactly one item") + out[key] = items if is_list else items[0] + return out + + +def encode_lookup( + *, + request_id: str, + phase: str, + name: str, + seq: int, + kind: str, + connection: object, + target: object, + params: object, +) -> bytes: + """A ``lookup`` frame (child side). ``target`` is the statement of a db lookup or the query of a + fhir one. Raises :class:`SandboxCodecError` for an argument outside the grammar, which the worker + turns into the lookup's own error type, as the in-process call would fail on a bad argument.""" + blobs = _Blobs() + if kind == LOOKUP_DB: + if params is not None and not isinstance(params, Mapping): + raise SandboxCodecError( + f"db_lookup params must be a mapping or None, not {type(params).__name__}" + ) + enc_params = None if params is None else enc_value(dict(params), blobs) + elif kind == LOOKUP_FHIR: + enc_params = _enc_fhir_params(params, blobs) + else: + raise SandboxCodecError(f"unknown lookup kind {kind!r}") + header = _envelope( + { + "id": request_id, + "phase": phase, + "name": name, + "seq": seq, + "kind": kind, + "connection": blobs.ref(connection), + "target": blobs.ref(target), + "params": enc_params, + }, + "lookup", + ) + return encode_frame(header, blobs.items) + + +def _check_binding(header: dict[str, Any], request_id: str, phase: str, name: str) -> None: + """Refuse a frame whose ``(id, phase, name)`` is not the dispatch's (see :func:`decode_response`).""" + if _req_str(header.get("id"), "response id") != request_id: + raise SandboxCodecError( + f"sandbox response correlation mismatch (expected request {request_id!r})" + ) + if _req_str(header.get("phase"), "response phase") != phase: + raise SandboxCodecError(f"sandbox response phase mismatch (expected {phase!r})") + if _req_str(header.get("name"), "response name") != name: + raise SandboxCodecError(f"sandbox response name mismatch (expected {name!r})") + + +def decode_answer( + body: bytes, *, request_id: str, phase: str, name: str +) -> Response | LookupRequest: + """:func:`decode_response`, plus the ``lookup`` frame a worker may write mid-dispatch at + ``mode=isolated``. Only an isolated session calls this; every other one calls + :func:`decode_response`, which refuses a ``lookup`` frame as an unexpected frame type, so no other + mode can be talked into brokering a lookup.""" + header, reader, kind = _open(body, ("ok", "fail", "lookup")) + if kind != "lookup": + return decode_response(body, request_id=request_id, phase=phase, name=name) + _check_binding(header, request_id, phase, name) + seq = _req_int(header.get("seq"), "lookup seq") + lookup_kind = header.get("kind") + if lookup_kind not in (LOOKUP_DB, LOOKUP_FHIR): + raise SandboxCodecError(f"unknown lookup kind {lookup_kind!r}") + # Walk order matches encode_lookup: connection, target, then params. + connection = reader.text(header.get("connection")) + target = reader.text(header.get("target")) + params: Any + if lookup_kind == LOOKUP_DB: + node = header.get("params") + params = None if node is None else dec_value(node, reader) + if params is not None and not isinstance(params, dict): + raise SandboxCodecError("db_lookup params must decode to a mapping") + else: + params = _dec_fhir_params(header.get("params"), reader) + reader.finish() + return LookupRequest( + seq=seq, + kind=lookup_kind, + connection=connection, + statement=target if lookup_kind == LOOKUP_DB else None, + query=target if lookup_kind == LOOKUP_FHIR else None, + params=params, + ) + + +def encode_lookup_ok(*, request_id: str, phase: str, name: str, seq: int, result: object) -> bytes: + """The parent's answer to one ``lookup`` frame: the rows of a db lookup or the dict of a fhir one, + in the closed value grammar. Raises :class:`SandboxCodecError` for a value outside it.""" + blobs = _Blobs() + header = _envelope( + { + "id": request_id, + "phase": phase, + "name": name, + "seq": seq, + "result": enc_value(result, blobs), + }, + "lookupok", + ) + return encode_frame(header, blobs.items) + + +def encode_lookup_fail(*, request_id: str, phase: str, name: str, seq: int, error: str) -> bytes: + """The parent's refusal of one ``lookup`` frame. The worker raises it in the Handler as the + lookup's own error type, so a Handler that catches the error sees what ``mode=off`` shows it.""" + blobs = _Blobs() + header = _envelope( + {"id": request_id, "phase": phase, "name": name, "seq": seq, "error": blobs.ref(error)}, + "lookupfail", + ) + return encode_frame(header, blobs.items) + + +def decode_lookup_reply( + body: bytes, *, request_id: str, phase: str, name: str, seq: int +) -> tuple[bool, object, str]: + """``(ok, result, error)`` for one ``lookupok``/``lookupfail`` frame (child side), bound to the + dispatch and to the ``seq`` of the lookup it answers.""" + header, reader, kind = _open(body, ("lookupok", "lookupfail")) + _check_binding(header, request_id, phase, name) + if _req_int(header.get("seq"), "lookup reply seq") != seq: + raise SandboxCodecError(f"sandbox lookup reply is out of sequence (expected {seq})") + if kind == "lookupfail": + error = reader.text(header.get("error")) + reader.finish() + return False, None, error + if "result" not in header: + raise SandboxCodecError("sandbox 'lookupok' frame carries no result") + result = dec_value(header["result"], reader) + reader.finish() + return True, result, "" diff --git a/messagefoundry/pipeline/_sandbox_worker.py b/messagefoundry/pipeline/_sandbox_worker.py index 54a16a2e2a..a18012941c 100644 --- a/messagefoundry/pipeline/_sandbox_worker.py +++ b/messagefoundry/pipeline/_sandbox_worker.py @@ -22,7 +22,9 @@ re-establishes the run-scoped context providers for the phase (substituting the boot-carried code sets, which the per-dispatch frame deliberately does not repeat), runs the function, and **describes** the result back as ``ok`` — or ``fail`` with ``kind`` ``denied`` (forbidden import, a - live ``db_lookup``/``fhir_lookup``, an undescribable result), ``error`` (a plain handler raise) or + live ``db_lookup``/``fhir_lookup`` at ``mode=subprocess``), ``error`` (a plain handler raise, an + undescribable result, or at ``mode=isolated`` a lookup error, the lookup having been asked of the + parent through :class:`_Broker` while the Handler ran) or ``memory`` (a ``MemoryError``, after which the worker exits so the parent starts a fresh one). The reply echoes the request's ``id``, ``phase`` and ``name`` so the parent can prove the frame answers the call it made. @@ -43,6 +45,7 @@ import logging import os import sys +import threading from dataclasses import replace from typing import Any @@ -205,8 +208,113 @@ def _install_import_guard(forbidden: tuple[str, ...]) -> None: sys.meta_path.insert(0, _ForbiddenImportFinder(forbidden)) -def _run_one(registry: Any, req: Any, code_sets: Any) -> tuple[bool, object, str, str]: - """Execute one router/handler request; return ``(ok, result, kind, error)``. Never raises.""" +class _Broker: + """The worker's half of the ``mode=isolated`` lookup broker (ADR 0147). + + Its two runners stand in for the engine's while a Handler runs: each writes one ``lookup`` frame + to the parent and blocks on the parent's one reply, so a ``db_lookup``/``fhir_lookup`` call + returns what the parent's own runner returned, or raises the lookup's own error type with the + parent's reason. The parent decides every call; this side only asks. + + Calls are serialized under one lock, because the pipe carries one exchange at a time. A call made + while no dispatch is in flight, such as from a thread a Handler left running, is refused rather + than written: the parent would read the frame as unsolicited and drop the worker, and its read of + stdin would take the next request frame as its reply. A desynchronized or closed pipe marks the + broker broken, and :func:`main` exits after the dispatch, so the parent starts a fresh worker.""" + + def __init__(self, stdin: Any, stdout: Any) -> None: + self._stdin = stdin + self._stdout = stdout + self._lock = threading.Lock() + self._req: Any = None + self._seq = 0 + self.broken = False + + def begin(self, req: Any) -> None: + with self._lock: + self._req, self._seq = req, 0 + + def end(self) -> None: + """Close the dispatch. Taken under the lock, so a lookup still in flight on another thread + finishes, inside the dispatch, before the answer frame is written.""" + with self._lock: + self._req = None + + def db(self, connection: str, statement: str, params: Any = None) -> Any: + from messagefoundry.config.db_lookup import DbLookupError + from messagefoundry.pipeline._sandbox_codec import LOOKUP_DB + + return self._ask(LOOKUP_DB, DbLookupError, "db_lookup", connection, statement, params) + + def fhir(self, connection: str, query: str, params: Any = None) -> Any: + from messagefoundry.config.fhir_lookup import FhirLookupError + from messagefoundry.pipeline._sandbox_codec import LOOKUP_FHIR + + return self._ask(LOOKUP_FHIR, FhirLookupError, "fhir_lookup", connection, query, params) + + def _ask( + self, + kind: str, + error: type[Exception], + fn: str, + connection: object, + target: object, + params: object, + ) -> Any: + from messagefoundry.pipeline import _sandbox_codec as codec + from messagefoundry.pipeline._sandbox_codec import SandboxError + from messagefoundry.pipeline.sandbox import _read_frame_bytes, _write_frame + + with self._lock: + req = self._req + if req is None or self.broken: + raise error(f"{fn}: no Handler dispatch is in flight in this sandbox worker") + self._seq += 1 + seq = self._seq + try: + frame = codec.encode_lookup( + request_id=req.request_id, + phase=req.phase, + name=req.name, + seq=seq, + kind=kind, + connection=connection, + target=target, + params=params, + ) + except SandboxError as exc: + # A bad argument fails as the call would in-process: the lookup's own error, and the + # pipe untouched. Nothing was written. + raise error(f"{fn}: its arguments cannot cross the sandbox: {exc}") from None + try: + _write_frame(self._stdout, frame) + body = _read_frame_bytes(self._stdin) + if body is None: + raise SandboxError("the engine closed the pipe") + ok, result, reason = codec.decode_lookup_reply( + body, request_id=req.request_id, phase=req.phase, name=req.name, seq=seq + ) + except (OSError, SandboxError) as exc: + # An over-cap frame raises before a byte is written, so only a pipe the parent + # abandoned, or a reply that does not answer this call, gets here. Either way the + # pipe no longer lines up with the protocol. + self.broken = True + raise error(f"{fn}: the sandbox lookup broker failed: {exc}") from None + if not ok: + raise error(reason) + return result + + +def _run_one( + registry: Any, req: Any, code_sets: Any, broker: _Broker | None = None +) -> tuple[bool, object, str, str]: + """Execute one router/handler request; return ``(ok, result, kind, error)``. Never raises. + + ``broker`` is set at ``mode=isolated`` (ADR 0147): a Handler's ``db_lookup``/``fhir_lookup`` + is then asked of the parent, and an error it raises is that Handler's error, as at ``mode=off``. + Without it, both lookups are refused in the worker, as ADR 0087 built them.""" + from messagefoundry.config import db_lookup as db_lookup_mod + from messagefoundry.config import fhir_lookup as fhir_lookup_mod from messagefoundry.config.db_lookup import DbLookupError from messagefoundry.config.fhir_lookup import FhirLookupError from messagefoundry.config.run_context import run_contexts @@ -243,8 +351,15 @@ def _run_one(registry: Any, req: Any, code_sets: Any) -> tuple[bool, object, str if fn is None: return False, None, "error", f"no such {phase} {name!r} in registry" + # The brokered runners exist only for a Handler: a Router or an accepts= predicate finds no + # runner, so its lookup raises the "unavailable here" error it raises at mode=off. + brokered = broker is not None and phase == "transform" try: - with run_contexts(run_context, phase=phase_key): + with ( + run_contexts(run_context, phase=phase_key), + db_lookup_mod.activated(broker.db if brokered and broker else None), + fhir_lookup_mod.activated(broker.fhir if brokered and broker else None), + ): result = fn(req.payload) if phase == "transform": # Materialise a CONTAINER return here, INSIDE the run context (BACKLOG #341). A @@ -269,8 +384,13 @@ def _run_one(registry: Any, req: Any, code_sets: Any) -> tuple[bool, object, str # ``[sandbox].mode`` never change the routing decision (ADR 0087). result = bool(result) except (DbLookupError, FhirLookupError) as exc: + if broker is not None: + # mode=isolated: the lookup ran, or was refused, through the parent, so its error is the + # Handler's error exactly as at mode=off. A Router's lookup lands here too, with the + # "unavailable here" text mode=off gives it. + return False, None, "error", _error_text(exc) # db_lookup/fhir_lookup bridge back onto the engine event loop (run_coroutine_threadsafe), - # which a subprocess boundary breaks — forbidden + fail-closed for this PR (ADR 0087). + # which a subprocess boundary breaks — forbidden + fail-closed at mode=subprocess (ADR 0087). return ( False, None, @@ -303,13 +423,13 @@ def _error_text(exc: BaseException) -> str: return type(exc).__name__ -def _respond(registry: Any, req: Any, code_sets: Any) -> bytes: +def _respond(registry: Any, req: Any, code_sets: Any, broker: _Broker | None = None) -> bytes: """Build the response frame for one request. Never raises — an undescribable result is reported — except ``MemoryError``, which :func:`main` answers.""" from messagefoundry.pipeline import _sandbox_codec as codec from messagefoundry.pipeline._sandbox_codec import SandboxError - ok, result, kind, error = _run_one(registry, req, code_sets) + ok, result, kind, error = _run_one(registry, req, code_sets, broker) if ok: try: return codec.encode_ok( @@ -377,7 +497,13 @@ def main() -> int: except (OSError, SandboxError): return 1 + # mode=isolated (ADR 0147): a Handler's lookups are asked of the parent over this same pipe. + broker = _Broker(stdin, stdout) if boot.broker else None while True: + if broker is not None and broker.broken: + # A lookup exchange went out of step with the protocol; the next request frame cannot be + # trusted to line up either. The parent reads EOF and starts a fresh worker. + return 1 frame = _read_frame_bytes(stdin) if frame is None: return 0 # parent closed the pipe — clean shutdown @@ -389,14 +515,18 @@ def main() -> int: # dead-letter) and respawns a clean child, rather than hanging to the wall cap. log.error("sandbox worker: undecodable request frame (%s)", exc) return 1 + if broker is not None: + broker.begin(req) try: - resp: bytes | None = _respond(registry, req, boot.code_sets) + resp: bytes | None = _respond(registry, req, boot.code_sets, broker) except MemoryError as exc: # Caught here rather than around the Handler alone, so a result too big to encode is # answered the same way. Only the text is kept: the reply is built AFTER this block, # because until it ends the traceback keeps the Handler's frames, and the memory their # locals hold, alive. resp, detail = None, _error_text(exc) + if broker is not None: + broker.end() # before the answer: no lookup frame may follow it if resp is None: # A call that ran out of memory may leave memory held (a module global, a cache, a # fragmented heap), and the next innocent message would pay for it. Answer, then exit so diff --git a/messagefoundry/pipeline/dryrun.py b/messagefoundry/pipeline/dryrun.py index 97bfbec101..392c40a907 100644 --- a/messagefoundry/pipeline/dryrun.py +++ b/messagefoundry/pipeline/dryrun.py @@ -339,7 +339,7 @@ def _accepted( if pred is None: kept.append(hname) continue - if sandbox is not None and sandbox.mode is SandboxMode.SUBPROCESS: + if sandbox is not None and sandbox.mode is not SandboxMode.OFF: # ADR 0087 parity: the predicate is user code and must run under the SAME isolation as the # Router/Handler it sits beside. Evaluating it here in the parent would let a predicate # reach the imports and resources the sandbox exists to deny. @@ -430,7 +430,7 @@ def route_only( # its "not even an allocation" promise rather than paying for a seam it never crosses. origin: tuple[str | bytes, str] | None = None raw_result: list[str] | str | None - if sandbox is not None and sandbox.mode is SandboxMode.SUBPROCESS: + if sandbox is not None and sandbox.mode is not SandboxMode.OFF: if derived: origin = (raw, ic.content_type.value) raw_result = cast( @@ -524,7 +524,7 @@ def transform_one( if payload is None: payload = _payload(raw, content_type) raw_result: HandlerResult - if sandbox is not None and sandbox.mode is SandboxMode.SUBPROCESS: + if sandbox is not None and sandbox.mode is not SandboxMode.OFF: raw_result = cast( "HandlerResult", run_sandboxed( diff --git a/messagefoundry/pipeline/engine.py b/messagefoundry/pipeline/engine.py index d49569eaa8..a67328f2b1 100644 --- a/messagefoundry/pipeline/engine.py +++ b/messagefoundry/pipeline/engine.py @@ -1438,20 +1438,14 @@ def add_registry(self, registry: Registry) -> RegistryRunner: """Run a code-first Connection/Router/Handler graph (one runner for the whole graph).""" # ADR 0087 (#197): render [sandbox] into the runner's policy + the (config_dir, env) source a # subprocess worker uses to re-load the graph. mode=off (the default) makes _sandbox_for a - # no-op — byte-identical, no child ever spawned; mode=subprocess spawns one persistent worker - # child per inbound that dispatches. ONE policy for the WHOLE graph: [sandbox] is engine-wide + # no-op — byte-identical, no child ever spawned; mode=subprocess or mode=isolated spawns one + # persistent worker child per inbound that dispatches. ONE policy for the WHOLE graph: [sandbox] is engine-wide # and no connection carries a per-connection sandbox field, so the mode chosen here governs # every Router and Handler in this process. - from messagefoundry.pipeline.sandbox import SandboxMode, SandboxPolicy - - _sb = self._sandbox_settings - sandbox_policy = SandboxPolicy( - mode=SandboxMode(_sb.mode), - wall_seconds=_sb.wall_seconds, - mem_mb=_sb.mem_mb, - startup_seconds=_sb.startup_seconds, - pass_environment=_sb.pass_environment, - ) + from messagefoundry.pipeline.sandbox import policy_from_settings + + # mode=isolated also reads [egress]: its lookup broker refuses a lookup whose list is empty. + sandbox_policy = policy_from_settings(self._sandbox_settings, self._egress_settings) sandbox_config_source = ( str(self.config_dir) if self.config_dir is not None else None, self._active_environment, diff --git a/messagefoundry/pipeline/sandbox.py b/messagefoundry/pipeline/sandbox.py index aba52222a3..b07d90976e 100644 --- a/messagefoundry/pipeline/sandbox.py +++ b/messagefoundry/pipeline/sandbox.py @@ -32,6 +32,14 @@ child's own re-read of ``codesets/`` is never authoritative), runs the function, and describes its result back. The worker is *long-lived* (one child per inbound), never a per-message fork — a fork per message would destroy the throughput target. +* ``isolated`` (ADR 0147, vault BACKLOG #3131) — the ``subprocess`` worker, with a Handler's + ``db_lookup``/``fhir_lookup`` brokered through the parent instead of refused: the worker writes a + ``lookup`` frame mid-dispatch and the parent answers it with the runner the in-process path uses + (:meth:`SandboxSession._serve_lookup`). The parent refuses one bound to a Router or ``accepts=`` + dispatch, and one whose ``[egress]`` list is empty, even under the ``block_unlisted_outbound`` + opt-out. **No OS-level confinement yet:** ADR 0147's Landlock/seccomp and AppContainer limbs are + unbuilt, so this mode confines exactly what ``subprocess`` does. Every check that means "runs in a + worker" reads ``is not SandboxMode.OFF``, so this mode takes every worker branch. **Both legs of the pipe are untrusted by contract.** The child runs exactly the code the sandbox exists to distrust: while it lives, a grandchild it spawns inherits fd 1 (the response pipe) and can @@ -61,8 +69,8 @@ On top of that address-space boundary the child adds defence-in-depth: a forbidden-import guard (``socket``/store/crypto), a wall-clock cap enforced by the parent (plus a POSIX memory cap when available), and a fail-closed refusal of the live -``db_lookup``/``fhir_lookup`` bridges (they re-enter the event loop, which a process boundary -breaks — forwarding them over IPC is a documented next-phase residual). The import guard is +``db_lookup``/``fhir_lookup`` bridges at ``mode=subprocess`` (they re-enter the event loop, which a +process boundary breaks; ``mode=isolated`` forwards them over the pipe instead). The import guard is **defence-in-depth only** — a module imported before it goes up keeps a live reference, so the address-space boundary and the codec are the load-bearing controls. @@ -102,15 +110,20 @@ from messagefoundry import proctree from messagefoundry.childenv import python_child_argv, worker_environment from messagefoundry.config.code_sets import CodeSet +from messagefoundry.config.db_lookup import DbLookupError, db_lookup +from messagefoundry.config.fhir_lookup import FhirLookupError, fhir_lookup from messagefoundry.config.run_context import RunContext from messagefoundry.config.settings import ( INSECURE_CONFIG_SOURCE_ESCAPE_ENV, + EgressSettings, + SandboxSettings, security_enforcement_env_names, ) from messagefoundry.config.snapshot import ConfigSnapshot from messagefoundry.controlchars import scrub_control_chars from messagefoundry.pipeline import _sandbox_codec as codec from messagefoundry.pipeline._sandbox_codec import GraphShape, SandboxCodecError, SandboxError +from messagefoundry.redaction import safe_exc __all__ = [ "SandboxMode", @@ -124,6 +137,7 @@ "graph_shape", "graph_differences", "run_sandboxed", + "policy_from_settings", "DEFAULT_FORBIDDEN_MODULES", "WORKER_MODULE", ] @@ -161,6 +175,10 @@ class SandboxMode(str, enum.Enum): # noqa: UP042 # docstring's mode bullets; [sandbox].mode is engine-wide, so that escape is too). OFF = "off" # in-process, byte-identical, zero overhead (default) SUBPROCESS = "subprocess" # persistent per-inbound worker child + # The SUBPROCESS worker, with db_lookup/fhir_lookup brokered through the parent (ADR 0147). Every + # check that means "runs in a worker" is `is not SandboxMode.OFF`, so this mode can never take an + # in-process branch that only SUBPROCESS was excluded from (vault BACKLOG #3131). + ISOLATED = "isolated" @dataclass(frozen=True) @@ -193,6 +211,33 @@ class SandboxPolicy: #: ``[sandbox].pass_environment``: the extra variable NAMES the worker is given. Settings load #: refuses the engine's own; this dataclass carries what it let through. pass_environment: tuple[str, ...] = () + #: ``[egress].allowed_db`` and ``[egress].allowed_http``, read only by the parent's lookup broker + #: at ``mode=isolated`` (ADR 0147). The broker refuses a ``db_lookup`` while the first is empty + #: and a ``fhir_lookup`` while the second is, even under the audited opt-out + #: ``[security].block_unlisted_outbound = false``, where an empty list would otherwise mean + #: unrestricted (owner ruling 2026-10-27, made under delegation; vault BACKLOG #3131). The hosts + #: themselves are checked where the in-process path checks them, when the lookup executors are + #: built, so the broker inherits that check rather than repeating it. Empty by default, so a + #: policy built without them brokers nothing. Never sent to the worker. + lookup_allowed_db: tuple[str, ...] = () + lookup_allowed_http: tuple[str, ...] = () + + +def policy_from_settings(settings: SandboxSettings, egress: EgressSettings) -> SandboxPolicy: + """The :class:`SandboxPolicy` the engine renders from ``[sandbox]`` and ``[egress]``. + + ``SandboxMode(settings.mode)`` raises ``ValueError`` on a mode it does not know, so a value + that reached this point without the settings model's own check still refuses to start rather + than being read as some other mode.""" + return SandboxPolicy( + mode=SandboxMode(settings.mode), + wall_seconds=settings.wall_seconds, + mem_mb=settings.mem_mb, + startup_seconds=settings.startup_seconds, + pass_environment=settings.pass_environment, + lookup_allowed_db=tuple(egress.allowed_db), + lookup_allowed_http=tuple(egress.allowed_http), + ) def graph_shape(registry: Any) -> GraphShape: @@ -211,6 +256,9 @@ def graph_shape(registry: Any) -> GraphShape: #: How a refusal names a difference in the config modules' bytes alone. _CONTENT_DIFFERS: Final = "config module content" +#: The accessor each lookup kind names, for the broker's refusals and log lines. +_LOOKUP_FN: Final = {codec.LOOKUP_DB: "db_lookup", codec.LOOKUP_FHIR: "fhir_lookup"} + #: How many names a refusal lists per side of one part of the graph. _NAMES_SHOWN: Final = 3 @@ -649,6 +697,8 @@ def __init__( self._generation = 0 self._lock = threading.Lock() self._closed = False + # The (accessor, connection) pairs whose empty-[egress] refusal this session has logged. + self._lookup_refusals_logged: set[tuple[str, str]] = set() @property def mode(self) -> SandboxMode: @@ -677,6 +727,7 @@ def _spawn(self) -> None: forbidden=self.policy.forbidden_modules, mem_mb=self.policy.mem_mb, code_sets=self._code_sets, + broker=self.policy.mode is SandboxMode.ISOLATED, ) except (OSError, ValueError, SandboxError) as exc: # WiringError is a ValueError raise SandboxError(f"failed to bootstrap sandbox worker: {exc}") from exc @@ -952,6 +1003,171 @@ def _live_worker(self) -> subprocess.Popen[bytes]: assert proc is not None return proc + # -- the lookup broker (mode=isolated, ADR 0147) ---------------------------- + + def _await_answer( + self, proc: subprocess.Popen[bytes], request_id: str, phase: str, name: str + ) -> codec.Response: + """The dispatch's ``ok``/``fail`` frame, after serving every ``lookup`` frame the worker + writes before it at ``mode=isolated``. Any other mode decodes with + :func:`codec.decode_response`, which refuses a ``lookup`` frame, so only an isolated session + ever brokers a lookup and no worker can ask another session to. + + ``wall_seconds`` bounds the WHOLE dispatch, the lookups the parent serves for it included: + the cap stays the authoritative bound it is at ``mode=subprocess``, and a feed whose lookups + are slow needs a larger cap. A lookup the parent is serving is not interrupted when the cap + runs out; its own bound (the runner's result timeout) ends it, and the dispatch then fails + on the cap. So one dispatch can outlast ``wall_seconds`` by at most one lookup's bound.""" + brokered = self.policy.mode is SandboxMode.ISOLATED + deadline = time.monotonic() + self.policy.wall_seconds + expected_seq = 1 + while True: + try: + frame = self._responses.get(timeout=max(0.0, deadline - time.monotonic())) + except queue.Empty: + # Wall cap exceeded — the authoritative resource bound on every platform. Kill the + # runaway child (a busy-loop can't wedge intake) and fail closed. + self._kill(proc) + raise SandboxError( + f"sandbox {phase} {name!r} exceeded the {self.policy.wall_seconds}s wall cap" + ) from None + if frame is _EOF: + self._kill(proc) + raise SandboxError(f"sandbox worker crashed while running {phase} {name!r}") + try: + # Decoding happens HERE, on the dispatch thread, inside this try — not on the daemon + # reader thread, where a rejection would be a silent reader death and a wall-cap hang. + answer = ( + codec.decode_answer(frame, request_id=request_id, phase=phase, name=name) + if brokered + else codec.decode_response(frame, request_id=request_id, phase=phase, name=name) + ) + if isinstance(answer, codec.LookupRequest) and answer.seq != expected_seq: + raise SandboxCodecError( + f"sandbox lookup is out of sequence (expected {expected_seq})" + ) + except SandboxError as exc: + # A desynchronized or forged frame: the worker's stream can no longer be trusted to + # line up with our requests, so drop it and respawn on the next message. + self._kill(proc) + raise SandboxError( + f"sandbox {phase} {name!r} returned an invalid frame: {exc}" + ) from exc + if isinstance(answer, codec.Response): + return answer + expected_seq += 1 + reply = self._lookup_reply(request_id, phase, name, answer) + assert proc.stdin is not None + try: + _write_frame(proc.stdin, reply) + except (OSError, SandboxError) as exc: + self._kill(proc) + raise SandboxError( + f"sandbox {phase} {name!r}: could not answer its worker's lookup: {exc}" + ) from exc + if time.monotonic() > deadline: + self._kill(proc) + raise SandboxError( + f"sandbox {phase} {name!r} exceeded the {self.policy.wall_seconds}s wall cap, " + "which counts the lookups the engine served for it" + ) + + def _lookup_reply( + self, request_id: str, phase: str, name: str, lookup: codec.LookupRequest + ) -> bytes: + """The ``lookupok``/``lookupfail`` frame answering one ``lookup``. Never raises: the worker + is blocked on this reply, so a result that cannot cross is a ``lookupfail`` it can report.""" + ok, result, error = self._serve_lookup(phase, lookup) + if ok: + try: + body = codec.encode_lookup_ok( + request_id=request_id, phase=phase, name=name, seq=lookup.seq, result=result + ) + except SandboxError as exc: + # At least a column type outside the closed grammar, such as a UUID. + ok, error = ( + False, + f"{_LOOKUP_FN[lookup.kind]} result cannot cross the sandbox: {exc}", + ) + else: + if len(body) <= _MAX_FRAME: + return body + ok, error = ( + False, + ( + f"{_LOOKUP_FN[lookup.kind]} result is larger than a sandbox frame " + f"({len(body)} bytes)" + ), + ) + return codec.encode_lookup_fail( + request_id=request_id, phase=phase, name=name, seq=lookup.seq, error=error + ) + + def _serve_lookup(self, phase: str, lookup: codec.LookupRequest) -> tuple[bool, object, str]: + """Answer one brokered lookup in the parent: ``(ok, result, error)``. + + The answer comes from the same ``db_lookup``/``fhir_lookup`` accessor a Handler calls at + ``mode=off``, on this dispatch thread, so it resolves the runner the transform worker + published for this transform and nothing else (ADR 0147 AC-2). Where none is published, as on + a Router, in a dry run, on the inline fast path or in a graph that declares no lookup, it + raises exactly what it raises at ``mode=off``. The executor behind the runner was built + against ``[egress]``, so each host is checked there, as in-process. The slice-B seam: a + broker-only check of a lookup's own arguments goes between the refusal below and the call. + """ + refusal = self._lookup_refusal(phase, lookup) + if refusal is not None: + return False, None, refusal + try: + if lookup.kind == codec.LOOKUP_DB: + assert lookup.statement is not None + return True, db_lookup(lookup.connection, lookup.statement, lookup.params), "" + assert lookup.query is not None + return True, fhir_lookup(lookup.connection, lookup.query, lookup.params), "" + except (DbLookupError, FhirLookupError) as exc: + # PHI-free by both types' contract: a connection name and a routing-safe reason. + return False, None, str(exc) + except Exception as exc: # noqa: BLE001 — the worker waits on this reply; report, never raise + return False, None, f"{_LOOKUP_FN[lookup.kind]} failed: {safe_exc(exc)}" + + def _lookup_refusal(self, phase: str, lookup: codec.LookupRequest) -> str | None: + """Why the parent refuses a brokered lookup outright, or ``None`` to serve it. + + Two refusals, each fail closed. A lookup is answered only for a Handler, so a request bound + to a Router or an ``accepts=`` dispatch is refused here even if a worker sends one; the child + has no lookup runner in those phases either. And a lookup whose ``[egress]`` list is empty is + refused even under the audited opt-out ``[security].block_unlisted_outbound = false``, where + the in-process path would treat that empty list as unrestricted (owner ruling 2026-10-27, made + under delegation; vault BACKLOG #3131).""" + fn = _LOOKUP_FN[lookup.kind] + if phase != "transform": + return ( + f"{fn}({lookup.connection!r}) is unavailable here — the engine answers a brokered " + "lookup only for a Handler, never for a Router or an accepts= predicate (ADR 0147)" + ) + if lookup.kind == codec.LOOKUP_DB: + allowed, key = self.policy.lookup_allowed_db, "allowed_db" + else: + allowed, key = self.policy.lookup_allowed_http, "allowed_http" + if allowed: + return None + if (fn, lookup.connection) not in self._lookup_refusals_logged: + # Once per session and connection: the cause is configuration, so every later message + # would log the same line. Each refused message still fails, so none is dropped. + self._lookup_refusals_logged.add((fn, lookup.connection)) + log.warning( + "inbound %r: a brokered %s on %r was refused: [egress].%s is empty " + "([sandbox].mode=isolated, ADR 0147)", + self._inbound, + fn, + scrub_control_chars(lookup.connection)[:80], + key, + ) + return ( + f"{fn}({lookup.connection!r}) refused: [egress].{key} is empty, and " + "[sandbox].mode=isolated brokers a lookup only to a host that list names, whatever " + "[security].block_unlisted_outbound says (ADR 0147)" + ) + # -- dispatch ------------------------------------------------------------- def dispatch( @@ -1001,29 +1217,7 @@ def dispatch( # reset the worker. self._kill(proc) raise SandboxError(f"failed to marshal sandbox {phase} {name!r}: {exc}") from exc - try: - frame = self._responses.get(timeout=self.policy.wall_seconds) - except queue.Empty: - # Wall cap exceeded — the authoritative resource bound on every platform. Kill the - # runaway child (a busy-loop can't wedge intake) and fail closed. - self._kill(proc) - raise SandboxError( - f"sandbox {phase} {name!r} exceeded the {self.policy.wall_seconds}s wall cap" - ) from None - if frame is _EOF: - self._kill(proc) - raise SandboxError(f"sandbox worker crashed while running {phase} {name!r}") - try: - # Decoding happens HERE, on the dispatch thread, inside this try — not on the daemon - # reader thread, where a rejection would be a silent reader death and a wall-cap hang. - resp = codec.decode_response(frame, request_id=request_id, phase=phase, name=name) - except SandboxError as exc: - # A desynchronized or forged frame: the worker's stream can no longer be trusted to - # line up with our requests, so drop it and respawn on the next message. - self._kill(proc) - raise SandboxError( - f"sandbox {phase} {name!r} returned an invalid frame: {exc}" - ) from exc + resp = self._await_answer(proc, request_id, phase, name) # One request, one frame: a second FRAME already queued means the worker is writing frames # nobody asked for, so this answer is not trustworthy either. (A queued EOF is only a dead # peer — it reaps the worker and leaves this proven answer standing.) diff --git a/messagefoundry/pipeline/wiring_runner.py b/messagefoundry/pipeline/wiring_runner.py index 951cf91143..7d8d9a1c28 100644 --- a/messagefoundry/pipeline/wiring_runner.py +++ b/messagefoundry/pipeline/wiring_runner.py @@ -179,6 +179,7 @@ from messagefoundry.pipeline.reply_wait import ReplyRendezvous from messagefoundry.pipeline.sandbox import ( GraphShape, + SandboxError, SandboxMode, SandboxPolicy, SandboxSession, @@ -5488,8 +5489,10 @@ def close_all() -> None: def _sandbox_for(self, name: str) -> SandboxSession | None: """The persistent sandbox worker for inbound ``name`` (ADR 0087), or ``None`` to run in-process. - Returns ``None`` — the byte-identical in-process path — unless ``[sandbox].mode=subprocess`` - AND a config dir is known. The child re-loads the graph to look a function up by name, from + Returns ``None`` — the byte-identical in-process path — unless ``[sandbox].mode`` is + ``subprocess`` or ``isolated`` AND a config dir is known. With no config dir, ``isolated`` + raises :class:`SandboxError` rather than degrade, because the in-process path would run its + lookups unbrokered (vault BACKLOG #3131); the rest of this paragraph is about ``subprocess``. The child re-loads the graph to look a function up by name, from ``Registry.config_snapshot``, the bytes the served graph was loaded from, and never reads the directory (vault BACKLOG #2596). A graph with no snapshot hands the session ``Registry.source_dir``, else the constructor's ``sandbox_config_source``, which the session @@ -5510,6 +5513,15 @@ def _sandbox_for(self, name: str) -> SandboxSession | None: self._sandbox_config_source[0] if self._sandbox_config_source else None ) if cfg_dir is None: + if policy.mode is SandboxMode.ISOLATED: + # FAIL CLOSED. The in-process path would run this inbound's Handlers with the + # engine's own db_lookup/fhir_lookup runners, unbrokered, under a setting that + # asked for the broker (vault BACKLOG #3131). The message dead-letters instead. + raise SandboxError( + f"[sandbox].mode=isolated cannot run inbound {name!r}: its graph has no " + "config directory for a worker to load, and this mode never runs a Router " + "or Handler in the engine process" + ) return None env = self._sandbox_config_source[1] if self._sandbox_config_source else None # The engine's code-set tables travel once per spawn in the boot frame (not per dispatch), @@ -6373,11 +6385,14 @@ async def _activate_fusion(self) -> bool: # Rather than run user config code unsandboxed under a config that asked for a sandbox, fail # CLOSED to the async (sandboxed) path. An IPC round-trip inside a fused hop would negate the # fusion anyway, so degrading is the right trade — never a silent isolation bypass. - if self._sandbox_policy is not None and self._sandbox_policy.mode is SandboxMode.SUBPROCESS: + # `is not OFF`, never `is SUBPROCESS`: every worker mode is isolation this hop would bypass + # (mode=isolated included, vault BACKLOG #3131). + if self._sandbox_policy is not None and self._sandbox_policy.mode is not SandboxMode.OFF: log.warning( - "fuse_thread_hops is set but [sandbox].mode=subprocess: fusion runs Router/Handler/" + "fuse_thread_hops is set but [sandbox].mode=%s: fusion runs Router/Handler/" "accepts= code in-process (unsandboxed), so it is DISABLED — running the async sandboxed " - "pipeline path. Turn the sandbox off to use fusion, or leave fusion off to keep isolation." + "pipeline path. Turn the sandbox off to use fusion, or leave fusion off to keep isolation.", + self._sandbox_policy.mode.value, ) return False backend = self.store.backend diff --git a/tests/test_checks.py b/tests/test_checks.py index 7ab8a9b5c3..69ae493ff5 100644 --- a/tests/test_checks.py +++ b/tests/test_checks.py @@ -653,7 +653,7 @@ def test_no_expect_keeps_default_semantics(tmp_path: Path) -> None: @pytest.mark.parametrize("bridge", sorted(_LOOKUP_CALLS)) -@pytest.mark.parametrize("sandbox_mode", ["off", "subprocess"]) +@pytest.mark.parametrize("sandbox_mode", ["off", "subprocess", "isolated"]) def test_a_lookup_handler_fails_the_gate_in_every_sandbox_mode( tmp_path: Path, bridge: str, sandbox_mode: str ) -> None: diff --git a/tests/test_sandbox_isolated.py b/tests/test_sandbox_isolated.py new file mode 100644 index 0000000000..2228262fdc --- /dev/null +++ b/tests/test_sandbox_isolated.py @@ -0,0 +1,569 @@ +# SPDX-License-Identifier: AGPL-3.0-or-later +# Copyright (C) 2026 MessageFoundry Foundation, LLC and contributors +"""``[sandbox].mode = "isolated"`` (ADR 0147, vault BACKLOG #3131). + +Two properties, each pinned per pipeline phase and per mode: + +1. **Where the code runs.** Every check that means "runs in a sandbox worker" reads ``is not OFF``, + so the isolated mode takes the worker branch of the Router, the ``accepts=`` predicate and the + Handler, exactly as ``subprocess`` does. Before the conversion those checks read ``is + SUBPROCESS``, and an isolated session ran each phase in the engine process. +2. **The brokered lookups.** In the isolated mode a Handler's ``db_lookup``/``fhir_lookup`` is + asked of the parent over the closed-tag codec, and the parent answers it with the runner the + in-process path uses. The parent refuses a request outside the Handler phase, and refuses one + whose ``[egress]`` list is empty, whatever ``[security].block_unlisted_outbound`` says (owner + ruling of 2026-10-27, made under delegation). ``subprocess`` still refuses both lookups in the + worker, and ``off`` runs them in-process. + +Synthetic HL7 only.""" + +from __future__ import annotations + +import asyncio +import os +from decimal import Decimal +from pathlib import Path +from typing import Any + +import pytest + +from messagefoundry.config import db_lookup as db_lookup_mod +from messagefoundry.config import fhir_lookup as fhir_lookup_mod +from messagefoundry.config.run_context import RunContext +from messagefoundry.config.settings import EgressSettings, SandboxSettings +from messagefoundry.config.wiring import Registry, load_config +from messagefoundry.fhirsearch import FhirRaw, FhirToken +from messagefoundry.pipeline import _sandbox_codec as codec +from messagefoundry.pipeline.dryrun import route_only, transform_one +from messagefoundry.pipeline.sandbox import ( + SandboxError, + SandboxMode, + SandboxPolicy, + SandboxSession, + policy_from_settings, +) +from messagefoundry.store import MessageStatus + +RAW = "MSH|^~\\&|SEND|F|RECV|F|20240101120000||ADT^A01|MSG00001|P|2.3\rPID|1||900001||DOE^JANE\r" + +_GRAPH = """ +import os + +from messagefoundry import ( + inbound, outbound, router, handler, MLLP, Send, db_lookup, fhir_lookup, FhirToken, FhirRaw, +) +from messagefoundry.config.db_lookup import DbLookupError + +PARENT = __PARENT__ + +inbound("IB_I", MLLP(port=19411), router="r_where") +inbound("IB_RL", MLLP(port=19413), router="r_lookup") +outbound("OB_I", MLLP(host="127.0.0.1", port=19412)) + + +def _where(): + return "parent" if os.getpid() == PARENT else "child" + + +@router("r_where") +def r_where(msg): + # Names the handler for the process it ran in, so the routing decision says where it ran. + return "h_" + _where() + + +@router("r_lookup") +def r_lookup(msg): + db_lookup("LK", "SELECT 1 AS v", {}) # unavailable on a Router, in every mode + return "h_child" + + +def _in_child(msg): + return _where() == "child" + + +@handler("h_child", accepts=_in_child) +def h_child(msg): + return Send("OB_I", _where()) + + +@handler("h_parent") +def h_parent(msg): + return Send("OB_I", _where()) + + +@handler("h_db") +def h_db(msg): + rows = db_lookup("LK", "SELECT v, d FROM t WHERE k = :k", {"k": msg.field("PID-3"), "n": 7}) + return Send("OB_I", f"{rows[0]['v']}|{type(rows[0]['d']).__name__}|{rows[0]['d']}") + + +@handler("h_db_caught") +def h_db_caught(msg): + try: + db_lookup("LK", "SELECT 1 AS v", {}) + except DbLookupError as exc: + return Send("OB_I", "caught:" + str(exc)) + return Send("OB_I", "not refused") + + +@handler("h_fhir") +def h_fhir(msg): + bundle = fhir_lookup( + "FH", "Patient", {"identifier": FhirToken("MRN", "900001"), "_sort": [FhirRaw("-date"), "x"]} + ) + return Send("OB_I", bundle["id"]) +""" + + +@pytest.fixture +def graph(tmp_path: Path) -> tuple[Registry, str]: + (tmp_path / "graph.py").write_text( + _GRAPH.replace("__PARENT__", str(os.getpid())), encoding="utf-8" + ) + return load_config(tmp_path), str(tmp_path) + + +def _session( + config_dir: str, + mode: SandboxMode, + *, + allowed_db: tuple[str, ...] = ("db.example",), + allowed_http: tuple[str, ...] = ("fhir.example",), +) -> SandboxSession: + return SandboxSession( + SandboxPolicy( + mode=mode, + wall_seconds=30.0, + lookup_allowed_db=allowed_db, + lookup_allowed_http=allowed_http, + ), + inbound="IB_I", + config_dir=config_dir, + env=None, + graph=None, + ) + + +def _bodies(registry: Registry, hname: str, session: SandboxSession | None) -> list[str]: + deliveries, _, _, _ = transform_one( + registry, hname, RAW, sandbox=session, run_context=RunContext() + ) + return [str(d.payload) for d in deliveries] + + +# --- 1. where each phase runs, per mode ---------------------------------------------------------- + + +@pytest.mark.parametrize( + ("mode", "where"), + [ + (SandboxMode.OFF, "parent"), + (SandboxMode.SUBPROCESS, "child"), + (SandboxMode.ISOLATED, "child"), + ], +) +def test_router_accepts_and_handler_run_where_the_mode_says( + graph: tuple[Registry, str], mode: SandboxMode, where: str +) -> None: + registry, config_dir = graph + session = _session(config_dir, mode) + try: + # The Router names the handler for the process it ran in. h_child's accepts= predicate + # declines anywhere but a worker, so a Router run in a worker beside a predicate run in the + # engine routes nowhere: each of the two phases is pinned on its own. + names = route_only( + registry, registry.inbound["IB_I"], RAW, sandbox=session, run_context=RunContext() + ) + assert names == ["h_" + where] + # The Handler reports the process it ran in. + assert _bodies(registry, "h_parent", session) == [where] + finally: + session.close() + + +# --- 2. the brokered lookups, per mode ----------------------------------------------------------- + + +class _Runners: + """Stand-ins for the engine's lookup runners, published the way the transform worker publishes + the real ones: on the thread that runs ``transform_one``, which is where the broker answers.""" + + def __init__(self) -> None: + self.db_calls: list[tuple[str, str, Any]] = [] + self.fhir_calls: list[tuple[str, str, Any]] = [] + + def db(self, connection: str, statement: str, params: Any) -> list[dict[str, Any]]: + self.db_calls.append((connection, statement, params)) + return [{"v": "VAL", "d": Decimal("12.50")}] + + def fhir(self, connection: str, query: str, params: Any) -> dict[str, Any]: + self.fhir_calls.append((connection, query, params)) + return {"resourceType": "Bundle", "id": "BUNDLE-1", "total": 1} + + +@pytest.fixture +def runners() -> Any: + r = _Runners() + with db_lookup_mod.activated(r.db), fhir_lookup_mod.activated(r.fhir): + yield r + + +def test_isolated_brokers_db_lookup_through_the_parent( + graph: tuple[Registry, str], runners: _Runners +) -> None: + registry, config_dir = graph + session = _session(config_dir, SandboxMode.ISOLATED) + try: + bodies = _bodies(registry, "h_db", session) + finally: + session.close() + # The row the parent's runner returned reached the Handler, a Decimal still a Decimal. + assert bodies == ["VAL|Decimal|12.50"] + assert runners.db_calls == [("LK", "SELECT v, d FROM t WHERE k = :k", {"k": "900001", "n": 7})] + + +def test_isolated_brokers_fhir_lookup_with_its_search_kinds( + graph: tuple[Registry, str], runners: _Runners +) -> None: + registry, config_dir = graph + session = _session(config_dir, SandboxMode.ISOLATED) + try: + bodies = _bodies(registry, "h_fhir", session) + finally: + session.close() + assert bodies == ["BUNDLE-1"] + # Each value arrives as the kind the Handler wrote, so the parent's executor screens it as it + # would in-process: a FhirToken's code half, never a FhirRaw. + assert runners.fhir_calls == [ + ( + "FH", + "Patient", + {"identifier": FhirToken("MRN", "900001"), "_sort": [FhirRaw("-date"), "x"]}, + ) + ] + + +def test_mode_off_runs_the_lookup_in_process( + graph: tuple[Registry, str], runners: _Runners +) -> None: + registry, _ = graph + assert _bodies(registry, "h_db", None) == ["VAL|Decimal|12.50"] + assert len(runners.db_calls) == 1 + + +def test_subprocess_still_refuses_both_lookups_in_the_worker( + graph: tuple[Registry, str], runners: _Runners +) -> None: + registry, config_dir = graph + session = _session(config_dir, SandboxMode.SUBPROCESS) + try: + for hname in ("h_db", "h_fhir"): + with pytest.raises(SandboxError, match="db_lookup/fhir_lookup is forbidden"): + _bodies(registry, hname, session) + finally: + session.close() + assert runners.db_calls == [] and runners.fhir_calls == [] + + +@pytest.mark.parametrize( + ("hname", "empty", "key"), + [ + ("h_db", {"allowed_db": ()}, "allowed_db"), + ("h_fhir", {"allowed_http": ()}, "allowed_http"), + ], +) +def test_isolated_refuses_a_lookup_whose_egress_list_is_empty( + graph: tuple[Registry, str], + runners: _Runners, + caplog: pytest.LogCaptureFixture, + hname: str, + empty: dict[str, tuple[str, ...]], + key: str, +) -> None: + registry, config_dir = graph + session = _session(config_dir, SandboxMode.ISOLATED, **empty) + try: + with caplog.at_level("WARNING", logger="messagefoundry.pipeline.sandbox"): + for _ in range(2): + with pytest.raises(SandboxError, match=rf"\[egress\]\.{key} is empty"): + _bodies(registry, hname, session) + finally: + session.close() + # Refused in the parent, before any runner ran: the published runner saw nothing. + assert runners.db_calls == [] and runners.fhir_calls == [] + # Logged once for the session, not once per message. + refusals = [r for r in caplog.records if "was refused" in r.getMessage()] + assert len(refusals) == 1 and key in refusals[0].getMessage() + + +def test_a_refusal_reaches_the_handler_as_its_lookup_error( + graph: tuple[Registry, str], runners: _Runners +) -> None: + # A Handler that catches DbLookupError sees the parent's refusal as that error, as at mode=off. + registry, config_dir = graph + session = _session(config_dir, SandboxMode.ISOLATED, allowed_db=()) + try: + (body,) = _bodies(registry, "h_db_caught", session) + finally: + session.close() + assert body.startswith("caught:db_lookup('LK') refused: [egress].allowed_db is empty") + + +def test_a_lookup_with_no_runner_in_the_parent_fails_as_at_mode_off( + graph: tuple[Registry, str], +) -> None: + # No runner published: a dry run, the inline fast path, or a graph with no lookup connection. + registry, config_dir = graph + session = _session(config_dir, SandboxMode.ISOLATED) + try: + with pytest.raises(SandboxError, match="unavailable here"): + _bodies(registry, "h_db", session) + finally: + session.close() + + +@pytest.mark.parametrize("mode", [SandboxMode.SUBPROCESS, SandboxMode.ISOLATED]) +def test_a_router_lookup_is_refused_in_every_worker_mode( + graph: tuple[Registry, str], runners: _Runners, mode: SandboxMode +) -> None: + registry, config_dir = graph + session = _session(config_dir, mode) + try: + with pytest.raises(SandboxError): + route_only( + registry, registry.inbound["IB_RL"], RAW, sandbox=session, run_context=RunContext() + ) + finally: + session.close() + assert runners.db_calls == [] + + +def test_the_parent_refuses_a_lookup_bound_to_a_router_dispatch(tmp_path: Path) -> None: + # The worker has no runner on a Router, so it never asks; the parent refuses one anyway. + session = _session(str(tmp_path), SandboxMode.ISOLATED) + lookup = codec.LookupRequest( + seq=1, kind=codec.LOOKUP_DB, connection="LK", statement="SELECT 1", query=None, params=None + ) + for phase in ("router", "accepts"): + refusal = session._lookup_refusal(phase, lookup) + assert refusal is not None and "only for a Handler" in refusal + assert session._lookup_refusal("transform", lookup) is None + + +def test_a_lookup_frame_drops_a_subprocess_worker() -> None: + # Only an isolated session decodes a lookup frame; any other refuses it as a protocol violation. + frame = codec.encode_lookup( + request_id="r1", + phase="transform", + name="h", + seq=1, + kind=codec.LOOKUP_DB, + connection="LK", + target="SELECT 1", + params=None, + ) + with pytest.raises(codec.SandboxCodecError, match="unexpected sandbox frame type 'lookup'"): + codec.decode_response(frame, request_id="r1", phase="transform", name="h") + answer = codec.decode_answer(frame, request_id="r1", phase="transform", name="h") + assert isinstance(answer, codec.LookupRequest) and answer.statement == "SELECT 1" + # Bound to the dispatch in flight (ADR 0147 AC-6): another dispatch's id is refused. + with pytest.raises(codec.SandboxCodecError, match="correlation mismatch"): + codec.decode_answer(frame, request_id="r2", phase="transform", name="h") + + +def test_a_lookup_reply_is_bound_to_its_sequence() -> None: + reply = codec.encode_lookup_ok( + request_id="r1", phase="transform", name="h", seq=2, result=[{"a": Decimal("1")}] + ) + assert codec.decode_lookup_reply( + reply, request_id="r1", phase="transform", name="h", seq=2 + ) == (True, [{"a": Decimal("1")}], "") + with pytest.raises(codec.SandboxCodecError, match="out of sequence"): + codec.decode_lookup_reply(reply, request_id="r1", phase="transform", name="h", seq=1) + + +# --- 3. the mode setting fails closed -------------------------------------------------------------- + + +def test_policy_reads_the_egress_lists_for_the_broker() -> None: + policy = policy_from_settings( + SandboxSettings(mode="isolated"), + EgressSettings(deny_by_default=False, allowed_db=["db.example"], allowed_http=[]), + ) + assert policy.mode is SandboxMode.ISOLATED + assert policy.lookup_allowed_db == ("db.example",) and policy.lookup_allowed_http == () + + +def test_an_unknown_mode_refuses() -> None: + with pytest.raises(ValueError, match="mode"): + SandboxSettings(mode="isolate") # type: ignore[arg-type] + # Past the model's own check too: the engine's rendering refuses rather than guess a mode. + smuggled = SandboxSettings.model_construct(mode="bogus") + with pytest.raises(ValueError, match="bogus"): + policy_from_settings(smuggled, EgressSettings()) + + +# --- 4. the running engine: the split transform worker, per mode ---------------------------------- + +_RUNNER_GRAPH = """ +from messagefoundry import inbound, outbound, router, handler, File, Send, DatabaseLookup, db_lookup + +inbound("IB_L", File(directory={inbox!r}, poll_seconds=0.05), router="r") +inbound("IB_W", File(directory={inbox_w!r}, poll_seconds=0.05), router="r_w") +outbound("OB_L", File(directory={outbox!r})) +DatabaseLookup("LK", server="db.example", database="D") + + +@router("r") +def r(msg): + return "h" + + +@router("r_w") +def r_w(msg): + return "h_write" + + +@handler("h") +def h(msg): + rows = db_lookup("LK", "SELECT v FROM t WHERE k = :k", {{"k": msg.field("PID-3")}}) + return Send("OB_L", rows[0]["v"]) + + +@handler("h_write") +def h_write(msg): + db_lookup("LK", "INSERT INTO t (k) VALUES (:k)", {{"k": "1"}}) + return Send("OB_L", "wrote") +""" + + +def _runner_dirs(tmp_path: Path) -> dict[str, str]: + dirs = {key: tmp_path / key for key in ("inbox", "inbox_w", "outbox", "cfg")} + for path in dirs.values(): + path.mkdir() + return {key: str(path) for key, path in dirs.items()} + + +async def _run_one_message( + tmp_path: Path, + monkeypatch: pytest.MonkeyPatch, + mode: str, + egress: EgressSettings, + *, + inbound_name: str = "IB_L", +) -> tuple[str, list[str], Any]: + """Start a runner on the graph, push one message through ``inbound_name``, and return its final + status, what was delivered, and the fake pool, whose cursor records any statement that ran.""" + from messagefoundry.pipeline.wiring_runner import RegistryRunner + from messagefoundry.store import MessageStore, Stage + from tests.test_db_lookup import _patch_pool + + pool = _patch_pool(monkeypatch, rows=[("FROM-DB",)], columns=["v"]) + dirs = _runner_dirs(tmp_path) + (Path(dirs["cfg"]) / "graph.py").write_text(_RUNNER_GRAPH.format(**dirs), encoding="utf-8") + egress = egress.model_copy(update={"allowed_file_dirs": [dirs["outbox"]]}) + store = await MessageStore.open(tmp_path / "isolated.db") + runner = RegistryRunner( + load_config(dirs["cfg"]), + store, + sandbox_policy=policy_from_settings( + SandboxSettings.model_validate({"mode": mode, "wall_seconds": 60.0}), egress + ), + sandbox_config_source=(dirs["cfg"], None), + egress=egress, + ) + await runner.start() + try: + mid = await store.enqueue_ingress( + channel_id=inbound_name, raw=RAW, control_id="MSG00001", message_type="ADT^A01" + ) + runner._wake_lane(Stage.INGRESS, inbound_name) + terminal = {MessageStatus.PROCESSED.value, MessageStatus.ERROR.value} + status = "" + for _ in range(1200): + msg = await store.get_message(mid) + assert msg is not None + status = str(msg["status"]) + if status in terminal: + break + await asyncio.sleep(0.05) + finally: + await runner.stop() + await store.close() + delivered = [p.read_text(encoding="utf-8") for p in Path(dirs["outbox"]).iterdir()] + return status, delivered, pool + + +_LISTED = EgressSettings(allowed_db=["db.example"]) +_PROCESSED = MessageStatus.PROCESSED.value +_ERROR = MessageStatus.ERROR.value + + +@pytest.mark.parametrize( + ("mode", "expect", "ran"), + [("off", _PROCESSED, True), ("subprocess", _ERROR, False), ("isolated", _PROCESSED, True)], +) +async def test_the_transform_worker_runs_a_lookup_per_mode( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, mode: str, expect: str, ran: bool +) -> None: + status, delivered, pool = await _run_one_message(tmp_path, monkeypatch, mode, _LISTED) + assert status == expect + assert delivered == (["FROM-DB"] if ran else []) + assert (pool.cursor_obj.executed is not None) is ran + + +async def test_isolated_refuses_an_empty_allowed_db_under_the_audited_opt_out( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # block_unlisted_outbound = false makes an empty allowed_db unrestricted: the executor builds and + # mode=off would dial. The broker refuses anyway (owner ruling 2026-10-27, under delegation). + opt_out = EgressSettings(deny_by_default=False) + status, delivered, pool = await _run_one_message(tmp_path, monkeypatch, "isolated", opt_out) + assert (status, delivered) == (_ERROR, []) + assert pool.cursor_obj.executed is None + + +async def test_a_brokered_write_is_refused_by_the_executors_read_only_test( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + # The broker answers through the same executor as mode=off, so its statement test applies. + status, delivered, pool = await _run_one_message( + tmp_path, monkeypatch, "isolated", _LISTED, inbound_name="IB_W" + ) + assert (status, delivered) == (_ERROR, []) + assert pool.cursor_obj.executed is None + + +def test_isolated_with_no_config_dir_refuses_rather_than_run_in_process(tmp_path: Path) -> None: + from messagefoundry.pipeline.wiring_runner import RegistryRunner + + registry = Registry() # a graph built in code: no source_dir, no snapshot + runner = RegistryRunner( + registry, + object(), # type: ignore[arg-type] + sandbox_policy=SandboxPolicy(mode=SandboxMode.ISOLATED), + egress=EgressSettings(deny_by_default=False), + ) + with pytest.raises(SandboxError, match="never runs a Router or Handler in the engine"): + runner._sandbox_for("IB_ANY") + # mode=subprocess keeps its documented degradation to in-process for such a graph. + runner._sandbox_policy = SandboxPolicy(mode=SandboxMode.SUBPROCESS) + assert runner._sandbox_for("IB_ANY") is None + + +@pytest.mark.parametrize("mode", [SandboxMode.SUBPROCESS, SandboxMode.ISOLATED]) +async def test_fusion_is_disabled_in_every_worker_mode( + mode: SandboxMode, caplog: pytest.LogCaptureFixture +) -> None: + from messagefoundry.pipeline.wiring_runner import RegistryRunner + + runner = RegistryRunner( + Registry(), + object(), # type: ignore[arg-type] + sandbox_policy=SandboxPolicy(mode=mode), + egress=EgressSettings(deny_by_default=False), + ) + runner._fuse_thread_hops = True + with caplog.at_level("WARNING"): + assert await runner._activate_fusion() is False + assert any(f"[sandbox].mode={mode.value}" in r.getMessage() for r in caplog.records) From 35bdd67464206ace51f0b2e924feb2078a548ad6 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Wed, 28 Oct 2026 02:07:57 -0500 Subject: [PATCH 02/15] Docs: [sandbox].mode = "isolated", ADR 0147 build status, changelog (BACKLOG #3131) CONFIGURATION.md [sandbox] says what isolated does and does not do: the broker, the empty-list refusal under the opt-out, Handler-only, the wall cap counting lookups, the config-dir refusal, and no OS confinement yet. ADR 0147 ticks the mode box, settled 2026-10-27 under delegation, and records which limbs are built and which remain. Proposed PR title: Sandbox mode isolated: the parent brokers db_lookup and fhir_lookup (BACKLOG #3131, slice A) Proposed banner #3131: PARTIAL, slice A built. [sandbox].mode = "isolated" exists; every worker check reads is-not-OFF; the parent-side broker answers db_lookup/fhir_lookup over the closed-tag codec through the in-process runner (AC-1, AC-2, AC-6 per-inbound, AC-5 for the broker), refusing a non-Handler phase and an empty allowed_db/allowed_http even under the opt-out. Remains: AC-3 Landlock+seccomp, AC-4 AppContainer, AC-7, the ADR's open boxes (loopback/pipe, kernel floor, #3065 fit), the benchmark, and pooled-worker AC-6 scoping. --- changelog.d/3131.added.md | 12 ++++++ docs/CONFIGURATION.md | 41 +++++++++++++++++-- ...e-ipc-brokered-sandbox-extends-adr-0087.md | 25 ++++++++++- 3 files changed, 73 insertions(+), 5 deletions(-) create mode 100644 changelog.d/3131.added.md diff --git a/changelog.d/3131.added.md b/changelog.d/3131.added.md new file mode 100644 index 0000000000..68dd2d7119 --- /dev/null +++ b/changelog.d/3131.added.md @@ -0,0 +1,12 @@ +- **`[sandbox].mode = "isolated"`: the subprocess sandbox worker, with `db_lookup` and + `fhir_lookup` brokered through the engine.** A Handler's lookup is sent to the engine over the + worker pipe and answered by the same lookup runner `mode = "off"` uses, so it meets the same + checks. The engine refuses a brokered lookup bound to a Router or an `accepts=` predicate, and one + whose `[egress].allowed_db` or `[egress].allowed_http` list is empty, even under the audited + opt-out `[security].block_unlisted_outbound = false`. `wall_seconds` counts the time the engine + spends serving a call's lookups. This mode adds no operating-system confinement yet: an + `isolated` worker can open files and sockets exactly as a `subprocess` worker can. See + [CONFIGURATION.md](../docs/CONFIGURATION.md), section `[sandbox]`. (vault `BACKLOG #3131`) +- **Every sandbox worker mode now disables `[pipeline].fuse_thread_hops`,** not only + `subprocess`, and a graph with no config directory refuses to run under `isolated` rather than + run its Handlers in the engine process with unbrokered lookups. (vault `BACKLOG #3131`) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index b69258f454..5471ce1ec9 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1261,6 +1261,41 @@ matters most in the other direction: setting `mode="off"` because one Handler ne takes every other Router and Handler out of the sandbox too. To isolate some feeds and not others you need separate engine processes. +**`mode="isolated"` is `subprocess` plus a lookup broker, and nothing more yet** ([ADR +0147](adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md), +vault BACKLOG #3131). It runs the same per-inbound worker, under the same caps and the same codec, and +everything this section says about `subprocess` holds for it except the lookups: + +- **A Handler's `db_lookup`/`fhir_lookup` works.** The worker sends the call to the engine over the + worker pipe, and the engine answers it with the same lookup runner it uses at `mode="off"`, against + the same `DatabaseLookup`/`FhirLookup` connections. So a lookup there meets every check it meets + in-process, at least the `[egress]` host check made when the connection is built, the read-only + statement test, `max_rows` and the result timeout. Its result, or its error, reaches the Handler as + it would at `mode="off"`, so a Handler that catches `DbLookupError` or `FhirLookupError` still can. + A `Decimal` column value arrives as a `Decimal`. A value outside the worker pipe's types, such as a + `UUID`, fails that lookup with an error that says so. +- **A lookup whose `[egress]` list is empty is refused,** `[egress].allowed_db` for `db_lookup` and + `[egress].allowed_http` for `fhir_lookup`. **That holds under the audited opt-out + `[security].block_unlisted_outbound = false` too,** where an empty list would otherwise mean + unrestricted, so the opt-out cannot open a lookup from the worker to an unlisted host. The first + refusal per inbound and connection logs a `WARNING`; each refused call fails like any lookup + error. +- **It is still a Handler-only call.** A Router or an `accepts=` predicate that calls either lookup + fails as it does at `mode="off"`, and the engine refuses a brokered call bound to either of them. + The dry run has no lookup runner, so it fails there as in every mode. +- **`wall_seconds` counts the lookups.** The cap bounds the whole call, including the time the engine + spends serving its lookups, so raise it for a feed whose lookups are slow. A lookup the engine is + serving finishes or times out on its own bound before the cap is checked, so one call can outrun + the cap by up to one lookup's timeout. +- **It needs a config directory.** A graph built in code with none cannot start a worker. Under + `subprocess` such a graph quietly runs in-process; under `isolated` each of its messages fails + instead, because in-process its lookups would not go through the broker. + +**What `isolated` does not do yet:** confine the worker at the operating-system level. ADR 0147's +Landlock and seccomp confinement on Linux and its AppContainer token on Windows are not built, so an +`isolated` worker can still open files and sockets as the service account, exactly as a `subprocess` +worker can. *What this boundary does NOT cover*, below, holds for both modes. + **What turning it on costs you.** At least these things change relative to `mode="off"`: | | At `mode="subprocess"` | @@ -1317,8 +1352,8 @@ under `serve`. And config code there sees the full environment, not the worker's of these. The dry run has no lookup runner, so the call raises there in every mode. Unless the Handler catches that error, the fixture records `ERROR`. The gate then fails unless its `.expect` file declares `ERROR`. Under `subprocess` that matches `serve`, which refuses the lookup; at -`mode="off"` it does not, because `serve` runs the lookup there. And -`[pipeline].fuse_thread_hops` is **hard-disabled** whenever `mode="subprocess"`: fusion runs +`mode="off"` and `mode="isolated"` it does not, because `serve` runs the lookup there. And +`[pipeline].fuse_thread_hops` is **hard-disabled** whenever `mode` is not `off`: fusion runs Router/Handler code in-process on an executor hop, so honouring both would silently unsandbox the code you asked to isolate. The runner fails closed to the async sandboxed path and logs it. To get fusion you must leave `mode="off"`. @@ -1375,7 +1410,7 @@ on a live feed; a Router that only reads is unaffected. Grep your config for `os | Key | Type | Default | Notes | |---|---|---|---| -| `mode` | enum | `off` | `off` (in-process, byte-identical, no subprocess) or `subprocess` (persistent per-inbound worker child). | +| `mode` | enum | `off` | `off` (in-process, byte-identical, no subprocess), `subprocess` (persistent per-inbound worker child), or `isolated` (the same worker, with `db_lookup`/`fhir_lookup` brokered through the engine; see *`mode="isolated"`* above). Any other value refuses to load. | | `wall_seconds` | float (>0) | 5.0 | **Authoritative** wall-clock cap per Router/Handler call on **every** platform — the parent kills a worker that overruns it, so a pathological busy-loop can't wedge intake. The worker sets no `RLIMIT_CPU`, so CPU that admin code spends outside a call, such as a thread left running or a grandchild that leaves the worker's process group, has no bound at all ([ADR 0087](adr/0087-sandbox-subprocess-isolation.md), amendment of 2026-10-07). | | `mem_mb` | int (≥1) or null | 512 | POSIX-only memory cap (MiB) inside the child (no-op on Windows). `null` disables it. **On Linux it is headroom above the loaded graph:** the worker measures its own `VmData` once `load_config` has built the graph, and sets `RLIMIT_DATA` to that plus `mem_mb`. So a graph bigger than `mem_mb` still starts, and address space a thread only reserves does not count. The worker also starts with `MALLOC_ARENA_MAX=2` unless you pass your own value through `pass_environment`, so threads cannot use up the headroom for the worker's whole life. On other POSIX systems it is an absolute `RLIMIT_AS` cap on the whole worker, and the worker logs a line saying so. A call that runs out of memory dead-letters with an error naming the memory cap, and its worker exits so the next message gets a fresh one ([ADR 0087](adr/0087-sandbox-subprocess-isolation.md), amendment of 2026-10-11). | | `pass_environment` | list of names | `[]` | Extra environment variable **names** the worker is given, beyond its allowlist. Use it for a variable your config code reads. Names only; the value comes from the engine's own environment at worker start. **Refused at load** if a name is one of the engine's own: any `MEFOR_*` name, and `VAULT_TOKEN` or `PGPASSWORD`. Give a variable your config reads a name outside `MEFOR_`. Whatever you list is readable by every Router and Handler, so do not list a secret of your own either. Env: `MEFOR_SANDBOX_PASS_ENVIRONMENT`, comma-separated. | diff --git a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md index 47fac4be8b..b6b6cd2160 100644 --- a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md +++ b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md @@ -174,8 +174,29 @@ unresearched. - [ ] The Landlock **network-allowlist kernel floor** (≥6.7 for net rules) + the seccomp profile; the degrade path on an older kernel (FS-only Landlock, or refuse `mode=isolated`). - [ ] A **throughput benchmark** of the broker round-trip + confinement vs the ADR 0087 subprocess baseline. -- [ ] Whether this ships as `[sandbox].mode=isolated` (a third mode above `subprocess`) or a sub-flag on - `subprocess`. +- [x] Whether this ships as `[sandbox].mode=isolated` (a third mode above `subprocess`) or a sub-flag on + `subprocess`. **Settled 2026-10-27: a third mode, `isolated`.** Decided by a Manager seat under + the owner's delegation, with two companion rulings: every check meaning "runs in a sandbox + worker" reads `is not SandboxMode.OFF`, and a brokered lookup refuses while its `[egress]` list + is empty, even under `[security].block_unlisted_outbound = false` (Amendment A's open AC-2 + question). See *Build status* below. - [ ] (Added 2026-10-07, Amendment A.) How the AppContainer spawn fits the worker's suspended start and kill-on-close job (vault BACKLOG #3065). - [ ] A **WASI / CPython-on-WASM feasibility spike** (C-extension deps, throughput) before re-scoring option 3. + +## Build status (2026-10-28, vault BACKLOG #3131 slice A) + +**Built:** the `isolated` mode and decision (1), the parent-held broker. A Handler's +`db_lookup`/`fhir_lookup` in the worker writes a `lookup` frame in the closed tag set, bound to the +dispatch's `(id, phase, name)` triple and a per-dispatch sequence number (AC-6, for a per-inbound +worker, whose lane the session already is). The parent answers it on the dispatch thread through the +same accessor and runner the in-process path uses (AC-1, AC-2), refuses one bound to a Router or +`accepts=` dispatch, and refuses one whose `[egress].allowed_db` or `[egress].allowed_http` is empty. +A refused or failed lookup is the Handler's error, routed to `ERROR`/dead-letter post-ACK (AC-5, for +the broker). `wall_seconds` bounds the whole dispatch, the brokered lookups included. + +**Not built:** decision (2), the OS confinement: AC-3 (Landlock and seccomp), AC-4 (the AppContainer +token), and AC-7 (no opening or signalling the parent or a sibling). The first two open boxes above, +the #3065 box and the throughput benchmark still gate them. AC-6's scoping for a pooled worker +(ADR 0187) is not built either, because no pooled worker exists yet. Until AC-3 and AC-4 land, an +`isolated` worker is confined exactly as a `subprocess` worker is. \ No newline at end of file From 02fff123946b65e88cab811b1b5086651e8d38fa Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Wed, 28 Oct 2026 02:32:47 -0500 Subject: [PATCH 03/15] Sandbox broker review fixes: segment order, bounded reply write, per-dispatch runners (BACKLOG #3131) From the round-one code review of this branch: - encode_lookup numbers segments in decode order (a long statement plus a long param no longer fails to decode). - The parent writes a lookup reply on a thread bounded by the wall cap, so a worker that stops reading cannot hold the session lock. - A worker runner serves only the dispatch it was published for; an oversized lookup fails the call without breaking the pipe; a broken exchange exits without answering, so its own message fails. - Refusal log is once per [egress] list; echoed names and lookup errors are bounded. decode_answer parses once; LookupRequest has one target. - fhir params are checked with fhirsearch's own resolve first. - An isolated runner with no config dir refuses at construction. - Stale "mode=subprocess" docs; ruling stated once; ADR wall claim. Proposed PR title: Sandbox mode isolated: the parent brokers db_lookup and fhir_lookup (BACKLOG #3131, slice A) Proposed banner #3131: PARTIAL, slice A built. mode="isolated"; worker checks read is-not-OFF; parent broker over the closed-tag codec through the in-process runner (AC-1, AC-2, AC-6 per-inbound, AC-5 for the broker), refusing non-Handler phases and an empty allowed_db/http even under the opt-out. Remains: AC-3, AC-4, AC-7, the ADR's open boxes, the benchmark, and pooled-worker AC-6 scoping. --- changelog.d/3131.added.md | 12 +- docs/CONFIGURATION.md | 2 +- ...e-ipc-brokered-sandbox-extends-adr-0087.md | 4 +- messagefoundry/config/settings.py | 10 +- messagefoundry/pipeline/_sandbox_codec.py | 64 ++++--- messagefoundry/pipeline/_sandbox_worker.py | 89 +++++---- messagefoundry/pipeline/sandbox.py | 149 ++++++++------- messagefoundry/pipeline/wiring_runner.py | 12 ++ tests/test_sandbox_isolated.py | 174 ++++++++++++++++-- 9 files changed, 363 insertions(+), 153 deletions(-) diff --git a/changelog.d/3131.added.md b/changelog.d/3131.added.md index 68dd2d7119..1f4901a592 100644 --- a/changelog.d/3131.added.md +++ b/changelog.d/3131.added.md @@ -4,9 +4,11 @@ checks. The engine refuses a brokered lookup bound to a Router or an `accepts=` predicate, and one whose `[egress].allowed_db` or `[egress].allowed_http` list is empty, even under the audited opt-out `[security].block_unlisted_outbound = false`. `wall_seconds` counts the time the engine - spends serving a call's lookups. This mode adds no operating-system confinement yet: an - `isolated` worker can open files and sockets exactly as a `subprocess` worker can. See - [CONFIGURATION.md](../docs/CONFIGURATION.md), section `[sandbox]`. (vault `BACKLOG #3131`) + spends serving a call's lookups, though a lookup already being served runs to its own timeout. + This mode adds no operating-system confinement yet: an `isolated` worker can open files and + sockets exactly as a `subprocess` worker can. See [CONFIGURATION.md](../docs/CONFIGURATION.md), + section `[sandbox]`. (vault `BACKLOG #3131`) - **Every sandbox worker mode now disables `[pipeline].fuse_thread_hops`,** not only - `subprocess`, and a graph with no config directory refuses to run under `isolated` rather than - run its Handlers in the engine process with unbrokered lookups. (vault `BACKLOG #3131`) + `subprocess`. Under `isolated`, a runner whose graph has no config directory refuses at + construction, rather than run its Handlers in the engine process with unbrokered lookups. + (vault `BACKLOG #3131`) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 5471ce1ec9..a6e19df2ae 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1288,7 +1288,7 @@ everything this section says about `subprocess` holds for it except the lookups: serving finishes or times out on its own bound before the cap is checked, so one call can outrun the cap by up to one lookup's timeout. - **It needs a config directory.** A graph built in code with none cannot start a worker. Under - `subprocess` such a graph quietly runs in-process; under `isolated` each of its messages fails + `subprocess` such a graph quietly runs in-process; under `isolated` the runner refuses to be built instead, because in-process its lookups would not go through the broker. **What `isolated` does not do yet:** confine the worker at the operating-system level. ADR 0147's diff --git a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md index b6b6cd2160..c9a21a4b2f 100644 --- a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md +++ b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md @@ -193,7 +193,9 @@ worker, whose lane the session already is). The parent answers it on the dispatc same accessor and runner the in-process path uses (AC-1, AC-2), refuses one bound to a Router or `accepts=` dispatch, and refuses one whose `[egress].allowed_db` or `[egress].allowed_http` is empty. A refused or failed lookup is the Handler's error, routed to `ERROR`/dead-letter post-ACK (AC-5, for -the broker). `wall_seconds` bounds the whole dispatch, the brokered lookups included. +the broker). `wall_seconds` counts the brokered lookups, and the parent's write of each reply is +bounded by it, but a lookup the parent is serving is not interrupted: one dispatch can outlast the +cap by up to one lookup's own timeout before it fails on the cap. **Not built:** decision (2), the OS confinement: AC-3 (Landlock and seccomp), AC-4 (the AppContainer token), and AC-7 (no opening or signalling the parent or a sibling). The first two open boxes above, diff --git a/messagefoundry/config/settings.py b/messagefoundry/config/settings.py index 9f215e0c3b..b02449a4ba 100644 --- a/messagefoundry/config/settings.py +++ b/messagefoundry/config/settings.py @@ -2305,7 +2305,7 @@ class PipelineSettings(_Section): # NOT re-read it — restart to change, exactly like claim_mode). Harness A/B via # MEFOR_PIPELINE_FUSE_THREAD_HOPS. # ONE OTHER SETTING CAN CANCEL THIS ONE, and you must be told here rather than in that knob's - # docs: the runner HARD-DISABLES fusion whenever [sandbox].mode is subprocess (it ships off, so + # docs: the runner HARD-DISABLES fusion whenever [sandbox].mode is not off (it ships off, so # this bites only a site that turned the sandbox on). Fusion runs Router/Handler/accepts= code # in-process on an executor hop, so honouring both would silently run unsandboxed the code a # config asked to isolate; the runner fails CLOSED to the async sandboxed path and logs it. To @@ -2404,15 +2404,13 @@ class SandboxSettings(_Section): process boundary breaks, so they fail closed inside the child. **A Handler needing either must run ``mode="off"``, or ``mode="isolated"``** — the off escape is supported and is not going away. ``mode="isolated"`` (ADR 0147, vault BACKLOG #3131) is the same worker with both lookups - brokered through the engine, refused while ``[egress].allowed_db`` (``allowed_http`` for - ``fhir_lookup``) is empty even under the ``block_unlisted_outbound`` opt-out. It adds no - OS-level confinement yet: ``docs/CONFIGURATION.md``, section ``[sandbox]``, says what it does. ``[sandbox]`` is a - single **engine-wide** section: :meth:`messagefoundry.pipeline.engine.Engine.add_registry` + brokered through the engine; ``docs/CONFIGURATION.md``, section ``[sandbox]``, says what it + does and does not do. ``[sandbox]`` is a single **engine-wide** section: :meth:`messagefoundry.pipeline.engine.Engine.add_registry` renders ONE ``SandboxPolicy`` for the whole graph and a connection carries no per-connection sandbox field, so ``mode="off"`` set for one Handler runs **every** Router and Handler in the process in-process, not just that one. * **``wall_seconds`` starts being enforced.** At ``mode="off"`` there is no timeout at all; at - ``mode="subprocess"`` the parent kills a worker that overruns and dead-letters that message + ``mode="subprocess"`` or ``mode="isolated"`` the parent kills a worker that overruns and dead-letters that message post-ACK. A busy-loop can no longer wedge intake, **and** a legitimately slow Handler that used to finish now dead-letters. ``startup_seconds`` and the POSIX ``mem_mb`` arm with it. * **Throughput** ~0.19 ms per dispatch with no reference view; a 20k-entry crosswalk ~4.5 ms diff --git a/messagefoundry/pipeline/_sandbox_codec.py b/messagefoundry/pipeline/_sandbox_codec.py index e0d4b6aca8..1172f035a6 100644 --- a/messagefoundry/pipeline/_sandbox_codec.py +++ b/messagefoundry/pipeline/_sandbox_codec.py @@ -1299,6 +1299,13 @@ def decode_response(body: bytes, *, request_id: str, phase: str, name: str) -> R :meth:`sandbox.SandboxSession.dispatch` supplies the other half — an unsolicited frame queued before or after a dispatch is fatal to the worker — so a pre-staged answer has no window at all.""" header, reader, kind = _open(body, ("ok", "fail")) + return _answer(header, reader, kind, request_id=request_id, phase=phase, name=name) + + +def _answer( + header: dict[str, Any], reader: _Reader, kind: str, *, request_id: str, phase: str, name: str +) -> Response: + """The ``ok``/``fail`` body of :func:`decode_response`, on a frame already opened.""" _check_binding(header, request_id, phase, name) if kind == "ok": if "result" not in header: @@ -1314,6 +1321,18 @@ def decode_response(body: bytes, *, request_id: str, phase: str, name: str) -> R return Response(ok=False, result=None, kind=fail_kind, error=error) +def _check_binding(header: dict[str, Any], request_id: str, phase: str, name: str) -> None: + """Refuse a frame whose ``(id, phase, name)`` is not the dispatch's (see :func:`decode_response`).""" + if _req_str(header.get("id"), "response id") != request_id: + raise SandboxCodecError( + f"sandbox response correlation mismatch (expected request {request_id!r})" + ) + if _req_str(header.get("phase"), "response phase") != phase: + raise SandboxCodecError(f"sandbox response phase mismatch (expected {phase!r})") + if _req_str(header.get("name"), "response name") != name: + raise SandboxCodecError(f"sandbox response name mismatch (expected {name!r})") + + # --- the lookup broker (mode=isolated, ADR 0147) ------------------------------ # While a Handler runs at mode=isolated, the worker may write ``lookup`` frames before its answer: one # per db_lookup/fhir_lookup call, each answered by one ``lookupok``/``lookupfail`` frame from the @@ -1328,21 +1347,21 @@ def decode_response(body: bytes, *, request_id: str, phase: str, name: str) -> R @dataclass(frozen=True) class LookupRequest: - """One decoded ``lookup`` frame (parent side). ``statement`` is set for a db lookup and - ``query`` for a fhir one; ``params`` is rebuilt as the in-process call would have received it.""" + """One decoded ``lookup`` frame (parent side). ``target`` is the statement of a db lookup or the + query of a fhir one; ``params`` is rebuilt as the in-process call would have received it.""" seq: int kind: str connection: str - statement: str | None - query: str | None + target: str params: Any def _enc_fhir_params(params: object, blobs: _Blobs) -> Any: """A ``fhir_lookup`` ``params`` mapping in its own small grammar: ``[key, is_list, items]`` per key, each item ``{"s": str}``, ``{"k": [system, code]}`` or ``{"r": raw}``. Only the three kinds - :mod:`messagefoundry.fhirsearch` takes cross; the parent's own executor screens their content.""" + :mod:`messagefoundry.fhirsearch` takes cross. The worker runs fhirsearch's own check before it + gets here, so a refusal below is a backstop; the parent's executor screens the content.""" if params is None: return None if not isinstance(params, Mapping): @@ -1363,11 +1382,8 @@ def _enc_fhir_params(params: object, blobs: _Blobs) -> Any: elif isinstance(one, FhirRaw): items.append({"r": blobs.ref(one.value)}) else: - # The words fhirsearch's own refusal uses; the value is withheld, as there. raise SandboxCodecError( - f"FHIR search parameter {key!r}: a search value must be a str (data), a " - f"FhirToken(system, code), or a FhirRaw(...) author-syntax value -- got " - f"{type(one).__name__}. (The value is withheld: it may carry PHI.)" + f"FHIR search parameter {key!r}: a {type(one).__name__} is not a search value" ) out.append([key, is_list, items]) return out @@ -1422,6 +1438,11 @@ def encode_lookup( fhir one. Raises :class:`SandboxCodecError` for an argument outside the grammar, which the worker turns into the lookup's own error type, as the in-process call would fail on a bad argument.""" blobs = _Blobs() + # Segment order IS walk order (see _Reader): connection, then target, then params, the order + # decode_answer reads them in. Building params first would number its segments ahead of the + # other two, and any lookup with a long params value and a long statement would not decode. + conn_ref = blobs.ref(connection) + target_ref = blobs.ref(target) if kind == LOOKUP_DB: if params is not None and not isinstance(params, Mapping): raise SandboxCodecError( @@ -1439,8 +1460,8 @@ def encode_lookup( "name": name, "seq": seq, "kind": kind, - "connection": blobs.ref(connection), - "target": blobs.ref(target), + "connection": conn_ref, + "target": target_ref, "params": enc_params, }, "lookup", @@ -1448,18 +1469,6 @@ def encode_lookup( return encode_frame(header, blobs.items) -def _check_binding(header: dict[str, Any], request_id: str, phase: str, name: str) -> None: - """Refuse a frame whose ``(id, phase, name)`` is not the dispatch's (see :func:`decode_response`).""" - if _req_str(header.get("id"), "response id") != request_id: - raise SandboxCodecError( - f"sandbox response correlation mismatch (expected request {request_id!r})" - ) - if _req_str(header.get("phase"), "response phase") != phase: - raise SandboxCodecError(f"sandbox response phase mismatch (expected {phase!r})") - if _req_str(header.get("name"), "response name") != name: - raise SandboxCodecError(f"sandbox response name mismatch (expected {name!r})") - - def decode_answer( body: bytes, *, request_id: str, phase: str, name: str ) -> Response | LookupRequest: @@ -1469,7 +1478,7 @@ def decode_answer( mode can be talked into brokering a lookup.""" header, reader, kind = _open(body, ("ok", "fail", "lookup")) if kind != "lookup": - return decode_response(body, request_id=request_id, phase=phase, name=name) + return _answer(header, reader, kind, request_id=request_id, phase=phase, name=name) _check_binding(header, request_id, phase, name) seq = _req_int(header.get("seq"), "lookup seq") lookup_kind = header.get("kind") @@ -1488,12 +1497,7 @@ def decode_answer( params = _dec_fhir_params(header.get("params"), reader) reader.finish() return LookupRequest( - seq=seq, - kind=lookup_kind, - connection=connection, - statement=target if lookup_kind == LOOKUP_DB else None, - query=target if lookup_kind == LOOKUP_FHIR else None, - params=params, + seq=seq, kind=lookup_kind, connection=connection, target=target, params=params ) diff --git a/messagefoundry/pipeline/_sandbox_worker.py b/messagefoundry/pipeline/_sandbox_worker.py index a18012941c..d4133613ec 100644 --- a/messagefoundry/pipeline/_sandbox_worker.py +++ b/messagefoundry/pipeline/_sandbox_worker.py @@ -211,16 +211,19 @@ def _install_import_guard(forbidden: tuple[str, ...]) -> None: class _Broker: """The worker's half of the ``mode=isolated`` lookup broker (ADR 0147). - Its two runners stand in for the engine's while a Handler runs: each writes one ``lookup`` frame - to the parent and blocks on the parent's one reply, so a ``db_lookup``/``fhir_lookup`` call - returns what the parent's own runner returned, or raises the lookup's own error type with the - parent's reason. The parent decides every call; this side only asks. - - Calls are serialized under one lock, because the pipe carries one exchange at a time. A call made - while no dispatch is in flight, such as from a thread a Handler left running, is refused rather - than written: the parent would read the frame as unsolicited and drop the worker, and its read of - stdin would take the next request frame as its reply. A desynchronized or closed pipe marks the - broker broken, and :func:`main` exits after the dispatch, so the parent starts a fresh worker.""" + :meth:`runners` hands out a dispatch's two runners, which stand in for the engine's while its + Handler runs: each writes one ``lookup`` frame to the parent and blocks on the parent's one reply, + so a ``db_lookup``/``fhir_lookup`` call returns what the parent's own runner returned, or raises the + lookup's own error type with the parent's reason. The parent decides every call; this side only + asks. + + Calls are serialized under one lock, because the pipe carries one exchange at a time. A runner + serves only the dispatch it was handed out for. A call from anywhere else, such as a thread a + Handler left running that holds an earlier dispatch's runner, is refused rather than written: + between dispatches the parent would read the frame as unsolicited and drop the worker, and during + another dispatch the lookup would be charged to that message. A desynchronized or closed pipe marks + the broker broken, and :func:`main` then exits without answering, so the message whose exchange + broke is the one that fails and the parent starts a fresh worker for the next.""" def __init__(self, stdin: Any, stdout: Any) -> None: self._stdin = stdin @@ -240,20 +243,36 @@ def end(self) -> None: with self._lock: self._req = None - def db(self, connection: str, statement: str, params: Any = None) -> Any: + def runners(self, req: Any) -> tuple[Any, Any]: + """The ``(db_lookup, fhir_lookup)`` runners for dispatch ``req`` alone.""" from messagefoundry.config.db_lookup import DbLookupError - from messagefoundry.pipeline._sandbox_codec import LOOKUP_DB + from messagefoundry.config.fhir_lookup import FhirLookupError + from messagefoundry.fhirsearch import resolve_search_pairs + from messagefoundry.pipeline._sandbox_codec import LOOKUP_DB, LOOKUP_FHIR - return self._ask(LOOKUP_DB, DbLookupError, "db_lookup", connection, statement, params) + def db(connection: str, statement: str, params: Any = None) -> Any: + return self._ask( + req, LOOKUP_DB, DbLookupError, "db_lookup", connection, statement, params + ) - def fhir(self, connection: str, query: str, params: Any = None) -> Any: - from messagefoundry.config.fhir_lookup import FhirLookupError - from messagefoundry.pipeline._sandbox_codec import LOOKUP_FHIR + def fhir(connection: str, query: str, params: Any = None) -> Any: + if params is not None: + # fhirsearch's own check, so a value of the wrong kind or carrying a separator fails + # with the words the engine's executor would give it at mode=off. The engine checks + # again on its side. + try: + resolve_search_pairs(params) + except ValueError as exc: + raise FhirLookupError(f"fhir_lookup on {connection!r}: {exc}") from None + return self._ask( + req, LOOKUP_FHIR, FhirLookupError, "fhir_lookup", connection, query, params + ) - return self._ask(LOOKUP_FHIR, FhirLookupError, "fhir_lookup", connection, query, params) + return db, fhir def _ask( self, + req: Any, kind: str, error: type[Exception], fn: str, @@ -262,13 +281,12 @@ def _ask( params: object, ) -> Any: from messagefoundry.pipeline import _sandbox_codec as codec - from messagefoundry.pipeline._sandbox_codec import SandboxError + from messagefoundry.pipeline._sandbox_codec import MAX_FRAME, SandboxError from messagefoundry.pipeline.sandbox import _read_frame_bytes, _write_frame with self._lock: - req = self._req - if req is None or self.broken: - raise error(f"{fn}: no Handler dispatch is in flight in this sandbox worker") + if self._req is not req or self.broken: + raise error(f"{fn}: called outside the Handler dispatch it was published for") self._seq += 1 seq = self._seq try: @@ -282,9 +300,12 @@ def _ask( target=target, params=params, ) + if len(frame) > MAX_FRAME: + raise SandboxError(f"the lookup frame is {len(frame)} bytes, over the cap") except SandboxError as exc: - # A bad argument fails as the call would in-process: the lookup's own error, and the - # pipe untouched. Nothing was written. + # A bad or oversized argument fails as the call would in-process: the lookup's own + # error. Nothing was written, so the pipe is still in step; the next call reuses seq. + self._seq -= 1 raise error(f"{fn}: its arguments cannot cross the sandbox: {exc}") from None try: _write_frame(self._stdout, frame) @@ -295,9 +316,8 @@ def _ask( body, request_id=req.request_id, phase=req.phase, name=req.name, seq=seq ) except (OSError, SandboxError) as exc: - # An over-cap frame raises before a byte is written, so only a pipe the parent - # abandoned, or a reply that does not answer this call, gets here. Either way the - # pipe no longer lines up with the protocol. + # A pipe the parent abandoned, or a reply that does not answer this call: the pipe no + # longer lines up with the protocol. self.broken = True raise error(f"{fn}: the sandbox lookup broker failed: {exc}") from None if not ok: @@ -353,12 +373,14 @@ def _run_one( # The brokered runners exist only for a Handler: a Router or an accepts= predicate finds no # runner, so its lookup raises the "unavailable here" error it raises at mode=off. - brokered = broker is not None and phase == "transform" + db_runner, fhir_runner = ( + broker.runners(req) if broker is not None and phase == "transform" else (None, None) + ) try: with ( run_contexts(run_context, phase=phase_key), - db_lookup_mod.activated(broker.db if brokered and broker else None), - fhir_lookup_mod.activated(broker.fhir if brokered and broker else None), + db_lookup_mod.activated(db_runner), + fhir_lookup_mod.activated(fhir_runner), ): result = fn(req.payload) if phase == "transform": @@ -500,10 +522,6 @@ def main() -> int: # mode=isolated (ADR 0147): a Handler's lookups are asked of the parent over this same pipe. broker = _Broker(stdin, stdout) if boot.broker else None while True: - if broker is not None and broker.broken: - # A lookup exchange went out of step with the protocol; the next request frame cannot be - # trusted to line up either. The parent reads EOF and starts a fresh worker. - return 1 frame = _read_frame_bytes(stdin) if frame is None: return 0 # parent closed the pipe — clean shutdown @@ -527,6 +545,11 @@ def main() -> int: resp, detail = None, _error_text(exc) if broker is not None: broker.end() # before the answer: no lookup frame may follow it + if broker.broken: + # A lookup exchange went out of step. Exit WITHOUT answering, so the parent reads EOF + # and fails THIS message, rather than answer and leave the next message to find a + # worker on its way out. + return 1 if resp is None: # A call that ran out of memory may leave memory held (a module global, a cache, a # fragmented heap), and the next innocent message would pay for it. Answer, then exit so diff --git a/messagefoundry/pipeline/sandbox.py b/messagefoundry/pipeline/sandbox.py index b07d90976e..94344017ee 100644 --- a/messagefoundry/pipeline/sandbox.py +++ b/messagefoundry/pipeline/sandbox.py @@ -35,9 +35,8 @@ * ``isolated`` (ADR 0147, vault BACKLOG #3131) — the ``subprocess`` worker, with a Handler's ``db_lookup``/``fhir_lookup`` brokered through the parent instead of refused: the worker writes a ``lookup`` frame mid-dispatch and the parent answers it with the runner the in-process path uses - (:meth:`SandboxSession._serve_lookup`). The parent refuses one bound to a Router or ``accepts=`` - dispatch, and one whose ``[egress]`` list is empty, even under the ``block_unlisted_outbound`` - opt-out. **No OS-level confinement yet:** ADR 0147's Landlock/seccomp and AppContainer limbs are + (:meth:`SandboxSession._serve_lookup`), refusing what :meth:`SandboxSession._lookup_refusal` + lists. **No OS-level confinement yet:** ADR 0147's Landlock/seccomp and AppContainer limbs are unbuilt, so this mode confines exactly what ``subprocess`` does. Every check that means "runs in a worker" reads ``is not SandboxMode.OFF``, so this mode takes every worker branch. @@ -212,13 +211,10 @@ class SandboxPolicy: #: refuses the engine's own; this dataclass carries what it let through. pass_environment: tuple[str, ...] = () #: ``[egress].allowed_db`` and ``[egress].allowed_http``, read only by the parent's lookup broker - #: at ``mode=isolated`` (ADR 0147). The broker refuses a ``db_lookup`` while the first is empty - #: and a ``fhir_lookup`` while the second is, even under the audited opt-out - #: ``[security].block_unlisted_outbound = false``, where an empty list would otherwise mean - #: unrestricted (owner ruling 2026-10-27, made under delegation; vault BACKLOG #3131). The hosts - #: themselves are checked where the in-process path checks them, when the lookup executors are - #: built, so the broker inherits that check rather than repeating it. Empty by default, so a - #: policy built without them brokers nothing. Never sent to the worker. + #: at ``mode=isolated`` (ADR 0147), which refuses a lookup while its list is empty + #: (:meth:`SandboxSession._lookup_refusal`). The hosts themselves are checked where the in-process + #: path checks them, when the lookup executors are built. Empty by default, so a policy built + #: without them brokers nothing. Never sent to the worker. lookup_allowed_db: tuple[str, ...] = () lookup_allowed_http: tuple[str, ...] = () @@ -259,6 +255,20 @@ def graph_shape(registry: Any) -> GraphShape: #: The accessor each lookup kind names, for the broker's refusals and log lines. _LOOKUP_FN: Final = {codec.LOOKUP_DB: "db_lookup", codec.LOOKUP_FHIR: "fhir_lookup"} +#: Longest error text the broker sends back to a worker. A lookup error can quote the connection +#: name the worker sent, and the worker chooses its length. +_LOOKUP_ERROR_CAP: Final = 2048 + + +def _shown_name(name: str) -> str: + """A connection name a worker sent, scrubbed and cut, for a refusal or a log line.""" + return scrub_control_chars(name)[:80] + + +def _bounded(text: str) -> str: + return text if len(text) <= _LOOKUP_ERROR_CAP else text[:_LOOKUP_ERROR_CAP] + " [cut]" + + #: How many names a refusal lists per side of one part of the graph. _NAMES_SHOWN: Final = 3 @@ -697,8 +707,8 @@ def __init__( self._generation = 0 self._lock = threading.Lock() self._closed = False - # The (accessor, connection) pairs whose empty-[egress] refusal this session has logged. - self._lookup_refusals_logged: set[tuple[str, str]] = set() + # The [egress] lists whose empty-list refusal this session has logged. + self._lookup_refusals_logged: set[str] = set() @property def mode(self) -> SandboxMode: @@ -1013,11 +1023,12 @@ def _await_answer( :func:`codec.decode_response`, which refuses a ``lookup`` frame, so only an isolated session ever brokers a lookup and no worker can ask another session to. - ``wall_seconds`` bounds the WHOLE dispatch, the lookups the parent serves for it included: - the cap stays the authoritative bound it is at ``mode=subprocess``, and a feed whose lookups - are slow needs a larger cap. A lookup the parent is serving is not interrupted when the cap - runs out; its own bound (the runner's result timeout) ends it, and the dispatch then fails - on the cap. So one dispatch can outlast ``wall_seconds`` by at most one lookup's bound.""" + ``wall_seconds`` counts the lookups the parent serves for the dispatch, so the cap stays the + bound it is at ``mode=subprocess``, and a feed whose lookups are slow needs a larger cap. A + lookup the parent is serving is not interrupted when the cap runs out: its own bound (the + runner's result timeout) ends it, and the dispatch then fails on the cap. So one dispatch can + outlast ``wall_seconds`` by at most one lookup's bound. The reply write is bounded by the cap + too (:meth:`_write_reply`).""" brokered = self.policy.mode is SandboxMode.ISOLATED deadline = time.monotonic() + self.policy.wall_seconds expected_seq = 1 @@ -1057,27 +1068,50 @@ def _await_answer( return answer expected_seq += 1 reply = self._lookup_reply(request_id, phase, name, answer) - assert proc.stdin is not None - try: - _write_frame(proc.stdin, reply) - except (OSError, SandboxError) as exc: - self._kill(proc) - raise SandboxError( - f"sandbox {phase} {name!r}: could not answer its worker's lookup: {exc}" - ) from exc - if time.monotonic() > deadline: + if not self._write_reply(proc, reply, deadline) or time.monotonic() > deadline: self._kill(proc) raise SandboxError( f"sandbox {phase} {name!r} exceeded the {self.policy.wall_seconds}s wall cap, " "which counts the lookups the engine served for it" ) + def _write_reply(self, proc: subprocess.Popen[bytes], body: bytes, deadline: float) -> bool: + """Write one lookup reply, bounded by the dispatch's wall cap; ``False`` if the cap ran out. + + On its own thread, because a reply can be larger than the pipe's buffer, and a worker that + sent a ``lookup`` frame and then stopped reading stdin would otherwise block this thread for + good while it holds the session lock: no wall cap, and a reload's ``close`` waiting behind it. + The caller kills the worker when the cap runs out, which closes the pipe and ends the write. + A broken pipe raises :class:`SandboxError` after the worker is killed.""" + assert proc.stdin is not None + stdin = proc.stdin + failure: list[BaseException] = [] + + def write() -> None: + try: + _write_frame(stdin, body) + except (OSError, ValueError, SandboxError) as exc: # ValueError: a pipe closed under it + failure.append(exc) + + writer = threading.Thread( + target=write, name=f"mf-sandbox-reply-{self._inbound}", daemon=True + ) + writer.start() + writer.join(max(0.0, deadline - time.monotonic())) + if writer.is_alive(): + return False + if failure: + self._kill(proc) + raise SandboxError(f"could not answer the sandbox worker's lookup: {failure[0]}") + return True + def _lookup_reply( self, request_id: str, phase: str, name: str, lookup: codec.LookupRequest ) -> bytes: """The ``lookupok``/``lookupfail`` frame answering one ``lookup``. Never raises: the worker is blocked on this reply, so a result that cannot cross is a ``lookupfail`` it can report.""" ok, result, error = self._serve_lookup(phase, lookup) + fn = _LOOKUP_FN[lookup.kind] if ok: try: body = codec.encode_lookup_ok( @@ -1085,20 +1119,11 @@ def _lookup_reply( ) except SandboxError as exc: # At least a column type outside the closed grammar, such as a UUID. - ok, error = ( - False, - f"{_LOOKUP_FN[lookup.kind]} result cannot cross the sandbox: {exc}", - ) + error = f"{fn} result cannot cross the sandbox: {exc}" else: if len(body) <= _MAX_FRAME: return body - ok, error = ( - False, - ( - f"{_LOOKUP_FN[lookup.kind]} result is larger than a sandbox frame " - f"({len(body)} bytes)" - ), - ) + error = f"{fn} result is larger than a sandbox frame ({len(body)} bytes)" return codec.encode_lookup_fail( request_id=request_id, phase=phase, name=name, seq=lookup.seq, error=error ) @@ -1111,38 +1136,35 @@ def _serve_lookup(self, phase: str, lookup: codec.LookupRequest) -> tuple[bool, published for this transform and nothing else (ADR 0147 AC-2). Where none is published, as on a Router, in a dry run, on the inline fast path or in a graph that declares no lookup, it raises exactly what it raises at ``mode=off``. The executor behind the runner was built - against ``[egress]``, so each host is checked there, as in-process. The slice-B seam: a - broker-only check of a lookup's own arguments goes between the refusal below and the call. - """ + against ``[egress]`` and runs its own statement test and row ceiling, so each applies here as + in-process. The slice-B seam: a broker-only check of a lookup's own arguments goes between the + refusal below and the call.""" refusal = self._lookup_refusal(phase, lookup) if refusal is not None: return False, None, refusal try: if lookup.kind == codec.LOOKUP_DB: - assert lookup.statement is not None - return True, db_lookup(lookup.connection, lookup.statement, lookup.params), "" - assert lookup.query is not None - return True, fhir_lookup(lookup.connection, lookup.query, lookup.params), "" + return True, db_lookup(lookup.connection, lookup.target, lookup.params), "" + return True, fhir_lookup(lookup.connection, lookup.target, lookup.params), "" except (DbLookupError, FhirLookupError) as exc: # PHI-free by both types' contract: a connection name and a routing-safe reason. - return False, None, str(exc) + return False, None, _bounded(str(exc)) except Exception as exc: # noqa: BLE001 — the worker waits on this reply; report, never raise return False, None, f"{_LOOKUP_FN[lookup.kind]} failed: {safe_exc(exc)}" def _lookup_refusal(self, phase: str, lookup: codec.LookupRequest) -> str | None: """Why the parent refuses a brokered lookup outright, or ``None`` to serve it. - Two refusals, each fail closed. A lookup is answered only for a Handler, so a request bound - to a Router or an ``accepts=`` dispatch is refused here even if a worker sends one; the child - has no lookup runner in those phases either. And a lookup whose ``[egress]`` list is empty is - refused even under the audited opt-out ``[security].block_unlisted_outbound = false``, where - the in-process path would treat that empty list as unrestricted (owner ruling 2026-10-27, made - under delegation; vault BACKLOG #3131).""" + A lookup is answered only for a Handler, so one bound to a Router or an ``accepts=`` dispatch + is refused even if a worker sends it; the child has no lookup runner in those phases either. + And one whose ``[egress]`` list is empty is refused, under the ruling ``docs/CONFIGURATION.md`` + states in section ``[sandbox]``, *``mode="isolated"``* (vault BACKLOG #3131).""" fn = _LOOKUP_FN[lookup.kind] + shown = _shown_name(lookup.connection) if phase != "transform": return ( - f"{fn}({lookup.connection!r}) is unavailable here — the engine answers a brokered " - "lookup only for a Handler, never for a Router or an accepts= predicate (ADR 0147)" + f"{fn}({shown!r}) is unavailable here — the engine answers a brokered lookup only " + "for a Handler, never for a Router or an accepts= predicate (ADR 0147)" ) if lookup.kind == codec.LOOKUP_DB: allowed, key = self.policy.lookup_allowed_db, "allowed_db" @@ -1150,22 +1172,21 @@ def _lookup_refusal(self, phase: str, lookup: codec.LookupRequest) -> str | None allowed, key = self.policy.lookup_allowed_http, "allowed_http" if allowed: return None - if (fn, lookup.connection) not in self._lookup_refusals_logged: - # Once per session and connection: the cause is configuration, so every later message - # would log the same line. Each refused message still fails, so none is dropped. - self._lookup_refusals_logged.add((fn, lookup.connection)) + if key not in self._lookup_refusals_logged: + # Once per session and list: the cause is the empty list, whatever connection a worker + # names, so the set holds at most two entries. Each refused call still fails. + self._lookup_refusals_logged.add(key) log.warning( "inbound %r: a brokered %s on %r was refused: [egress].%s is empty " "([sandbox].mode=isolated, ADR 0147)", self._inbound, fn, - scrub_control_chars(lookup.connection)[:80], + shown, key, ) return ( - f"{fn}({lookup.connection!r}) refused: [egress].{key} is empty, and " - "[sandbox].mode=isolated brokers a lookup only to a host that list names, whatever " - "[security].block_unlisted_outbound says (ADR 0147)" + f"{fn}({shown!r}) refused: [egress].{key} is empty, and [sandbox].mode=isolated " + "brokers a lookup only to a host that list names (ADR 0147)" ) # -- dispatch ------------------------------------------------------------- @@ -1252,9 +1273,9 @@ def run_sandboxed( With ``session is None`` or ``session.mode is OFF`` this is exactly ``fn(payload)`` — in-process, byte-identical, zero overhead (the parity default); nothing above that branch touches ``payload``, - ``origin``, or the codec. With ``session.mode is SUBPROCESS`` the call is marshalled to the - persistent worker via :meth:`SandboxSession.dispatch`, enforcing the forbidden-import / resource - caps and raising :class:`SandboxError` on any violation. + ``origin``, or the codec. In every other mode (``subprocess``, ``isolated``) the call is + marshalled to the persistent worker via :meth:`SandboxSession.dispatch`, enforcing the + forbidden-import / resource caps and raising :class:`SandboxError` on any violation. ``origin`` is the ``(raw, content_type)`` the caller built ``payload`` from — pass it **only** when the caller derived the payload itself, so the child can rebuild it byte-faithfully; ``None`` makes diff --git a/messagefoundry/pipeline/wiring_runner.py b/messagefoundry/pipeline/wiring_runner.py index 7d8d9a1c28..fa78a19559 100644 --- a/messagefoundry/pipeline/wiring_runner.py +++ b/messagefoundry/pipeline/wiring_runner.py @@ -1317,6 +1317,18 @@ def __init__( # construction (a /config/reload does NOT re-read it — restart to change, like claim_mode). self._sandbox_policy = sandbox_policy self._sandbox_config_source = sandbox_config_source + if ( + sandbox_policy is not None + and sandbox_policy.mode is SandboxMode.ISOLATED + and registry.source_dir is None + and (sandbox_config_source is None or sandbox_config_source[0] is None) + ): + # Refused here, before a listener ACKs anything, rather than once per message by + # _sandbox_for, whose refusal stays as the backstop for a reload (vault BACKLOG #3131). + raise ValueError( + "[sandbox].mode=isolated needs a config directory for its workers to load, and " + "this graph has none: it never runs a Router or Handler in the engine process" + ) self._sandbox_sessions: dict[str, SandboxSession] = {} self._sandbox_graph_shape: tuple[Registry, GraphShape] | None = None # Sessions read that cache from their worker threads. Two building it at once would each diff --git a/tests/test_sandbox_isolated.py b/tests/test_sandbox_isolated.py index 2228262fdc..d83fce78f5 100644 --- a/tests/test_sandbox_isolated.py +++ b/tests/test_sandbox_isolated.py @@ -21,6 +21,7 @@ import asyncio import os +import time from decimal import Decimal from pathlib import Path from typing import Any @@ -342,7 +343,7 @@ def test_the_parent_refuses_a_lookup_bound_to_a_router_dispatch(tmp_path: Path) # The worker has no runner on a Router, so it never asks; the parent refuses one anyway. session = _session(str(tmp_path), SandboxMode.ISOLATED) lookup = codec.LookupRequest( - seq=1, kind=codec.LOOKUP_DB, connection="LK", statement="SELECT 1", query=None, params=None + seq=1, kind=codec.LOOKUP_DB, connection="LK", target="SELECT 1", params=None ) for phase in ("router", "accepts"): refusal = session._lookup_refusal(phase, lookup) @@ -365,7 +366,7 @@ def test_a_lookup_frame_drops_a_subprocess_worker() -> None: with pytest.raises(codec.SandboxCodecError, match="unexpected sandbox frame type 'lookup'"): codec.decode_response(frame, request_id="r1", phase="transform", name="h") answer = codec.decode_answer(frame, request_id="r1", phase="transform", name="h") - assert isinstance(answer, codec.LookupRequest) and answer.statement == "SELECT 1" + assert isinstance(answer, codec.LookupRequest) and answer.target == "SELECT 1" # Bound to the dispatch in flight (ADR 0147 AC-6): another dispatch's id is refused. with pytest.raises(codec.SandboxCodecError, match="correlation mismatch"): codec.decode_answer(frame, request_id="r2", phase="transform", name="h") @@ -537,23 +538,30 @@ async def test_a_brokered_write_is_refused_by_the_executors_read_only_test( def test_isolated_with_no_config_dir_refuses_rather_than_run_in_process(tmp_path: Path) -> None: from messagefoundry.pipeline.wiring_runner import RegistryRunner - registry = Registry() # a graph built in code: no source_dir, no snapshot - runner = RegistryRunner( - registry, - object(), # type: ignore[arg-type] - sandbox_policy=SandboxPolicy(mode=SandboxMode.ISOLATED), - egress=EgressSettings(deny_by_default=False), - ) - with pytest.raises(SandboxError, match="never runs a Router or Handler in the engine"): - runner._sandbox_for("IB_ANY") + def runner_for(mode: SandboxMode) -> RegistryRunner: + return RegistryRunner( + Registry(), # a graph built in code: no source_dir, no snapshot + object(), # type: ignore[arg-type] + sandbox_policy=SandboxPolicy(mode=mode), + sandbox_config_source=(None, None), + egress=EgressSettings(deny_by_default=False), + ) + + # Refused at construction, before anything is received. + with pytest.raises(ValueError, match="never runs a Router or Handler in the engine"): + runner_for(SandboxMode.ISOLATED) # mode=subprocess keeps its documented degradation to in-process for such a graph. - runner._sandbox_policy = SandboxPolicy(mode=SandboxMode.SUBPROCESS) + runner = runner_for(SandboxMode.SUBPROCESS) assert runner._sandbox_for("IB_ANY") is None + # And the per-message backstop, for a policy that reaches the runner by another route. + runner._sandbox_policy = SandboxPolicy(mode=SandboxMode.ISOLATED) + with pytest.raises(SandboxError, match="never runs a Router or Handler in the engine"): + runner._sandbox_for("IB_ANY") @pytest.mark.parametrize("mode", [SandboxMode.SUBPROCESS, SandboxMode.ISOLATED]) async def test_fusion_is_disabled_in_every_worker_mode( - mode: SandboxMode, caplog: pytest.LogCaptureFixture + mode: SandboxMode, caplog: pytest.LogCaptureFixture, tmp_path: Path ) -> None: from messagefoundry.pipeline.wiring_runner import RegistryRunner @@ -561,9 +569,149 @@ async def test_fusion_is_disabled_in_every_worker_mode( Registry(), object(), # type: ignore[arg-type] sandbox_policy=SandboxPolicy(mode=mode), + sandbox_config_source=(str(tmp_path), None), egress=EgressSettings(deny_by_default=False), ) runner._fuse_thread_hops = True with caplog.at_level("WARNING"): assert await runner._activate_fusion() is False assert any(f"[sandbox].mode={mode.value}" in r.getMessage() for r in caplog.records) + + +# --- 5. round-one review fixes --------------------------------------------------------------------- + + +@pytest.mark.parametrize("kind", [codec.LOOKUP_DB, codec.LOOKUP_FHIR]) +def test_a_lookup_with_long_arguments_round_trips(kind: str) -> None: + # Every slot past the inline threshold rides a segment, and segments must be numbered in the + # order the decoder reads them: connection, target, then params. + long = "x" * 5000 + params: Any = {"k": long} if kind == codec.LOOKUP_DB else {"identifier": FhirToken("S", long)} + frame = codec.encode_lookup( + request_id="r1", + phase="transform", + name="h", + seq=1, + kind=kind, + connection="C" * 5000, + target="SELECT " + long if kind == codec.LOOKUP_DB else "Patient" + long, + params=params, + ) + answer = codec.decode_answer(frame, request_id="r1", phase="transform", name="h") + assert isinstance(answer, codec.LookupRequest) + assert answer.connection == "C" * 5000 and answer.params == params + + +def _worker_broker() -> Any: + """The worker's ``_Broker``. Importing the worker installs a stderr root handler, which is right + in its child process and wrong here, so the root logger is restored after the import, as the + ``sandbox_run_one`` fixture in tests/test_db_lookup.py does.""" + import logging + + root = logging.getLogger() + handlers, level = list(root.handlers), root.level + try: + from messagefoundry.pipeline import _sandbox_worker + finally: + for h in list(root.handlers): + root.removeHandler(h) + for h in handlers: + root.addHandler(h) + root.setLevel(level) + return _sandbox_worker._Broker + + +def test_a_runner_serves_only_the_dispatch_it_was_published_for() -> None: + import io + from types import SimpleNamespace + + from messagefoundry.config.db_lookup import DbLookupError + + broker_cls = _worker_broker() + out = io.BytesIO() + broker = broker_cls(io.BytesIO(), out) + first = SimpleNamespace(request_id="a", phase="transform", name="h") + second = SimpleNamespace(request_id="b", phase="transform", name="h") + broker.begin(first) + stale_db, _ = broker.runners(first) + broker.end() + broker.begin(second) + # A thread left over from the first message still holds its runner: refused, nothing written. + with pytest.raises(DbLookupError, match="outside the Handler dispatch"): + stale_db("LK", "SELECT 1", None) + assert out.getvalue() == b"" + assert not broker.broken + + +def test_an_oversized_lookup_fails_the_call_and_keeps_the_pipe_in_step( + monkeypatch: pytest.MonkeyPatch, +) -> None: + import io + from types import SimpleNamespace + + from messagefoundry.config.db_lookup import DbLookupError + + monkeypatch.setattr(codec, "MAX_FRAME", 1024) + broker_cls = _worker_broker() + out = io.BytesIO() + broker = broker_cls(io.BytesIO(), out) + req = SimpleNamespace(request_id="a", phase="transform", name="h") + broker.begin(req) + db, _ = broker.runners(req) + with pytest.raises(DbLookupError, match="cannot cross the sandbox"): + db("LK", "SELECT 1", {"k": "x" * 4096}) + assert out.getvalue() == b"" and not broker.broken + + +def test_a_worker_that_stops_reading_cannot_hold_the_dispatch_past_the_cap( + graph: tuple[Registry, str], +) -> None: + # A Handler forges a lookup whose reply is far larger than the pipe buffer, then sleeps instead + # of reading it. Unbounded, the parent's write of the reply blocks with the session lock held, + # and the wall cap never fires. The cap must still end the dispatch. + def big(connection: str, statement: str, params: Any) -> list[dict[str, Any]]: + return [{"v": "x" * 2_000_000}] + + _, config_dir = graph + source = (Path(config_dir) / "graph.py").read_text(encoding="utf-8") + _FORGER + (Path(config_dir) / "graph.py").write_text(source, encoding="utf-8") + registry = load_config(config_dir) + session = SandboxSession( + SandboxPolicy( + mode=SandboxMode.ISOLATED, wall_seconds=3.0, lookup_allowed_db=("db.example",) + ), + inbound="IB_I", + config_dir=config_dir, + env=None, + graph=None, + ) + try: + started = time.monotonic() + with db_lookup_mod.activated(big), pytest.raises(SandboxError, match="wall cap"): + _bodies(registry, "h_forge_lookup", session) + assert time.monotonic() - started < 30 + finally: + session.close() + + +_FORGER = """ + +@handler("h_forge_lookup") +def h_forge_lookup(msg): + import os, sys, time + + from messagefoundry.pipeline import _sandbox_codec as codec + + # The dispatch's request object is a local of the worker's main loop. + frame = sys._getframe() + while frame is not None and "req" not in frame.f_locals: + frame = frame.f_back + req = frame.f_locals["req"] + body = codec.encode_lookup( + request_id=req.request_id, phase=req.phase, name=req.name, seq=1, kind="db", + connection="LK", target="SELECT 1", params=None, + ) + os.write(1, len(body).to_bytes(4, "big") + body) + time.sleep(60) # never reads the reply + return Send("OB_I", "unreachable") +""" From 415353d7a7d58e63688292ce818e1df4eb6cfa8f Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Wed, 28 Oct 2026 03:00:17 -0500 Subject: [PATCH 04/15] Sandbox broker round-two fixes: refusal order, lookup bounded by the wall cap (BACKLOG #3131) From the round-two code review of this branch: - A lookup with no runner fails with the mode=off "unavailable here" error before any [egress] refusal (db_lookup/fhir_lookup is_available). - The parent serves a lookup and writes its reply on one helper thread joined to the wall cap, so a slow lookup cannot hold the session lock past the cap. _write_frame completes a short raw write. - The worker describes lookup arguments before its non-reentrant lock (prepare_lookup), so a Mapping that itself looks up cannot deadlock; a broken pipe gets its own message; the fhir pre-screen is dropped so the engine's executor screens after its availability check. - The parent's crash text counts the lookups served; stale comments and docs fixed; the ADR ends in a newline. New from-None sites classified. Proposed PR title: Sandbox mode isolated: the parent brokers db_lookup and fhir_lookup (BACKLOG #3131, slice A) Proposed banner #3131: PARTIAL, slice A built. mode="isolated"; worker checks read is-not-OFF; parent broker over the closed-tag codec through the in-process runner (AC-1, AC-2, AC-6 per-inbound, AC-5 for the broker), refusing non-Handler phases and an empty allowed_db/http even under the opt-out. Remains: AC-3, AC-4, AC-7, the ADR's open boxes, the benchmark, and pooled-worker AC-6 scoping. --- changelog.d/3131.added.md | 4 +- docs/CONFIGURATION.md | 17 +-- ...e-ipc-brokered-sandbox-extends-adr-0087.md | 8 +- messagefoundry/config/db_lookup.py | 8 ++ messagefoundry/config/fhir_lookup.py | 8 ++ messagefoundry/pipeline/_sandbox_codec.py | 72 ++++++++---- messagefoundry/pipeline/_sandbox_worker.py | 59 +++++----- messagefoundry/pipeline/sandbox.py | 106 ++++++++++++------ messagefoundry/pipeline/wiring_runner.py | 11 +- tests/test_from_none_is_not_redaction.py | 12 +- tests/test_sandbox_isolated.py | 104 ++++++++++++++++- 11 files changed, 300 insertions(+), 109 deletions(-) diff --git a/changelog.d/3131.added.md b/changelog.d/3131.added.md index 1f4901a592..1115351080 100644 --- a/changelog.d/3131.added.md +++ b/changelog.d/3131.added.md @@ -3,8 +3,8 @@ worker pipe and answered by the same lookup runner `mode = "off"` uses, so it meets the same checks. The engine refuses a brokered lookup bound to a Router or an `accepts=` predicate, and one whose `[egress].allowed_db` or `[egress].allowed_http` list is empty, even under the audited - opt-out `[security].block_unlisted_outbound = false`. `wall_seconds` counts the time the engine - spends serving a call's lookups, though a lookup already being served runs to its own timeout. + opt-out `[security].block_unlisted_outbound = false`. `wall_seconds` bounds the whole call, + the time the engine spends serving its lookups included. This mode adds no operating-system confinement yet: an `isolated` worker can open files and sockets exactly as a `subprocess` worker can. See [CONFIGURATION.md](../docs/CONFIGURATION.md), section `[sandbox]`. (vault `BACKLOG #3131`) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index a6e19df2ae..511b1c5f0c 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1261,10 +1261,10 @@ matters most in the other direction: setting `mode="off"` because one Handler ne takes every other Router and Handler out of the sandbox too. To isolate some feeds and not others you need separate engine processes. -**`mode="isolated"` is `subprocess` plus a lookup broker, and nothing more yet** ([ADR +**`mode="isolated"` is `subprocess` plus a lookup broker** ([ADR 0147](adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md), -vault BACKLOG #3131). It runs the same per-inbound worker, under the same caps and the same codec, and -everything this section says about `subprocess` holds for it except the lookups: +vault BACKLOG #3131). It runs the same per-inbound worker, under the same caps and the same codec. +It differs from `subprocess` in at least these ways: - **A Handler's `db_lookup`/`fhir_lookup` works.** The worker sends the call to the engine over the worker pipe, and the engine answers it with the same lookup runner it uses at `mode="off"`, against @@ -1278,15 +1278,16 @@ everything this section says about `subprocess` holds for it except the lookups: `[egress].allowed_http` for `fhir_lookup`. **That holds under the audited opt-out `[security].block_unlisted_outbound = false` too,** where an empty list would otherwise mean unrestricted, so the opt-out cannot open a lookup from the worker to an unlisted host. The first - refusal per inbound and connection logs a `WARNING`; each refused call fails like any lookup - error. + refusal per inbound and list logs a `WARNING`; each refused call fails like any lookup error. + A call made where no lookup runner exists fails with the `mode="off"` error first, so a graph + that declares no `DatabaseLookup` is never told to edit `[egress]`. - **It is still a Handler-only call.** A Router or an `accepts=` predicate that calls either lookup fails as it does at `mode="off"`, and the engine refuses a brokered call bound to either of them. The dry run has no lookup runner, so it fails there as in every mode. - **`wall_seconds` counts the lookups.** The cap bounds the whole call, including the time the engine - spends serving its lookups, so raise it for a feed whose lookups are slow. A lookup the engine is - serving finishes or times out on its own bound before the cap is checked, so one call can outrun - the cap by up to one lookup's timeout. + spends serving its lookups, so raise it for a feed whose lookups are slow. When the cap runs out + during a lookup, the call fails at once and its worker is recycled; the lookup itself runs on to + its own timeout in the engine, and its result is discarded. - **It needs a config directory.** A graph built in code with none cannot start a worker. Under `subprocess` such a graph quietly runs in-process; under `isolated` the runner refuses to be built instead, because in-process its lookups would not go through the broker. diff --git a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md index c9a21a4b2f..d470db8a3c 100644 --- a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md +++ b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md @@ -193,12 +193,12 @@ worker, whose lane the session already is). The parent answers it on the dispatc same accessor and runner the in-process path uses (AC-1, AC-2), refuses one bound to a Router or `accepts=` dispatch, and refuses one whose `[egress].allowed_db` or `[egress].allowed_http` is empty. A refused or failed lookup is the Handler's error, routed to `ERROR`/dead-letter post-ACK (AC-5, for -the broker). `wall_seconds` counts the brokered lookups, and the parent's write of each reply is -bounded by it, but a lookup the parent is serving is not interrupted: one dispatch can outlast the -cap by up to one lookup's own timeout before it fails on the cap. +the broker). `wall_seconds` bounds the whole dispatch, the brokered lookups and the writes of their +replies included; a lookup the cap cuts off runs on to its own timeout in the parent, and its result +is discarded. **Not built:** decision (2), the OS confinement: AC-3 (Landlock and seccomp), AC-4 (the AppContainer token), and AC-7 (no opening or signalling the parent or a sibling). The first two open boxes above, the #3065 box and the throughput benchmark still gate them. AC-6's scoping for a pooled worker (ADR 0187) is not built either, because no pooled worker exists yet. Until AC-3 and AC-4 land, an -`isolated` worker is confined exactly as a `subprocess` worker is. \ No newline at end of file +`isolated` worker is confined exactly as a `subprocess` worker is. diff --git a/messagefoundry/config/db_lookup.py b/messagefoundry/config/db_lookup.py index ecfc79263a..5aa783f425 100644 --- a/messagefoundry/config/db_lookup.py +++ b/messagefoundry/config/db_lookup.py @@ -40,6 +40,7 @@ "DbLookupError", "LookupRunner", "db_lookup", + "is_available", "set_active", "reset", "activated", @@ -93,6 +94,13 @@ def activated(runner: LookupRunner | None) -> Iterator[None]: _active.reset(token) +def is_available() -> bool: + """Whether a lookup runner is published here, so :func:`db_lookup` would run rather than raise + its "unavailable here" error. The ``mode=isolated`` broker asks this before any refusal of its + own, so a call that would fail that way at ``mode=off`` fails that way there too (ADR 0147).""" + return _active.get() is not None + + def db_lookup( connection: str, statement: str, params: Mapping[str, Any] | None = None ) -> list[dict[str, Any]]: diff --git a/messagefoundry/config/fhir_lookup.py b/messagefoundry/config/fhir_lookup.py index fa052f5ea6..224c168377 100644 --- a/messagefoundry/config/fhir_lookup.py +++ b/messagefoundry/config/fhir_lookup.py @@ -53,6 +53,7 @@ "FhirLookupError", "FhirLookupRunner", "fhir_lookup", + "is_available", "set_active", "reset", "activated", @@ -115,6 +116,13 @@ def activated(runner: FhirLookupRunner | None) -> Iterator[None]: _active.reset(token) +def is_available() -> bool: + """Whether a FHIR-lookup runner is published here, so :func:`fhir_lookup` would run rather than + raise its "unavailable here" error. The ``mode=isolated`` broker asks this before any refusal of + its own (ADR 0147).""" + return _active.get() is not None + + def fhir_lookup( connection: str, query: str, diff --git a/messagefoundry/pipeline/_sandbox_codec.py b/messagefoundry/pipeline/_sandbox_codec.py index 1172f035a6..2bf768112c 100644 --- a/messagefoundry/pipeline/_sandbox_codec.py +++ b/messagefoundry/pipeline/_sandbox_codec.py @@ -84,6 +84,7 @@ "LOOKUP_DB", "LOOKUP_FHIR", "LookupRequest", + "PreparedLookup", "Request", "Response", "build_payload", @@ -99,9 +100,11 @@ "encode_fail", "encode_frame", "encode_lookup", + "encode_prepared_lookup", "encode_lookup_fail", "encode_lookup_ok", "encode_ok", + "prepare_lookup", "encode_ready", "encode_request", "enc_value", @@ -112,7 +115,7 @@ class SandboxError(RuntimeError): """A sandboxed Router/Handler was denied, timed out, crashed, or could not be marshalled. - Raised only on the ``subprocess`` path (``off`` never raises this). The caller treats it exactly + Raised only in a worker mode, ``subprocess`` or ``isolated`` (``off`` never raises this). The caller treats it exactly like any other post-ACK router/transform failure: ``ERROR``/dead-letter, no NAK. Defined **here**, not in :mod:`messagefoundry.pipeline.sandbox`, only because @@ -1423,20 +1426,24 @@ def _dec_fhir_params(node: Any, reader: _Reader) -> dict[str, Any] | None: return out -def encode_lookup( - *, - request_id: str, - phase: str, - name: str, - seq: int, - kind: str, - connection: object, - target: object, - params: object, -) -> bytes: - """A ``lookup`` frame (child side). ``target`` is the statement of a db lookup or the query of a - fhir one. Raises :class:`SandboxCodecError` for an argument outside the grammar, which the worker - turns into the lookup's own error type, as the in-process call would fail on a bad argument.""" +@dataclass(frozen=True) +class PreparedLookup: + """A ``lookup`` frame's arguments, described but not yet framed. Describing them runs the + Handler's own code (a ``Mapping`` it passed is read), so the worker does it before it takes its + broker lock; :func:`encode_prepared_lookup` then frames it with no user code at all.""" + + kind: str + slots: dict[str, Any] + blobs: tuple[str, ...] + + +def prepare_lookup( + *, kind: str, connection: object, target: object, params: object +) -> PreparedLookup: + """Describe one lookup's arguments. ``target`` is the statement of a db lookup or the query of + a fhir one. Raises :class:`SandboxCodecError` for an argument outside the grammar, which the + worker turns into the lookup's own error type, as the in-process call would fail on a bad + argument.""" blobs = _Blobs() # Segment order IS walk order (see _Reader): connection, then target, then params, the order # decode_answer reads them in. Building params first would number its segments ahead of the @@ -1453,20 +1460,45 @@ def encode_lookup( enc_params = _enc_fhir_params(params, blobs) else: raise SandboxCodecError(f"unknown lookup kind {kind!r}") + return PreparedLookup( + kind=kind, + slots={"connection": conn_ref, "target": target_ref, "params": enc_params}, + blobs=tuple(blobs.items), + ) + + +def encode_prepared_lookup( + prepared: PreparedLookup, *, request_id: str, phase: str, name: str, seq: int +) -> bytes: + """Frame a :class:`PreparedLookup` for the dispatch in flight (child side).""" header = _envelope( { "id": request_id, "phase": phase, "name": name, "seq": seq, - "kind": kind, - "connection": conn_ref, - "target": target_ref, - "params": enc_params, + "kind": prepared.kind, + **prepared.slots, }, "lookup", ) - return encode_frame(header, blobs.items) + return encode_frame(header, prepared.blobs) + + +def encode_lookup( + *, + request_id: str, + phase: str, + name: str, + seq: int, + kind: str, + connection: object, + target: object, + params: object, +) -> bytes: + """:func:`prepare_lookup` and :func:`encode_prepared_lookup` in one call.""" + prepared = prepare_lookup(kind=kind, connection=connection, target=target, params=params) + return encode_prepared_lookup(prepared, request_id=request_id, phase=phase, name=name, seq=seq) def decode_answer( diff --git a/messagefoundry/pipeline/_sandbox_worker.py b/messagefoundry/pipeline/_sandbox_worker.py index d4133613ec..86895a56a6 100644 --- a/messagefoundry/pipeline/_sandbox_worker.py +++ b/messagefoundry/pipeline/_sandbox_worker.py @@ -9,7 +9,7 @@ decode path cannot name a type, import a module, or reach ``__reduce__``. Nothing is pickled in either direction, in either process. -1. **Bootstrap** — reads one ``boot`` frame ``{snapshot, forbidden, mem_mb, code_sets}``, loads +1. **Bootstrap** — reads one ``boot`` frame ``{snapshot, forbidden, mem_mb, code_sets, broker}``, loads the message :class:`~messagefoundry.config.wiring.Registry` from the engine's config snapshot (``load_snapshot``, the loader the engine uses, over the bytes the engine loaded and checked; the worker never reads the config directory, vault BACKLOG #2596), @@ -247,7 +247,6 @@ def runners(self, req: Any) -> tuple[Any, Any]: """The ``(db_lookup, fhir_lookup)`` runners for dispatch ``req`` alone.""" from messagefoundry.config.db_lookup import DbLookupError from messagefoundry.config.fhir_lookup import FhirLookupError - from messagefoundry.fhirsearch import resolve_search_pairs from messagefoundry.pipeline._sandbox_codec import LOOKUP_DB, LOOKUP_FHIR def db(connection: str, statement: str, params: Any = None) -> Any: @@ -256,14 +255,8 @@ def db(connection: str, statement: str, params: Any = None) -> Any: ) def fhir(connection: str, query: str, params: Any = None) -> Any: - if params is not None: - # fhirsearch's own check, so a value of the wrong kind or carrying a separator fails - # with the words the engine's executor would give it at mode=off. The engine checks - # again on its side. - try: - resolve_search_pairs(params) - except ValueError as exc: - raise FhirLookupError(f"fhir_lookup on {connection!r}: {exc}") from None + # The engine's executor screens each search value, as at mode=off, and after its own + # "unavailable here" check, so that is the error a call with no runner gets. return self._ask( req, LOOKUP_FHIR, FhirLookupError, "fhir_lookup", connection, query, params ) @@ -284,36 +277,40 @@ def _ask( from messagefoundry.pipeline._sandbox_codec import MAX_FRAME, SandboxError from messagefoundry.pipeline.sandbox import _read_frame_bytes, _write_frame + try: + # Before the lock: describing the arguments reads the Handler's own Mapping, whose code + # could call a lookup itself, and the lock is not re-entrant. + prepared = codec.prepare_lookup( + kind=kind, connection=connection, target=target, params=params + ) + except SandboxError as exc: + # A bad argument fails as the call would in-process: the lookup's own error. Nothing is + # written, so the pipe stays in step. + raise error(f"{fn}: its arguments cannot cross the sandbox: {exc}") from None with self._lock: - if self._req is not req or self.broken: + if self.broken: + raise error(f"{fn}: an earlier lookup in this dispatch broke the sandbox pipe") + if self._req is not req: raise error(f"{fn}: called outside the Handler dispatch it was published for") - self._seq += 1 - seq = self._seq - try: - frame = codec.encode_lookup( - request_id=req.request_id, - phase=req.phase, - name=req.name, - seq=seq, - kind=kind, - connection=connection, - target=target, - params=params, + frame = codec.encode_prepared_lookup( + prepared, + request_id=req.request_id, + phase=req.phase, + name=req.name, + seq=self._seq + 1, + ) + if len(frame) > MAX_FRAME: + raise error( + f"{fn}: its arguments are too large for the sandbox ({len(frame)} bytes)" ) - if len(frame) > MAX_FRAME: - raise SandboxError(f"the lookup frame is {len(frame)} bytes, over the cap") - except SandboxError as exc: - # A bad or oversized argument fails as the call would in-process: the lookup's own - # error. Nothing was written, so the pipe is still in step; the next call reuses seq. - self._seq -= 1 - raise error(f"{fn}: its arguments cannot cross the sandbox: {exc}") from None + self._seq += 1 try: _write_frame(self._stdout, frame) body = _read_frame_bytes(self._stdin) if body is None: raise SandboxError("the engine closed the pipe") ok, result, reason = codec.decode_lookup_reply( - body, request_id=req.request_id, phase=req.phase, name=req.name, seq=seq + body, request_id=req.request_id, phase=req.phase, name=req.name, seq=self._seq ) except (OSError, SandboxError) as exc: # A pipe the parent abandoned, or a reply that does not answer this call: the pipe no diff --git a/messagefoundry/pipeline/sandbox.py b/messagefoundry/pipeline/sandbox.py index 94344017ee..0783f167d4 100644 --- a/messagefoundry/pipeline/sandbox.py +++ b/messagefoundry/pipeline/sandbox.py @@ -92,6 +92,7 @@ from __future__ import annotations +import contextvars import enum import logging import queue @@ -110,7 +111,9 @@ from messagefoundry.childenv import python_child_argv, worker_environment from messagefoundry.config.code_sets import CodeSet from messagefoundry.config.db_lookup import DbLookupError, db_lookup +from messagefoundry.config.db_lookup import is_available as db_lookup_available from messagefoundry.config.fhir_lookup import FhirLookupError, fhir_lookup +from messagefoundry.config.fhir_lookup import is_available as fhir_lookup_available from messagefoundry.config.run_context import RunContext from messagefoundry.config.settings import ( INSECURE_CONFIG_SOURCE_ESCAPE_ENV, @@ -192,8 +195,8 @@ class SandboxPolicy: ``mode=off`` (the shipped default) is the zero-overhead, byte-identical parity mode. ``wall_seconds`` is the **authoritative** cap on every platform — the parent kills a worker that overruns it (so a - pathological busy-loop Router/Handler can never wedge intake), and it is enforced **only** at - ``mode=subprocess``: the in-process path has no timeout at all. There is no ``RLIMIT_CPU``, so + pathological busy-loop Router/Handler can never wedge intake), and it is enforced **only** in + a worker mode (``subprocess``, ``isolated``): the in-process path has no timeout at all. There is no ``RLIMIT_CPU``, so CPU that admin code spends outside a call has no bound (ADR 0087, amendment of 2026-10-07). ``mem_mb`` adds a POSIX memory backstop *inside* the child where the ``resource`` module exists (a no-op on Windows). On Linux it is headroom above the loaded graph: ``RLIMIT_DATA`` at the @@ -276,7 +279,7 @@ def _bounded(text: str) -> str: def _shown(names: set[str] | frozenset[str]) -> str: """Up to :data:`_NAMES_SHOWN` names for an error text. Half of them come from the worker, so each is scrubbed and cut before it reaches a log line.""" - listed = [scrub_control_chars(name)[:80] for name in sorted(names)[:_NAMES_SHOWN]] + listed = [_shown_name(name) for name in sorted(names)[:_NAMES_SHOWN]] more = len(names) - len(listed) return ", ".join(listed) + (f" and {more} more" if more > 0 else "") @@ -513,11 +516,23 @@ def _write_frame(stream: Any, body: bytes) -> None: pipe; the caller maps either to :class:`SandboxError`.""" if len(body) > _MAX_FRAME: raise SandboxError(f"sandbox frame too large: {len(body)} bytes") - stream.write(_LEN.pack(len(body))) - stream.write(body) + _write_all(stream, _LEN.pack(len(body))) + _write_all(stream, body) stream.flush() +def _write_all(stream: Any, data: bytes) -> None: + """Write every byte of ``data``. The parent's end of the worker pipe is unbuffered + (``bufsize=0``), and a raw ``write`` may return a short count, as a large one interrupted by a + signal can; dropping the remainder would hand the worker a torn frame.""" + view = memoryview(data) + while view: + written = stream.write(view) + if not written: # None is a non-blocking raw stream with no room; the pipes here block + raise OSError("sandbox pipe accepted no bytes") + view = view[written:] + + def _read_exact(stream: Any, n: int) -> bytes | None: """Read exactly ``n`` bytes, or ``None`` on EOF (a closed/crashed peer).""" chunks: list[bytes] = [] @@ -1023,12 +1038,9 @@ def _await_answer( :func:`codec.decode_response`, which refuses a ``lookup`` frame, so only an isolated session ever brokers a lookup and no worker can ask another session to. - ``wall_seconds`` counts the lookups the parent serves for the dispatch, so the cap stays the - bound it is at ``mode=subprocess``, and a feed whose lookups are slow needs a larger cap. A - lookup the parent is serving is not interrupted when the cap runs out: its own bound (the - runner's result timeout) ends it, and the dispatch then fails on the cap. So one dispatch can - outlast ``wall_seconds`` by at most one lookup's bound. The reply write is bounded by the cap - too (:meth:`_write_reply`).""" + ``wall_seconds`` bounds the whole dispatch, the lookups the parent serves for it included + (:meth:`_serve_bounded`), so the cap stays the bound it is at ``mode=subprocess`` and a feed + whose lookups are slow needs a larger cap.""" brokered = self.policy.mode is SandboxMode.ISOLATED deadline = time.monotonic() + self.policy.wall_seconds expected_seq = 1 @@ -1044,7 +1056,9 @@ def _await_answer( ) from None if frame is _EOF: self._kill(proc) - raise SandboxError(f"sandbox worker crashed while running {phase} {name!r}") + served = expected_seq - 1 + after = f", after {served} brokered lookup(s)" if served else "" + raise SandboxError(f"sandbox worker crashed while running {phase} {name!r}{after}") try: # Decoding happens HERE, on the dispatch thread, inside this try — not on the daemon # reader thread, where a rejection would be a silent reader death and a wall-cap hang. @@ -1067,38 +1081,53 @@ def _await_answer( if isinstance(answer, codec.Response): return answer expected_seq += 1 - reply = self._lookup_reply(request_id, phase, name, answer) - if not self._write_reply(proc, reply, deadline) or time.monotonic() > deadline: + if not self._serve_bounded(proc, request_id, phase, name, answer, deadline): self._kill(proc) raise SandboxError( f"sandbox {phase} {name!r} exceeded the {self.policy.wall_seconds}s wall cap, " "which counts the lookups the engine served for it" ) - def _write_reply(self, proc: subprocess.Popen[bytes], body: bytes, deadline: float) -> bool: - """Write one lookup reply, bounded by the dispatch's wall cap; ``False`` if the cap ran out. - - On its own thread, because a reply can be larger than the pipe's buffer, and a worker that - sent a ``lookup`` frame and then stopped reading stdin would otherwise block this thread for - good while it holds the session lock: no wall cap, and a reload's ``close`` waiting behind it. - The caller kills the worker when the cap runs out, which closes the pipe and ends the write. - A broken pipe raises :class:`SandboxError` after the worker is killed.""" + def _serve_bounded( + self, + proc: subprocess.Popen[bytes], + request_id: str, + phase: str, + name: str, + lookup: codec.LookupRequest, + deadline: float, + ) -> bool: + """Serve one lookup and write its reply, within the dispatch's wall cap; ``False`` if the + cap ran out first, and the caller then kills the worker. + + Both halves run on a helper thread, because either can block past the cap while this thread + holds the session lock, and a reload's ``close`` waits on that lock. The lookup can wait on + its runner for the runner's own result timeout, longer than the default cap. The reply can + be larger than the pipe's buffer, and a worker that sent a ``lookup`` frame and then stopped + reading stdin would hold the write for good. The thread runs in a copy of this thread's + context, so it sees the lookup runner the transform worker published. A lookup still running + when the cap ends is left to finish on its own bound; the kill closes the pipe, which ends a + blocked write. A broken pipe raises :class:`SandboxError` after the worker is killed.""" assert proc.stdin is not None stdin = proc.stdin failure: list[BaseException] = [] - def write() -> None: + def serve() -> None: try: - _write_frame(stdin, body) + _write_frame(stdin, self._lookup_reply(request_id, phase, name, lookup)) except (OSError, ValueError, SandboxError) as exc: # ValueError: a pipe closed under it failure.append(exc) - writer = threading.Thread( - target=write, name=f"mf-sandbox-reply-{self._inbound}", daemon=True + context = contextvars.copy_context() + helper = threading.Thread( + target=context.run, + args=(serve,), + name=f"mf-sandbox-lookup-{self._inbound}", + daemon=True, ) - writer.start() - writer.join(max(0.0, deadline - time.monotonic())) - if writer.is_alive(): + helper.start() + helper.join(max(0.0, deadline - time.monotonic())) + if helper.is_alive(): return False if failure: self._kill(proc) @@ -1132,18 +1161,21 @@ def _serve_lookup(self, phase: str, lookup: codec.LookupRequest) -> tuple[bool, """Answer one brokered lookup in the parent: ``(ok, result, error)``. The answer comes from the same ``db_lookup``/``fhir_lookup`` accessor a Handler calls at - ``mode=off``, on this dispatch thread, so it resolves the runner the transform worker + ``mode=off``, in this dispatch's context, so it resolves the runner the transform worker published for this transform and nothing else (ADR 0147 AC-2). Where none is published, as on - a Router, in a dry run, on the inline fast path or in a graph that declares no lookup, it - raises exactly what it raises at ``mode=off``. The executor behind the runner was built + a Router, in a dry run, on the inline fast path or in a graph that declares no lookup, the call + raises exactly what it raises at ``mode=off``, and it is made before any refusal of the + broker's own, so that is the error the Handler sees. The executor behind the runner was built against ``[egress]`` and runs its own statement test and row ceiling, so each applies here as in-process. The slice-B seam: a broker-only check of a lookup's own arguments goes between the refusal below and the call.""" - refusal = self._lookup_refusal(phase, lookup) + is_db = lookup.kind == codec.LOOKUP_DB + available = db_lookup_available() if is_db else fhir_lookup_available() + refusal = self._lookup_refusal(phase, lookup) if available else None if refusal is not None: return False, None, refusal try: - if lookup.kind == codec.LOOKUP_DB: + if is_db: return True, db_lookup(lookup.connection, lookup.target, lookup.params), "" return True, fhir_lookup(lookup.connection, lookup.target, lookup.params), "" except (DbLookupError, FhirLookupError) as exc: @@ -1158,7 +1190,8 @@ def _lookup_refusal(self, phase: str, lookup: codec.LookupRequest) -> str | None A lookup is answered only for a Handler, so one bound to a Router or an ``accepts=`` dispatch is refused even if a worker sends it; the child has no lookup runner in those phases either. And one whose ``[egress]`` list is empty is refused, under the ruling ``docs/CONFIGURATION.md`` - states in section ``[sandbox]``, *``mode="isolated"``* (vault BACKLOG #3131).""" + states in section ``[sandbox]``, *``mode="isolated"``* (vault BACKLOG #3131). The first such + refusal per session and list logs a WARNING.""" fn = _LOOKUP_FN[lookup.kind] shown = _shown_name(lookup.connection) if phase != "transform": @@ -1173,8 +1206,7 @@ def _lookup_refusal(self, phase: str, lookup: codec.LookupRequest) -> str | None if allowed: return None if key not in self._lookup_refusals_logged: - # Once per session and list: the cause is the empty list, whatever connection a worker - # names, so the set holds at most two entries. Each refused call still fails. + # Keyed by the list, whatever connection a worker names, so it holds at most two entries. self._lookup_refusals_logged.add(key) log.warning( "inbound %r: a brokered %s on %r was refused: [egress].%s is empty " diff --git a/messagefoundry/pipeline/wiring_runner.py b/messagefoundry/pipeline/wiring_runner.py index fa78a19559..066eba506b 100644 --- a/messagefoundry/pipeline/wiring_runner.py +++ b/messagefoundry/pipeline/wiring_runner.py @@ -5483,7 +5483,7 @@ def _engine_graph_shape(self) -> GraphShape: async def _close_sandbox_sessions(self) -> None: """Drop every sandbox session and close its worker, off the loop: each ``close()`` waits on - a process. A no-op unless ``[sandbox].mode=subprocess`` spawned any. The next dispatch on an + a process. A no-op unless a worker sandbox mode spawned any. The next dispatch on an inbound makes a fresh session, whose worker loads the config as it is then. A dispatch already holding a closed session is retried or re-pended by :meth:`_off_loop_sandboxed`, never dead-lettered as a Router or Handler fault.""" @@ -9896,10 +9896,11 @@ async def _prepare_routed( lookup_stack.enter_context(db_lookup_activated(self._run_lookup)) if self._fhir_lookup_executor is not None: lookup_stack.enter_context(fhir_lookup_activated(self._run_fhir_lookup)) - # ADR 0087 (#197): sandbox=subprocess marshals the Handler to the per-inbound - # worker (transform_rc travels with it); a db_lookup/fhir_lookup inside a - # sandboxed Handler fails closed there (they can't bridge across the process - # boundary in this PR). sandbox=None is the byte-identical in-process path. + # ADR 0087 (#197): a worker sandbox mode marshals the Handler to the per-inbound + # worker (transform_rc travels with it). A db_lookup/fhir_lookup inside it fails + # closed at mode=subprocess, and at mode=isolated is brokered back to this + # thread, which is why the runners above are published either way (ADR 0147). + # sandbox=None is the byte-identical in-process path. outcome = await self._off_loop_sandboxed( name, transform_one, diff --git a/tests/test_from_none_is_not_redaction.py b/tests/test_from_none_is_not_redaction.py index 1fe60d0ac3..158cbd263b 100644 --- a/tests/test_from_none_is_not_redaction.py +++ b/tests/test_from_none_is_not_redaction.py @@ -221,9 +221,19 @@ class _Allowed: _SAFE + "queue.Empty carries nothing", ), _Allowed( - "messagefoundry/pipeline/sandbox.py::SandboxSession.dispatch::Empty::SandboxError", + "messagefoundry/pipeline/sandbox.py::SandboxSession._await_answer::Empty::SandboxError", _SAFE + "queue.Empty carries nothing", ), + _Allowed( + "messagefoundry/pipeline/_sandbox_worker.py::_Broker._ask::SandboxError::error", + _SAFE + "the lookup error quotes the codec refusal, which names a slot, a type or a size " + "and never a value; the cause is dropped so the Handler sees the mode=off error type alone", + ), + _Allowed( + "messagefoundry/pipeline/_sandbox_worker.py::_Broker._ask::(OSError, SandboxError)::error", + _SAFE + "the lookup error quotes the pipe or frame refusal, which names a frame and never " + "its content (decode_frame keeps only safe_exc text)", + ), _Allowed( "messagefoundry/store/store.py::parse_audit_anchor::ValueError::AuditAnchorError", _SAFE + "int() quotes the anchor's count text, and the new error carries the whole anchor", diff --git a/tests/test_sandbox_isolated.py b/tests/test_sandbox_isolated.py index d83fce78f5..46c7bfd455 100644 --- a/tests/test_sandbox_isolated.py +++ b/tests/test_sandbox_isolated.py @@ -49,6 +49,7 @@ _GRAPH = """ import os +from collections.abc import Mapping from messagefoundry import ( inbound, outbound, router, handler, MLLP, Send, db_lookup, fhir_lookup, FhirToken, FhirRaw, @@ -107,6 +108,24 @@ def h_db_caught(msg): return Send("OB_I", "not refused") +class _LazyParams(Mapping): + # A Mapping whose values come from another lookup, read while the outer call describes it. + def __iter__(self): + return iter(["k"]) + + def __len__(self): + return 1 + + def __getitem__(self, key): + return db_lookup("LK", "SELECT inner", {})[0]["v"] + + +@handler("h_db_nested") +def h_db_nested(msg): + rows = db_lookup("LK", "SELECT outer", _LazyParams()) + return Send("OB_I", rows[0]["v"]) + + @handler("h_fhir") def h_fhir(msg): bundle = fhir_lookup( @@ -658,7 +677,7 @@ def test_an_oversized_lookup_fails_the_call_and_keeps_the_pipe_in_step( req = SimpleNamespace(request_id="a", phase="transform", name="h") broker.begin(req) db, _ = broker.runners(req) - with pytest.raises(DbLookupError, match="cannot cross the sandbox"): + with pytest.raises(DbLookupError, match="too large for the sandbox"): db("LK", "SELECT 1", {"k": "x" * 4096}) assert out.getvalue() == b"" and not broker.broken @@ -715,3 +734,86 @@ def h_forge_lookup(msg): time.sleep(60) # never reads the reply return Send("OB_I", "unreachable") """ + + +# --- 6. round-two review fixes --------------------------------------------------------------------- + + +def test_a_lookup_with_no_runner_is_not_told_to_edit_egress( + graph: tuple[Registry, str], caplog: pytest.LogCaptureFixture +) -> None: + # No runner and an empty list, as a site with no database has: the mode=off error, no WARNING. + registry, config_dir = graph + session = _session(config_dir, SandboxMode.ISOLATED, allowed_db=()) + try: + with ( + caplog.at_level("WARNING", logger="messagefoundry.pipeline.sandbox"), + pytest.raises(SandboxError, match="unavailable here") as raised, + ): + _bodies(registry, "h_db", session) + finally: + session.close() + assert "allowed_db" not in str(raised.value) + assert not [r for r in caplog.records if "was refused" in r.getMessage()] + + +def test_a_slow_lookup_is_cut_off_by_the_wall_cap(graph: tuple[Registry, str]) -> None: + def slow(connection: str, statement: str, params: Any) -> list[dict[str, Any]]: + time.sleep(20) + return [{"v": "late", "d": Decimal(0)}] + + registry, config_dir = graph + session = SandboxSession( + SandboxPolicy(mode=SandboxMode.ISOLATED, wall_seconds=2.0, lookup_allowed_db=("db",)), + inbound="IB_I", + config_dir=config_dir, + env=None, + graph=None, + ) + try: + # Warm the worker first, so the timing below measures the cap rather than a cold start. + _bodies(registry, "h_parent", session) + started = time.monotonic() + with db_lookup_mod.activated(slow), pytest.raises(SandboxError, match="wall cap"): + _bodies(registry, "h_db", session) + assert time.monotonic() - started < 15 + finally: + session.close() + + +def test_a_lookup_made_while_describing_another_does_not_deadlock( + graph: tuple[Registry, str], +) -> None: + calls: list[str] = [] + + def runner(connection: str, statement: str, params: Any) -> list[dict[str, Any]]: + calls.append(statement) + return [{"v": "INNER" if statement == "SELECT inner" else f"OUTER:{params['k']}"}] + + registry, config_dir = graph + session = _session(config_dir, SandboxMode.ISOLATED) + try: + with db_lookup_mod.activated(runner): + assert _bodies(registry, "h_db_nested", session) == ["OUTER:INNER"] + finally: + session.close() + assert calls == ["SELECT inner", "SELECT outer"] + + +def test_a_short_write_is_completed() -> None: + from messagefoundry.pipeline.sandbox import _write_frame + + class Trickle: + def __init__(self) -> None: + self.data = bytearray() + + def write(self, chunk: Any) -> int: + self.data.extend(bytes(chunk)[:3]) + return min(3, len(chunk)) + + def flush(self) -> None: + pass + + out = Trickle() + _write_frame(out, b"0123456789") + assert bytes(out.data) == (10).to_bytes(4, "big") + b"0123456789" From 3c4ba9289c71e859ade5f63f49373acbba788f0f Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Thu, 29 Oct 2026 09:34:31 -0500 Subject: [PATCH 05/15] Sandbox broker: fixes for the Lander hold at 415353d7 (BACKLOG #3131) The four behaviour changes the covering review round-472dd562 found, each with a test that fails on 415353d7: 1. At most one brokered lookup is outstanding per session. A lookup the wall cap cut off is remembered; until it ends, later lookups on the session get an immediate lookupfail instead of piling up. 2. A codec refusal while framing a lookup is the lookup's own error (DbLookupError/FhirLookupError), as at mode=off, not "denied". 3. A wrong-kind fhir search value crosses as a ForeignSearchValue type name, so the engine's executor refuses it with mode=off's words and after its own "unavailable here" check. 4. An unexpected exception while serving a lookup fails the dispatch at once with a fixed cause, logged by type only. Proposed PR title: Sandbox mode isolated: the parent brokers db_lookup and fhir_lookup (BACKLOG #3131, slice A) Proposed banner #3131: PARTIAL, slice A built. mode="isolated"; worker checks read is-not-OFF; parent broker over the closed-tag codec through the in-process runner (AC-1, AC-2, AC-6 per-inbound, AC-5 for the broker), refusing non-Handler phases and an empty allowed_db/http even under the opt-out. Remains: AC-3, AC-4, AC-7, the ADR's open boxes, the benchmark, and pooled-worker AC-6 scoping. --- docs/CONFIGURATION.md | 3 +- ...e-ipc-brokered-sandbox-extends-adr-0087.md | 3 +- messagefoundry/fhirsearch.py | 17 +- messagefoundry/pipeline/_sandbox_codec.py | 17 +- messagefoundry/pipeline/_sandbox_worker.py | 20 ++- messagefoundry/pipeline/sandbox.py | 65 +++++++- tests/test_from_none_is_not_redaction.py | 4 +- tests/test_sandbox_isolated.py | 147 ++++++++++++++++++ 8 files changed, 254 insertions(+), 22 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 511b1c5f0c..30ecca943b 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1287,7 +1287,8 @@ It differs from `subprocess` in at least these ways: - **`wall_seconds` counts the lookups.** The cap bounds the whole call, including the time the engine spends serving its lookups, so raise it for a feed whose lookups are slow. When the cap runs out during a lookup, the call fails at once and its worker is recycled; the lookup itself runs on to - its own timeout in the engine, and its result is discarded. + its own timeout in the engine, and its result is discarded. Until it ends, every other lookup on + that inbound is refused at once, so a hung backend holds at most one lookup per inbound. - **It needs a config directory.** A graph built in code with none cannot start a worker. Under `subprocess` such a graph quietly runs in-process; under `isolated` the runner refuses to be built instead, because in-process its lookups would not go through the broker. diff --git a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md index d470db8a3c..0cbc69dcef 100644 --- a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md +++ b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md @@ -195,7 +195,8 @@ same accessor and runner the in-process path uses (AC-1, AC-2), refuses one boun A refused or failed lookup is the Handler's error, routed to `ERROR`/dead-letter post-ACK (AC-5, for the broker). `wall_seconds` bounds the whole dispatch, the brokered lookups and the writes of their replies included; a lookup the cap cuts off runs on to its own timeout in the parent, and its result -is discarded. +is discarded. While it runs, the session refuses every other lookup, so at most one is outstanding +per inbound. **Not built:** decision (2), the OS confinement: AC-3 (Landlock and seccomp), AC-4 (the AppContainer token), and AC-7 (no opening or signalling the parent or a sibling). The first two open boxes above, diff --git a/messagefoundry/fhirsearch.py b/messagefoundry/fhirsearch.py index ef411dcd88..f0bdcb10bc 100644 --- a/messagefoundry/fhirsearch.py +++ b/messagefoundry/fhirsearch.py @@ -88,6 +88,7 @@ "FhirSearchParams", "FhirSearchValue", "FhirToken", + "ForeignSearchValue", "resolve_search_pairs", ] @@ -137,6 +138,19 @@ class FhirRaw: value: str +@final +@dataclass(frozen=True, slots=True) +class ForeignSearchValue: + """A search value of a kind :func:`fhir_lookup` does not take, carried by its type NAME only. + + Not a kind an author writes. The ``mode=isolated`` sandbox (ADR 0147) sends a Handler's + ``params`` to the engine, and a value of any other type cannot cross the pipe, so it crosses as + this. :func:`resolve_search_pairs` refuses it with the words it gives the original value, in the + engine and after the engine's own checks, so the Handler sees what ``mode=off`` shows it.""" + + type_name: str + + #: One search value: data (``str``), a token whose halves are declared (``FhirToken``), or author #: syntax (``FhirRaw``). FhirSearchValue = str | FhirToken | FhirRaw @@ -173,10 +187,11 @@ def _resolve_one(key: str, value: FhirSearchValue) -> str: return f"{value.system}|{_screen(key, value.code, half='code half of the FhirToken')}" if isinstance(value, FhirRaw): return value.value + got = value.type_name if isinstance(value, ForeignSearchValue) else type(value).__name__ raise ValueError( f"FHIR search parameter {key!r}: a search value must be a str (data), a " f"FhirToken(system, code), or a FhirRaw(...) author-syntax value -- got " - f"{type(value).__name__}. A number is not a kind here: wrap it, str(value). " + f"{got}. A number is not a kind here: wrap it, str(value). " f"(The value is withheld: it may carry PHI.)" ) diff --git a/messagefoundry/pipeline/_sandbox_codec.py b/messagefoundry/pipeline/_sandbox_codec.py index 2bf768112c..fe54259c8d 100644 --- a/messagefoundry/pipeline/_sandbox_codec.py +++ b/messagefoundry/pipeline/_sandbox_codec.py @@ -72,7 +72,7 @@ handler_item_fault, handler_result_items, ) -from messagefoundry.fhirsearch import FhirRaw, FhirToken +from messagefoundry.fhirsearch import FhirRaw, FhirToken, ForeignSearchValue from messagefoundry.parsing.message import Message, RawMessage from messagefoundry.redaction import safe_exc @@ -1362,9 +1362,10 @@ class LookupRequest: def _enc_fhir_params(params: object, blobs: _Blobs) -> Any: """A ``fhir_lookup`` ``params`` mapping in its own small grammar: ``[key, is_list, items]`` per - key, each item ``{"s": str}``, ``{"k": [system, code]}`` or ``{"r": raw}``. Only the three kinds - :mod:`messagefoundry.fhirsearch` takes cross. The worker runs fhirsearch's own check before it - gets here, so a refusal below is a backstop; the parent's executor screens the content.""" + key, each item ``{"s": str}``, ``{"k": [system, code]}``, ``{"r": raw}``, or ``{"x": type name}`` + for a value of any other kind, which arrives as a + :class:`~messagefoundry.fhirsearch.ForeignSearchValue`. Nothing is screened here: the parent's + executor judges every value, after its own "unavailable here" and connection checks.""" if params is None: return None if not isinstance(params, Mapping): @@ -1385,9 +1386,9 @@ def _enc_fhir_params(params: object, blobs: _Blobs) -> Any: elif isinstance(one, FhirRaw): items.append({"r": blobs.ref(one.value)}) else: - raise SandboxCodecError( - f"FHIR search parameter {key!r}: a {type(one).__name__} is not a search value" - ) + # Any other kind crosses by its type name alone, never its value, so the engine's + # executor refuses it with the words it uses at mode=off, after its own checks. + items.append({"x": blobs.ref(type(one).__name__)}) out.append([key, is_list, items]) return out @@ -1418,6 +1419,8 @@ def _dec_fhir_params(node: Any, reader: _Reader) -> dict[str, Any] | None: items.append(FhirToken(reader.text(pair[0]), reader.text(pair[1]))) elif tag == "r": items.append(FhirRaw(reader.text(arg))) + elif tag == "x": + items.append(ForeignSearchValue(reader.text(arg))) else: raise SandboxCodecError(f"unknown fhir_lookup param tag {tag!r}") if not is_list and len(items) != 1: diff --git a/messagefoundry/pipeline/_sandbox_worker.py b/messagefoundry/pipeline/_sandbox_worker.py index 86895a56a6..1a09915d0a 100644 --- a/messagefoundry/pipeline/_sandbox_worker.py +++ b/messagefoundry/pipeline/_sandbox_worker.py @@ -292,13 +292,19 @@ def _ask( raise error(f"{fn}: an earlier lookup in this dispatch broke the sandbox pipe") if self._req is not req: raise error(f"{fn}: called outside the Handler dispatch it was published for") - frame = codec.encode_prepared_lookup( - prepared, - request_id=req.request_id, - phase=req.phase, - name=req.name, - seq=self._seq + 1, - ) + try: + frame = codec.encode_prepared_lookup( + prepared, + request_id=req.request_id, + phase=req.phase, + name=req.name, + seq=self._seq + 1, + ) + except SandboxError as exc: + # A frame the codec refuses (over a header or segment cap) fails as a bad argument + # does, as the lookup's own error, as at mode=off: the Handler can catch it, and + # nothing was written, so the pipe stays in step. + raise error(f"{fn}: its arguments cannot cross the sandbox: {exc}") from None if len(frame) > MAX_FRAME: raise error( f"{fn}: its arguments are too large for the sandbox ({len(frame)} bytes)" diff --git a/messagefoundry/pipeline/sandbox.py b/messagefoundry/pipeline/sandbox.py index 0783f167d4..1344d8cfa3 100644 --- a/messagefoundry/pipeline/sandbox.py +++ b/messagefoundry/pipeline/sandbox.py @@ -724,6 +724,8 @@ def __init__( self._closed = False # The [egress] lists whose empty-list refusal this session has logged. self._lookup_refusals_logged: set[str] = set() + # The helper thread of a lookup the wall cap cut off, while it still runs: see _serve_bounded. + self._stale_lookup: threading.Thread | None = None @property def mode(self) -> SandboxMode: @@ -1105,18 +1107,41 @@ def _serve_bounded( its runner for the runner's own result timeout, longer than the default cap. The reply can be larger than the pipe's buffer, and a worker that sent a ``lookup`` frame and then stopped reading stdin would hold the write for good. The thread runs in a copy of this thread's - context, so it sees the lookup runner the transform worker published. A lookup still running - when the cap ends is left to finish on its own bound; the kill closes the pipe, which ends a - blocked write. A broken pipe raises :class:`SandboxError` after the worker is killed.""" + context, so it sees the lookup runner the transform worker published. The kill closes the + pipe, which ends a blocked write. A broken pipe raises :class:`SandboxError` after the worker + is killed. + + **At most one brokered lookup is outstanding per session.** A lookup the cap cut off runs on + to its own bound (the runner's result timeout), and while it does, every later lookup on this + session is refused at once with a ``lookupfail`` naming it, rather than start another beside + it. Unbounded, a hung backend would gather one abandoned lookup per dispatch, about six per + inbound at the default caps, against the executor and the connection pool. + + **An unexpected fault in the engine fails the dispatch at once,** with a fixed cause and the + worker killed, rather than leave the worker waiting on a reply that will never come until the + cap. The log line names the exception type only, never its text, which could carry message + content.""" assert proc.stdin is not None stdin = proc.stdin failure: list[BaseException] = [] + fault: list[BaseException] = [] + stale = self._stale_lookup + if stale is not None and not stale.is_alive(): + stale = self._stale_lookup = None + busy = stale is not None def serve() -> None: try: - _write_frame(stdin, self._lookup_reply(request_id, phase, name, lookup)) + reply = ( + self._busy_reply(request_id, phase, name, lookup) + if busy + else self._lookup_reply(request_id, phase, name, lookup) + ) + _write_frame(stdin, reply) except (OSError, ValueError, SandboxError) as exc: # ValueError: a pipe closed under it failure.append(exc) + except Exception as exc: # noqa: BLE001 — reported by the dispatch thread, never lost + fault.append(exc) context = contextvars.copy_context() helper = threading.Thread( @@ -1128,12 +1153,44 @@ def serve() -> None: helper.start() helper.join(max(0.0, deadline - time.monotonic())) if helper.is_alive(): + if not busy: + self._stale_lookup = helper return False + if fault: + self._kill(proc) + log.error( + "inbound %r: the engine failed while serving a brokered lookup for %s %r (%s)", + self._inbound, + phase, + name, + type(fault[0]).__name__, + ) + raise SandboxError( + f"sandbox {phase} {name!r} failed while serving its worker's lookup: an " + "unexpected engine error, logged by type" + ) if failure: self._kill(proc) raise SandboxError(f"could not answer the sandbox worker's lookup: {failure[0]}") return True + def _busy_reply( + self, request_id: str, phase: str, name: str, lookup: codec.LookupRequest + ) -> bytes: + """The ``lookupfail`` for a lookup asked while an earlier one the cap cut off still runs.""" + fn = _LOOKUP_FN[lookup.kind] + return codec.encode_lookup_fail( + request_id=request_id, + phase=phase, + name=name, + seq=lookup.seq, + error=( + f"{fn}({_shown_name(lookup.connection)!r}) refused: an earlier lookup on this " + "inbound is still running after the wall cap cut it off, and the engine runs one " + "at a time (ADR 0147)" + ), + ) + def _lookup_reply( self, request_id: str, phase: str, name: str, lookup: codec.LookupRequest ) -> bytes: diff --git a/tests/test_from_none_is_not_redaction.py b/tests/test_from_none_is_not_redaction.py index 158cbd263b..8d648b623f 100644 --- a/tests/test_from_none_is_not_redaction.py +++ b/tests/test_from_none_is_not_redaction.py @@ -227,7 +227,9 @@ class _Allowed: _Allowed( "messagefoundry/pipeline/_sandbox_worker.py::_Broker._ask::SandboxError::error", _SAFE + "the lookup error quotes the codec refusal, which names a slot, a type or a size " - "and never a value; the cause is dropped so the Handler sees the mode=off error type alone", + "and never a value; the cause is dropped so the Handler sees the mode=off error type alone. " + "Two sites: describing the arguments, and framing them", + count=2, ), _Allowed( "messagefoundry/pipeline/_sandbox_worker.py::_Broker._ask::(OSError, SandboxError)::error", diff --git a/tests/test_sandbox_isolated.py b/tests/test_sandbox_isolated.py index 46c7bfd455..074a51d70c 100644 --- a/tests/test_sandbox_isolated.py +++ b/tests/test_sandbox_isolated.py @@ -21,6 +21,7 @@ import asyncio import os +import threading import time from decimal import Decimal from pathlib import Path @@ -126,6 +127,17 @@ def h_db_nested(msg): return Send("OB_I", rows[0]["v"]) +@handler("h_fhir_badkind") +def h_fhir_badkind(msg): + from messagefoundry.config.fhir_lookup import FhirLookupError + + try: + fhir_lookup("FH", "Patient", {"_count": 10, "x": ["ok", 2.5]}) + except FhirLookupError as exc: + return Send("OB_I", str(exc)) + return Send("OB_I", "not refused") + + @handler("h_fhir") def h_fhir(msg): bundle = fhir_lookup( @@ -817,3 +829,138 @@ def flush(self) -> None: out = Trickle() _write_frame(out, b"0123456789") assert bytes(out.data) == (10).to_bytes(4, "big") + b"0123456789" + + +# --- 7. the Lander's covering review of 415353d7 (round-472dd562) ---------------------------------- + + +def test_a_lookup_the_cap_cut_off_is_the_only_one_outstanding(graph: tuple[Registry, str]) -> None: + # A hung backend: every lookup blocks until the test releases it. Each dispatch hits the cap. + # Unbounded, each later dispatch starts another lookup beside the abandoned ones. + release = threading.Event() + lock = threading.Lock() + state = {"calls": 0, "in_flight": 0, "peak": 0} + + def hung(connection: str, statement: str, params: Any) -> list[dict[str, Any]]: + with lock: + state["calls"] += 1 + state["in_flight"] += 1 + state["peak"] = max(state["peak"], state["in_flight"]) + try: + release.wait(60) + return [{"v": "late", "d": Decimal(0)}] + finally: + with lock: + state["in_flight"] -= 1 + + registry, config_dir = graph + session = SandboxSession( + SandboxPolicy(mode=SandboxMode.ISOLATED, wall_seconds=2.0, lookup_allowed_db=("db",)), + inbound="IB_I", + config_dir=config_dir, + env=None, + graph=None, + ) + try: + with db_lookup_mod.activated(hung): + with pytest.raises(SandboxError, match="wall cap"): + _bodies(registry, "h_db", session) + # The next dispatch is refused its lookup at once rather than start a second one. + started = time.monotonic() + with pytest.raises(SandboxError, match="still running"): + _bodies(registry, "h_db", session) + assert time.monotonic() - started < 15 + assert (state["calls"], state["peak"]) == (1, 1) + # Once the abandoned lookup ends, the session serves lookups again. + release.set() + deadline = time.monotonic() + 10 + while state["in_flight"] and time.monotonic() < deadline: + time.sleep(0.05) + with db_lookup_mod.activated(hung): + assert _bodies(registry, "h_db", session)[0].startswith("late|") + finally: + release.set() + session.close() + + +def test_a_codec_refusal_while_framing_is_the_lookups_own_error( + monkeypatch: pytest.MonkeyPatch, +) -> None: + import io + from types import SimpleNamespace + + from messagefoundry.config.db_lookup import DbLookupError + + # Framing refuses here: one long param is one segment, over a cap of none. + monkeypatch.setattr(codec, "_MAX_BLOBS", 0) + broker = _worker_broker()(io.BytesIO(), io.BytesIO()) + req = SimpleNamespace(request_id="a", phase="transform", name="h") + broker.begin(req) + db, _ = broker.runners(req) + with pytest.raises(DbLookupError, match="cannot cross the sandbox"): + db("LK", "SELECT 1", {"k": "x" * 5000}) + assert not broker.broken + + +def _executor_like(connection: str, query: str, params: Any) -> dict[str, Any]: + """The FHIR executor's screening, as mode=off meets it (transports/fhir.py ``read``).""" + from messagefoundry.config.fhir_lookup import FhirLookupError + from messagefoundry.fhirsearch import resolve_search_pairs + + try: + resolve_search_pairs(params or {}) + except ValueError as exc: + raise FhirLookupError(f"fhir_lookup on {connection!r}: {exc}") from exc + return {"resourceType": "Bundle", "id": "B"} + + +def test_a_wrong_kind_search_value_fails_as_at_mode_off(graph: tuple[Registry, str]) -> None: + registry, config_dir = graph + session = _session(config_dir, SandboxMode.ISOLATED) + try: + with fhir_lookup_mod.activated(_executor_like): + off = _bodies(registry, "h_fhir_badkind", None) + isolated = _bodies(registry, "h_fhir_badkind", session) + # No runner: the "unavailable here" error, in both modes, before any value is judged. + off_bare = _bodies(registry, "h_fhir_badkind", None) + isolated_bare = _bodies(registry, "h_fhir_badkind", session) + finally: + session.close() + assert "got int" in off[0] + assert isolated == off + assert "unavailable here" in off_bare[0] + assert isolated_bare == off_bare + + +def test_an_unexpected_engine_fault_fails_the_dispatch_promptly( + graph: tuple[Registry, str], + runners: _Runners, + monkeypatch: pytest.MonkeyPatch, + caplog: pytest.LogCaptureFixture, +) -> None: + def explode(self: Any, *args: Any) -> bytes: + raise RuntimeError("PID|1||900001||DOE^JANE") + + monkeypatch.setattr(SandboxSession, "_lookup_reply", explode) + registry, config_dir = graph + session = SandboxSession( + SandboxPolicy(mode=SandboxMode.ISOLATED, wall_seconds=20.0, lookup_allowed_db=("db",)), + inbound="IB_I", + config_dir=config_dir, + env=None, + graph=None, + ) + try: + _bodies(registry, "h_parent", session) # warm the worker + started = time.monotonic() + with ( + caplog.at_level("DEBUG"), + pytest.raises(SandboxError, match="failed while serving") as raised, + ): + _bodies(registry, "h_db", session) + assert time.monotonic() - started < 10 + finally: + session.close() + assert "DOE^JANE" not in str(raised.value) + assert not [r for r in caplog.records if "DOE^JANE" in r.getMessage()] + assert any("RuntimeError" in r.getMessage() for r in caplog.records) From ac08b2a83aeec63b17f709d3b7432c388e7a9571 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Thu, 29 Oct 2026 09:51:52 -0500 Subject: [PATCH 06/15] Sandbox broker: covering-review fixes, the busy bound per connection (BACKLOG #3131) From the covering review round-765cc1528e of 3c4ba9289c: - The one-outstanding bound is per (kind, connection), not per session, and is the lookup half's completion, not thread liveness, so a stuck reply write cannot mark a connection busy for good. - A later lookup on a busy connection waits for the stale one up to the rest of its own cap, then is refused, after the availability and [egress] checks, so a stall does not turn the backlog into instant refusals. - A fault in a helper the cap cut off is logged by type, not dropped. - Docs and ADR say what the bound covers and what it does not (a db query its runner gave up on; a reload). - ForeignSearchValue is out of fhirsearch __all__; codec docstring says what the worker still refuses; tests use a sentinel and the stale completion, and cover a healthy second connection during a stall. Proposed PR title: Sandbox mode isolated: the parent brokers db_lookup and fhir_lookup (BACKLOG #3131, slice A) Proposed banner #3131: PARTIAL, slice A built. mode="isolated"; worker checks read is-not-OFF; parent broker over the closed-tag codec through the in-process runner (AC-1, AC-2, AC-6 per-inbound, AC-5 for the broker), refusing non-Handler phases and an empty allowed_db/http even under the opt-out. Remains: AC-3, AC-4, AC-7, the ADR's open boxes, the benchmark, and pooled-worker AC-6 scoping. --- docs/CONFIGURATION.md | 7 +- ...e-ipc-brokered-sandbox-extends-adr-0087.md | 5 +- messagefoundry/fhirsearch.py | 3 +- messagefoundry/pipeline/_sandbox_codec.py | 6 +- messagefoundry/pipeline/sandbox.py | 117 ++++++++++-------- tests/test_sandbox_isolated.py | 26 ++-- 6 files changed, 92 insertions(+), 72 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 30ecca943b..5be8f134e7 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1287,8 +1287,11 @@ It differs from `subprocess` in at least these ways: - **`wall_seconds` counts the lookups.** The cap bounds the whole call, including the time the engine spends serving its lookups, so raise it for a feed whose lookups are slow. When the cap runs out during a lookup, the call fails at once and its worker is recycled; the lookup itself runs on to - its own timeout in the engine, and its result is discarded. Until it ends, every other lookup on - that inbound is refused at once, so a hung backend holds at most one lookup per inbound. + its own timeout in the engine, and its result is discarded. Until it ends, a later lookup on the + same connection from that inbound waits for it, for at most the rest of its own call's cap, and is + refused if it is still waiting then. That bounds what the engine's broker waits on, not the + database: a `db_lookup` its runner gave up on can still be running there, and a reload starts with + no record of a lookup still running. - **It needs a config directory.** A graph built in code with none cannot start a worker. Under `subprocess` such a graph quietly runs in-process; under `isolated` the runner refuses to be built instead, because in-process its lookups would not go through the broker. diff --git a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md index 0cbc69dcef..18be63792f 100644 --- a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md +++ b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md @@ -195,8 +195,9 @@ same accessor and runner the in-process path uses (AC-1, AC-2), refuses one boun A refused or failed lookup is the Handler's error, routed to `ERROR`/dead-letter post-ACK (AC-5, for the broker). `wall_seconds` bounds the whole dispatch, the brokered lookups and the writes of their replies included; a lookup the cap cuts off runs on to its own timeout in the parent, and its result -is discarded. While it runs, the session refuses every other lookup, so at most one is outstanding -per inbound. +is discarded. While it runs, a later lookup on the same connection waits for it, for at most the +rest of its own cap, and is then refused, so the broker waits on one lookup per connection and +session. A database query the runner itself gave up on can still run past that. **Not built:** decision (2), the OS confinement: AC-3 (Landlock and seccomp), AC-4 (the AppContainer token), and AC-7 (no opening or signalling the parent or a sibling). The first two open boxes above, diff --git a/messagefoundry/fhirsearch.py b/messagefoundry/fhirsearch.py index f0bdcb10bc..2ebfbbb2c2 100644 --- a/messagefoundry/fhirsearch.py +++ b/messagefoundry/fhirsearch.py @@ -88,7 +88,6 @@ "FhirSearchParams", "FhirSearchValue", "FhirToken", - "ForeignSearchValue", "resolve_search_pairs", ] @@ -143,7 +142,7 @@ class FhirRaw: class ForeignSearchValue: """A search value of a kind :func:`fhir_lookup` does not take, carried by its type NAME only. - Not a kind an author writes. The ``mode=isolated`` sandbox (ADR 0147) sends a Handler's + Not a kind an author writes, and not in ``__all__``: it is the sandbox codec's carrier. The ``mode=isolated`` sandbox (ADR 0147) sends a Handler's ``params`` to the engine, and a value of any other type cannot cross the pipe, so it crosses as this. :func:`resolve_search_pairs` refuses it with the words it gives the original value, in the engine and after the engine's own checks, so the Handler sees what ``mode=off`` shows it.""" diff --git a/messagefoundry/pipeline/_sandbox_codec.py b/messagefoundry/pipeline/_sandbox_codec.py index fe54259c8d..9ee5153b7b 100644 --- a/messagefoundry/pipeline/_sandbox_codec.py +++ b/messagefoundry/pipeline/_sandbox_codec.py @@ -1364,8 +1364,10 @@ def _enc_fhir_params(params: object, blobs: _Blobs) -> Any: """A ``fhir_lookup`` ``params`` mapping in its own small grammar: ``[key, is_list, items]`` per key, each item ``{"s": str}``, ``{"k": [system, code]}``, ``{"r": raw}``, or ``{"x": type name}`` for a value of any other kind, which arrives as a - :class:`~messagefoundry.fhirsearch.ForeignSearchValue`. Nothing is screened here: the parent's - executor judges every value, after its own "unavailable here" and connection checks.""" + :class:`~messagefoundry.fhirsearch.ForeignSearchValue`. Values are not screened here: the + parent's executor judges each one, after its own "unavailable here" and connection checks. A + key that is not a ``str``, and ``params`` that is not a mapping, are still refused here, in the + worker, before those checks.""" if params is None: return None if not isinstance(params, Mapping): diff --git a/messagefoundry/pipeline/sandbox.py b/messagefoundry/pipeline/sandbox.py index 1344d8cfa3..3dae38027c 100644 --- a/messagefoundry/pipeline/sandbox.py +++ b/messagefoundry/pipeline/sandbox.py @@ -724,8 +724,8 @@ def __init__( self._closed = False # The [egress] lists whose empty-list refusal this session has logged. self._lookup_refusals_logged: set[str] = set() - # The helper thread of a lookup the wall cap cut off, while it still runs: see _serve_bounded. - self._stale_lookup: threading.Thread | None = None + # Per (kind, connection), the completion of a lookup the wall cap cut off: see _serve_bounded. + self._stale_lookups: dict[tuple[str, str], threading.Event] = {} @property def mode(self) -> SandboxMode: @@ -1107,41 +1107,45 @@ def _serve_bounded( its runner for the runner's own result timeout, longer than the default cap. The reply can be larger than the pipe's buffer, and a worker that sent a ``lookup`` frame and then stopped reading stdin would hold the write for good. The thread runs in a copy of this thread's - context, so it sees the lookup runner the transform worker published. The kill closes the - pipe, which ends a blocked write. A broken pipe raises :class:`SandboxError` after the worker - is killed. + context, so it sees the lookup runner the transform worker published. Killing the worker + normally ends a blocked write. A broken pipe raises :class:`SandboxError` after the worker is + killed. - **At most one brokered lookup is outstanding per session.** A lookup the cap cut off runs on - to its own bound (the runner's result timeout), and while it does, every later lookup on this - session is refused at once with a ``lookupfail`` naming it, rather than start another beside - it. Unbounded, a hung backend would gather one abandoned lookup per dispatch, about six per - inbound at the default caps, against the executor and the connection pool. + **A lookup the cap cut off is remembered per connection** until its runner returns + (:meth:`_serve_lookup` says what a later lookup on that connection then does). The record is + the lookup half's completion, not the thread's: a reply write that never ends cannot keep the + connection marked busy. **An unexpected fault in the engine fails the dispatch at once,** with a fixed cause and the - worker killed, rather than leave the worker waiting on a reply that will never come until the - cap. The log line names the exception type only, never its text, which could carry message - content.""" + worker killed, rather than leave the worker waiting for a reply until the cap. The helper logs + it by exception type only, never its text, which could carry message content, and it does so + even when the cap has already cut the helper off.""" assert proc.stdin is not None stdin = proc.stdin failure: list[BaseException] = [] fault: list[BaseException] = [] - stale = self._stale_lookup - if stale is not None and not stale.is_alive(): - stale = self._stale_lookup = None - busy = stale is not None + for key in [key for key, done in self._stale_lookups.items() if done.is_set()]: + del self._stale_lookups[key] + done = threading.Event() def serve() -> None: try: - reply = ( - self._busy_reply(request_id, phase, name, lookup) - if busy - else self._lookup_reply(request_id, phase, name, lookup) - ) + try: + reply = self._lookup_reply(request_id, phase, name, lookup, deadline) + finally: + done.set() _write_frame(stdin, reply) except (OSError, ValueError, SandboxError) as exc: # ValueError: a pipe closed under it failure.append(exc) - except Exception as exc: # noqa: BLE001 — reported by the dispatch thread, never lost + except Exception as exc: # noqa: BLE001 — logged here, failed by the dispatch thread fault.append(exc) + log.error( + "inbound %r: the engine failed while serving a brokered lookup for %s %r (%s)", + self._inbound, + phase, + name, + type(exc).__name__, + ) context = contextvars.copy_context() helper = threading.Thread( @@ -1153,18 +1157,11 @@ def serve() -> None: helper.start() helper.join(max(0.0, deadline - time.monotonic())) if helper.is_alive(): - if not busy: - self._stale_lookup = helper + if not done.is_set(): + self._stale_lookups[(lookup.kind, lookup.connection)] = done return False if fault: self._kill(proc) - log.error( - "inbound %r: the engine failed while serving a brokered lookup for %s %r (%s)", - self._inbound, - phase, - name, - type(fault[0]).__name__, - ) raise SandboxError( f"sandbox {phase} {name!r} failed while serving its worker's lookup: an " "unexpected engine error, logged by type" @@ -1174,29 +1171,17 @@ def serve() -> None: raise SandboxError(f"could not answer the sandbox worker's lookup: {failure[0]}") return True - def _busy_reply( - self, request_id: str, phase: str, name: str, lookup: codec.LookupRequest - ) -> bytes: - """The ``lookupfail`` for a lookup asked while an earlier one the cap cut off still runs.""" - fn = _LOOKUP_FN[lookup.kind] - return codec.encode_lookup_fail( - request_id=request_id, - phase=phase, - name=name, - seq=lookup.seq, - error=( - f"{fn}({_shown_name(lookup.connection)!r}) refused: an earlier lookup on this " - "inbound is still running after the wall cap cut it off, and the engine runs one " - "at a time (ADR 0147)" - ), - ) - def _lookup_reply( - self, request_id: str, phase: str, name: str, lookup: codec.LookupRequest + self, + request_id: str, + phase: str, + name: str, + lookup: codec.LookupRequest, + deadline: float, ) -> bytes: - """The ``lookupok``/``lookupfail`` frame answering one ``lookup``. Never raises: the worker - is blocked on this reply, so a result that cannot cross is a ``lookupfail`` it can report.""" - ok, result, error = self._serve_lookup(phase, lookup) + """The ``lookupok``/``lookupfail`` frame answering one ``lookup``. A result that cannot cross + is a ``lookupfail`` the worker can report.""" + ok, result, error = self._serve_lookup(phase, lookup, deadline) fn = _LOOKUP_FN[lookup.kind] if ok: try: @@ -1214,7 +1199,9 @@ def _lookup_reply( request_id=request_id, phase=phase, name=name, seq=lookup.seq, error=error ) - def _serve_lookup(self, phase: str, lookup: codec.LookupRequest) -> tuple[bool, object, str]: + def _serve_lookup( + self, phase: str, lookup: codec.LookupRequest, deadline: float + ) -> tuple[bool, object, str]: """Answer one brokered lookup in the parent: ``(ok, result, error)``. The answer comes from the same ``db_lookup``/``fhir_lookup`` accessor a Handler calls at @@ -1225,12 +1212,23 @@ def _serve_lookup(self, phase: str, lookup: codec.LookupRequest) -> tuple[bool, broker's own, so that is the error the Handler sees. The executor behind the runner was built against ``[egress]`` and runs its own statement test and row ceiling, so each applies here as in-process. The slice-B seam: a broker-only check of a lookup's own arguments goes between the - refusal below and the call.""" + refusals below and the call. + + **One lookup at a time per connection.** While a lookup the cap cut off on this connection + is still waiting on its runner, a new one waits for it, for at most the rest of its own + dispatch's cap less a small margin, and is refused if it is still waiting then. So a hung + backend holds one broker call per connection and session, not one per message, and a + transient stall does not turn the backlog into instant refusals.""" is_db = lookup.kind == codec.LOOKUP_DB available = db_lookup_available() if is_db else fhir_lookup_available() refusal = self._lookup_refusal(phase, lookup) if available else None if refusal is not None: return False, None, refusal + stale = self._stale_lookups.get((lookup.kind, lookup.connection)) if available else None + if stale is not None: + margin = min(0.5, self.policy.wall_seconds / 10) + if not stale.wait(max(0.0, deadline - margin - time.monotonic())): + return False, None, self._busy_refusal(lookup) try: if is_db: return True, db_lookup(lookup.connection, lookup.target, lookup.params), "" @@ -1241,6 +1239,15 @@ def _serve_lookup(self, phase: str, lookup: codec.LookupRequest) -> tuple[bool, except Exception as exc: # noqa: BLE001 — the worker waits on this reply; report, never raise return False, None, f"{_LOOKUP_FN[lookup.kind]} failed: {safe_exc(exc)}" + @staticmethod + def _busy_refusal(lookup: codec.LookupRequest) -> str: + fn = _LOOKUP_FN[lookup.kind] + return ( + f"{fn}({_shown_name(lookup.connection)!r}) refused: an earlier lookup on this " + "connection is still running after the wall cap cut it off, and the engine serves " + "one at a time per connection (ADR 0147)" + ) + def _lookup_refusal(self, phase: str, lookup: codec.LookupRequest) -> str | None: """Why the parent refuses a brokered lookup outright, or ``None`` to serve it. diff --git a/tests/test_sandbox_isolated.py b/tests/test_sandbox_isolated.py index 074a51d70c..77b30baeb0 100644 --- a/tests/test_sandbox_isolated.py +++ b/tests/test_sandbox_isolated.py @@ -834,7 +834,9 @@ def flush(self) -> None: # --- 7. the Lander's covering review of 415353d7 (round-472dd562) ---------------------------------- -def test_a_lookup_the_cap_cut_off_is_the_only_one_outstanding(graph: tuple[Registry, str]) -> None: +def test_a_lookup_the_cap_cut_off_is_the_only_one_outstanding( + graph: tuple[Registry, str], runners: _Runners +) -> None: # A hung backend: every lookup blocks until the test releases it. Each dispatch hits the cap. # Unbounded, each later dispatch starts another lookup beside the abandoned ones. release = threading.Event() @@ -855,7 +857,12 @@ def hung(connection: str, statement: str, params: Any) -> list[dict[str, Any]]: registry, config_dir = graph session = SandboxSession( - SandboxPolicy(mode=SandboxMode.ISOLATED, wall_seconds=2.0, lookup_allowed_db=("db",)), + SandboxPolicy( + mode=SandboxMode.ISOLATED, + wall_seconds=2.0, + lookup_allowed_db=("db",), + lookup_allowed_http=("fhir",), + ), inbound="IB_I", config_dir=config_dir, env=None, @@ -870,12 +877,13 @@ def hung(connection: str, statement: str, params: Any) -> list[dict[str, Any]]: with pytest.raises(SandboxError, match="still running"): _bodies(registry, "h_db", session) assert time.monotonic() - started < 15 + # Another connection is not held up by the stalled one. + with fhir_lookup_mod.activated(runners.fhir): + assert _bodies(registry, "h_fhir", session) == ["BUNDLE-1"] assert (state["calls"], state["peak"]) == (1, 1) - # Once the abandoned lookup ends, the session serves lookups again. + # Once the abandoned lookup ends, the session serves lookups on it again: the next one waits + # for the stale lookup's completion rather than poll the runner's own counter. release.set() - deadline = time.monotonic() + 10 - while state["in_flight"] and time.monotonic() < deadline: - time.sleep(0.05) with db_lookup_mod.activated(hung): assert _bodies(registry, "h_db", session)[0].startswith("late|") finally: @@ -939,7 +947,7 @@ def test_an_unexpected_engine_fault_fails_the_dispatch_promptly( caplog: pytest.LogCaptureFixture, ) -> None: def explode(self: Any, *args: Any) -> bytes: - raise RuntimeError("PID|1||900001||DOE^JANE") + raise RuntimeError("FAULT-SENTINEL-7f3a") monkeypatch.setattr(SandboxSession, "_lookup_reply", explode) registry, config_dir = graph @@ -961,6 +969,6 @@ def explode(self: Any, *args: Any) -> bytes: assert time.monotonic() - started < 10 finally: session.close() - assert "DOE^JANE" not in str(raised.value) - assert not [r for r in caplog.records if "DOE^JANE" in r.getMessage()] + assert "FAULT-SENTINEL-7f3a" not in str(raised.value) + assert not [r for r in caplog.records if "FAULT-SENTINEL-7f3a" in r.getMessage()] assert any("RuntimeError" in r.getMessage() for r in caplog.records) From e70a35173b71b535c3ef4d4cfe8f9d5170d8c9f2 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Thu, 29 Oct 2026 10:12:51 -0500 Subject: [PATCH 07/15] Sandbox broker: a cut-off waiter never replaces a hung lookup's record (BACKLOG #3131) From the covering review round-e5f9a6d716 of ac08b2a83a: - A helper the cap cut off while it waited on a busy connection no longer replaces the hung lookup's record, which let the next dispatch start a second call on the hung connection (test fails without it). - The busy wait leaves a quarter of the cap (at most 2 s) for the Handler to handle the refusal; docs and ADR say so; the test uses a 4 s cap so the margin is not a CI timing race. - Stale test comment, a 150-column docstring line, and a shadowed name. Proposed PR title: Sandbox mode isolated: the parent brokers db_lookup and fhir_lookup (BACKLOG #3131, slice A) Proposed banner #3131: PARTIAL, slice A built. mode="isolated"; worker checks read is-not-OFF; parent broker over the closed-tag codec through the in-process runner (AC-1, AC-2, AC-6 per-inbound, AC-5 for the broker), refusing non-Handler phases and an empty allowed_db/http even under the opt-out. Remains: AC-3, AC-4, AC-7, the ADR's open boxes, the benchmark, and pooled-worker AC-6 scoping. --- docs/CONFIGURATION.md | 5 ++- ...e-ipc-brokered-sandbox-extends-adr-0087.md | 4 +- messagefoundry/fhirsearch.py | 6 +-- messagefoundry/pipeline/sandbox.py | 16 ++++--- tests/test_sandbox_isolated.py | 44 ++++++++++++++++++- 5 files changed, 61 insertions(+), 14 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 5be8f134e7..e3eca0b2e6 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1288,8 +1288,9 @@ It differs from `subprocess` in at least these ways: spends serving its lookups, so raise it for a feed whose lookups are slow. When the cap runs out during a lookup, the call fails at once and its worker is recycled; the lookup itself runs on to its own timeout in the engine, and its result is discarded. Until it ends, a later lookup on the - same connection from that inbound waits for it, for at most the rest of its own call's cap, and is - refused if it is still waiting then. That bounds what the engine's broker waits on, not the + same connection from that inbound waits for it, until a quarter of its own call's cap (at most 2 + seconds) is left, and is refused if it is still waiting then, leaving that quarter for the Handler + to handle the refusal. That bounds what the engine's broker waits on, not the database: a `db_lookup` its runner gave up on can still be running there, and a reload starts with no record of a lookup still running. - **It needs a config directory.** A graph built in code with none cannot start a worker. Under diff --git a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md index 18be63792f..d690de30b8 100644 --- a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md +++ b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md @@ -195,8 +195,8 @@ same accessor and runner the in-process path uses (AC-1, AC-2), refuses one boun A refused or failed lookup is the Handler's error, routed to `ERROR`/dead-letter post-ACK (AC-5, for the broker). `wall_seconds` bounds the whole dispatch, the brokered lookups and the writes of their replies included; a lookup the cap cuts off runs on to its own timeout in the parent, and its result -is discarded. While it runs, a later lookup on the same connection waits for it, for at most the -rest of its own cap, and is then refused, so the broker waits on one lookup per connection and +is discarded. While it runs, a later lookup on the same connection waits for it until a quarter of +its own cap is left, and is then refused, so the broker waits on one lookup per connection and session. A database query the runner itself gave up on can still run past that. **Not built:** decision (2), the OS confinement: AC-3 (Landlock and seccomp), AC-4 (the AppContainer diff --git a/messagefoundry/fhirsearch.py b/messagefoundry/fhirsearch.py index 2ebfbbb2c2..e8a8bf7fe3 100644 --- a/messagefoundry/fhirsearch.py +++ b/messagefoundry/fhirsearch.py @@ -142,9 +142,9 @@ class FhirRaw: class ForeignSearchValue: """A search value of a kind :func:`fhir_lookup` does not take, carried by its type NAME only. - Not a kind an author writes, and not in ``__all__``: it is the sandbox codec's carrier. The ``mode=isolated`` sandbox (ADR 0147) sends a Handler's - ``params`` to the engine, and a value of any other type cannot cross the pipe, so it crosses as - this. :func:`resolve_search_pairs` refuses it with the words it gives the original value, in the + Not a kind an author writes, and not in ``__all__``: it is the sandbox codec's carrier. The + ``mode=isolated`` sandbox (ADR 0147) sends a Handler's ``params`` to the engine, and a value of + any other type cannot cross the pipe, so it crosses as this. :func:`resolve_search_pairs` refuses it with the words it gives the original value, in the engine and after the engine's own checks, so the Handler sees what ``mode=off`` shows it.""" type_name: str diff --git a/messagefoundry/pipeline/sandbox.py b/messagefoundry/pipeline/sandbox.py index 3dae38027c..0722df01e4 100644 --- a/messagefoundry/pipeline/sandbox.py +++ b/messagefoundry/pipeline/sandbox.py @@ -1124,7 +1124,7 @@ def _serve_bounded( stdin = proc.stdin failure: list[BaseException] = [] fault: list[BaseException] = [] - for key in [key for key, done in self._stale_lookups.items() if done.is_set()]: + for key in [key for key, ended in self._stale_lookups.items() if ended.is_set()]: del self._stale_lookups[key] done = threading.Event() @@ -1157,8 +1157,13 @@ def serve() -> None: helper.start() helper.join(max(0.0, deadline - time.monotonic())) if helper.is_alive(): - if not done.is_set(): - self._stale_lookups[(lookup.kind, lookup.connection)] = done + key = (lookup.kind, lookup.connection) + earlier = self._stale_lookups.get(key) + # Never replace a record whose lookup still runs: a helper the cap cut off while it was + # still waiting on that record has not called the runner, and replacing it would let the + # next dispatch start a second call beside the one that is actually hung. + if not done.is_set() and (earlier is None or earlier.is_set()): + self._stale_lookups[key] = done return False if fault: self._kill(proc) @@ -1216,7 +1221,8 @@ def _serve_lookup( **One lookup at a time per connection.** While a lookup the cap cut off on this connection is still waiting on its runner, a new one waits for it, for at most the rest of its own - dispatch's cap less a small margin, and is refused if it is still waiting then. So a hung + dispatch's cap less a quarter of the cap (at most 2 s), and is refused if it is still waiting + then. The quarter is what the Handler has left to handle the refusal before the cap. So a hung backend holds one broker call per connection and session, not one per message, and a transient stall does not turn the backlog into instant refusals.""" is_db = lookup.kind == codec.LOOKUP_DB @@ -1226,7 +1232,7 @@ def _serve_lookup( return False, None, refusal stale = self._stale_lookups.get((lookup.kind, lookup.connection)) if available else None if stale is not None: - margin = min(0.5, self.policy.wall_seconds / 10) + margin = min(2.0, self.policy.wall_seconds / 4) if not stale.wait(max(0.0, deadline - margin - time.monotonic())): return False, None, self._busy_refusal(lookup) try: diff --git a/tests/test_sandbox_isolated.py b/tests/test_sandbox_isolated.py index 77b30baeb0..9b00fa7123 100644 --- a/tests/test_sandbox_isolated.py +++ b/tests/test_sandbox_isolated.py @@ -859,7 +859,7 @@ def hung(connection: str, statement: str, params: Any) -> list[dict[str, Any]]: session = SandboxSession( SandboxPolicy( mode=SandboxMode.ISOLATED, - wall_seconds=2.0, + wall_seconds=4.0, lookup_allowed_db=("db",), lookup_allowed_http=("fhir",), ), @@ -872,7 +872,8 @@ def hung(connection: str, statement: str, params: Any) -> list[dict[str, Any]]: with db_lookup_mod.activated(hung): with pytest.raises(SandboxError, match="wall cap"): _bodies(registry, "h_db", session) - # The next dispatch is refused its lookup at once rather than start a second one. + # The next dispatch waits for the stalled lookup, then is refused its own rather than + # start a second one beside it. started = time.monotonic() with pytest.raises(SandboxError, match="still running"): _bodies(registry, "h_db", session) @@ -972,3 +973,42 @@ def explode(self: Any, *args: Any) -> bytes: assert "FAULT-SENTINEL-7f3a" not in str(raised.value) assert not [r for r in caplog.records if "FAULT-SENTINEL-7f3a" in r.getMessage()] assert any("RuntimeError" in r.getMessage() for r in caplog.records) + + +def test_a_cut_off_waiter_does_not_replace_the_hung_lookups_record( + graph: tuple[Registry, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # Dispatch B's helper is still building its refusal when B's cap fires. Its record must not + # replace the hung lookup's, or dispatch C would start a second call on the hung connection. + release = threading.Event() + calls: list[str] = [] + + def hung(connection: str, statement: str, params: Any) -> list[dict[str, Any]]: + calls.append(statement) + release.wait(60) + return [{"v": "late", "d": Decimal(0)}] + + real_refusal = SandboxSession._busy_refusal + + def slow_refusal(lookup: codec.LookupRequest) -> str: + time.sleep(2.0) # past the 1 s margin at wall_seconds=4 + return real_refusal(lookup) + + monkeypatch.setattr(SandboxSession, "_busy_refusal", staticmethod(slow_refusal)) + registry, config_dir = graph + session = SandboxSession( + SandboxPolicy(mode=SandboxMode.ISOLATED, wall_seconds=4.0, lookup_allowed_db=("db",)), + inbound="IB_I", + config_dir=config_dir, + env=None, + graph=None, + ) + try: + with db_lookup_mod.activated(hung): + for _ in range(3): # A hangs; B and C each find the connection busy + with pytest.raises(SandboxError): + _bodies(registry, "h_db", session) + assert len(calls) == 1 + finally: + release.set() + session.close() From 8637a721993f2f815529cffca82284cc2c3c6ea8 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Thu, 29 Oct 2026 10:42:19 -0500 Subject: [PATCH 08/15] Sandbox broker: each lookup records itself before its runner call (BACKLOG #3131) From the covering review round-29332d0b90 of e70a35173b: - Each brokered lookup registers its own completion for its connection, under a lock, just before it calls the runner, instead of the dispatch thread inferring a record at cut-off. A helper whose dispatch the cap already ended never calls the runner (test fails without it: 3 calls). - An engine-side error in building the reply goes to the type-only fault arm; only the pipe write maps to "could not answer". - The ADR points at CONFIGURATION.md for the wait rule instead of restating it; fhirsearch _resolve_one types ForeignSearchValue. Proposed PR title: Sandbox mode isolated: the parent brokers db_lookup and fhir_lookup (BACKLOG #3131, slice A) Proposed banner #3131: PARTIAL, slice A built. mode="isolated"; worker checks read is-not-OFF; parent broker over the closed-tag codec through the in-process runner (AC-1, AC-2, AC-6 per-inbound, AC-5 for the broker), refusing non-Handler phases and an empty allowed_db/http even under the opt-out. Remains: AC-3, AC-4, AC-7, the ADR's open boxes, the benchmark, and pooled-worker AC-6 scoping. --- ...e-ipc-brokered-sandbox-extends-adr-0087.md | 4 +- messagefoundry/fhirsearch.py | 2 +- messagefoundry/pipeline/sandbox.py | 75 +++++++++++-------- tests/test_sandbox_isolated.py | 65 +++++++++++++++- 4 files changed, 109 insertions(+), 37 deletions(-) diff --git a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md index d690de30b8..aa49a28345 100644 --- a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md +++ b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md @@ -195,8 +195,8 @@ same accessor and runner the in-process path uses (AC-1, AC-2), refuses one boun A refused or failed lookup is the Handler's error, routed to `ERROR`/dead-letter post-ACK (AC-5, for the broker). `wall_seconds` bounds the whole dispatch, the brokered lookups and the writes of their replies included; a lookup the cap cuts off runs on to its own timeout in the parent, and its result -is discarded. While it runs, a later lookup on the same connection waits for it until a quarter of -its own cap is left, and is then refused, so the broker waits on one lookup per connection and +is discarded. While it runs, a later lookup on the same connection waits for it and may then be +refused, by the rule `docs/CONFIGURATION.md` states in section `[sandbox]`, so the broker waits on one lookup per connection and session. A database query the runner itself gave up on can still run past that. **Not built:** decision (2), the OS confinement: AC-3 (Landlock and seccomp), AC-4 (the AppContainer diff --git a/messagefoundry/fhirsearch.py b/messagefoundry/fhirsearch.py index e8a8bf7fe3..edd2a9fcbc 100644 --- a/messagefoundry/fhirsearch.py +++ b/messagefoundry/fhirsearch.py @@ -176,7 +176,7 @@ def _screen(key: str, value: str, *, half: str) -> str: ) -def _resolve_one(key: str, value: FhirSearchValue) -> str: +def _resolve_one(key: str, value: FhirSearchValue | ForeignSearchValue) -> str: """One ``params`` value to the string that goes on the URL, before percent-encoding.""" if isinstance(value, str): return _screen(key, value, half="value") diff --git a/messagefoundry/pipeline/sandbox.py b/messagefoundry/pipeline/sandbox.py index 0722df01e4..ede365864e 100644 --- a/messagefoundry/pipeline/sandbox.py +++ b/messagefoundry/pipeline/sandbox.py @@ -724,8 +724,9 @@ def __init__( self._closed = False # The [egress] lists whose empty-list refusal this session has logged. self._lookup_refusals_logged: set[str] = set() - # Per (kind, connection), the completion of a lookup the wall cap cut off: see _serve_bounded. + # Per (kind, connection), the completion of the lookup last started on it: see _serve_lookup. self._stale_lookups: dict[tuple[str, str], threading.Event] = {} + self._stale_lock = threading.Lock() @property def mode(self) -> SandboxMode: @@ -1111,9 +1112,9 @@ def _serve_bounded( normally ends a blocked write. A broken pipe raises :class:`SandboxError` after the worker is killed. - **A lookup the cap cut off is remembered per connection** until its runner returns - (:meth:`_serve_lookup` says what a later lookup on that connection then does). The record is - the lookup half's completion, not the thread's: a reply write that never ends cannot keep the + **Each lookup records itself per connection while its runner runs** (:meth:`_serve_lookup` + registers it, and says what a later lookup on that connection then does). The record is the + lookup half's completion, not the thread's: a reply write that never ends cannot keep the connection marked busy. **An unexpected fault in the engine fails the dispatch at once,** with a fixed cause and the @@ -1124,19 +1125,14 @@ def _serve_bounded( stdin = proc.stdin failure: list[BaseException] = [] fault: list[BaseException] = [] - for key in [key for key, ended in self._stale_lookups.items() if ended.is_set()]: - del self._stale_lookups[key] + with self._stale_lock: + for key in [key for key, ended in self._stale_lookups.items() if ended.is_set()]: + del self._stale_lookups[key] done = threading.Event() def serve() -> None: try: - try: - reply = self._lookup_reply(request_id, phase, name, lookup, deadline) - finally: - done.set() - _write_frame(stdin, reply) - except (OSError, ValueError, SandboxError) as exc: # ValueError: a pipe closed under it - failure.append(exc) + reply = self._lookup_reply(request_id, phase, name, lookup, deadline, done) except Exception as exc: # noqa: BLE001 — logged here, failed by the dispatch thread fault.append(exc) log.error( @@ -1146,6 +1142,13 @@ def serve() -> None: name, type(exc).__name__, ) + return + finally: + done.set() + try: + _write_frame(stdin, reply) + except (OSError, ValueError, SandboxError) as exc: # ValueError: a pipe closed under it + failure.append(exc) context = contextvars.copy_context() helper = threading.Thread( @@ -1157,13 +1160,6 @@ def serve() -> None: helper.start() helper.join(max(0.0, deadline - time.monotonic())) if helper.is_alive(): - key = (lookup.kind, lookup.connection) - earlier = self._stale_lookups.get(key) - # Never replace a record whose lookup still runs: a helper the cap cut off while it was - # still waiting on that record has not called the runner, and replacing it would let the - # next dispatch start a second call beside the one that is actually hung. - if not done.is_set() and (earlier is None or earlier.is_set()): - self._stale_lookups[key] = done return False if fault: self._kill(proc) @@ -1183,10 +1179,11 @@ def _lookup_reply( name: str, lookup: codec.LookupRequest, deadline: float, + done: threading.Event, ) -> bytes: """The ``lookupok``/``lookupfail`` frame answering one ``lookup``. A result that cannot cross is a ``lookupfail`` the worker can report.""" - ok, result, error = self._serve_lookup(phase, lookup, deadline) + ok, result, error = self._serve_lookup(phase, lookup, deadline, done) fn = _LOOKUP_FN[lookup.kind] if ok: try: @@ -1205,7 +1202,7 @@ def _lookup_reply( ) def _serve_lookup( - self, phase: str, lookup: codec.LookupRequest, deadline: float + self, phase: str, lookup: codec.LookupRequest, deadline: float, done: threading.Event ) -> tuple[bool, object, str]: """Answer one brokered lookup in the parent: ``(ok, result, error)``. @@ -1219,22 +1216,34 @@ def _serve_lookup( in-process. The slice-B seam: a broker-only check of a lookup's own arguments goes between the refusals below and the call. - **One lookup at a time per connection.** While a lookup the cap cut off on this connection - is still waiting on its runner, a new one waits for it, for at most the rest of its own - dispatch's cap less a quarter of the cap (at most 2 s), and is refused if it is still waiting - then. The quarter is what the Handler has left to handle the refusal before the cap. So a hung - backend holds one broker call per connection and session, not one per message, and a - transient stall does not turn the backlog into instant refusals.""" + **One lookup at a time per connection.** Each lookup registers ``done``, its completion, for + its connection just before it calls the runner, under ``_stale_lock``. A later lookup on that + connection, which can only meet one the wall cap cut off since a session serves one dispatch + at a time, waits on that record by the rule ``docs/CONFIGURATION.md`` states (section + ``[sandbox]``, *``mode="isolated"``*), and is refused if it still runs then. A helper whose + own dispatch the cap has already ended never calls the runner, so no call starts that nobody + waits for and nothing records.""" is_db = lookup.kind == codec.LOOKUP_DB available = db_lookup_available() if is_db else fhir_lookup_available() refusal = self._lookup_refusal(phase, lookup) if available else None if refusal is not None: return False, None, refusal - stale = self._stale_lookups.get((lookup.kind, lookup.connection)) if available else None - if stale is not None: - margin = min(2.0, self.policy.wall_seconds / 4) - if not stale.wait(max(0.0, deadline - margin - time.monotonic())): - return False, None, self._busy_refusal(lookup) + if available: + key = (lookup.kind, lookup.connection) + with self._stale_lock: + stale = self._stale_lookups.get(key) + if stale is not None: + margin = min(2.0, self.policy.wall_seconds / 4) + if not stale.wait(max(0.0, deadline - margin - time.monotonic())): + return False, None, self._busy_refusal(lookup) + with self._stale_lock: + if time.monotonic() >= deadline: + # This dispatch is already over: the worker is being killed and nobody waits. + return False, None, f"{_LOOKUP_FN[lookup.kind]}: the call's wall cap ran out" + current = self._stale_lookups.get(key) + if current is not None and not current.is_set(): + return False, None, self._busy_refusal(lookup) + self._stale_lookups[key] = done try: if is_db: return True, db_lookup(lookup.connection, lookup.target, lookup.params), "" diff --git a/tests/test_sandbox_isolated.py b/tests/test_sandbox_isolated.py index 9b00fa7123..1b34a78eba 100644 --- a/tests/test_sandbox_isolated.py +++ b/tests/test_sandbox_isolated.py @@ -1006,9 +1006,72 @@ def slow_refusal(lookup: codec.LookupRequest) -> str: try: with db_lookup_mod.activated(hung): for _ in range(3): # A hangs; B and C each find the connection busy - with pytest.raises(SandboxError): + with pytest.raises(SandboxError, match="wall cap|still running"): _bodies(registry, "h_db", session) assert len(calls) == 1 finally: release.set() session.close() + + +def test_a_helper_past_its_deadline_never_calls_the_runner( + graph: tuple[Registry, str], monkeypatch: pytest.MonkeyPatch +) -> None: + # Dispatch B's helper starts so late that B's cap has already ended, and by then the hung lookup + # A has finished. It must not call the runner: nobody waits for that call and nothing records it, + # so the next dispatch would start another beside it. + release_a, release_all = threading.Event(), threading.Event() + lock = threading.Lock() + state = {"calls": 0, "in_flight": 0, "peak": 0} + + def hung(connection: str, statement: str, params: Any) -> list[dict[str, Any]]: + with lock: + state["calls"] += 1 + first = state["calls"] == 1 + state["in_flight"] += 1 + state["peak"] = max(state["peak"], state["in_flight"]) + try: + (release_a if first else release_all).wait(60) + return [{"v": "late", "d": Decimal(0)}] + finally: + with lock: + state["in_flight"] -= 1 + + go = threading.Event() + go.set() + real = SandboxSession._lookup_refusal + + def gated(self: SandboxSession, phase: str, lookup: codec.LookupRequest) -> str | None: + go.wait(30) # stands in for a helper thread scheduled late, past its dispatch's cap + return real(self, phase, lookup) + + monkeypatch.setattr(SandboxSession, "_lookup_refusal", gated) + registry, config_dir = graph + session = SandboxSession( + SandboxPolicy(mode=SandboxMode.ISOLATED, wall_seconds=2.0, lookup_allowed_db=("db",)), + inbound="IB_I", + config_dir=config_dir, + env=None, + graph=None, + ) + try: + with db_lookup_mod.activated(hung): + with pytest.raises(SandboxError, match="wall cap"): + _bodies(registry, "h_db", session) # A: hangs, cut off + go.clear() + with pytest.raises(SandboxError, match="wall cap"): + _bodies(registry, "h_db", session) # B: its helper is held before the runner + release_a.set() + deadline = time.monotonic() + 10 + while state["in_flight"] and time.monotonic() < deadline: + time.sleep(0.05) + go.set() # B's helper goes on, past B's deadline, and finds A finished + time.sleep(0.5) + release_all.set() + assert _bodies(registry, "h_db", session)[0].startswith("late|") # C + assert (state["calls"], state["peak"]) == (2, 1) # A and C; never B + finally: + release_a.set() + release_all.set() + go.set() + session.close() From 6db7214a2106d185d970d5a307c15966b8598ff6 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Thu, 29 Oct 2026 11:14:15 -0500 Subject: [PATCH 09/15] Sandbox broker: an abandoned helper never calls the runner (BACKLOG #3131) From the covering review round-043d86bf26 of 8637a72199, closing the Lander's hold item at e70a35173 without a clock comparison: - The dispatch thread sets an abandoned event under _stale_lock when it gives up on a helper; the helper checks it under the same lock before it registers and calls the runner. Every runner call is recorded, and none starts that nobody waits for (test fails on e70a35173: 3 calls). - Each runner call registers, then removes, its own record; a waiter loops on the record by the documented rule; the busy refusal is built outside the lock; the dispatch thread no longer sweeps the records. - An unexpected error from the reply write is a type-only fault again. - The late-helper test waits on A's record, not the runner's counter; fhirsearch _resolve_one takes object; the ADR says the helper thread answers, and its long sentence is split. Proposed PR title: Sandbox mode isolated: the parent brokers db_lookup and fhir_lookup (BACKLOG #3131, slice A) Proposed banner #3131: PARTIAL, slice A built. mode="isolated"; worker checks read is-not-OFF; parent broker over the closed-tag codec through the in-process runner (AC-1, AC-2, AC-6 per-inbound, AC-5 for the broker), refusing non-Handler phases and an empty allowed_db/http even under the opt-out. Remains: AC-3, AC-4, AC-7, the ADR's open boxes, the benchmark, and pooled-worker AC-6 scoping. --- ...e-ipc-brokered-sandbox-extends-adr-0087.md | 10 +- messagefoundry/fhirsearch.py | 2 +- messagefoundry/pipeline/sandbox.py | 103 +++++++++++------- tests/test_sandbox_isolated.py | 15 ++- 4 files changed, 80 insertions(+), 50 deletions(-) diff --git a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md index aa49a28345..73addf750c 100644 --- a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md +++ b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md @@ -189,15 +189,15 @@ unresearched. **Built:** the `isolated` mode and decision (1), the parent-held broker. A Handler's `db_lookup`/`fhir_lookup` in the worker writes a `lookup` frame in the closed tag set, bound to the dispatch's `(id, phase, name)` triple and a per-dispatch sequence number (AC-6, for a per-inbound -worker, whose lane the session already is). The parent answers it on the dispatch thread through the -same accessor and runner the in-process path uses (AC-1, AC-2), refuses one bound to a Router or +worker, whose lane the session already is). The parent answers it on a helper thread, in the dispatch's +context, through the same accessor and runner the in-process path uses (AC-1, AC-2), refuses one bound to a Router or `accepts=` dispatch, and refuses one whose `[egress].allowed_db` or `[egress].allowed_http` is empty. A refused or failed lookup is the Handler's error, routed to `ERROR`/dead-letter post-ACK (AC-5, for the broker). `wall_seconds` bounds the whole dispatch, the brokered lookups and the writes of their replies included; a lookup the cap cuts off runs on to its own timeout in the parent, and its result -is discarded. While it runs, a later lookup on the same connection waits for it and may then be -refused, by the rule `docs/CONFIGURATION.md` states in section `[sandbox]`, so the broker waits on one lookup per connection and -session. A database query the runner itself gave up on can still run past that. +is discarded. While it runs, a later lookup on the same connection waits for it, by the rule +`docs/CONFIGURATION.md` states in section `[sandbox]`. So at most one runner call runs per connection +and session. A database query the runner itself gave up on can still run past that. **Not built:** decision (2), the OS confinement: AC-3 (Landlock and seccomp), AC-4 (the AppContainer token), and AC-7 (no opening or signalling the parent or a sibling). The first two open boxes above, diff --git a/messagefoundry/fhirsearch.py b/messagefoundry/fhirsearch.py index edd2a9fcbc..ea358cfb8c 100644 --- a/messagefoundry/fhirsearch.py +++ b/messagefoundry/fhirsearch.py @@ -176,7 +176,7 @@ def _screen(key: str, value: str, *, half: str) -> str: ) -def _resolve_one(key: str, value: FhirSearchValue | ForeignSearchValue) -> str: +def _resolve_one(key: str, value: object) -> str: """One ``params`` value to the string that goes on the URL, before percent-encoding.""" if isinstance(value, str): return _screen(key, value, half="value") diff --git a/messagefoundry/pipeline/sandbox.py b/messagefoundry/pipeline/sandbox.py index ede365864e..aedd75b71e 100644 --- a/messagefoundry/pipeline/sandbox.py +++ b/messagefoundry/pipeline/sandbox.py @@ -724,7 +724,8 @@ def __init__( self._closed = False # The [egress] lists whose empty-list refusal this session has logged. self._lookup_refusals_logged: set[str] = set() - # Per (kind, connection), the completion of the lookup last started on it: see _serve_lookup. + # Per (kind, connection), the completion event of the runner call running on it, removed by + # that call when it returns: see _serve_lookup. self._stale_lookups: dict[tuple[str, str], threading.Event] = {} self._stale_lock = threading.Lock() @@ -1125,30 +1126,33 @@ def _serve_bounded( stdin = proc.stdin failure: list[BaseException] = [] fault: list[BaseException] = [] - with self._stale_lock: - for key in [key for key, ended in self._stale_lookups.items() if ended.is_set()]: - del self._stale_lookups[key] - done = threading.Event() + # Set by this thread, under _stale_lock, the moment it gives up on the helper. The helper + # reads it under the same lock before it registers and calls the runner, so the two threads + # agree on whether the dispatch is over without comparing clocks (see _serve_lookup). + abandoned = threading.Event() + + def report(exc: BaseException) -> None: + fault.append(exc) + log.error( + "inbound %r: the engine failed while serving a brokered lookup for %s %r (%s)", + self._inbound, + phase, + name, + type(exc).__name__, + ) def serve() -> None: try: - reply = self._lookup_reply(request_id, phase, name, lookup, deadline, done) - except Exception as exc: # noqa: BLE001 — logged here, failed by the dispatch thread - fault.append(exc) - log.error( - "inbound %r: the engine failed while serving a brokered lookup for %s %r (%s)", - self._inbound, - phase, - name, - type(exc).__name__, - ) + reply = self._lookup_reply(request_id, phase, name, lookup, deadline, abandoned) + except Exception as exc: # noqa: BLE001 — logged by type, failed by the dispatch thread + report(exc) return - finally: - done.set() try: _write_frame(stdin, reply) except (OSError, ValueError, SandboxError) as exc: # ValueError: a pipe closed under it failure.append(exc) + except Exception as exc: # noqa: BLE001 — logged by type, failed by the dispatch thread + report(exc) context = contextvars.copy_context() helper = threading.Thread( @@ -1160,6 +1164,8 @@ def serve() -> None: helper.start() helper.join(max(0.0, deadline - time.monotonic())) if helper.is_alive(): + with self._stale_lock: + abandoned.set() return False if fault: self._kill(proc) @@ -1179,11 +1185,11 @@ def _lookup_reply( name: str, lookup: codec.LookupRequest, deadline: float, - done: threading.Event, + abandoned: threading.Event, ) -> bytes: """The ``lookupok``/``lookupfail`` frame answering one ``lookup``. A result that cannot cross is a ``lookupfail`` the worker can report.""" - ok, result, error = self._serve_lookup(phase, lookup, deadline, done) + ok, result, error = self._serve_lookup(phase, lookup, deadline, abandoned) fn = _LOOKUP_FN[lookup.kind] if ok: try: @@ -1202,7 +1208,11 @@ def _lookup_reply( ) def _serve_lookup( - self, phase: str, lookup: codec.LookupRequest, deadline: float, done: threading.Event + self, + phase: str, + lookup: codec.LookupRequest, + deadline: float, + abandoned: threading.Event, ) -> tuple[bool, object, str]: """Answer one brokered lookup in the parent: ``(ok, result, error)``. @@ -1216,34 +1226,39 @@ def _serve_lookup( in-process. The slice-B seam: a broker-only check of a lookup's own arguments goes between the refusals below and the call. - **One lookup at a time per connection.** Each lookup registers ``done``, its completion, for - its connection just before it calls the runner, under ``_stale_lock``. A later lookup on that - connection, which can only meet one the wall cap cut off since a session serves one dispatch - at a time, waits on that record by the rule ``docs/CONFIGURATION.md`` states (section - ``[sandbox]``, *``mode="isolated"``*), and is refused if it still runs then. A helper whose - own dispatch the cap has already ended never calls the runner, so no call starts that nobody - waits for and nothing records.""" + **One runner call at a time per connection.** Under ``_stale_lock``, a lookup registers its + own completion event for its connection, then calls the runner, and removes the record when + the runner returns. A later lookup on that connection can only meet one the wall cap cut off, + since a session serves one dispatch at a time. It waits on that record by the rule + ``docs/CONFIGURATION.md`` states (section ``[sandbox]``, *``mode="isolated"``*), and is + refused if it still runs then. **A helper whose dispatch was abandoned never calls the + runner.** The dispatch thread sets ``abandoned`` under the same lock when it gives up, and the + helper checks it there before it registers. So every runner call is recorded, and none starts + that nobody waits for.""" is_db = lookup.kind == codec.LOOKUP_DB available = db_lookup_available() if is_db else fhir_lookup_available() refusal = self._lookup_refusal(phase, lookup) if available else None if refusal is not None: return False, None, refusal + key = (lookup.kind, lookup.connection) + done = threading.Event() if available: - key = (lookup.kind, lookup.connection) - with self._stale_lock: - stale = self._stale_lookups.get(key) - if stale is not None: - margin = min(2.0, self.policy.wall_seconds / 4) - if not stale.wait(max(0.0, deadline - margin - time.monotonic())): - return False, None, self._busy_refusal(lookup) - with self._stale_lock: - if time.monotonic() >= deadline: - # This dispatch is already over: the worker is being killed and nobody waits. - return False, None, f"{_LOOKUP_FN[lookup.kind]}: the call's wall cap ran out" - current = self._stale_lookups.get(key) - if current is not None and not current.is_set(): + margin = min(2.0, self.policy.wall_seconds / 4) + while True: + with self._stale_lock: + if abandoned.is_set(): + # The dispatch is over: its worker is being killed and nobody waits. + return ( + False, + None, + f"{_LOOKUP_FN[lookup.kind]}: the call's wall cap ran out", + ) + current = self._stale_lookups.get(key) + if current is None: + self._stale_lookups[key] = done + break + if not current.wait(max(0.0, deadline - margin - time.monotonic())): return False, None, self._busy_refusal(lookup) - self._stale_lookups[key] = done try: if is_db: return True, db_lookup(lookup.connection, lookup.target, lookup.params), "" @@ -1253,6 +1268,12 @@ def _serve_lookup( return False, None, _bounded(str(exc)) except Exception as exc: # noqa: BLE001 — the worker waits on this reply; report, never raise return False, None, f"{_LOOKUP_FN[lookup.kind]} failed: {safe_exc(exc)}" + finally: + if available: + with self._stale_lock: + if self._stale_lookups.get(key) is done: + del self._stale_lookups[key] + done.set() @staticmethod def _busy_refusal(lookup: codec.LookupRequest) -> str: diff --git a/tests/test_sandbox_isolated.py b/tests/test_sandbox_isolated.py index 1b34a78eba..97ccac2c9a 100644 --- a/tests/test_sandbox_isolated.py +++ b/tests/test_sandbox_isolated.py @@ -978,8 +978,8 @@ def explode(self: Any, *args: Any) -> bytes: def test_a_cut_off_waiter_does_not_replace_the_hung_lookups_record( graph: tuple[Registry, str], monkeypatch: pytest.MonkeyPatch ) -> None: - # Dispatch B's helper is still building its refusal when B's cap fires. Its record must not - # replace the hung lookup's, or dispatch C would start a second call on the hung connection. + # Dispatch B's helper is still building its refusal when B's cap fires, and C then finds the + # hung lookup still recorded. Neither may start a second call on the hung connection. release = threading.Event() calls: list[str] = [] @@ -1062,9 +1062,18 @@ def gated(self: SandboxSession, phase: str, lookup: codec.LookupRequest) -> str with pytest.raises(SandboxError, match="wall cap"): _bodies(registry, "h_db", session) # B: its helper is held before the runner release_a.set() + # Wait for A's own record to go, not the runner's counter: only then is the connection + # free, so B's helper can only be stopped by its dispatch having been abandoned. + key = (codec.LOOKUP_DB, "LK") deadline = time.monotonic() + 10 - while state["in_flight"] and time.monotonic() < deadline: + + def a_finished() -> bool: + record = session._stale_lookups.get(key) + return record is None or record.is_set() + + while not a_finished() and time.monotonic() < deadline: time.sleep(0.05) + assert a_finished() go.set() # B's helper goes on, past B's deadline, and finds A finished time.sleep(0.5) release_all.set() From bf993aa7ce522146bcfff51298244d27d8a6abda Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Thu, 29 Oct 2026 11:44:27 -0500 Subject: [PATCH 10/15] Sandbox broker: the late-runner check also reads the deadline (BACKLOG #3131) From the covering review round-25b36f23af of e70a35173..6db7214a21: - The helper checks the deadline beside the abandoned event before it registers, and skips writing a reply once abandoned. The docstring says it is a check, not a fence: a call that passed it an instant before the dispatch gave up is recorded and holds the connection. - The records are renamed _running_lookups/_running_lock; key and event are made only where they are used. - CONFIGURATION.md and the ADR say a cut-off lookup that never reached its runner is never started; the ADR paragraph is rewrapped. - Tests join the lookup helper threads before reading counts, and the late-helper test waits on A's helper thread (fails on e70a35173 with 3 runner calls, passes here). Proposed PR title: Sandbox mode isolated: the parent brokers db_lookup and fhir_lookup (BACKLOG #3131, slice A) Proposed banner #3131: PARTIAL, slice A built. mode="isolated"; worker checks read is-not-OFF; parent broker over the closed-tag codec through the in-process runner (AC-1, AC-2, AC-6 per-inbound, AC-5 for the broker), refusing non-Handler phases and an empty allowed_db/http even under the opt-out. Remains: AC-3, AC-4, AC-7, the ADR's open boxes, the benchmark, and pooled-worker AC-6 scoping. --- docs/CONFIGURATION.md | 5 ++- ...e-ipc-brokered-sandbox-extends-adr-0087.md | 23 +++++----- messagefoundry/pipeline/sandbox.py | 43 ++++++++++--------- tests/test_sandbox_isolated.py | 36 ++++++++++------ 4 files changed, 59 insertions(+), 48 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index e3eca0b2e6..308b13a6ad 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1286,8 +1286,9 @@ It differs from `subprocess` in at least these ways: The dry run has no lookup runner, so it fails there as in every mode. - **`wall_seconds` counts the lookups.** The cap bounds the whole call, including the time the engine spends serving its lookups, so raise it for a feed whose lookups are slow. When the cap runs out - during a lookup, the call fails at once and its worker is recycled; the lookup itself runs on to - its own timeout in the engine, and its result is discarded. Until it ends, a later lookup on the + during a lookup, the call fails at once and its worker is recycled. A lookup that has already + reached its runner runs on to its own timeout in the engine, and its result is discarded; one + that has not yet reached it is never started. Until it ends, a later lookup on the same connection from that inbound waits for it, until a quarter of its own call's cap (at most 2 seconds) is left, and is refused if it is still waiting then, leaving that quarter for the Handler to handle the refusal. That bounds what the engine's broker waits on, not the diff --git a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md index 73addf750c..6b2bf746fd 100644 --- a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md +++ b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md @@ -187,17 +187,18 @@ unresearched. ## Build status (2026-10-28, vault BACKLOG #3131 slice A) **Built:** the `isolated` mode and decision (1), the parent-held broker. A Handler's -`db_lookup`/`fhir_lookup` in the worker writes a `lookup` frame in the closed tag set, bound to the -dispatch's `(id, phase, name)` triple and a per-dispatch sequence number (AC-6, for a per-inbound -worker, whose lane the session already is). The parent answers it on a helper thread, in the dispatch's -context, through the same accessor and runner the in-process path uses (AC-1, AC-2), refuses one bound to a Router or -`accepts=` dispatch, and refuses one whose `[egress].allowed_db` or `[egress].allowed_http` is empty. -A refused or failed lookup is the Handler's error, routed to `ERROR`/dead-letter post-ACK (AC-5, for -the broker). `wall_seconds` bounds the whole dispatch, the brokered lookups and the writes of their -replies included; a lookup the cap cuts off runs on to its own timeout in the parent, and its result -is discarded. While it runs, a later lookup on the same connection waits for it, by the rule -`docs/CONFIGURATION.md` states in section `[sandbox]`. So at most one runner call runs per connection -and session. A database query the runner itself gave up on can still run past that. +`db_lookup`/`fhir_lookup` in the worker writes a `lookup` frame in the closed tag set. The frame is +bound to the dispatch's `(id, phase, name)` triple and a per-dispatch sequence number (AC-6, for a +per-inbound worker, whose lane the session already is). The parent answers it on a helper thread, in +the dispatch's context, through the same accessor and runner the in-process path uses (AC-1, AC-2). +It refuses one bound to a Router or `accepts=` dispatch, and one whose `[egress].allowed_db` or +`[egress].allowed_http` is empty. A refused or failed lookup is the Handler's error, routed to +`ERROR`/dead-letter post-ACK (AC-5, for the broker). `wall_seconds` bounds the whole dispatch, the +brokered lookups and the writes of their replies included. A lookup that already reached its runner +when the cap ran out runs on to its own timeout in the parent, and its result is discarded; one that +had not is never started. While it runs, a later lookup on the same connection waits for it, by the +rule `docs/CONFIGURATION.md` states in section `[sandbox]`. So at most one runner call runs per +connection and session. A database query the runner itself gave up on can still run past that. **Not built:** decision (2), the OS confinement: AC-3 (Landlock and seccomp), AC-4 (the AppContainer token), and AC-7 (no opening or signalling the parent or a sibling). The first two open boxes above, diff --git a/messagefoundry/pipeline/sandbox.py b/messagefoundry/pipeline/sandbox.py index aedd75b71e..34fa455853 100644 --- a/messagefoundry/pipeline/sandbox.py +++ b/messagefoundry/pipeline/sandbox.py @@ -726,8 +726,8 @@ def __init__( self._lookup_refusals_logged: set[str] = set() # Per (kind, connection), the completion event of the runner call running on it, removed by # that call when it returns: see _serve_lookup. - self._stale_lookups: dict[tuple[str, str], threading.Event] = {} - self._stale_lock = threading.Lock() + self._running_lookups: dict[tuple[str, str], threading.Event] = {} + self._running_lock = threading.Lock() @property def mode(self) -> SandboxMode: @@ -1126,9 +1126,8 @@ def _serve_bounded( stdin = proc.stdin failure: list[BaseException] = [] fault: list[BaseException] = [] - # Set by this thread, under _stale_lock, the moment it gives up on the helper. The helper - # reads it under the same lock before it registers and calls the runner, so the two threads - # agree on whether the dispatch is over without comparing clocks (see _serve_lookup). + # Set by this thread the moment it gives up on the helper. The helper checks it, with the + # deadline, before it registers and calls the runner, and before it writes its reply. abandoned = threading.Event() def report(exc: BaseException) -> None: @@ -1147,6 +1146,8 @@ def serve() -> None: except Exception as exc: # noqa: BLE001 — logged by type, failed by the dispatch thread report(exc) return + if abandoned.is_set(): + return # the worker is being killed; nothing reads this reply try: _write_frame(stdin, reply) except (OSError, ValueError, SandboxError) as exc: # ValueError: a pipe closed under it @@ -1164,8 +1165,7 @@ def serve() -> None: helper.start() helper.join(max(0.0, deadline - time.monotonic())) if helper.is_alive(): - with self._stale_lock: - abandoned.set() + abandoned.set() return False if fault: self._kill(proc) @@ -1226,36 +1226,37 @@ def _serve_lookup( in-process. The slice-B seam: a broker-only check of a lookup's own arguments goes between the refusals below and the call. - **One runner call at a time per connection.** Under ``_stale_lock``, a lookup registers its + **One runner call at a time per connection.** Under ``_running_lock``, a lookup registers its own completion event for its connection, then calls the runner, and removes the record when the runner returns. A later lookup on that connection can only meet one the wall cap cut off, since a session serves one dispatch at a time. It waits on that record by the rule ``docs/CONFIGURATION.md`` states (section ``[sandbox]``, *``mode="isolated"``*), and is - refused if it still runs then. **A helper whose dispatch was abandoned never calls the - runner.** The dispatch thread sets ``abandoned`` under the same lock when it gives up, and the - helper checks it there before it registers. So every runner call is recorded, and none starts - that nobody waits for.""" + refused if it still runs then. Every runner call is recorded, so a connection never has two. + **A helper does not call the runner once its dispatch is over:** it checks ``abandoned`` and + the deadline just before it registers. That is a check, not a fence. A helper that passed it + an instant before the dispatch gave up still calls the runner, and that call is recorded and + holds the connection like any other.""" is_db = lookup.kind == codec.LOOKUP_DB available = db_lookup_available() if is_db else fhir_lookup_available() refusal = self._lookup_refusal(phase, lookup) if available else None if refusal is not None: return False, None, refusal - key = (lookup.kind, lookup.connection) - done = threading.Event() if available: + key = (lookup.kind, lookup.connection) + done = threading.Event() margin = min(2.0, self.policy.wall_seconds / 4) while True: - with self._stale_lock: - if abandoned.is_set(): + with self._running_lock: + if abandoned.is_set() or time.monotonic() >= deadline: # The dispatch is over: its worker is being killed and nobody waits. return ( False, None, f"{_LOOKUP_FN[lookup.kind]}: the call's wall cap ran out", ) - current = self._stale_lookups.get(key) + current = self._running_lookups.get(key) if current is None: - self._stale_lookups[key] = done + self._running_lookups[key] = done break if not current.wait(max(0.0, deadline - margin - time.monotonic())): return False, None, self._busy_refusal(lookup) @@ -1270,9 +1271,9 @@ def _serve_lookup( return False, None, f"{_LOOKUP_FN[lookup.kind]} failed: {safe_exc(exc)}" finally: if available: - with self._stale_lock: - if self._stale_lookups.get(key) is done: - del self._stale_lookups[key] + with self._running_lock: + if self._running_lookups.get(key) is done: + del self._running_lookups[key] done.set() @staticmethod diff --git a/tests/test_sandbox_isolated.py b/tests/test_sandbox_isolated.py index 97ccac2c9a..ec79cf2637 100644 --- a/tests/test_sandbox_isolated.py +++ b/tests/test_sandbox_isolated.py @@ -633,6 +633,15 @@ def test_a_lookup_with_long_arguments_round_trips(kind: str) -> None: assert answer.connection == "C" * 5000 and answer.params == params +def _join_lookup_helpers(*, exclude_hung: bool = False, timeout: float = 15.0) -> None: + """Wait for the broker's lookup helper threads to finish, so a count read next is final. With + ``exclude_hung``, a helper still inside a runner the test holds is left running.""" + for thread in threading.enumerate(): + if not thread.name.startswith("mf-sandbox-lookup-"): + continue + thread.join(3.0 if exclude_hung else timeout) + + def _worker_broker() -> Any: """The worker's ``_Broker``. Importing the worker installs a stderr root handler, which is right in its child process and wrong here, so the root logger is restored after the import, as the @@ -1006,8 +1015,9 @@ def slow_refusal(lookup: codec.LookupRequest) -> str: try: with db_lookup_mod.activated(hung): for _ in range(3): # A hangs; B and C each find the connection busy - with pytest.raises(SandboxError, match="wall cap|still running"): + with pytest.raises(SandboxError, match="wall cap"): _bodies(registry, "h_db", session) + _join_lookup_helpers(exclude_hung=True) assert len(calls) == 1 finally: release.set() @@ -1058,24 +1068,22 @@ def gated(self: SandboxSession, phase: str, lookup: codec.LookupRequest) -> str with db_lookup_mod.activated(hung): with pytest.raises(SandboxError, match="wall cap"): _bodies(registry, "h_db", session) # A: hangs, cut off + a_helpers = [ + thread + for thread in threading.enumerate() + if thread.name.startswith("mf-sandbox-lookup-") + ] go.clear() with pytest.raises(SandboxError, match="wall cap"): _bodies(registry, "h_db", session) # B: its helper is held before the runner release_a.set() - # Wait for A's own record to go, not the runner's counter: only then is the connection - # free, so B's helper can only be stopped by its dispatch having been abandoned. - key = (codec.LOOKUP_DB, "LK") - deadline = time.monotonic() + 10 - - def a_finished() -> bool: - record = session._stale_lookups.get(key) - return record is None or record.is_set() - - while not a_finished() and time.monotonic() < deadline: - time.sleep(0.05) - assert a_finished() + # Wait for A's own helper to finish, not the runner's counter: only then is the connection + # free, so B's helper can only be stopped by its dispatch being over. + for thread in a_helpers: + thread.join(10) + assert not any(thread.is_alive() for thread in a_helpers) go.set() # B's helper goes on, past B's deadline, and finds A finished - time.sleep(0.5) + _join_lookup_helpers() release_all.set() assert _bodies(registry, "h_db", session)[0].startswith("late|") # C assert (state["calls"], state["peak"]) == (2, 1) # A and C; never B From dd14daea3df0bfc1daa50989d35fdf4f43557036 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Thu, 29 Oct 2026 12:04:30 -0500 Subject: [PATCH 11/15] Sandbox broker: say exactly what the cap stops; tighten the race tests (BACKLOG #3131) From the covering review round-ce17257ecf of bf993aa7ce: - CONFIGURATION.md no longer says a cut-off lookup is "never started": one a moment from its runner call can still start, and then counts like any other. The ADR links to that statement instead of restating it, and the pronoun naming the lookup that holds the connection is fixed. - key and the completion event are bound on every path again; the write comment says only "abandoned" guards the write. - The late-helper test joins only A's own helper (a set difference), and its comment names the call count as the guard; the cut-off-waiter test pins the exact cap text. Proposed PR title: Sandbox mode isolated: the parent brokers db_lookup and fhir_lookup (BACKLOG #3131, slice A) Proposed banner #3131: PARTIAL, slice A built. mode="isolated"; worker checks read is-not-OFF; parent broker over the closed-tag codec through the in-process runner (AC-1, AC-2, AC-6 per-inbound, AC-5 for the broker), refusing non-Handler phases and an empty allowed_db/http even under the opt-out. Remains: AC-3, AC-4, AC-7, the ADR's open boxes, the benchmark, and pooled-worker AC-6 scoping. --- docs/CONFIGURATION.md | 7 ++++--- ...handler-code-ipc-brokered-sandbox-extends-adr-0087.md | 8 +++----- messagefoundry/pipeline/sandbox.py | 6 +++--- tests/test_sandbox_isolated.py | 9 +++++---- 4 files changed, 15 insertions(+), 15 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 308b13a6ad..b6f9da3459 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1287,9 +1287,10 @@ It differs from `subprocess` in at least these ways: - **`wall_seconds` counts the lookups.** The cap bounds the whole call, including the time the engine spends serving its lookups, so raise it for a feed whose lookups are slow. When the cap runs out during a lookup, the call fails at once and its worker is recycled. A lookup that has already - reached its runner runs on to its own timeout in the engine, and its result is discarded; one - that has not yet reached it is never started. Until it ends, a later lookup on the - same connection from that inbound waits for it, until a quarter of its own call's cap (at most 2 + reached its runner runs on to its own timeout in the engine, and its result is discarded. One + that has not reached it is normally not started, though one that was a moment from its runner + call when the cap ran out can still start, and then counts like any other. While a lookup runs on + this way, a later lookup on the same connection from that inbound waits for it, until a quarter of its own call's cap (at most 2 seconds) is left, and is refused if it is still waiting then, leaving that quarter for the Handler to handle the refusal. That bounds what the engine's broker waits on, not the database: a `db_lookup` its runner gave up on can still be running there, and a reload starts with diff --git a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md index 6b2bf746fd..0f1ffbb3b5 100644 --- a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md +++ b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md @@ -194,11 +194,9 @@ the dispatch's context, through the same accessor and runner the in-process path It refuses one bound to a Router or `accepts=` dispatch, and one whose `[egress].allowed_db` or `[egress].allowed_http` is empty. A refused or failed lookup is the Handler's error, routed to `ERROR`/dead-letter post-ACK (AC-5, for the broker). `wall_seconds` bounds the whole dispatch, the -brokered lookups and the writes of their replies included. A lookup that already reached its runner -when the cap ran out runs on to its own timeout in the parent, and its result is discarded; one that -had not is never started. While it runs, a later lookup on the same connection waits for it, by the -rule `docs/CONFIGURATION.md` states in section `[sandbox]`. So at most one runner call runs per -connection and session. A database query the runner itself gave up on can still run past that. +brokered lookups and the writes of their replies included. What happens to a lookup the cap cuts +off, and to a later lookup on the same connection, is stated once, in `docs/CONFIGURATION.md` +section `[sandbox]`. At most one runner call runs per connection and session. **Not built:** decision (2), the OS confinement: AC-3 (Landlock and seccomp), AC-4 (the AppContainer token), and AC-7 (no opening or signalling the parent or a sibling). The first two open boxes above, diff --git a/messagefoundry/pipeline/sandbox.py b/messagefoundry/pipeline/sandbox.py index 34fa455853..506deef047 100644 --- a/messagefoundry/pipeline/sandbox.py +++ b/messagefoundry/pipeline/sandbox.py @@ -1127,7 +1127,7 @@ def _serve_bounded( failure: list[BaseException] = [] fault: list[BaseException] = [] # Set by this thread the moment it gives up on the helper. The helper checks it, with the - # deadline, before it registers and calls the runner, and before it writes its reply. + # deadline, before it registers and calls the runner, and checks it alone before it writes. abandoned = threading.Event() def report(exc: BaseException) -> None: @@ -1241,9 +1241,9 @@ def _serve_lookup( refusal = self._lookup_refusal(phase, lookup) if available else None if refusal is not None: return False, None, refusal + key = (lookup.kind, lookup.connection) + done = threading.Event() if available: - key = (lookup.kind, lookup.connection) - done = threading.Event() margin = min(2.0, self.policy.wall_seconds / 4) while True: with self._running_lock: diff --git a/tests/test_sandbox_isolated.py b/tests/test_sandbox_isolated.py index ec79cf2637..3d8c91fd84 100644 --- a/tests/test_sandbox_isolated.py +++ b/tests/test_sandbox_isolated.py @@ -1015,7 +1015,7 @@ def slow_refusal(lookup: codec.LookupRequest) -> str: try: with db_lookup_mod.activated(hung): for _ in range(3): # A hangs; B and C each find the connection busy - with pytest.raises(SandboxError, match="wall cap"): + with pytest.raises(SandboxError, match="exceeded the 4.0s wall cap"): _bodies(registry, "h_db", session) _join_lookup_helpers(exclude_hung=True) assert len(calls) == 1 @@ -1028,8 +1028,8 @@ def test_a_helper_past_its_deadline_never_calls_the_runner( graph: tuple[Registry, str], monkeypatch: pytest.MonkeyPatch ) -> None: # Dispatch B's helper starts so late that B's cap has already ended, and by then the hung lookup - # A has finished. It must not call the runner: nobody waits for that call and nothing records it, - # so the next dispatch would start another beside it. + # A has finished. It must not call the runner: nobody waits for that call. The guard is the call + # count; peak alone would miss it, since such a call is recorded and C would wait for it. release_a, release_all = threading.Event(), threading.Event() lock = threading.Lock() state = {"calls": 0, "in_flight": 0, "peak": 0} @@ -1066,11 +1066,12 @@ def gated(self: SandboxSession, phase: str, lookup: codec.LookupRequest) -> str ) try: with db_lookup_mod.activated(hung): + earlier = set(threading.enumerate()) with pytest.raises(SandboxError, match="wall cap"): _bodies(registry, "h_db", session) # A: hangs, cut off a_helpers = [ thread - for thread in threading.enumerate() + for thread in set(threading.enumerate()) - earlier if thread.name.startswith("mf-sandbox-lookup-") ] go.clear() From b26cf208d6f90f3c3544e458c3855867e951e107 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Thu, 29 Oct 2026 12:18:28 -0500 Subject: [PATCH 12/15] Docs: the broker's one-call bound does not bound the database (BACKLOG #3131) From the covering review round-90beef0c7f of dd14daea3d: - ADR 0147 keeps the qualifier the last commit dropped: the broker runs one runner call per connection and session, which does not bound a query the runner gave up on, or survive a reload. The pointer to CONFIGURATION.md is a real link now. - CONFIGURATION.md says a late-started lookup is recorded and holds the connection, not that it "counts"; the bullet is rewrapped. - The late-helper test asserts it found A's helper before it waits. Proposed PR title: Sandbox mode isolated: the parent brokers db_lookup and fhir_lookup (BACKLOG #3131, slice A) Proposed banner #3131: PARTIAL, slice A built. mode="isolated"; worker checks read is-not-OFF; parent broker over the closed-tag codec through the in-process runner (AC-1, AC-2, AC-6 per-inbound, AC-5 for the broker), refusing non-Handler phases and an empty allowed_db/http even under the opt-out. Remains: AC-3, AC-4, AC-7, the ADR's open boxes, the benchmark, and pooled-worker AC-6 scoping. --- docs/CONFIGURATION.md | 17 ++++++++--------- ...ode-ipc-brokered-sandbox-extends-adr-0087.md | 6 ++++-- tests/test_sandbox_isolated.py | 1 + 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index b6f9da3459..28c9f8381e 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1287,15 +1287,14 @@ It differs from `subprocess` in at least these ways: - **`wall_seconds` counts the lookups.** The cap bounds the whole call, including the time the engine spends serving its lookups, so raise it for a feed whose lookups are slow. When the cap runs out during a lookup, the call fails at once and its worker is recycled. A lookup that has already - reached its runner runs on to its own timeout in the engine, and its result is discarded. One - that has not reached it is normally not started, though one that was a moment from its runner - call when the cap ran out can still start, and then counts like any other. While a lookup runs on - this way, a later lookup on the same connection from that inbound waits for it, until a quarter of its own call's cap (at most 2 - seconds) is left, and is refused if it is still waiting then, leaving that quarter for the Handler - to handle the refusal. That bounds what the engine's broker waits on, not the - database: a `db_lookup` its runner gave up on can still be running there, and a reload starts with - no record of a lookup still running. -- **It needs a config directory.** A graph built in code with none cannot start a worker. Under + reached its runner runs on to its own timeout in the engine, and its result is discarded. One that + has not reached it is normally not started. One that was a moment from its runner call when the + cap ran out can still start; it is then recorded and holds the connection like any other. While a + lookup runs on this way, a later lookup on the same connection from that inbound waits for it, + until a quarter of its own call's cap (at most 2 seconds) is left. It is refused if it is still + waiting then, which leaves that quarter for the Handler to handle the refusal. That bounds what + the engine's broker waits on, not the database: a `db_lookup` its runner gave up on can still be + running there, and a reload starts with no record of a lookup still running.- **It needs a config directory.** A graph built in code with none cannot start a worker. Under `subprocess` such a graph quietly runs in-process; under `isolated` the runner refuses to be built instead, because in-process its lookups would not go through the broker. diff --git a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md index 0f1ffbb3b5..5c66ce5e85 100644 --- a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md +++ b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md @@ -195,8 +195,10 @@ It refuses one bound to a Router or `accepts=` dispatch, and one whose `[egress] `[egress].allowed_http` is empty. A refused or failed lookup is the Handler's error, routed to `ERROR`/dead-letter post-ACK (AC-5, for the broker). `wall_seconds` bounds the whole dispatch, the brokered lookups and the writes of their replies included. What happens to a lookup the cap cuts -off, and to a later lookup on the same connection, is stated once, in `docs/CONFIGURATION.md` -section `[sandbox]`. At most one runner call runs per connection and session. +off, and to a later lookup on the same connection, is stated in +[CONFIGURATION.md, `[sandbox]`](../CONFIGURATION.md#sandbox). The broker runs at most one runner call +per connection and session. That does not bound the database: a query the runner itself gave up on +can still be running, and a reload starts a session with no record of it. **Not built:** decision (2), the OS confinement: AC-3 (Landlock and seccomp), AC-4 (the AppContainer token), and AC-7 (no opening or signalling the parent or a sibling). The first two open boxes above, diff --git a/tests/test_sandbox_isolated.py b/tests/test_sandbox_isolated.py index 3d8c91fd84..d2af22933b 100644 --- a/tests/test_sandbox_isolated.py +++ b/tests/test_sandbox_isolated.py @@ -1074,6 +1074,7 @@ def gated(self: SandboxSession, phase: str, lookup: codec.LookupRequest) -> str for thread in set(threading.enumerate()) - earlier if thread.name.startswith("mf-sandbox-lookup-") ] + assert len(a_helpers) == 1 # the sync point below depends on finding A's helper go.clear() with pytest.raises(SandboxError, match="wall cap"): _bodies(registry, "h_db", session) # B: its helper is held before the runner From 87024da93afd6d20c7483d01110cab005198731a Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Thu, 29 Oct 2026 12:34:24 -0500 Subject: [PATCH 13/15] Docs: restore the config-dir bullet; scope the one-call bound to a session (BACKLOG #3131) From the covering review round-7905f465ad of b26cf208d6: - The previous rewrap glued "- **It needs a config directory.**" to the end of the wall_seconds bullet; it is a list item again. - CONFIGURATION.md states the bound once: within one inbound and one session; a reload replaces the session, two inbounds can each run a lookup on one connection, and the database is not bounded. The ADR links there instead of restating it; the _serve_lookup docstring is scoped to one session. - The late-helper test finds exactly one helper each for A and B by set difference and joins only those. Proposed PR title: Sandbox mode isolated: the parent brokers db_lookup and fhir_lookup (BACKLOG #3131, slice A) Proposed banner #3131: PARTIAL, slice A built. mode="isolated"; worker checks read is-not-OFF; parent broker over the closed-tag codec through the in-process runner (AC-1, AC-2, AC-6 per-inbound, AC-5 for the broker), refusing non-Handler phases and an empty allowed_db/http even under the opt-out. Remains: AC-3, AC-4, AC-7, the ADR's open boxes, the benchmark, and pooled-worker AC-6 scoping. --- docs/CONFIGURATION.md | 8 +++--- ...e-ipc-brokered-sandbox-extends-adr-0087.md | 6 ++--- messagefoundry/pipeline/sandbox.py | 3 ++- tests/test_sandbox_isolated.py | 27 ++++++++++++------- 4 files changed, 26 insertions(+), 18 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 28c9f8381e..715d8586f2 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1292,9 +1292,11 @@ It differs from `subprocess` in at least these ways: cap ran out can still start; it is then recorded and holds the connection like any other. While a lookup runs on this way, a later lookup on the same connection from that inbound waits for it, until a quarter of its own call's cap (at most 2 seconds) is left. It is refused if it is still - waiting then, which leaves that quarter for the Handler to handle the refusal. That bounds what - the engine's broker waits on, not the database: a `db_lookup` its runner gave up on can still be - running there, and a reload starts with no record of a lookup still running.- **It needs a config directory.** A graph built in code with none cannot start a worker. Under + waiting then, which leaves that quarter for the Handler to handle the refusal. This rule holds + within one inbound and one session. A reload replaces the session, so a lookup still running in + the old one is not waited for, and two inbounds can each run a lookup on one connection. It does + not bound the database either: a `db_lookup` its runner gave up on can still be running there. +- **It needs a config directory.** A graph built in code with none cannot start a worker. Under `subprocess` such a graph quietly runs in-process; under `isolated` the runner refuses to be built instead, because in-process its lookups would not go through the broker. diff --git a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md index 5c66ce5e85..0fecbb2f25 100644 --- a/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md +++ b/docs/adr/0147-hardened-runtime-isolation-for-router-handler-code-ipc-brokered-sandbox-extends-adr-0087.md @@ -195,10 +195,8 @@ It refuses one bound to a Router or `accepts=` dispatch, and one whose `[egress] `[egress].allowed_http` is empty. A refused or failed lookup is the Handler's error, routed to `ERROR`/dead-letter post-ACK (AC-5, for the broker). `wall_seconds` bounds the whole dispatch, the brokered lookups and the writes of their replies included. What happens to a lookup the cap cuts -off, and to a later lookup on the same connection, is stated in -[CONFIGURATION.md, `[sandbox]`](../CONFIGURATION.md#sandbox). The broker runs at most one runner call -per connection and session. That does not bound the database: a query the runner itself gave up on -can still be running, and a reload starts a session with no record of it. +off, and to a later lookup on the same connection, is stated once, with the limits of that bound, +in [CONFIGURATION.md, `[sandbox]`](../CONFIGURATION.md#sandbox), under `mode="isolated"`. **Not built:** decision (2), the OS confinement: AC-3 (Landlock and seccomp), AC-4 (the AppContainer token), and AC-7 (no opening or signalling the parent or a sibling). The first two open boxes above, diff --git a/messagefoundry/pipeline/sandbox.py b/messagefoundry/pipeline/sandbox.py index 506deef047..aa6840e49e 100644 --- a/messagefoundry/pipeline/sandbox.py +++ b/messagefoundry/pipeline/sandbox.py @@ -1231,7 +1231,8 @@ def _serve_lookup( the runner returns. A later lookup on that connection can only meet one the wall cap cut off, since a session serves one dispatch at a time. It waits on that record by the rule ``docs/CONFIGURATION.md`` states (section ``[sandbox]``, *``mode="isolated"``*), and is - refused if it still runs then. Every runner call is recorded, so a connection never has two. + refused if it still runs then. Every runner call is recorded, so a connection never has two in + one session (a reload replaces the session; ``docs/CONFIGURATION.md`` states the limits). **A helper does not call the runner once its dispatch is over:** it checks ``abandoned`` and the deadline just before it registers. That is a check, not a fence. A helper that passed it an instant before the dispatch gave up still calls the runner, and that call is recorded and diff --git a/tests/test_sandbox_isolated.py b/tests/test_sandbox_isolated.py index d2af22933b..be0f1e5746 100644 --- a/tests/test_sandbox_isolated.py +++ b/tests/test_sandbox_isolated.py @@ -642,6 +642,15 @@ def _join_lookup_helpers(*, exclude_hung: bool = False, timeout: float = 15.0) - thread.join(3.0 if exclude_hung else timeout) +def _new_lookup_helpers(earlier: set[threading.Thread]) -> list[threading.Thread]: + """The broker's lookup helper threads started since ``earlier`` was taken.""" + return [ + thread + for thread in set(threading.enumerate()) - earlier + if thread.name.startswith("mf-sandbox-lookup-") + ] + + def _worker_broker() -> Any: """The worker's ``_Broker``. Importing the worker installs a stderr root handler, which is right in its child process and wrong here, so the root logger is restored after the import, as the @@ -1069,23 +1078,21 @@ def gated(self: SandboxSession, phase: str, lookup: codec.LookupRequest) -> str earlier = set(threading.enumerate()) with pytest.raises(SandboxError, match="wall cap"): _bodies(registry, "h_db", session) # A: hangs, cut off - a_helpers = [ - thread - for thread in set(threading.enumerate()) - earlier - if thread.name.startswith("mf-sandbox-lookup-") - ] - assert len(a_helpers) == 1 # the sync point below depends on finding A's helper + # Exactly one helper each for A and B: the sync points below depend on finding them. + (a_helper,) = _new_lookup_helpers(earlier) go.clear() + earlier = set(threading.enumerate()) with pytest.raises(SandboxError, match="wall cap"): _bodies(registry, "h_db", session) # B: its helper is held before the runner + (b_helper,) = _new_lookup_helpers(earlier) release_a.set() # Wait for A's own helper to finish, not the runner's counter: only then is the connection # free, so B's helper can only be stopped by its dispatch being over. - for thread in a_helpers: - thread.join(10) - assert not any(thread.is_alive() for thread in a_helpers) + a_helper.join(10) + assert not a_helper.is_alive() go.set() # B's helper goes on, past B's deadline, and finds A finished - _join_lookup_helpers() + b_helper.join(10) + assert not b_helper.is_alive() release_all.set() assert _bodies(registry, "h_db", session)[0].startswith("late|") # C assert (state["calls"], state["peak"]) == (2, 1) # A and C; never B From cb3dbb2c133ba0ef73a82b6249bcbcf6f0072306 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Thu, 29 Oct 2026 21:54:13 -0500 Subject: [PATCH 14/15] Sandbox broker: scope the busy refusal to the session; pin each late-runner check BACKLOG #3131, slice A. From the covering review round-6bb475e616 of e70a35173..87024da93: - The busy refusal a dead-letter carries said "the engine serves one at a time per connection"; it now says this inbound's sandbox session does, matching CONFIGURATION.md. - A new test pins the deadline check and the abandoned check each on its own; the late-helper test passed with either one deleted. - The cut-off-waiters test joins B's and C's own helpers and asserts they ended, instead of a 3 s join of every helper that could skip one. Correction to earlier messages on this branch: "never calls the runner" and "none starts that nobody waits for" overstate the code. The check before the runner call is a check, not a fence; a call that passed it just before the cap fired still runs, and is recorded. --- messagefoundry/pipeline/sandbox.py | 4 +- tests/test_sandbox_isolated.py | 63 ++++++++++++++++++++++++------ 2 files changed, 52 insertions(+), 15 deletions(-) diff --git a/messagefoundry/pipeline/sandbox.py b/messagefoundry/pipeline/sandbox.py index aa6840e49e..9adc7787c8 100644 --- a/messagefoundry/pipeline/sandbox.py +++ b/messagefoundry/pipeline/sandbox.py @@ -1282,8 +1282,8 @@ def _busy_refusal(lookup: codec.LookupRequest) -> str: fn = _LOOKUP_FN[lookup.kind] return ( f"{fn}({_shown_name(lookup.connection)!r}) refused: an earlier lookup on this " - "connection is still running after the wall cap cut it off, and the engine serves " - "one at a time per connection (ADR 0147)" + "connection is still running after the wall cap cut it off, and this inbound's " + "sandbox session serves one at a time per connection (ADR 0147)" ) def _lookup_refusal(self, phase: str, lookup: codec.LookupRequest) -> str | None: diff --git a/tests/test_sandbox_isolated.py b/tests/test_sandbox_isolated.py index be0f1e5746..90a1aaf42a 100644 --- a/tests/test_sandbox_isolated.py +++ b/tests/test_sandbox_isolated.py @@ -633,15 +633,6 @@ def test_a_lookup_with_long_arguments_round_trips(kind: str) -> None: assert answer.connection == "C" * 5000 and answer.params == params -def _join_lookup_helpers(*, exclude_hung: bool = False, timeout: float = 15.0) -> None: - """Wait for the broker's lookup helper threads to finish, so a count read next is final. With - ``exclude_hung``, a helper still inside a runner the test holds is left running.""" - for thread in threading.enumerate(): - if not thread.name.startswith("mf-sandbox-lookup-"): - continue - thread.join(3.0 if exclude_hung else timeout) - - def _new_lookup_helpers(earlier: set[threading.Thread]) -> list[threading.Thread]: """The broker's lookup helper threads started since ``earlier`` was taken.""" return [ @@ -993,22 +984,26 @@ def explode(self: Any, *args: Any) -> bytes: assert any("RuntimeError" in r.getMessage() for r in caplog.records) -def test_a_cut_off_waiter_does_not_replace_the_hung_lookups_record( +def test_cut_off_waiters_never_start_a_second_call_on_a_hung_connection( graph: tuple[Registry, str], monkeypatch: pytest.MonkeyPatch ) -> None: - # Dispatch B's helper is still building its refusal when B's cap fires, and C then finds the - # hung lookup still recorded. Neither may start a second call on the hung connection. + # A's lookup hangs and stays recorded. Dispatch B's helper is still building its refusal when + # B's cap fires, and C then finds A's record too. Neither may start a second call on it. release = threading.Event() calls: list[str] = [] + hung_helpers: list[threading.Thread] = [] + refusing_helpers: list[threading.Thread] = [] def hung(connection: str, statement: str, params: Any) -> list[dict[str, Any]]: calls.append(statement) + hung_helpers.append(threading.current_thread()) release.wait(60) return [{"v": "late", "d": Decimal(0)}] real_refusal = SandboxSession._busy_refusal def slow_refusal(lookup: codec.LookupRequest) -> str: + refusing_helpers.append(threading.current_thread()) time.sleep(2.0) # past the 1 s margin at wall_seconds=4 return real_refusal(lookup) @@ -1026,8 +1021,14 @@ def slow_refusal(lookup: codec.LookupRequest) -> str: for _ in range(3): # A hangs; B and C each find the connection busy with pytest.raises(SandboxError, match="exceeded the 4.0s wall cap"): _bodies(registry, "h_db", session) - _join_lookup_helpers(exclude_hung=True) + # B's and C's helpers must be over before the count is read, or a late call could land + # after it. Each named itself in the refusal it was building when its cap fired. + assert len(refusing_helpers) == 2 + for thread in refusing_helpers: + thread.join(10) + assert not thread.is_alive() assert len(calls) == 1 + assert hung_helpers[0].is_alive() # control: A really is still inside the runner finally: release.set() session.close() @@ -1101,3 +1102,39 @@ def gated(self: SandboxSession, phase: str, lookup: codec.LookupRequest) -> str release_all.set() go.set() session.close() + + +@pytest.mark.parametrize("over", ["deadline", "abandoned"]) +def test_each_late_runner_check_stops_the_call_alone(tmp_path: Path, over: str) -> None: + # The test above meets both checks at once, so it passes with either one deleted. Here only one + # says the dispatch is over: a deadline already past with nobody gone, or an abandoned dispatch + # with time left. The window between the deadline and abandoned.set() is the first case. + calls: list[str] = [] + + def runner(connection: str, statement: str, params: Any) -> list[dict[str, Any]]: + calls.append(statement) + return [] + + session = _session(str(tmp_path), SandboxMode.ISOLATED) + lookup = codec.LookupRequest( + seq=1, kind=codec.LOOKUP_DB, connection="LK", target="SELECT 1", params=None + ) + abandoned = threading.Event() + if over == "abandoned": + abandoned.set() + deadline = time.monotonic() + 30 + else: + deadline = time.monotonic() - 0.01 + try: + with db_lookup_mod.activated(runner): + ok, _, error = session._serve_lookup("transform", lookup, deadline, abandoned) + assert (ok, calls) == (False, []) + assert "wall cap ran out" in error + # Control: the same lookup, with time left and its dispatch still waiting, is served. + ok, _, _ = session._serve_lookup( + "transform", lookup, time.monotonic() + 30, threading.Event() + ) + assert (ok, calls) == (True, ["SELECT 1"]) + assert session._running_lookups == {} + finally: + session.close() From 31ee152cb01d59b37af851e3d7a0c4e1a484c085 Mon Sep 17 00:00:00 2001 From: Scott Hall Date: Thu, 29 Oct 2026 22:10:06 -0500 Subject: [PATCH 15/15] Sandbox broker: put the busy refusal's scope first; say when a session restarts BACKLOG #3131, slice A. From the covering review round-547430f655 of e70a35173..cb3dbb2c13: - A dead-letter keeps about 200 characters of the busy refusal, so the session scope and "still running" now come first, ADR pointer last. - CONFIGURATION.md says when an inbound's sandbox session starts afresh (a reload or a stop), instead of naming only a reload. Correction to 6db7214a21's message: there is no _stale_lock. The dispatch thread sets the abandoned event under no lock; the helper reads it, with the deadline, under _running_lock before it registers. Proposed PR title: Sandbox mode isolated: the parent brokers db_lookup and fhir_lookup (BACKLOG #3131, slice A) Proposed banner #3131: PARTIAL, slice A built. mode="isolated"; worker checks read is-not-OFF; parent broker over the closed-tag codec through the in-process runner (AC-1, AC-2, AC-6 per-inbound, AC-5 for the broker), refusing non-Handler phases and an empty allowed_db/http even under the opt-out. Remains: AC-3, AC-4, AC-7, the ADR's open boxes, the benchmark, and pooled-worker AC-6 scoping. --- docs/CONFIGURATION.md | 7 ++++--- messagefoundry/pipeline/sandbox.py | 7 ++++--- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index 715d8586f2..83e4d08756 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1293,9 +1293,10 @@ It differs from `subprocess` in at least these ways: lookup runs on this way, a later lookup on the same connection from that inbound waits for it, until a quarter of its own call's cap (at most 2 seconds) is left. It is refused if it is still waiting then, which leaves that quarter for the Handler to handle the refusal. This rule holds - within one inbound and one session. A reload replaces the session, so a lookup still running in - the old one is not waited for, and two inbounds can each run a lookup on one connection. It does - not bound the database either: a `db_lookup` its runner gave up on can still be running there. + per inbound, within one sandbox session: the engine starts an inbound's session afresh at a + reload or a stop, and a lookup still running from before then is not waited for. Two inbounds can + each run a lookup on one connection. It does not bound the database either: a `db_lookup` its + runner gave up on can still be running there. - **It needs a config directory.** A graph built in code with none cannot start a worker. Under `subprocess` such a graph quietly runs in-process; under `isolated` the runner refuses to be built instead, because in-process its lookups would not go through the broker. diff --git a/messagefoundry/pipeline/sandbox.py b/messagefoundry/pipeline/sandbox.py index 9adc7787c8..523225be8b 100644 --- a/messagefoundry/pipeline/sandbox.py +++ b/messagefoundry/pipeline/sandbox.py @@ -1281,9 +1281,10 @@ def _serve_lookup( def _busy_refusal(lookup: codec.LookupRequest) -> str: fn = _LOOKUP_FN[lookup.kind] return ( - f"{fn}({_shown_name(lookup.connection)!r}) refused: an earlier lookup on this " - "connection is still running after the wall cap cut it off, and this inbound's " - "sandbox session serves one at a time per connection (ADR 0147)" + # Short, scope first: a dead-letter keeps about the first 200 characters of this. + f"{fn}({_shown_name(lookup.connection)!r}) refused: this inbound's sandbox session " + "runs one lookup per connection, and one the wall cap cut off is still running " + "(ADR 0147)" ) def _lookup_refusal(self, phase: str, lookup: codec.LookupRequest) -> str | None: