From c2e2eeb1524e63cda46fa1726c990c5b53f550cc Mon Sep 17 00:00:00 2001 From: cdeust Date: Mon, 10 Aug 2026 17:07:51 +0200 Subject: [PATCH 1/2] fix(ci): close docker_smoke.sh's stdin-before-drain race in the bare-container gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docker_smoke.sh drove the container with `printf '%s' "$REQUESTS" | docker run --rm -i ...`: printf closes its end of the pipe (the container's stdin) the instant the batch is written, before any response has been read. That is the exact anti-pattern PR #331 fixed in scripts/mcp_host_client.py for a local subprocess: closing stdin is the MCP shutdown signal (2025-06-18 SS Lifecycle -> Shutdown -> stdio), not an end-of-input marker, and mcp 2.0.0's _handle_request drops an in-flight response write rather than deliver it once EOF fires the cancel scope. Verified against jsonrpc_dispatcher.py that this cancellation path is method-agnostic, so tools/list (id=3) is exactly as vulnerable as any other request — matching this gate's observed signature ("no valid tools/list response (id=3)", empty stderr, no JSON-RPC error frame) and its history of intermittent failures on unrelated PRs and on main itself. Fix: scripts/docker_smoke_client.py drives the container the same way mcp_host_client.py drives a local server — keep stdin open until every expected response id has arrived (mcp_host_client.drain_exchange, a new generic primitive extracted from _exchange, behavior-preserving), close it only then. docker_smoke.sh now builds the image and delegates the run+exchange+assert sequence to this module; the watchdog still docker-kills the container via --cidfile on the same 60s deadline (fires at most once, never retries). Deterministic reproduction (no clock, no docker, no retries): - tests_py/infrastructure/test_stdio_eof_drain.py adds TestDockerSmokeToolsListLostBeforeDrain, using docker_smoke's own id=3/tools/list request against the real mcp 2.0.0 SDK: the write-then-close-before-drain shape drops the response (reproducing the gate's literal historical failure), the drain-then-close shape does not. - tests_py/scripts/test_docker_smoke_client.py pins that docker_smoke_client's exchange never closes stdin before both expected ids are read, for its own three-frame batch. Corroborating measurement (not the gate, since the race is probabilistic): 20/20 real `docker build` + docker_smoke.sh runs passed locally against the fixed image, versus the historical ~1-in-5 failure rate on the old script (main run at 12:29 today, PRs #423-425). Co-Authored-By: Claude --- scripts/docker_smoke.sh | 190 ++----------- scripts/docker_smoke_client.py | 255 ++++++++++++++++++ scripts/mcp_host_client.py | 35 ++- .../infrastructure/test_stdio_eof_drain.py | 127 ++++++++- tests_py/scripts/test_docker_smoke_client.py | 172 ++++++++++++ 5 files changed, 603 insertions(+), 176 deletions(-) create mode 100644 scripts/docker_smoke_client.py create mode 100644 tests_py/scripts/test_docker_smoke_client.py diff --git a/scripts/docker_smoke.sh b/scripts/docker_smoke.sh index 7acafca8..6a1656f6 100755 --- a/scripts/docker_smoke.sh +++ b/scripts/docker_smoke.sh @@ -9,12 +9,15 @@ # nothing in CI exercised it — every CI job installs the `[postgresql]` extra # and/or starts a database, so the "psycopg absent, no DB" path was never hit. # -# This script builds the production image from the repo root Dockerfile, runs -# it with no environment variables and no linked services, sends `initialize` -# + `tools/list` over stdio (the real MCP transport — no HTTP port is -# exposed), and asserts the advertised tool count is at least the standalone -# baseline. `>=` (not `==`) so the gate does not need editing every time a -# new tool ships — it only fires when the count regresses. +# This script builds the production image from the repo root Dockerfile, then +# delegates the run+exchange+assert sequence to `scripts/docker_smoke_client.py` +# (see that module's docstring for why: the stdio exchange with the container +# must keep stdin open until every expected response has arrived, which bash +# `printf | docker run` cannot do — it closes the container's stdin the +# instant the batch is written, which is what made this gate intermittent +# rather than simply broken; commit 18d4505 documents the interleaving, PR +# #331 fixed the identical defect for a local subprocess via +# `scripts/mcp_host_client.py`, and this script now reuses that fix). # # Usage: # scripts/docker_smoke.sh # build image + smoke test @@ -42,6 +45,13 @@ IMAGE="${CORTEX_SMOKE_IMAGE:-cortex-smoke:local}" REPO_ROOT="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" SKIP_BUILD=0 +# source: DOCKER_RUN_TIMEOUT_SECONDS below is not new — it is the same +# budget this script has used for its `docker run` watchdog since the +# watchdog was introduced (measured 2026-07-30 against a deliberately +# hanging test image; see `scripts/docker_smoke_client.py`'s docstring for +# the kill-mechanism rationale, which the watchdog now implements). +DOCKER_RUN_TIMEOUT_SECONDS=60 + for arg in "$@"; do case "$arg" in --skip-build) SKIP_BUILD=1 ;; @@ -57,164 +67,10 @@ if [[ "$SKIP_BUILD" -eq 0 ]]; then docker build -t "$IMAGE" -f "${REPO_ROOT}/Dockerfile" "$REPO_ROOT" fi -# Bare contract: no -e flags, no --link, no compose network. The container -# must self-select the SQLite fallback (CORTEX_RUNTIME=cowork, set in the -# Dockerfile itself) with zero external services. -# `notifications/initialized` carries NO "id". JSON-RPC 2.0 §4.1: "A -# Notification is a Request object without an 'id' member" — the presence of -# an id is the ONLY thing that distinguishes the two, so an id here made the -# server route the message to `ClientRequest`, whose method union does not -# contain any `notifications/*` member (verified against mcp.types: -# ClientRequest = ping|initialize|completion/complete|logging/setLevel| -# prompts/*|resources/*|tools/*|tasks/*; `notifications/initialized` lives -# only in ClientNotification). The server answered id=2 with -32602 and -# logged "28 validation errors for ClientRequest", one per union member. -# -# That is what made this gate FLAKY rather than simply broken: the malformed -# frame put the server on an error path mid-handshake, and whether it still -# answered id=3 before stdin EOF shut it down was a race. Same commit -# 18d4505 failed at 21:52Z and passed at 22:12Z. A correct handshake has no -# such error path. -REQUESTS=$'{"jsonrpc":"2.0","id":1,"method":"initialize","params":{"protocolVersion":"2024-11-05","capabilities":{},"clientInfo":{"name":"docker-smoke","version":"0"}}}\n{"jsonrpc":"2.0","method":"notifications/initialized"}\n{"jsonrpc":"2.0","id":3,"method":"tools/list","params":{}}\n' - -echo "docker_smoke: running ${IMAGE} with zero env vars, sending initialize + tools/list over stdio ..." >&2 - -# Unique per run: two smoke runs on one runner must not read each other's -# diagnostics, and a stale file from a previous run must not be mistaken for -# this run's output. -# -# Explicit XXXXXX template, not `mktemp -t `: GNU coreutils mktemp -# (the ubuntu-latest runner) requires the template to end in at least three -# X's and errors "too few X's in template" otherwise, while BSD/macOS mktemp -# treats -t's argument as a prefix and appends its own suffix. The bare -t -# form therefore passes locally on macOS and fails only on CI. -STDERR_LOG="$(mktemp "${TMPDIR:-/tmp}/docker_smoke_stderr.XXXXXX")" -PROTOCOL_ERRORS="$(mktemp "${TMPDIR:-/tmp}/docker_smoke_protocol_errors.XXXXXX")" -RAW_OUTPUT_FILE="$(mktemp "${TMPDIR:-/tmp}/docker_smoke_stdout.XXXXXX")" -# -u: name only, no file created — `docker run --cidfile` refuses to start -# if its target path already exists (mktemp's normal behavior creates an -# empty file, which would trip that check). -CID_FILE="$(mktemp -u "${TMPDIR:-/tmp}/docker_smoke_cid.XXXXXX")" -trap 'rm -f "$STDERR_LOG" "$PROTOCOL_ERRORS" "$RAW_OUTPUT_FILE" "$CID_FILE"' EXIT - -# source: 60s is not new here — it is the SAME budget this script already -# used for its `timeout 60` / `gtimeout 60` wrapper before this change; -# named once so the watchdog below (which replaces that wrapper — see the -# measurement note) does not carry a second copy of the same literal. -DOCKER_RUN_TIMEOUT_SECONDS=60 - -# Portable timeout for the CLIENT side (image pull, auth, daemon -# connection): GNU coreutils `timeout` ships on ubuntu-latest (GitHub -# Actions runner) but not on macOS by default (`gtimeout` from `brew -# install coreutils` is the local-dev equivalent). Falls back to no -# CLIENT-side wrapper when neither exists — the watchdog below still -# bounds the CONTAINER side either way (see the next comment). -TIMEOUT_CMD="" -if command -v timeout >/dev/null 2>&1; then - TIMEOUT_CMD="timeout ${DOCKER_RUN_TIMEOUT_SECONDS}" -elif command -v gtimeout >/dev/null 2>&1; then - TIMEOUT_CMD="gtimeout ${DOCKER_RUN_TIMEOUT_SECONDS}" -fi - -# `timeout`/`gtimeout` alone is NOT a sufficient bound for a hung -# CONTAINER: measured 2026-07-30 against a deliberately hanging test image -# (`ENTRYPOINT sh -c "sleep infinity"`) that `gtimeout 5 docker run --rm -i -# ` did NOT return even ~30s past its 5s deadline, and the container -# was still `docker ps`-visible afterward — `timeout`'s SIGTERM reaches the -# `docker run` CLIENT process, but that process does not reliably forward -# it to a CONTAINER blocked on unrelated work (this repo's Docker Desktop -# 29.1.4; not re-verified against every Docker version). `docker kill -# ` (via --cidfile) IS the mechanism measured to stop the -# container immediately — the watchdog below does that, uniformly on every -# platform, IN ADDITION to $TIMEOUT_CMD (which still helps bound a -# CLIENT-side hang, e.g. before any container exists to `docker kill`). -# This is a deadline (a worst-case bound on a single run), not a retry loop -# — it fires at most once, only as a last-resort kill switch, and never -# re-attempts the request. -printf '%s' "$REQUESTS" | $TIMEOUT_CMD docker run --rm -i --cidfile="$CID_FILE" "$IMAGE" >"$RAW_OUTPUT_FILE" 2>"$STDERR_LOG" & -DOCKER_RUN_PID=$! -( - sleep "$DOCKER_RUN_TIMEOUT_SECONDS" - if [[ -f "$CID_FILE" ]]; then - docker kill "$(cat "$CID_FILE")" >/dev/null 2>&1 || true - else - # No container ever started (client-side hang) — fall back to signaling - # the client process directly. - kill "$DOCKER_RUN_PID" 2>/dev/null || true - fi -) & -WATCHDOG_PID=$! -wait "$DOCKER_RUN_PID" 2>/dev/null || true -kill "$WATCHDOG_PID" 2>/dev/null || true -wait "$WATCHDOG_PID" 2>/dev/null || true -RAW_OUTPUT="$(cat "$RAW_OUTPUT_FILE")" - -if [[ -z "$RAW_OUTPUT" ]]; then - echo "docker_smoke: FAIL — empty stdout from container. stderr:" >&2 - cat "$STDERR_LOG" >&2 || true - exit 1 -fi - -TOOL_COUNT="$(printf '%s' "$RAW_OUTPUT" | python3 -c ' -import json -import sys - -# Any JSON-RPC error frame is reported, not just a missing id=3: a broken -# handshake shows up as an error on id=1/id=2, and blaming "no tools/list -# response" for it sent the last investigation to the wrong end of the -# exchange. Errors go to a side file so the caller can quote them. -count = None -errors = [] -for line in sys.stdin: - line = line.strip() - if not line: - continue - try: - msg = json.loads(line) - except json.JSONDecodeError: - continue - if "error" in msg: - errors.append( - " id={} code={} message={}".format( - msg.get("id"), - msg["error"].get("code"), - msg["error"].get("message"), - ) - ) - if msg.get("id") == 3 and "result" in msg: - count = len(msg["result"].get("tools", [])) - -with open(sys.argv[1], "w") as fh: - fh.write("\n".join(errors)) - -print("NONE" if count is None else count) -' "$PROTOCOL_ERRORS")" - -# A protocol error is a failure even when tools/list happens to answer: it -# means the container rejected a frame this script sent, and the last time -# that was tolerated it made the gate intermittent rather than red. -if [[ -s "$PROTOCOL_ERRORS" ]]; then - echo "docker_smoke: FAIL — the container returned JSON-RPC error frames:" >&2 - cat "$PROTOCOL_ERRORS" >&2 - echo "--- container stderr ---" >&2 - cat "$STDERR_LOG" >&2 || true - exit 1 -fi - -if [[ "$TOOL_COUNT" == "NONE" || -z "$TOOL_COUNT" ]]; then - echo "docker_smoke: FAIL — no valid tools/list response (id=3) found in container stdout." >&2 - echo "--- raw stdout ---" >&2 - printf '%s\n' "$RAW_OUTPUT" >&2 - echo "--- stderr ---" >&2 - cat "$STDERR_LOG" >&2 || true - exit 1 -fi - -if [[ "$TOOL_COUNT" -lt "$MIN_TOOL_COUNT" ]]; then - echo "docker_smoke: FAIL — bare-container tools/list returned ${TOOL_COUNT} tools, expected >= ${MIN_TOOL_COUNT}." >&2 - echo "This is the exact regression fixed in commit 5d71069c (fix/bare-container-contract)." >&2 - exit 1 -fi - -echo "docker_smoke: PASS — bare-container tools/list returned ${TOOL_COUNT} tools (>= ${MIN_TOOL_COUNT})." >&2 -exit 0 +# `exec` replaces this shell with the Python driver: the driver's exit code +# becomes this script's exit code, and there is no shell-level stdio piping +# left for a race to hide in. +exec python3 "${REPO_ROOT}/scripts/docker_smoke_client.py" \ + --image "$IMAGE" \ + --min-tools "$MIN_TOOL_COUNT" \ + --timeout "$DOCKER_RUN_TIMEOUT_SECONDS" diff --git a/scripts/docker_smoke_client.py b/scripts/docker_smoke_client.py new file mode 100644 index 00000000..44477ed2 --- /dev/null +++ b/scripts/docker_smoke_client.py @@ -0,0 +1,255 @@ +#!/usr/bin/env python3 +"""Python driver for `scripts/docker_smoke.sh`'s bare-container exchange. + +`docker_smoke.sh` used to pipe its whole request batch into `docker run -i` +via `printf '%s' "$REQUESTS" | docker run --rm -i ... "$IMAGE"`: `printf` +closes its end of the pipe -- the container's stdin -- the instant it has +written the last byte, before a single response has been read. That is +exactly the anti-pattern `mcp_host_client.py` exists to retire (see its +module docstring): closing stdin is the MCP shutdown signal (2025-06-18 +SS Lifecycle -> Shutdown -> stdio), not an end-of-input marker, and mcp +2.0.0's `_handle_request` drops an in-flight response write rather than +deliver it once EOF has fired the cancel scope -- regardless of which +method the request named. Verified 2026-08-10 against +`mcp/shared/jsonrpc_dispatcher.py::_dispatch_request`/`_handle_request` +(mcp 2.0.0, the version `uv.lock` pins): every accepted request, including +`tools/list`, is dispatched through this one generic path -- there is no +per-method branch that would make `tools/list` immune. That is the +mechanism behind this gate's signature failure, "no valid tools/list +response (id=3)": the container's answer was started, then cancelled by an +EOF that arrived before it could be delivered, and the SDK's own rule +("prefer possibly-zero answers over possibly-two", +`jsonrpc_dispatcher.py::_handle_request`) means it settles as no answer at +all -- silently, no error frame, exactly the "stderr empty, no JSON-RPC +error frame" signature this gate has shown intermittently since it was +added. + +This module keeps the container's stdin open until the expected response +ids have arrived (`mcp_host_client.drain_exchange`), closing it only then +-- the same fix `mcp_host_client.py` applies to a local subprocess, +applied here to a `docker run -i` child. `docker run -i` forwards this +process's stdin/stdout to the container's stdin/stdout as an ordinary pipe +pair, so the identical fix applies verbatim; the only genuinely +docker-specific piece is the watchdog, which must `docker kill` the +CONTAINER (via `--cidfile`) rather than signal the local `docker run` +client process -- measured 2026-07-30 against a deliberately hanging test +image (`ENTRYPOINT sh -c "sleep infinity"`) where a client-side SIGTERM did +not stop it (`docker ps` still showed it running well past the deadline). +This is a deadline (a worst-case bound on one run), not a retry: it fires +at most once and never re-attempts the request. +""" + +from __future__ import annotations + +import argparse +import json +from pathlib import Path +import subprocess +import sys +import tempfile +import threading + +_REPO_ROOT = str(Path(__file__).resolve().parent.parent) +if _REPO_ROOT not in sys.path: + sys.path.insert(0, _REPO_ROOT) + +from scripts.mcp_host_client import ContractError, drain_exchange # noqa: E402 + +# source: scripts/docker_smoke.sh's original REQUESTS heredoc (git history, +# commit 18d4505 and its bare-container-contract predecessor 5d71069c) -- +# the exact three-frame handshake a bare-container registry indexer +# (Glama et al.) sends: `initialize`, `notifications/initialized` (a +# notification -- carries no "id", JSON-RPC 2.0 SS4.1), then `tools/list`. +PROTOCOL_VERSION = "2024-11-05" +TOOLS_LIST_ID = 3 + + +def _messages() -> list[dict[str, object]]: + return [ + { + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": { + "protocolVersion": PROTOCOL_VERSION, + "capabilities": {}, + "clientInfo": {"name": "docker-smoke", "version": "0"}, + }, + }, + {"jsonrpc": "2.0", "method": "notifications/initialized"}, + {"jsonrpc": "2.0", "id": TOOLS_LIST_ID, "method": "tools/list", "params": {}}, + ] + + +def frame_text() -> str: + return "".join( + json.dumps(message, separators=(",", ":")) + "\n" for message in _messages() + ) + + +def expected_ids() -> frozenset[int]: + return frozenset( + message["id"] for message in _messages() if isinstance(message.get("id"), int) + ) + + +class SmokeFailureError(RuntimeError): + """The bare-container contract was not satisfied.""" + + +def _kill_container(cidfile: Path, process: subprocess.Popen[str]) -> None: + """Watchdog target: stop the CONTAINER, not just the local `docker run` + client -- see this module's docstring for why a client-side kill is not + sufficient. + """ + if cidfile.exists() and cidfile.stat().st_size > 0: + cid = cidfile.read_text().strip() + if cid: + subprocess.run( + ["docker", "kill", cid], + stdout=subprocess.DEVNULL, + stderr=subprocess.DEVNULL, + check=False, + ) + else: + # No container ever started (client-side hang) -- fall back to + # signaling the client process directly. + process.kill() + + +def run(image: str, timeout: int) -> tuple[dict[int, dict[str, object]], str]: + """Run the container, drain the exchange, and reap it. + + Returns (responses, container_stderr). Raises `ContractError` if the + container's stdout carried a line that is not valid JSON-RPC (spec + MUST, 2025-06-18 SS Transports -> stdio: "The server MUST NOT write + anything to its stdout that is not a valid MCP message"). + """ + with tempfile.TemporaryDirectory() as tmp_dir: + cidfile = Path(tmp_dir) / "cid" + # stderr goes to a file rather than a pipe: a pipe nobody drains + # while the exchange is reading stdout would block the container + # once its stderr buffer filled (same rationale as + # `mcp_host_client.run_client`). + with tempfile.TemporaryFile(mode="w+", encoding="utf-8") as stderr_file: + process = subprocess.Popen( + ["docker", "run", "--rm", "-i", f"--cidfile={cidfile}", image], + stdin=subprocess.PIPE, + stdout=subprocess.PIPE, + stderr=stderr_file, + text=True, + ) + stdin, stdout = process.stdin, process.stdout + if ( + stdin is None or stdout is None + ): # pragma: no cover - PIPE requested above + raise ContractError("docker run: pipes were not created") + watchdog = threading.Timer( + timeout, _kill_container, args=(cidfile, process) + ) + watchdog.start() + try: + responses = drain_exchange(stdin, stdout, frame_text(), expected_ids()) + process.wait() + finally: + watchdog.cancel() + stdout.close() + stderr_file.seek(0) + stderr_text = stderr_file.read() + return responses, stderr_text + + +def protocol_errors(responses: dict[int, dict[str, object]]) -> list[str]: + """Any response frame carrying a JSON-RPC error -- not just id=3 -- is a + failure even when `tools/list` happens to answer: it means the + container rejected a frame this script sent, and the last time that + was tolerated it made this gate intermittent rather than simply red + (see `docker_smoke.sh`'s git history, commit 18d4505).""" + lines = [] + for request_id, message in sorted(responses.items()): + if "error" in message: + error = message["error"] + lines.append( + f" id={request_id} code={error.get('code')} " + f"message={error.get('message')}" + ) + return lines + + +def tool_count(responses: dict[int, dict[str, object]]) -> int: + message = responses.get(TOOLS_LIST_ID) + if message is None: + raise SmokeFailureError( + f"no valid tools/list response (id={TOOLS_LIST_ID}) found in " + "container stdout." + ) + result = message.get("result") + if not isinstance(result, dict): + raise SmokeFailureError(f"tools/list returned no object result: {result!r}") + tools = result.get("tools") + if not isinstance(tools, list): + raise SmokeFailureError(f"tools/list returned tools={tools!r}, expected a list") + return len(tools) + + +def _fail(message: str, stderr_text: str | None = None) -> int: + print(f"docker_smoke: FAIL — {message}", file=sys.stderr) + if stderr_text is not None: + print("--- container stderr ---", file=sys.stderr) + print(stderr_text, file=sys.stderr) + return 1 + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument("--image", required=True, help="image tag to run") + parser.add_argument( + "--min-tools", + type=int, + required=True, + help="minimum tools/list count to accept", + ) + parser.add_argument( + "--timeout", type=int, default=60, help="watchdog deadline in seconds" + ) + args = parser.parse_args(argv) + + print( + f"docker_smoke: running {args.image} with zero env vars, sending " + "initialize + tools/list over stdio ...", + file=sys.stderr, + ) + try: + responses, stderr_text = run(args.image, args.timeout) + except ContractError as error: + return _fail(str(error)) + + if errors := protocol_errors(responses): + return _fail( + "the container returned JSON-RPC error frames:\n" + "\n".join(errors), + stderr_text, + ) + + try: + count = tool_count(responses) + except SmokeFailureError as error: + return _fail(str(error), stderr_text) + + if count < args.min_tools: + return _fail( + f"bare-container tools/list returned {count} tools, expected >= " + f"{args.min_tools}. This is the exact regression fixed in commit " + "5d71069c (fix/bare-container-contract).", + stderr_text, + ) + + print( + f"docker_smoke: PASS — bare-container tools/list returned {count} " + f"tools (>= {args.min_tools}).", + file=sys.stderr, + ) + return 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/mcp_host_client.py b/scripts/mcp_host_client.py index ae84843d..a3ed5f2a 100644 --- a/scripts/mcp_host_client.py +++ b/scripts/mcp_host_client.py @@ -36,6 +36,7 @@ from __future__ import annotations +from collections.abc import Iterable from dataclasses import dataclass import json import os @@ -173,19 +174,31 @@ def absorb(line: str, responses: dict[int, dict[str, object]]) -> None: responses[request_id] = message -def _exchange( - stdin: IO[str], stdout: IO[str], client_name: str +def drain_exchange( + stdin: IO[str], + stdout: IO[str], + frame_text: str, + expected_ids: Iterable[int], ) -> dict[int, dict[str, object]]: - """Send the batch, read until every expected id is answered, then and - only then close stdin -- the protocol's shutdown signal. + """Write ``frame_text``, read until every id in ``expected_ids`` is + answered, then and only then close stdin -- the protocol's shutdown + signal. + + This is the generic primitive `_exchange` below specializes to the + fixed six-message contract batch: any caller driving an MCP stdio + server with its own batch (see `scripts/docker_smoke_client.py`, whose + batch is the three-message `initialize` / `notifications/initialized` + / `tools/list` docker_smoke.sh sends) gets the same ordering guarantee + -- read every expected response first, close stdin second -- without + re-deriving it. Terminates on either event: the last awaited id arriving, or stdout - reaching EOF (the server exited or the watchdog killed it). Neither is + reaching EOF (the server exited or a watchdog killed it). Neither is a clock. """ - outstanding = set(expected_request_ids()) + outstanding = set(expected_ids) responses: dict[int, dict[str, object]] = {} - stdin.write(frames(client_name)) + stdin.write(frame_text) stdin.flush() while outstanding: line = stdout.readline() @@ -199,6 +212,14 @@ def _exchange( return responses +def _exchange( + stdin: IO[str], stdout: IO[str], client_name: str +) -> dict[int, dict[str, object]]: + """Send the fixed six-message contract batch and drain it (see + `drain_exchange`).""" + return drain_exchange(stdin, stdout, frames(client_name), expected_request_ids()) + + def run_client(case: ContractCase) -> dict[int, dict[str, object]]: """Spawn the server, exchange one batch as a conformant host, reap it. diff --git a/tests_py/infrastructure/test_stdio_eof_drain.py b/tests_py/infrastructure/test_stdio_eof_drain.py index 76d27f5b..f68f569b 100644 --- a/tests_py/infrastructure/test_stdio_eof_drain.py +++ b/tests_py/infrastructure/test_stdio_eof_drain.py @@ -111,14 +111,20 @@ def _request_ids() -> set[int]: async def _collect( - reader: object, answered: dict[int, object], done: anyio.Event + reader: object, + answered: dict[int, object], + done: anyio.Event, + expected_ids: set[int] | None = None, ) -> None: + """Collect response frames until ``expected_ids`` (default: this + module's own ``_request_ids()``) is fully answered.""" + awaited = _request_ids() if expected_ids is None else expected_ids async with reader: # type: ignore[attr-defined] async for item in reader: # type: ignore[attr-defined] message = item.message if isinstance(message, (types.JSONRPCResponse, types.JSONRPCError)): answered[message.id] = message - if _request_ids() <= answered.keys(): + if awaited <= answered.keys(): done.set() @@ -127,6 +133,44 @@ async def _send_batch(writer: object) -> None: await writer.send(SessionMessage(frame)) # type: ignore[attr-defined] +# source: scripts/docker_smoke_client.py's own request batch — ties this +# reproduction to docker_smoke.sh's LITERAL historical failure message +# ("no valid tools/list response (id=3)"), not just the same defect class +# under a different method/id. +_DOCKER_SMOKE_GATED_ID = 3 + + +def _docker_smoke_batch() -> list[types.JSONRPCRequest | types.JSONRPCNotification]: + return [ + types.JSONRPCRequest( + jsonrpc="2.0", + id=1, + method="initialize", + params={ + "protocolVersion": "2024-11-05", + "capabilities": {}, + "clientInfo": {"name": "docker-smoke", "version": "0"}, + }, + ), + types.JSONRPCNotification(jsonrpc="2.0", method="notifications/initialized"), + types.JSONRPCRequest( + jsonrpc="2.0", + id=_DOCKER_SMOKE_GATED_ID, + method="tools/list", + params={}, + ), + ] + + +def _docker_smoke_request_ids() -> set[int]: + return {f.id for f in _docker_smoke_batch() if isinstance(f, types.JSONRPCRequest)} + + +async def _send_docker_smoke_batch(writer: object) -> None: + for frame in _docker_smoke_batch(): + await writer.send(SessionMessage(frame)) # type: ignore[attr-defined] + + class TestEofBeforeDrainLosesAnAcceptedRequest: """The old harness shape: signal shutdown, then hope for answers.""" @@ -193,3 +237,82 @@ async def drive() -> None: assert all( isinstance(answered[i], types.JSONRPCResponse) for i in _request_ids() ), "draining first must yield real results, not shutdown errors" + + +class TestDockerSmokeToolsListLostBeforeDrain: + """Pins docker_smoke.sh's LITERAL historical failure ('no valid + tools/list response (id=3)', empty stderr, no JSON-RPC error frame) to + this mechanism, using the script's own request id and method — not just + the same defect class under `tools/call`/id=2 above. Before + `scripts/docker_smoke_client.py` existed, `docker_smoke.sh` drove the + container with exactly the shape the first test below reproduces: + `printf | docker run` writes the whole batch and closes the container's + stdin the instant the write finishes, without ever reading a response + first. + """ + + @pytest.mark.asyncio + async def test_shutdown_before_drain_drops_tools_list(self) -> None: + parked = anyio.Event() + answered: dict[int, object] = {} + done = anyio.Event() + server = _server() + read_writer, read_stream = anyio.create_memory_object_stream(0) + write_stream, write_reader = anyio.create_memory_object_stream(0) + gated = _GatedWriteStream(write_stream, _DOCKER_SMOKE_GATED_ID, parked) + + async def drive() -> None: + await _send_docker_smoke_batch(read_writer) + await parked.wait() # the tools/list write has started, unlanded + await read_writer.aclose() # EOF == shutdown signal + + async with anyio.create_task_group() as tg: + tg.start_soon( + _collect, write_reader, answered, done, _docker_smoke_request_ids() + ) + tg.start_soon(drive) + await server._lowlevel_server.run( + read_stream, + gated, + server._lowlevel_server.create_initialization_options(), + ) + + assert 1 in answered, "initialize is handled inline; never subject to this" + assert _DOCKER_SMOKE_GATED_ID not in answered, ( + "docker_smoke.sh's exact failure, reproduced deterministically: " + "the container's tools/list (id=3) answer was started and then " + "cancelled by an EOF that arrived before it could be delivered — " + "'no valid tools/list response (id=3)', empty stderr, no error " + "frame. This is what the old `printf | docker run` shape could " + "hit; scripts/docker_smoke_client.py cannot, by construction " + "(see the class below)." + ) + + @pytest.mark.asyncio + async def test_drain_then_shutdown_answers_tools_list(self) -> None: + answered: dict[int, object] = {} + done = anyio.Event() + server = _server() + read_writer, read_stream = anyio.create_memory_object_stream(0) + write_stream, write_reader = anyio.create_memory_object_stream(0) + + async def drive() -> None: + await _send_docker_smoke_batch(read_writer) + await done.wait() # tools/list answered — THEN shut down + await read_writer.aclose() + + async with anyio.create_task_group() as tg: + tg.start_soon( + _collect, write_reader, answered, done, _docker_smoke_request_ids() + ) + tg.start_soon(drive) + await server._lowlevel_server.run( + read_stream, + write_stream, + server._lowlevel_server.create_initialization_options(), + ) + + assert _docker_smoke_request_ids() <= answered.keys() + assert isinstance(answered[_DOCKER_SMOKE_GATED_ID], types.JSONRPCResponse), ( + "draining first must yield a real tools/list result, not a shutdown error" + ) diff --git a/tests_py/scripts/test_docker_smoke_client.py b/tests_py/scripts/test_docker_smoke_client.py new file mode 100644 index 00000000..eba929f5 --- /dev/null +++ b/tests_py/scripts/test_docker_smoke_client.py @@ -0,0 +1,172 @@ +"""`scripts/docker_smoke_client.py` must drain the container's exchange +before it signals shutdown, for the exact three-frame batch +`docker_smoke.sh` sends (`initialize` id=1, `notifications/initialized`, +`tools/list` id=3). + +This is the same ordering contract `tests_py/scripts/test_mcp_host_client.py` +pins for the six-message contract batch, applied to this module's own +batch/ids — no subprocess and no docker, no clock: the exchange reads every +expected response first, and stdin is provably still open at each of those +reads. The mechanism this guards against — closing stdin (the container's +shutdown signal, 2025-06-18 SS Lifecycle -> Shutdown -> stdio) before a +response has been read causes mcp 2.0.0 to drop it silently, regardless of +which method it answers — is forced deterministically against the real SDK +in `tests_py/infrastructure/test_stdio_eof_drain.py`; here we pin the client +side for this script's own request shape. +""" + +from __future__ import annotations + +import pytest + +from scripts import docker_smoke_client as smoke_client +from scripts import mcp_host_client as host_client + + +class _RecordingStdin: + """Captures what the exchange writes, and when it closes.""" + + def __init__(self) -> None: + self.written: list[str] = [] + self.flushed = 0 + self.closed = False + + def write(self, text: str) -> None: + self.written.append(text) + + def flush(self) -> None: + self.flushed += 1 + + def close(self) -> None: + self.closed = True + + +class _ScriptedStdout: + """Replays scripted server output, recording stdin's state at each read.""" + + def __init__(self, lines: list[str], stdin: _RecordingStdin) -> None: + self._lines = list(lines) + self._stdin = stdin + self.stdin_closed_at_read: list[bool] = [] + + def readline(self) -> str: + self.stdin_closed_at_read.append(self._stdin.closed) + if not self._lines: + return "" + return self._lines.pop(0) + + def read(self) -> str: + return "".join(self._lines) + + +def _response_line(request_id: int) -> str: + return '{"jsonrpc":"2.0","id":%d,"result":{"tools":[]}}\n' % request_id + + +def _scripted_drain(lines: list[str]) -> tuple[dict, _RecordingStdin, _ScriptedStdout]: + stdin = _RecordingStdin() + stdout = _ScriptedStdout(lines, stdin) + responses = host_client.drain_exchange( + stdin, stdout, smoke_client.frame_text(), smoke_client.expected_ids() + ) + return responses, stdin, stdout + + +class TestDockerSmokeBatchDrainsBeforeShutdown: + def test_expected_ids_are_exactly_init_and_tools_list(self) -> None: + assert smoke_client.expected_ids() == frozenset({1, smoke_client.TOOLS_LIST_ID}) + + def test_both_responses_are_collected(self) -> None: + responses, _, _ = _scripted_drain( + [_response_line(1), _response_line(smoke_client.TOOLS_LIST_ID)] + ) + assert set(responses) == {1, smoke_client.TOOLS_LIST_ID} + + def test_stdin_stays_open_until_the_tools_list_response_is_read(self) -> None: + _, stdin, stdout = _scripted_drain( + [_response_line(1), _response_line(smoke_client.TOOLS_LIST_ID)] + ) + assert stdout.stdin_closed_at_read == [False, False], ( + "the exchange closed the container's stdin — its shutdown signal — " + "before reading the tools/list response; that is the exact defect " + "this module exists to prevent (the bash `printf | docker run` " + "anti-pattern it replaced)" + ) + assert stdin.closed, "stdin must be closed once the batch is answered" + + def test_the_batch_written_matches_docker_smoke_shs_original_requests(self) -> None: + _, stdin, _ = _scripted_drain( + [_response_line(1), _response_line(smoke_client.TOOLS_LIST_ID)] + ) + written = "".join(stdin.written) + assert '"method":"initialize"' in written + assert '"method":"notifications/initialized"' in written + assert '"method":"tools/list"' in written + assert '"id":1' in written + assert f'"id":{smoke_client.TOOLS_LIST_ID}' in written + + def test_a_response_arriving_after_eof_that_precedes_it_is_lost(self) -> None: + """The scenario the old bash script was blind to: if the container's + `tools/list` write had not yet reached the pipe when EOF fired + (`tests_py/infrastructure/test_stdio_eof_drain.py` forces this + against the real SDK), a reader that only looks at what arrived + before EOF never sees it. `drain_exchange` cannot exhibit this — it + keeps stdin open exactly until id=3 is read — but a reader that + stops at the first EOF, matching the old script's + `wait "$DOCKER_RUN_PID"; RAW_OUTPUT="$(cat "$RAW_OUTPUT_FILE")"` + shape, would report only what happened to be flushed already.""" + # id=3's line never appears — this is what "EOF strictly before the + # response could be delivered" looks like from the reading side. + responses, stdin, stdout = _scripted_drain([_response_line(1)]) + assert set(responses) == {1} + assert smoke_client.TOOLS_LIST_ID not in responses + assert stdin.closed + + +class TestToolCount: + def test_missing_tools_list_response_is_a_smoke_failure(self) -> None: + with pytest.raises( + smoke_client.SmokeFailureError, match="no valid tools/list response" + ): + smoke_client.tool_count({1: {"result": {}}}) + + def test_non_object_result_is_a_smoke_failure(self) -> None: + with pytest.raises(smoke_client.SmokeFailureError, match="no object result"): + smoke_client.tool_count({smoke_client.TOOLS_LIST_ID: {"result": None}}) + + def test_non_list_tools_field_is_a_smoke_failure(self) -> None: + with pytest.raises(smoke_client.SmokeFailureError, match="expected a list"): + smoke_client.tool_count( + {smoke_client.TOOLS_LIST_ID: {"result": {"tools": "nope"}}} + ) + + def test_the_advertised_tool_count_is_returned(self) -> None: + tools = [{"name": f"t{i}"} for i in range(52)] + count = smoke_client.tool_count( + {smoke_client.TOOLS_LIST_ID: {"result": {"tools": tools}}} + ) + assert count == 52 + + +class TestProtocolErrors: + def test_no_errors_on_clean_responses(self) -> None: + responses = { + 1: {"result": {}}, + smoke_client.TOOLS_LIST_ID: {"result": {"tools": []}}, + } + assert smoke_client.protocol_errors(responses) == [] + + def test_an_error_frame_on_any_id_is_reported(self) -> None: + responses = { + 1: {"result": {}}, + 2: { + "error": { + "code": -32602, + "message": "28 validation errors for ClientRequest", + } + }, + } + errors = smoke_client.protocol_errors(responses) + assert len(errors) == 1 + assert "code=-32602" in errors[0] + assert "28 validation errors" in errors[0] From 5b0c57ee6359566c9927b8ec0dc7a180d82020ee Mon Sep 17 00:00:00 2001 From: cdeust Date: Mon, 10 Aug 2026 17:22:38 +0200 Subject: [PATCH 2/2] fix(ci): satisfy the base-ref craftsmanship gate for the docker_smoke fix MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit git merge-base HEAD origin/main was 4 commits behind origin/main — rebased first (per PR #423-425's shared cause), then re-ran the CI's exact invocation, `check_craftsmanship.py --base origin/main`, which diffs against the base ref's baseline rather than the working tree's and caught three real violations `check_craftsmanship.py` alone (no --base) does not: - scripts/docker_smoke_client.py::main exceeded the 40-line method cap (CLAUDE.md's local tightening of coding-standards.md §4.2) — split into _build_parser()/_evaluate()/main(), each under the limit. - scripts/docker_smoke_client.py::TOOLS_LIST_ID (value 3) had no `# source:` comment — added one, same source as PROTOCOL_VERSION above it (docker_smoke.sh's original REQUESTS heredoc, where tools/list was request id=3). - tests_py/infrastructure/test_stdio_eof_drain.py grew to 318 lines, over the 300-line file cap, after the prior commit appended the docker-smoke-specific pinning test to it. Split that test class into a new sibling file, test_docker_smoke_stdio_eof_drain.py, which imports (not duplicates) _server/_GatedWriteStream/_collect from the original — both files now under the cap, no test content lost. No production logic changed: pytest (1448 passed / 35 skipped), ruff check/format, and shellcheck all still clean; the deterministic reproduction (real mcp SDK, docker_smoke's own id=3/tools/list request, write-then-close loses it / drain-then-close does not) is unchanged, just relocated. Co-Authored-By: Claude --- scripts/docker_smoke_client.py | 44 ++++-- .../test_docker_smoke_stdio_eof_drain.py | 133 ++++++++++++++++++ .../infrastructure/test_stdio_eof_drain.py | 123 +--------------- 3 files changed, 169 insertions(+), 131 deletions(-) create mode 100644 tests_py/infrastructure/test_docker_smoke_stdio_eof_drain.py diff --git a/scripts/docker_smoke_client.py b/scripts/docker_smoke_client.py index 44477ed2..4b844c19 100644 --- a/scripts/docker_smoke_client.py +++ b/scripts/docker_smoke_client.py @@ -61,6 +61,9 @@ # (Glama et al.) sends: `initialize`, `notifications/initialized` (a # notification -- carries no "id", JSON-RPC 2.0 SS4.1), then `tools/list`. PROTOCOL_VERSION = "2024-11-05" +# source: same docker_smoke.sh REQUESTS heredoc as PROTOCOL_VERSION above -- +# `tools/list` was request id=3 in that batch (id=1 is `initialize`; +# `notifications/initialized` is a notification and carries no id at all). TOOLS_LIST_ID = 3 @@ -200,7 +203,7 @@ def _fail(message: str, stderr_text: str | None = None) -> int: return 1 -def main(argv: list[str] | None = None) -> int: +def _build_parser() -> argparse.ArgumentParser: parser = argparse.ArgumentParser(description=__doc__) parser.add_argument("--image", required=True, help="image tag to run") parser.add_argument( @@ -212,18 +215,15 @@ def main(argv: list[str] | None = None) -> int: parser.add_argument( "--timeout", type=int, default=60, help="watchdog deadline in seconds" ) - args = parser.parse_args(argv) + return parser - print( - f"docker_smoke: running {args.image} with zero env vars, sending " - "initialize + tools/list over stdio ...", - file=sys.stderr, - ) - try: - responses, stderr_text = run(args.image, args.timeout) - except ContractError as error: - return _fail(str(error)) +def _evaluate( + responses: dict[int, dict[str, object]], stderr_text: str, min_tools: int +) -> int: + """Apply docker_smoke.sh's original pass/fail conditions, in order: + protocol errors first, then the tools/list contract, then the count + floor.""" if errors := protocol_errors(responses): return _fail( "the container returned JSON-RPC error frames:\n" + "\n".join(errors), @@ -235,21 +235,37 @@ def main(argv: list[str] | None = None) -> int: except SmokeFailureError as error: return _fail(str(error), stderr_text) - if count < args.min_tools: + if count < min_tools: return _fail( f"bare-container tools/list returned {count} tools, expected >= " - f"{args.min_tools}. This is the exact regression fixed in commit " + f"{min_tools}. This is the exact regression fixed in commit " "5d71069c (fix/bare-container-contract).", stderr_text, ) print( f"docker_smoke: PASS — bare-container tools/list returned {count} " - f"tools (>= {args.min_tools}).", + f"tools (>= {min_tools}).", file=sys.stderr, ) return 0 +def main(argv: list[str] | None = None) -> int: + args = _build_parser().parse_args(argv) + + print( + f"docker_smoke: running {args.image} with zero env vars, sending " + "initialize + tools/list over stdio ...", + file=sys.stderr, + ) + try: + responses, stderr_text = run(args.image, args.timeout) + except ContractError as error: + return _fail(str(error)) + + return _evaluate(responses, stderr_text, args.min_tools) + + if __name__ == "__main__": raise SystemExit(main()) diff --git a/tests_py/infrastructure/test_docker_smoke_stdio_eof_drain.py b/tests_py/infrastructure/test_docker_smoke_stdio_eof_drain.py new file mode 100644 index 00000000..cfde95d8 --- /dev/null +++ b/tests_py/infrastructure/test_docker_smoke_stdio_eof_drain.py @@ -0,0 +1,133 @@ +"""Pins docker_smoke.sh's LITERAL historical failure ('no valid tools/list +response (id=3)', empty stderr, no JSON-RPC error frame) to the EOF-before- +drain mechanism `test_stdio_eof_drain.py` forces deterministically, using +`scripts/docker_smoke_client.py`'s own request id and method — not just the +same defect class under a different id/method. + +Reuses that module's `_server`/`_GatedWriteStream`/`_collect` rather than +duplicating them (split into this sibling file only because appending here +would have pushed `test_stdio_eof_drain.py` over its own 300-line cap). + +Before `scripts/docker_smoke_client.py` existed, `docker_smoke.sh` drove the +container with exactly the shape `test_shutdown_before_drain_drops_tools_list` +below reproduces: `printf | docker run` writes the whole batch and closes the +container's stdin the instant the write finishes, without ever reading a +response first. +""" + +from __future__ import annotations + +import anyio +import mcp.types as types +import pytest +from mcp.shared.message import SessionMessage + +from tests_py.infrastructure.test_stdio_eof_drain import ( + _collect, + _GatedWriteStream, + _server, +) + +# source: scripts/docker_smoke_client.py's own request batch (TOOLS_LIST_ID) +# — ties this reproduction to docker_smoke.sh's literal historical failure +# message, not just the same defect class under a different method/id. +_DOCKER_SMOKE_GATED_ID = 3 + + +def _docker_smoke_batch() -> list[types.JSONRPCRequest | types.JSONRPCNotification]: + return [ + types.JSONRPCRequest( + jsonrpc="2.0", + id=1, + method="initialize", + params={ + "protocolVersion": "2024-11-05", + "capabilities": {}, + "clientInfo": {"name": "docker-smoke", "version": "0"}, + }, + ), + types.JSONRPCNotification(jsonrpc="2.0", method="notifications/initialized"), + types.JSONRPCRequest( + jsonrpc="2.0", + id=_DOCKER_SMOKE_GATED_ID, + method="tools/list", + params={}, + ), + ] + + +def _docker_smoke_request_ids() -> set[int]: + return {f.id for f in _docker_smoke_batch() if isinstance(f, types.JSONRPCRequest)} + + +async def _send_docker_smoke_batch(writer: object) -> None: + for frame in _docker_smoke_batch(): + await writer.send(SessionMessage(frame)) # type: ignore[attr-defined] + + +class TestDockerSmokeToolsListLostBeforeDrain: + @pytest.mark.asyncio + async def test_shutdown_before_drain_drops_tools_list(self) -> None: + parked = anyio.Event() + answered: dict[int, object] = {} + done = anyio.Event() + server = _server() + read_writer, read_stream = anyio.create_memory_object_stream(0) + write_stream, write_reader = anyio.create_memory_object_stream(0) + gated = _GatedWriteStream(write_stream, _DOCKER_SMOKE_GATED_ID, parked) + + async def drive() -> None: + await _send_docker_smoke_batch(read_writer) + await parked.wait() # the tools/list write has started, unlanded + await read_writer.aclose() # EOF == shutdown signal + + async with anyio.create_task_group() as tg: + tg.start_soon( + _collect, write_reader, answered, done, _docker_smoke_request_ids() + ) + tg.start_soon(drive) + await server._lowlevel_server.run( + read_stream, + gated, + server._lowlevel_server.create_initialization_options(), + ) + + assert 1 in answered, "initialize is handled inline; never subject to this" + assert _DOCKER_SMOKE_GATED_ID not in answered, ( + "docker_smoke.sh's exact failure, reproduced deterministically: " + "the container's tools/list (id=3) answer was started and then " + "cancelled by an EOF that arrived before it could be delivered — " + "'no valid tools/list response (id=3)', empty stderr, no error " + "frame. This is what the old `printf | docker run` shape could " + "hit; scripts/docker_smoke_client.py cannot, by construction " + "(see the test below)." + ) + + @pytest.mark.asyncio + async def test_drain_then_shutdown_answers_tools_list(self) -> None: + answered: dict[int, object] = {} + done = anyio.Event() + server = _server() + read_writer, read_stream = anyio.create_memory_object_stream(0) + write_stream, write_reader = anyio.create_memory_object_stream(0) + + async def drive() -> None: + await _send_docker_smoke_batch(read_writer) + await done.wait() # tools/list answered — THEN shut down + await read_writer.aclose() + + async with anyio.create_task_group() as tg: + tg.start_soon( + _collect, write_reader, answered, done, _docker_smoke_request_ids() + ) + tg.start_soon(drive) + await server._lowlevel_server.run( + read_stream, + write_stream, + server._lowlevel_server.create_initialization_options(), + ) + + assert _docker_smoke_request_ids() <= answered.keys() + assert isinstance(answered[_DOCKER_SMOKE_GATED_ID], types.JSONRPCResponse), ( + "draining first must yield a real tools/list result, not a shutdown error" + ) diff --git a/tests_py/infrastructure/test_stdio_eof_drain.py b/tests_py/infrastructure/test_stdio_eof_drain.py index f68f569b..629cbef7 100644 --- a/tests_py/infrastructure/test_stdio_eof_drain.py +++ b/tests_py/infrastructure/test_stdio_eof_drain.py @@ -25,6 +25,12 @@ not violating it. The correction therefore belongs to the client: read your answers first, close stdin second. That is what `scripts/mcp_host_client.py` does and what `test_drain_then_shutdown_answers_every_request` pins. + +`_server`/`_GatedWriteStream`/`_collect` are reused (not duplicated) by +`test_docker_smoke_stdio_eof_drain.py`, which pins the identical mechanism +for `scripts/docker_smoke_client.py`'s own request id/method — kept in a +sibling module rather than appended here to stay under this file's own +300-line cap. """ from __future__ import annotations @@ -133,44 +139,6 @@ async def _send_batch(writer: object) -> None: await writer.send(SessionMessage(frame)) # type: ignore[attr-defined] -# source: scripts/docker_smoke_client.py's own request batch — ties this -# reproduction to docker_smoke.sh's LITERAL historical failure message -# ("no valid tools/list response (id=3)"), not just the same defect class -# under a different method/id. -_DOCKER_SMOKE_GATED_ID = 3 - - -def _docker_smoke_batch() -> list[types.JSONRPCRequest | types.JSONRPCNotification]: - return [ - types.JSONRPCRequest( - jsonrpc="2.0", - id=1, - method="initialize", - params={ - "protocolVersion": "2024-11-05", - "capabilities": {}, - "clientInfo": {"name": "docker-smoke", "version": "0"}, - }, - ), - types.JSONRPCNotification(jsonrpc="2.0", method="notifications/initialized"), - types.JSONRPCRequest( - jsonrpc="2.0", - id=_DOCKER_SMOKE_GATED_ID, - method="tools/list", - params={}, - ), - ] - - -def _docker_smoke_request_ids() -> set[int]: - return {f.id for f in _docker_smoke_batch() if isinstance(f, types.JSONRPCRequest)} - - -async def _send_docker_smoke_batch(writer: object) -> None: - for frame in _docker_smoke_batch(): - await writer.send(SessionMessage(frame)) # type: ignore[attr-defined] - - class TestEofBeforeDrainLosesAnAcceptedRequest: """The old harness shape: signal shutdown, then hope for answers.""" @@ -237,82 +205,3 @@ async def drive() -> None: assert all( isinstance(answered[i], types.JSONRPCResponse) for i in _request_ids() ), "draining first must yield real results, not shutdown errors" - - -class TestDockerSmokeToolsListLostBeforeDrain: - """Pins docker_smoke.sh's LITERAL historical failure ('no valid - tools/list response (id=3)', empty stderr, no JSON-RPC error frame) to - this mechanism, using the script's own request id and method — not just - the same defect class under `tools/call`/id=2 above. Before - `scripts/docker_smoke_client.py` existed, `docker_smoke.sh` drove the - container with exactly the shape the first test below reproduces: - `printf | docker run` writes the whole batch and closes the container's - stdin the instant the write finishes, without ever reading a response - first. - """ - - @pytest.mark.asyncio - async def test_shutdown_before_drain_drops_tools_list(self) -> None: - parked = anyio.Event() - answered: dict[int, object] = {} - done = anyio.Event() - server = _server() - read_writer, read_stream = anyio.create_memory_object_stream(0) - write_stream, write_reader = anyio.create_memory_object_stream(0) - gated = _GatedWriteStream(write_stream, _DOCKER_SMOKE_GATED_ID, parked) - - async def drive() -> None: - await _send_docker_smoke_batch(read_writer) - await parked.wait() # the tools/list write has started, unlanded - await read_writer.aclose() # EOF == shutdown signal - - async with anyio.create_task_group() as tg: - tg.start_soon( - _collect, write_reader, answered, done, _docker_smoke_request_ids() - ) - tg.start_soon(drive) - await server._lowlevel_server.run( - read_stream, - gated, - server._lowlevel_server.create_initialization_options(), - ) - - assert 1 in answered, "initialize is handled inline; never subject to this" - assert _DOCKER_SMOKE_GATED_ID not in answered, ( - "docker_smoke.sh's exact failure, reproduced deterministically: " - "the container's tools/list (id=3) answer was started and then " - "cancelled by an EOF that arrived before it could be delivered — " - "'no valid tools/list response (id=3)', empty stderr, no error " - "frame. This is what the old `printf | docker run` shape could " - "hit; scripts/docker_smoke_client.py cannot, by construction " - "(see the class below)." - ) - - @pytest.mark.asyncio - async def test_drain_then_shutdown_answers_tools_list(self) -> None: - answered: dict[int, object] = {} - done = anyio.Event() - server = _server() - read_writer, read_stream = anyio.create_memory_object_stream(0) - write_stream, write_reader = anyio.create_memory_object_stream(0) - - async def drive() -> None: - await _send_docker_smoke_batch(read_writer) - await done.wait() # tools/list answered — THEN shut down - await read_writer.aclose() - - async with anyio.create_task_group() as tg: - tg.start_soon( - _collect, write_reader, answered, done, _docker_smoke_request_ids() - ) - tg.start_soon(drive) - await server._lowlevel_server.run( - read_stream, - write_stream, - server._lowlevel_server.create_initialization_options(), - ) - - assert _docker_smoke_request_ids() <= answered.keys() - assert isinstance(answered[_DOCKER_SMOKE_GATED_ID], types.JSONRPCResponse), ( - "draining first must yield a real tools/list result, not a shutdown error" - )