De-flake conformance CI: solo re-verification, spawn-storm reduction, result artifacts - #3043
De-flake conformance CI: solo re-verification, spawn-storm reduction, result artifacts#3043maxisbey merged 4 commits into
Conversation
Sorry, something went wrong.
… result artifacts The client conformance suite launches every scenario's client subprocess concurrently; on a 2-vCPU runner the resulting contention intermittently pushes sse-retry's reconnect-timing measurement past its tolerance and fails the job (~4-5% of runs, including pushes to main). - .github/actions/conformance/run-client.sh: wraps the client suite legs. On failure, scenarios listed as unexpected failures are re-run alone on the then-quiet runner: a real failure fails again; a contention artifact passes and the job goes green with a FLAKE_RESCUED marker saved into the results directory. Stale-baseline errors and infra failures are never retried, and an unparseable failure list falls back to the original exit code, so the wrapper cannot green-wash anything but solo-verified passes. - conformance.yml: uv sync gains --compile-bytecode and the editable sources are pre-compiled, so ~40 concurrent interpreters stop racing to byte-compile the same modules during the measurement window; the client command execs the synced venv's interpreter directly instead of paying uv's lockfile re-check in every spawn; both jobs save --output-dir results and upload them when the job fails or a flake was rescued. - everything-server: test_sampling passes related_request_id so the sampling request rides the originating tools/call SSE stream instead of racing the client's standalone GET stream (a dropped request there hangs tools-call-sampling to the 60s client timeout).
…tempt A solo failure on the quiet runner already disproves the contention hypothesis, so a second attempt was the blind retry the wrapper exists to avoid; CONFORMANCE_SOLO_ATTEMPTS remains the override.
There was a problem hiding this comment.
Additional findings (outside current diff — PR may have been updated during review):
-
🟡 log.txt:1-12 — This PR adds a stray log.txt at the repository root containing a fake conformance-suite summary (ANSI codes, "Unexpected failures (not in baseline):", "✗ sse-retry") — it looks like a local fixture used while developing run-client.sh's log parsing that was committed by accident. Nothing references it and the PR description doesn't mention it; please drop it from the PR (or move it under a fixtures location if it's meant to be used by a test).
Extended reasoning...What the file is. The diff adds a new 12-line log.txt at the repository root. Its contents are a synthetic conformance-suite summary: a "Total: 1 passed, 2 failed, 0 warnings" line, raw ANSI escape sequences (\x1b[33m, \x1b[31m), an "Expected failures (in baseline):" block, a "Stale baseline entries (now passing - remove from baseline):" block, and an "Unexpected failures (not in baseline):" block listing ✗ sse-retry. This is exactly the summary format that the new .github/actions/conformance/run-client.sh parses with its sed/grep pipeline (the sed -n '/^Unexpected failures (not in baseline):$/,/^$/p' extraction, the ^Stale baseline entries guard, and the ANSI-stripping sed 's/\x1b\[[0-9;]*m//g'), so it is almost certainly a hand-crafted local fixture used while developing and testing the wrapper's parsing logic.\n\nNothing uses it. Grepping the repository for log.txt finds only unrelated matches (pip-log.txt in .gitignore and errlog.txt fixtures under tests/transports/stdio/). Neither run-client.sh, conformance.yml, nor any test references a repo-root log.txt — the wrapper writes its own log to a mktemp file at runtime, so this file plays no role in CI or in any test.\n\nIt was not intended to be part of the PR. The PR description enumerates the changes as "CI-only plus one example-server fix" and lists exactly three changes (the run-client.sh wrapper, the workflow spawn-storm/artifact changes, and the everything-server related_request_id fix); log.txt is not mentioned anywhere. It is also not covered by .gitignore (the existing pip-log.txt entry doesn't match, and there is no *.txt/log.txt pattern), which is how it slipped into the commit.\n\nImpact. Nothing breaks functionally — the file is inert. The cost is source-tree pollution: a confusingly named artifact at the repo root that future readers will wonder about, and that ships in the sdist/source checkout for no reason.\n\nConcrete walk-through. (1) The author develops run-client.sh locally and needs a sample suite log to exercise the ANSI-strip + section-extraction parsing, so they save one as log.txt in the repo root. (2) They run something like sed 's/\x1b\[[0-9;]*m//g' log.txt | sed -n '/^Unexpected failures/...' to confirm the extraction yields sse-retry and the stale-baseline guard fires. (3) A later git add of the working tree picks up log.txt because no ignore rule matches it. (4) The file lands in the PR unreferenced. (5) After merge, every checkout carries a stray log.txt at the root that no code reads.\n\nFix. Remove log.txt from the PR (git rm log.txt). If the author wants to keep a parsing fixture, move it under .github/actions/conformance/ (or a tests fixtures directory) with a descriptive name and actually reference it from a test of the wrapper's parsing — but as-is it should simply be deleted.
Sorry, something went wrong.
grep -q quitting at the first match SIGPIPEs the printf feeding it; with pipefail the pipeline reports 141 and the guard is skipped exactly when the pattern is present. A here-string has no writer to kill.
|
Re the stray log.txt finding: removed in b4558ce (it was a review-time parsing fixture swept in by a bulk git add), and conformance-results/ is now gitignored so local repro runs cannot repeat the pattern. |
Sorry, something went wrong.
There was a problem hiding this comment.
The stale-baseline guard issue I flagged earlier is fixed in a2072c3 and I found no further bugs — but since the flake-rescue policy (excusing failures that only reproduce under parallel load) and the ~100-line bash wrapper are a deliberate CI design choice, this deserves a maintainer's sign-off rather than an automated approval.
Extended reasoning...Overview
This PR is CI-only plus one example-server fix: a new run-client.sh wrapper (~104 lines of bash) that re-runs unexpected conformance failures solo before failing the job, workflow changes to reduce interpreter spawn contention (--compile-bytecode, direct .venv/bin/python invocation) and upload result artifacts, a .gitignore entry, and adding related_request_id to the everything-server's test_sampling tool so the sampling request routes to the tools/call POST stream instead of a possibly-not-yet-open GET stream.
Security risks
Low. No changes to src/ or any shipped SDK code. The workflow keeps the pinned, sha256-verified harness tarball flow unchanged, and the artifact upload only publishes conformance result files. The bash wrapper validates extracted scenario names against a strict character allowlist before reusing them in commands, which mitigates injection from harness output.
Level of scrutiny
Moderate. Nothing here affects users of the SDK, but the wrapper encodes a policy decision — failures that only reproduce under 40-way parallel load are excused after passing solo — which trades a small amount of failure-detection fidelity for CI stability. That trade-off, and whether the guard rails (stale-baseline check, name validation, pass-through on unparseable output, FLAKE_RESCUED marker + artifact upload) are sufficient to prevent green-washing real regressions, is a judgment call maintainers should own. The bash logic is also intricate enough (arg re-parsing, PIPESTATUS handling, pipefail interactions — one of which I flagged and the author fixed) that a human read is warranted.
Other factors
My earlier inline comment about the SIGPIPE/pipefail fail-open in the stale-baseline guard was addressed in a2072c3 with a here-string and re-verified by the author; the current diff reflects the fix. The bug hunting system found no issues on the current revision. The PR description documents extensive local end-to-end testing of the wrapper's pass-through, rescue, still-broken, stale-baseline, and infra-failure paths, and the everything-server fix was verified in both directions.
Sorry, something went wrong.
De-flakes the conformance CI entirely on the python-sdk side: unexpected suite failures are re-verified solo before failing the job, the interpreter spawn storm that causes the timing flakes is reduced at the source, results are uploaded as artifacts whenever they matter, and the one server-side race in the everything-server is fixed.
Motivation and Context
sse-retry fails the client conformance job on ~4–5% of runs (including pushes to main, e.g. https://github.com/modelcontextprotocol/python-sdk/actions/runs/28443055485), always the same way: the harness runs all ~40 scenarios' client subprocesses concurrently on a 2-vCPU runner, and the resulting contention pushes the scenario's wall-clock reconnect measurement into its warning band. The client is spec-compliant — the overshoot is scheduler latency, not SDK behavior (idle the measured delay is ~503ms against a 700ms gate; with the suite pinned to 2 cores it reaches ~670ms+).
Three changes, all local to this repo:
Both jobs now save --output-dir results and upload them when the job fails or a flake was rescued, so every future incident carries its measured numbers (today the suite summary prints only counts for warning-level findings).
Related: the upstream scenario fix is proposed in modelcontextprotocol/conformance#375; this PR makes python-sdk CI robust regardless of when/how that lands, and the wrapper simply never fires once the upstream fix is pinned.
How Has This Been Tested?
The wrapper was driven end-to-end against a locally-packed harness tarball identical in behavior to the CI pin, with real and simulated clients:
The everything-server fix was verified both directions over streamable HTTP with no GET stream open: on the base code the sampling request is dropped and the call times out; with the fix it arrives on the tools/call POST stream and completes (also passes the harness's tools-call-sampling scenario 1/1).
Breaking Changes
None. CI-only plus one example-server fix.
Types of changes
Checklist
Additional context
The solo reruns intentionally drop --expected-failures: a scenario in the unexpected list is by definition not baselined, so the solo verdict needs no baseline context (and single-scenario semantics are a strict superset — warnings and client errors still fail). Solo results land in <output-dir>-solo next to the suite's, so uploaded artifacts are self-contained for both the rescued and the still-fails cases.
AI Disclaimer