ci(sdk): run released 0.5.x SDK suites against the current agent - #1121
Open
kvinwang wants to merge 3 commits into
Open
ci(sdk): run released 0.5.x SDK suites against the current agent#1121kvinwang wants to merge 3 commits into
kvinwang wants to merge 3 commits into
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 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.10, v0.5.11], withfail-fast: falseso one failing tag does not hide the other.Result: an empty skip list
Both tags pass in full, in all four languages, with nothing skipped. The freeze currently holds with no exceptions.
Two 0.6.0 changes were expected to need skip entries and did not:
EmitEventalways fails now. Rust, Go and JS never tested it; Python'stest_emit_eventasserts HTTP 400 whenever the simulator endpoint is set — the simulator had no RTMR to extend at v0.5.11 either — and the 0.6.0 stub fails with exactly HTTP 400, so the released assertion still holds.GetQuoteis Intel TDX only now. The simulator serves a TDX quote, so it answers, which is what the released suites assert.The skip-list policy is written down where the list lives: an entry must name a sanctioned change and point at the
CHANGELOGentry or spec section that records it, and 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 withsdk/run-tests.sh, because the compat runner needs one simulator held across several SDK checkouts.Extracting it exposed a bug in the original: bash only elides the fork for a subshell's last command when no traps are installed, and
run-tests.shinstalls several. So$!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.Verification
Ran locally against the current
next:v0.5.11andv0.5.10each green across Rust, Go, Python and JS. The refactoredsdk/run-tests.shwas re-run afterwards and is unaffected (136 passed). Workflow YAML parses.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).