ci(sdk): run released 0.5.x SDK suites against the current agent - #1121
Merged
Conversation
dstack 0.6.0 froze the unversioned guest-agent API at exactly what v0.5.11 served, and the promise that comes with the freeze is that a released 0.5.x SDK keeps working against a 0.6 agent unchanged. Nothing tested that. The descriptor-digest test pins the proto's shape, but a shape can hold while behaviour moves underneath it, and every suite in this repo is edited in the same commit as the code it covers -- so a break is invisible exactly when it matters. `sdk/compat/run-compat-tests.sh <tag>` builds the agent-backed simulator from the current checkout, then checks the SDKs out at `<tag>` with `git worktree` and runs their own suites against it. Old client, new agent, and the only thing crossing between the two trees is the wire protocol. A released client cannot be edited to accommodate a change, which is the property a pinned-in-repo test cannot have. Both tags pass in full today, with an empty skip list in all four languages: the freeze currently holds with no exceptions. Two 0.6.0 changes were expected to need entries and did not -- `EmitEvent` fails with the HTTP 400 the released Python suite already asserted, and `GetQuote`'s TDX-only restriction does not bite a simulator that serves a TDX quote. The skip-list policy is written down where the list lives: an entry must name a sanctioned change and where it is recorded, and a growing list is the failure signal rather than the fix. The simulator lifecycle moves to `sdk/simulator/lifecycle.sh`, shared with `sdk/run-tests.sh`, because the compat runner needs to hold one simulator across several SDK checkouts. Extracting it surfaced a bug in the original: the subshell that starts the simulator is not elided by bash when traps are installed, so `$!` was the subshell rather than the simulator. Cleanup killed the wrapper and left the simulator orphaned, holding its binary open until the next run's build failed with "Text file busy" -- which is what the intermittent stale-socket failures were. `exec` in the subshell fixes it.
`sdk/run-tests.sh` passed shellcheck only because its `set -Eeuo pipefail` and its `cd` were in the same file. Moving the lifecycle into a sourced library separated them, and shellcheck was right to complain: a sourced file inherits whatever options its caller happens to have set, so the guard has to be explicit. `cd ... || exit 1` rather than a suppression. The `# shellcheck source=` hints stay so `shellcheck -x` still checks across the boundary locally; SC1091 is disabled at those two lines because the hook runs without `-x` and cannot follow them.
An adversarial review of this job found the headline claim overstated in two ways and the `exec` rationale simply wrong. The matrix ran v0.5.10 and v0.5.11, whose `sdk/` trees are the same object -- `git rev-parse v0.5.10:sdk v0.5.11:sdk` prints one hash twice. "Both tags pass" was one result reported as two, at the cost of a second seven-minute job. The pair is now v0.5.9 and v0.5.11: v0.5.11 is the tag the freeze is defined against, and v0.5.9 is the newest one whose SDKs actually differ from it. Verified green. A released client only exercises what it already knew about, so this job sees breaking drift and is blind to additive drift: add a field to a frozen message and every old client ignores it. That is not hypothetical -- the proto's own comment records the surface acquiring `GpuInfo` and `AttestGpu` between v0.5.11 and 0.6.0, exactly the drift this job would sleep through. Additions are caught by the descriptor-digest test instead, and the README now says which check owns which failure mode rather than implying this one owns both. The `exec` comment blamed traps for the lost pid. Measured, it takes both an installed trap and a body that runs something before the binary: either alone still gets the fork elided. The table is in the comment now, because a reason that is nearly right is what the next reader will propagate. Also: the `EmitEvent` skip-list note explained why no entry was needed, but the released assertion it cites pins 400 whether the method works or is a stub, so it would pass either way -- the note now says the coverage is zero rather than implying it was checked. Two CHANGELOG references pointed at a 0.6.0 section that does not exist yet. The ERR trap fired once per stack frame under `set -E`, dumping the simulator log repeatedly and scrolling the real failure away; it prints once now, guarded by a file rather than a variable because the duplicate came from a subshell. And the README records that the JS leg is not hermetic (no lockfile at the tag, `latest` pins) and that both runners share socket paths.
This PR diagnosed the lost-pid bug -- with traps installed and a body that runs something before the binary, bash does not elide the subshell fork, so `$!` is the wrapper and cleanup leaves the simulator orphaned holding its binary open -- and fixed it in the extracted `sdk/simulator/lifecycle.sh`. `dstack/run-tests.sh` is the third runner over the same `sdk/simulator/` directory: same binary, same four socket paths, and its own transcription of the same lifecycle, including the same bug. `rust.yml` invokes it, so the leak and the "Text file busy" it causes on the next run stayed in CI while the two SDK runners were fixed. It shared every path with the extracted lifecycle already, so it sources it instead of keeping a third copy. The README's concurrency warning counted two runners; there are three.
…ches
The v0.5.9 leg bought nothing. `git diff --stat v0.5.9 v0.5.11 -- sdk/` is one
line -- a reqwest dependency spec in sdk/rust/Cargo.toml -- with every client
and test source in all four languages byte-identical. It was chosen on "the
tree hash differs", which is the same defect the adjacent comment cites when
rejecting v0.5.10, and it cost a second uncached four-language build to report
one agreement as two results.
v0.5.8 is the newest tag whose clients actually differ, and it went red
immediately, in two languages:
tests/test_client.py::test_emit_event "should not raise" -> 400
tests/test_client.py::test_sync_emit_event same, sync client -> 400
dstack_client_usage (rust example) emit_event(...).await? -> exit 1
All three are `EmitEvent`, removed in 0.6.0 (CHANGELOG.md, `[Unreleased]` /
Removed). So the claim this job shipped with -- an empty skip list, the freeze
holding with no exceptions -- was an artifact of the tag pair, not a result.
There is one sanctioned break on the frozen surface and it is now recorded.
v0.5.11 cannot see it. Its three assertions about the method were all rewritten
in v0.5.9..v0.5.11 ("fix(ci): restore simulator test stability") to tolerate
failure when DSTACK_SIMULATOR_ENDPOINT is set, so under this job they pass
without exercising anything. That is precisely the failure mode the job exists
to prevent -- an assertion edited alongside the change that broke it -- and it
was invisible while the matrix only ran clients that carried the edit.
Skip lists are therefore keyed by tag. `test_emit_event` exists under the same
name at both tags and asserts opposite things; one global entry would silence
the real break and the vacuous pass together.
Also: examples are skippable now (`cargo run --example` has no name filter, so
the unit is the whole binary, and the entry says what that costs -- nothing
here, the other four steps are covered by tests/test_client.rs in the same
leg); the `usage()` example no longer suggests the v0.5.10/v0.5.11 pair the
file itself warns is byte-identical; and PYTHON_SKIP lists
`test_emit_event_validation` explicitly, because pytest deselects by nodeid
prefix and takes it either way -- listing it keeps the list equal to the "3
deselected" the run reports.
Verified locally against the current checkout: v0.5.8 and v0.5.11 both green
across Rust, Go, Python and JS (v0.5.8: 107 passed / 3 deselected + example
skipped; v0.5.11: 110 passed / 0 deselected; 118 JS tests each). Dropping the
skip entries reproduces the three failures.
kvinwang
enabled auto-merge
August 25, 2026 03:17
kvinwang
disabled auto-merge
August 25, 2026 03:17
19 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #1116, which named this as a known follow-up: "CI job running pinned v0.5.10/v0.5.11 released SDKs against the new agent (compat regression)."
Why
0.6.0 froze the unversioned guest-agent API at exactly what v0.5.11 served, and the promise that comes with the freeze is that a released 0.5.x SDK keeps working against a 0.6 agent unchanged. Nothing tested that promise.
The descriptor-digest test added in #1116 pins the proto's shape — it catches an added field or a renumbering — but a shape can hold while behaviour moves underneath it. And every suite in this repo is edited in the same commit as the code it covers, so an assertion gets updated alongside the change that broke it and the break becomes invisible exactly when it matters.
A released client cannot be edited to accommodate a change. That is the property this job buys.
What it does
sdk/compat/run-compat-tests.sh <tag>builds the agent-backed simulator from the current checkout, then checks the SDKs out as they shipped at<tag>viagit worktreeand runs their own test suites against it. Nothing from the tag's tree is built into the agent, nothing from the current tree is copied into the SDKs; the only thing crossing between them is the wire protocol, which is the entire subject of the test..github/workflows/sdk-compat.yamlruns one tag per matrix job over[v0.5.8, v0.5.11], withfail-fast: falseso one failing tag does not hide the other.Result: one sanctioned break, and why the tag pair is the whole ballgame
v0.5.11 skips nothing — every released test and example passes.
v0.5.8 skips three, all of them
EmitEvent, which 0.6.0 removed (CHANGELOG.md,[Unreleased]/ Removed: "runtime RTMR3 events are system-owned and cannot be extended by apps"):tests/test_client.py::test_emit_eventHTTPStatusError400tests/test_client.py::test_sync_emit_eventHTTPStatusError400dstack_client_usage(Rust example)emit_event(...).await?tests/test_client.py::test_emit_event_validationValueErrorThe first three are one sanctioned break, now recorded rather than assumed. The fourth is listed only because pytest deselects by nodeid prefix, so an entry for
::test_emit_eventtakes::test_emit_event_validationwith it whether named or not; naming it keeps the list equal to the3 deselectedthe run reports. It costs nothing — it raises before a request is built and never contacts the agent.GetQuotegoing Intel-TDX-only needed no entry and that one is reassuring: the simulator serves a TDX quote, so it answers, and both tags exercise the path and agree.The pair is doing the work here. Every v0.5.11 assertion about
EmitEventis vacuous under a simulator endpoint — Go and JS never test it,assert_emit_event_behaviorasserts HTTP 400 whether the method works or is a stub, and the example swallows the error whenDSTACK_SIMULATOR_ENDPOINTis set. All three accommodations were added across v0.5.9..v0.5.11 infix(ci): restore simulator test stability, so the suite would pass against a simulator that could not extend an RTMR. v0.5.8 predates them and still asserts the call succeeds. A matrix that ran only v0.5.11 would report an empty skip list and have checked nothing here.That is also why the lists are keyed by tag:
test_emit_eventexists under the same name at both tags and asserts opposite things, so one global entry would silence the real break and the vacuous pass together.The skip-list policy is written down where the list lives: an entry must name a sanctioned change, point at the
CHANGELOGentry or spec section that records it, and say what the skip costs — fordstack_client_usage, nothing, since its other four steps (Info,GetKey,GetQuote,GetTlsKey) are each covered bytests/test_client.rsin the same leg. A growing skip list is the failure signal, not the fix. If you cannot write the justification comment, the frozen surface has drifted and the agent is what needs fixing.A bug this surfaced
The simulator lifecycle moves to
sdk/simulator/lifecycle.sh, shared by all three runners that use it, because the compat runner needs one simulator held across several SDK checkouts.Extracting it exposed a bug in the original. Bash elides the fork for a subshell only when no trap must outlive it and the subshell's body is a single command;
run-tests.shinstalls traps and does acdfirst, so neither held and$!was the subshell, not the simulator. Cleanup killed the wrapper and left the simulator orphaned, holding its binary open until the next run'sbuild.shfailed withText file busy— which is what the intermittent stale-socket failures in SDK runs have been.execin the subshell fixes it.dstack/run-tests.shhad its own transcription of the same lifecycle, with the same bug, over the same binary and the same four socket paths — andrust.ymlruns it. It now sources the shared lifecycle instead of keeping a third copy.Verification
Ran locally against the current checkout, both tags in one invocation, exit 0:
v0.5.8dstack_client_usageskippedv0.5.11Dropping the skip entries reproduces exactly the three failures in the table above, which is how they were found.
dstack/run-tests.shre-run afterwards: exit 0, 122 test binaries green, andpgrepconfirms no simulator survives the run — the orphan theexecfix targets. Its simulator's parent is the script itself rather than an intermediate subshell, which is the observable form of the fix.Workflow YAML parses;
shellcheckandbash -nclean on all three scripts.Two things
run-tests.shdoes are deliberately left out of the compat runner, both documented insdk/compat/README.md:pdm run check(lints the released SDK's source with today's ruff and mypy — says nothing about the wire surface and fails on tool version drift alone) and theno_stdbuild check (a compile-time property of the old types crate, no agent involved).