From 159481676c88fa6fde986380c9f58f00284b4d62 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 24 Aug 2026 09:18:51 -0700 Subject: [PATCH 1/5] ci(sdk): run released 0.5.x SDK suites against the current agent 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 ` builds the agent-backed simulator from the current checkout, then checks the SDKs out at `` 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. --- .github/workflows/sdk-compat.yaml | 70 ++++++++ sdk/compat/README.md | 67 ++++++++ sdk/compat/run-compat-tests.sh | 258 ++++++++++++++++++++++++++++++ sdk/run-tests.sh | 71 +------- sdk/simulator/lifecycle.sh | 102 ++++++++++++ 5 files changed, 502 insertions(+), 66 deletions(-) create mode 100644 .github/workflows/sdk-compat.yaml create mode 100644 sdk/compat/README.md create mode 100755 sdk/compat/run-compat-tests.sh create mode 100644 sdk/simulator/lifecycle.sh diff --git a/.github/workflows/sdk-compat.yaml b/.github/workflows/sdk-compat.yaml new file mode 100644 index 000000000..8a1db2b1c --- /dev/null +++ b/.github/workflows/sdk-compat.yaml @@ -0,0 +1,70 @@ +# SPDX-FileCopyrightText: © 2025 Phala Network +# +# SPDX-License-Identifier: Apache-2.0 + +name: SDK compatibility + +permissions: + contents: read + +on: + push: + branches: [next, 'release/**'] + pull_request: + branches: [next, 'release/**'] + +env: + CARGO_TERM_COLOR: always + # Both the current tree and the released tags pin channel "1.92" in + # rust-toolchain.toml. Without this, rustup would treat that as a toolchain + # distinct from the "1.92.0" installed below and re-download it -- along with + # the three cross-compilation targets the pin asks for, none of which the SDK + # suites need. The compat job is about the wire surface, not about + # reproducing each tag's toolchain provisioning. + RUSTUP_TOOLCHAIN: 1.92.0 + +jobs: + sdk-compat: + name: ${{ matrix.tag }} SDKs vs current agent + runs-on: ubuntu-latest + strategy: + # Each tag is an independent claim; one failing should not hide the other. + fail-fast: false + matrix: + tag: [v0.5.10, v0.5.11] + steps: + - uses: actions/checkout@v5 + with: + # The job checks the released SDKs out with `git worktree add `, + # which needs that tag's commit and its full tree locally. A shallow + # clone plus `fetch-tags` gives the refs but not a guarantee about the + # objects behind them, so take the whole history: the clone is a + # rounding error next to the Rust build in this job. + fetch-depth: 0 + + - name: Install Rust + uses: dtolnay/rust-toolchain@1.92.0 + + - name: Install Go + uses: actions/setup-go@v5 + with: + # The `go` directive in the released sdk/go/go.mod. + go-version: '1.24' + + - name: Install Node + uses: actions/setup-node@v5 + with: + # The released sdk/js/package.json asks for node >=18; 20 is what the + # JS SDK release workflow publishes from. + node-version: '20' + + - name: Install Python + uses: actions/setup-python@v5 + with: + # The released sdk/python/pyproject.toml asks for >=3.10; 3.11 is what + # the Python SDK release workflow builds with. The runner script + # installs PDM if it is missing, the same way sdk/run-tests.sh does. + python-version: '3.11' + + - name: Released SDKs against the current agent + run: ./sdk/compat/run-compat-tests.sh ${{ matrix.tag }} diff --git a/sdk/compat/README.md b/sdk/compat/README.md new file mode 100644 index 000000000..be35cc94e --- /dev/null +++ b/sdk/compat/README.md @@ -0,0 +1,67 @@ +# SDK compatibility regression + +dstack 0.6.0 froze the unversioned guest-agent API at exactly the surface +v0.5.11 served. `DstackGuest` and `Worker` in +`dstack/guest-agent/rpc/proto/agent_rpc.proto` take no new methods, no new +fields and no renumbering, and every new capability goes to `dstack.guest.v1` +instead. The promise that comes with the freeze is that a released 0.5.x SDK +keeps working, unchanged, against a 0.6 agent. + +This directory is what turns that promise into a test. + +## What it does + +`run-compat-tests.sh ` builds the agent-backed simulator from the **current +checkout**, then checks out the SDKs **as they shipped** at `` and runs +their own test suites against that simulator. + +``` +sdk/compat/run-compat-tests.sh v0.5.11 +sdk/compat/run-compat-tests.sh v0.5.10 v0.5.11 # one simulator, both tags +``` + +The released SDKs come from `git worktree add `, so they are the published +code down to the byte, not a reconstruction. Nothing from the tag's tree is +built into the agent and nothing from the current tree is copied into the SDKs. +The only thing crossing between the two is the wire protocol, which is the +entire subject of the test. + +That asymmetry is the point. A test suite pinned inside this repo drifts with +the repo: the assertion gets updated in the same commit that changes the +behaviour, and the break is invisible. A released client cannot be edited to +accommodate a change, so it fails when the surface moves — which is what a real +deployed 0.5.x application would do. + +The suites run the same four languages `sdk/run-tests.sh` runs — Rust, Go, +Python, JS — including their purely local tests (compose hashing, env +encryption, signature verification vectors). Those need no agent and should pass +unchanged; they are not skipped just because they are not client calls. + +CI runs one tag per matrix job (`.github/workflows/sdk-compat.yaml`). Passing +several tags in one local invocation builds and starts the simulator once and +shares a Cargo target directory across them. + +Two things `sdk/run-tests.sh` does are deliberately left out: `pdm run check` +(it lints the released SDK's source with today's ruff and mypy, which says +nothing about the agent's wire surface and fails on tool version drift alone) +and the `no_std` build check (a compile-time property of the old types crate, +with no agent involved). + +## The skip list + +The script carries a per-language skip list. Every entry names a behaviour +0.6.0 deliberately changed on the frozen surface, with a pointer to where that +decision is recorded — a `CHANGELOG.md` entry, or `docs/guest-api-v1.md`. + +**A growing skip list is the failure signal, not the fix.** The list existing at +all is a small admission that the freeze has exceptions; every addition to it +enlarges that admission. When an old suite fails, there are exactly two +outcomes: + +1. The failure matches a sanctioned change. Add it, with a comment naming the + change and the record it lives in. +2. It does not. Then the frozen surface has drifted, and the agent is what needs + fixing. + +There is no third case where a test is skipped because it is inconvenient. If +you cannot write the justification comment, you are looking at outcome 2. diff --git a/sdk/compat/run-compat-tests.sh b/sdk/compat/run-compat-tests.sh new file mode 100755 index 000000000..7cd9627e8 --- /dev/null +++ b/sdk/compat/run-compat-tests.sh @@ -0,0 +1,258 @@ +#!/bin/bash + +# SPDX-FileCopyrightText: © 2025 Phala Network +# +# SPDX-License-Identifier: Apache-2.0 + +# Compatibility regression: run released SDK test suites against a current agent. +# +# dstack 0.6.0 froze the unversioned guest-agent API at exactly what v0.5.11 +# served (`DstackGuest` and `Worker` in agent_rpc.proto), so a released 0.5.x +# SDK keeps working against a 0.6 agent unchanged. This script is what makes +# that a testable claim rather than a promise: for each released tag it checks +# out the SDKs *as they shipped* and runs their own test suites against a +# simulator built from the CURRENT tree. Old client, new agent -- any drift in +# the frozen surface fails here. +# +# The simulator is always the current one. The SDKs are always the old ones. +# Nothing from the tag's tree is built into the agent, and nothing from the +# current tree is copied into the SDKs; the only thing crossing between them is +# the wire protocol, which is the whole subject of the test. +# +# Usage: sdk/compat/run-compat-tests.sh [...] +# e.g. sdk/compat/run-compat-tests.sh v0.5.11 +# sdk/compat/run-compat-tests.sh v0.5.10 v0.5.11 # one simulator, both tags +# +# CI runs one tag per matrix job. Passing several tags locally builds and starts +# the simulator once and shares one Cargo target directory across them. +# +# --------------------------------------------------------------------------- +# Skip-list policy +# --------------------------------------------------------------------------- +# +# Each skip entry below names a behaviour 0.6.0 deliberately changed, with a +# pointer to where that decision is written down. Nothing else belongs there. +# +# A growing skip list is not maintenance: it is the signal that the frozen +# v0.5.11 surface has drifted, which is the one thing this job exists to catch. +# If an old test fails and you cannot point at a CHANGELOG entry or a spec that +# sanctions the change, it is a regression -- fix the agent, not this list. + +set -Eeuo pipefail + +COMPAT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)" +SDK_DIR="$(cd "$COMPAT_DIR/.." && pwd -P)" +REPO_ROOT="$(cd "$SDK_DIR/.." && pwd -P)" + +# shellcheck source=../simulator/lifecycle.sh +source "$SDK_DIR/simulator/lifecycle.sh" + +# Old SDK builds go to a target directory of their own: shared across tags so a +# second tag reuses the first one's dependency build, and kept out of the +# simulator's so the two workspaces neither thrash each other's artifacts nor +# leave the tag's build where `sdk/simulator/build.sh` looks for the binary. +# Applied only to the old Rust suite, never to the simulator build. +COMPAT_CARGO_TARGET_DIR="${COMPAT_CARGO_TARGET_DIR:-$REPO_ROOT/dstack/target/sdk-compat}" + +WORKTREES=() + +cleanup() { + local worktree + for worktree in "${WORKTREES[@]+"${WORKTREES[@]}"}"; do + git -C "$REPO_ROOT" worktree remove --force "$worktree" 2>/dev/null || true + rm -rf "$worktree" + done + WORKTREES=() + simulator_stop +} + +trap 'simulator_print_logs' ERR +trap cleanup EXIT INT TERM + +usage() { + echo "usage: ${BASH_SOURCE[0]} [...]" >&2 + echo " e.g. ${BASH_SOURCE[0]} v0.5.10 v0.5.11" >&2 +} + +# --------------------------------------------------------------------------- +# Skip lists -- see the policy at the top of this file. +# --------------------------------------------------------------------------- + +# All four lists are empty, and that is the result, not an oversight: the +# v0.5.10 and v0.5.11 suites pass in full against the 0.6.0 agent, so the freeze +# currently holds with no exceptions. Two 0.6.0 changes were expected to land +# here and did not: +# +# - `EmitEvent` always fails now (CHANGELOG 0.6.0, Removed: "runtime RTMR3 +# events are system-owned"). Rust, Go and JS never tested it. Python's +# `test_emit_event` asserts HTTP 400 whenever DSTACK_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. +# - `GetQuote` is Intel TDX only now (CHANGELOG 0.6.0, Changed). The simulator +# serves a TDX quote, so it answers, which is what the released suites +# assert. +# +# Entries with spaces must be quoted; a bare word list splits on them. + +# Rust: `cargo test -- --skip `, matched against the full test path. +RUST_SKIP=( +) + +# Go: `go test -skip `, matched against the test name. +GO_SKIP=( +) + +# Python: `pytest --deselect ::`. Matched as a nodeid prefix, so +# `::test_foo` also deselects `::test_foo_bar` -- name the test exactly. +PYTHON_SKIP=( +) + +# JS: vitest has no negative name filter, so the entries are woven into one +# negative-lookahead `--testNamePattern`. They are matched as substrings of the +# full test name, `describe` prefixes included. +JS_SKIP=( +) + +run_rust_suite() { + local sdk_root="$1" + local skip_args=() + local pattern + + for pattern in "${RUST_SKIP[@]+"${RUST_SKIP[@]}"}"; do + skip_args+=(--skip "$pattern") + done + + echo "=== rust ===" + ( + cd "$sdk_root/rust" + export CARGO_TARGET_DIR="$COMPAT_CARGO_TARGET_DIR" + cargo test -- --show-output "${skip_args[@]+"${skip_args[@]}"}" + # The examples are client exercises too: they drive the agent end to end + # the way a README reader would. + cargo run --example tappd_client_usage + cargo run --example dstack_client_usage + ) +} + +run_go_suite() { + local sdk_root="$1" + local skip_args=() + local joined="" + local pattern + + for pattern in "${GO_SKIP[@]+"${GO_SKIP[@]}"}"; do + joined+="${joined:+|}$pattern" + done + if [[ -n "$joined" ]]; then + skip_args+=(-skip "$joined") + fi + + echo "=== go ===" + ( + cd "$sdk_root/go" + go clean -testcache + go test -v "${skip_args[@]+"${skip_args[@]}"}" ./dstack + DSTACK_SIMULATOR_ENDPOINT="$TAPPD_SIMULATOR_ENDPOINT" \ + go test -v "${skip_args[@]+"${skip_args[@]}"}" ./tappd + ) +} + +run_python_suite() { + local sdk_root="$1" + local skip_args=() + local pattern + + for pattern in "${PYTHON_SKIP[@]+"${PYTHON_SKIP[@]}"}"; do + skip_args+=(--deselect "$pattern") + done + + echo "=== python ===" + ( + cd "$sdk_root/python" + if ! command -v pdm >/dev/null 2>&1; then + echo "Installing PDM..." + pip install pdm + fi + pdm install --dev + # `pdm run check` is deliberately not run: it lints the released SDK's + # source with today's ruff and mypy, which says nothing about the agent's + # wire surface and would fail on tool version drift alone. + pdm run pytest "${skip_args[@]+"${skip_args[@]}"}" + ) +} + +run_js_suite() { + local sdk_root="$1" + local name_args=() + local joined="" + local pattern + + for pattern in "${JS_SKIP[@]+"${JS_SKIP[@]}"}"; do + joined+="${joined:+|}$pattern" + done + if [[ -n "$joined" ]]; then + name_args+=(--testNamePattern "^(?!.*(?:$joined))") + fi + + echo "=== js ===" + ( + cd "$sdk_root/js" + npm install + npx vitest --run "${name_args[@]+"${name_args[@]}"}" + ) +} + +run_tag() { + local tag="$1" + local worktree + + worktree="$(mktemp -d -t "dstack-sdk-compat-${tag}-XXXXXX")" + WORKTREES+=("$worktree") + git -C "$REPO_ROOT" worktree add --detach --quiet "$worktree" "refs/tags/$tag" + + echo + echo "############################################################" + echo "# $tag SDKs against the current agent" + echo "# sdks: $worktree/sdk" + echo "# simulator: $DSTACK_SIMULATOR_ENDPOINT" + echo "############################################################" + + run_rust_suite "$worktree/sdk" + run_go_suite "$worktree/sdk" + run_python_suite "$worktree/sdk" + run_js_suite "$worktree/sdk" + + # Drop it now rather than at exit, so running several tags does not keep a + # full checkout per tag on disk. `cleanup` retries harmlessly at exit. + git -C "$REPO_ROOT" worktree remove --force "$worktree" + rm -rf "$worktree" + + echo "--- $tag: all suites passed against the current agent" +} + +main() { + if [[ $# -lt 1 ]]; then + usage + exit 2 + fi + + local tag + for tag in "$@"; do + if ! git -C "$REPO_ROOT" rev-parse --verify --quiet "refs/tags/$tag^{commit}" >/dev/null; then + echo "unknown tag: $tag -- fetch tags first (git fetch --tags)" >&2 + exit 1 + fi + done + + simulator_start + + for tag in "$@"; do + run_tag "$tag" + done + + echo + echo "compat: $* passed against the agent in $REPO_ROOT" +} + +main "$@" diff --git a/sdk/run-tests.sh b/sdk/run-tests.sh index a16e4a3c5..10734c159 100755 --- a/sdk/run-tests.sh +++ b/sdk/run-tests.sh @@ -8,75 +8,14 @@ set -Eeuo pipefail ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)" -SIMULATOR_DIR="$ROOT_DIR/simulator" -SIMULATOR_LOG="$SIMULATOR_DIR/dstack-simulator.log" -DSTACK_SOCKET="$SIMULATOR_DIR/dstack.sock" -TAPPD_SOCKET="$SIMULATOR_DIR/tappd.sock" -GUEST_SOCKET="$SIMULATOR_DIR/guest.sock" -EXTERNAL_SOCKET="$SIMULATOR_DIR/external.sock" -SIMULATOR_PID="" -cleanup() { - if [[ -n "${SIMULATOR_PID:-}" ]]; then - kill "$SIMULATOR_PID" 2>/dev/null || true - wait "$SIMULATOR_PID" 2>/dev/null || true - fi - rm -f "$DSTACK_SOCKET" "$TAPPD_SOCKET" "$GUEST_SOCKET" "$EXTERNAL_SOCKET" -} +# shellcheck source=simulator/lifecycle.sh +source "$ROOT_DIR/simulator/lifecycle.sh" -print_simulator_logs() { - if [[ -f "$SIMULATOR_LOG" ]]; then - echo "Last simulator logs:" - tail -100 "$SIMULATOR_LOG" || true - fi -} +trap 'simulator_print_logs' ERR +trap simulator_stop EXIT INT TERM -wait_for_socket() { - local socket_path="$1" - local name="$2" - - for _ in {1..100}; do - if [[ -S "$socket_path" ]]; then - return 0 - fi - if [[ -n "${SIMULATOR_PID:-}" ]] && ! kill -0 "$SIMULATOR_PID" 2>/dev/null; then - echo "Simulator exited before $name socket became ready." - print_simulator_logs - return 1 - fi - sleep 0.2 - done - - echo "Timed out waiting for $name socket at $socket_path" - print_simulator_logs - return 1 -} - -trap 'print_simulator_logs' ERR -trap cleanup EXIT INT TERM - -rm -f \ - "$DSTACK_SOCKET" \ - "$TAPPD_SOCKET" \ - "$GUEST_SOCKET" \ - "$EXTERNAL_SOCKET" \ - "$SIMULATOR_LOG" -export DSTACK_SIMULATOR_ENDPOINT="$DSTACK_SOCKET" -export TAPPD_SIMULATOR_ENDPOINT="$TAPPD_SOCKET" - -( - cd "$SIMULATOR_DIR" - ./build.sh -) - -( - cd "$SIMULATOR_DIR" - ./dstack-simulator >"$SIMULATOR_LOG" 2>&1 -) & -SIMULATOR_PID=$! - -wait_for_socket "$DSTACK_SOCKET" "dstack" -wait_for_socket "$TAPPD_SOCKET" "tappd" +simulator_start pushd "$ROOT_DIR/rust" cargo test -- --show-output diff --git a/sdk/simulator/lifecycle.sh b/sdk/simulator/lifecycle.sh new file mode 100644 index 000000000..47099bb88 --- /dev/null +++ b/sdk/simulator/lifecycle.sh @@ -0,0 +1,102 @@ +#!/bin/bash + +# SPDX-FileCopyrightText: © 2025 Daniel Sharifi +# SPDX-FileCopyrightText: © 2025 Phala Network +# +# SPDX-License-Identifier: Apache-2.0 + +# Build/start/stop for the agent-backed simulator, shared by the SDK test +# runners: `sdk/run-tests.sh` and `sdk/compat/run-compat-tests.sh`. Both need a +# simulator built from the current checkout listening on the same two sockets, +# and the compat runner needs to keep one alive across several SDK checkouts, +# so the lifecycle lives here rather than being transcribed twice. +# +# Source this file, do not execute it: `simulator_start` exports +# DSTACK_SIMULATOR_ENDPOINT and TAPPD_SIMULATOR_ENDPOINT into the caller's +# environment, which is how every SDK test suite finds the agent. Traps stay +# with the caller so that cleanup order is visible in the script that owns it; +# the caller is expected to install `simulator_stop` on EXIT and +# `simulator_print_logs` on ERR. + +SIMULATOR_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)" +SIMULATOR_LOG="$SIMULATOR_DIR/dstack-simulator.log" +DSTACK_SOCKET="$SIMULATOR_DIR/dstack.sock" +TAPPD_SOCKET="$SIMULATOR_DIR/tappd.sock" +GUEST_SOCKET="$SIMULATOR_DIR/guest.sock" +EXTERNAL_SOCKET="$SIMULATOR_DIR/external.sock" +SIMULATOR_PID="" + +simulator_stop() { + if [[ -n "${SIMULATOR_PID:-}" ]]; then + kill "$SIMULATOR_PID" 2>/dev/null || true + wait "$SIMULATOR_PID" 2>/dev/null || true + SIMULATOR_PID="" + fi + rm -f "$DSTACK_SOCKET" "$TAPPD_SOCKET" "$GUEST_SOCKET" "$EXTERNAL_SOCKET" +} + +simulator_print_logs() { + if [[ -f "$SIMULATOR_LOG" ]]; then + echo "Last simulator logs:" + tail -100 "$SIMULATOR_LOG" || true + fi +} + +simulator_wait_for_socket() { + local socket_path="$1" + local name="$2" + + for _ in {1..100}; do + if [[ -S "$socket_path" ]]; then + return 0 + fi + if [[ -n "${SIMULATOR_PID:-}" ]] && ! kill -0 "$SIMULATOR_PID" 2>/dev/null; then + echo "Simulator exited before $name socket became ready." + simulator_print_logs + return 1 + fi + sleep 0.2 + done + + echo "Timed out waiting for $name socket at $socket_path" + simulator_print_logs + return 1 +} + +simulator_build() { + ( + cd "$SIMULATOR_DIR" + ./build.sh + ) +} + +# Builds the simulator from the current checkout, starts it, waits for both +# sockets, and exports the endpoints the SDK suites read. +simulator_start() { + rm -f \ + "$DSTACK_SOCKET" \ + "$TAPPD_SOCKET" \ + "$GUEST_SOCKET" \ + "$EXTERNAL_SOCKET" \ + "$SIMULATOR_LOG" + + export DSTACK_SIMULATOR_ENDPOINT="$DSTACK_SOCKET" + export TAPPD_SIMULATOR_ENDPOINT="$TAPPD_SOCKET" + + simulator_build + + # `exec` matters: without it bash keeps the subshell around as a parent of + # the simulator -- it only elides the fork for a subshell's last command + # when no traps are installed, and the callers install several. `$!` would + # then be the subshell, `simulator_stop` would kill that and leave the + # simulator orphaned, holding the binary open so the next run's build.sh + # fails to overwrite it with "Text file busy". + ( + cd "$SIMULATOR_DIR" + exec ./dstack-simulator >"$SIMULATOR_LOG" 2>&1 + ) & + SIMULATOR_PID=$! + + simulator_wait_for_socket "$DSTACK_SOCKET" "dstack" + simulator_wait_for_socket "$TAPPD_SOCKET" "tappd" +} From fe2fc548dbce24c6d3b3ee604a61e90e76ffe231 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 24 Aug 2026 09:25:40 -0700 Subject: [PATCH 2/5] fix(sdk): satisfy shellcheck in the extracted simulator lifecycle `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. --- sdk/compat/run-compat-tests.sh | 1 + sdk/run-tests.sh | 1 + sdk/simulator/lifecycle.sh | 8 ++++++-- 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/sdk/compat/run-compat-tests.sh b/sdk/compat/run-compat-tests.sh index 7cd9627e8..5301d1e53 100755 --- a/sdk/compat/run-compat-tests.sh +++ b/sdk/compat/run-compat-tests.sh @@ -45,6 +45,7 @@ SDK_DIR="$(cd "$COMPAT_DIR/.." && pwd -P)" REPO_ROOT="$(cd "$SDK_DIR/.." && pwd -P)" # shellcheck source=../simulator/lifecycle.sh +# shellcheck disable=SC1091 # the hook runs without -x, so it cannot follow this source "$SDK_DIR/simulator/lifecycle.sh" # Old SDK builds go to a target directory of their own: shared across tags so a diff --git a/sdk/run-tests.sh b/sdk/run-tests.sh index 10734c159..867619149 100755 --- a/sdk/run-tests.sh +++ b/sdk/run-tests.sh @@ -10,6 +10,7 @@ set -Eeuo pipefail ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)" # shellcheck source=simulator/lifecycle.sh +# shellcheck disable=SC1091 # the hook runs without -x, so it cannot follow this source "$ROOT_DIR/simulator/lifecycle.sh" trap 'simulator_print_logs' ERR diff --git a/sdk/simulator/lifecycle.sh b/sdk/simulator/lifecycle.sh index 47099bb88..2b16be273 100644 --- a/sdk/simulator/lifecycle.sh +++ b/sdk/simulator/lifecycle.sh @@ -65,7 +65,11 @@ simulator_wait_for_socket() { simulator_build() { ( - cd "$SIMULATOR_DIR" + # `|| exit` rather than relying on the caller's `set -e`: this file is + # sourced, so it inherits whatever shell options the caller happens to + # have set, and building in the wrong directory is not a failure worth + # discovering three steps later. + cd "$SIMULATOR_DIR" || exit 1 ./build.sh ) } @@ -92,7 +96,7 @@ simulator_start() { # simulator orphaned, holding the binary open so the next run's build.sh # fails to overwrite it with "Text file busy". ( - cd "$SIMULATOR_DIR" + cd "$SIMULATOR_DIR" || exit 1 exec ./dstack-simulator >"$SIMULATOR_LOG" 2>&1 ) & SIMULATOR_PID=$! From 85de3daa53a5c607c663d0e46d8bc0a543148ceb Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 24 Aug 2026 09:57:25 -0700 Subject: [PATCH 3/5] fix(sdk): make the compat job's coverage claim match what it checks 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. --- .github/workflows/sdk-compat.yaml | 7 ++++- sdk/compat/README.md | 46 +++++++++++++++++++++++++++--- sdk/compat/run-compat-tests.sh | 47 +++++++++++++++++++++++-------- sdk/simulator/.gitignore | 1 + sdk/simulator/lifecycle.sh | 41 ++++++++++++++++++++++----- 5 files changed, 118 insertions(+), 24 deletions(-) diff --git a/.github/workflows/sdk-compat.yaml b/.github/workflows/sdk-compat.yaml index 8a1db2b1c..e3fe2deec 100644 --- a/.github/workflows/sdk-compat.yaml +++ b/.github/workflows/sdk-compat.yaml @@ -31,7 +31,12 @@ jobs: # Each tag is an independent claim; one failing should not hide the other. fail-fast: false matrix: - tag: [v0.5.10, v0.5.11] + # v0.5.11 is the tag the freeze is defined against, and v0.5.9 is the + # newest tag whose SDKs actually differ from it. Not v0.5.10, the + # obvious other choice: `git rev-parse v0.5.10:sdk v0.5.11:sdk` returns + # one hash, so that pair would run the same suites twice and report the + # agreement as two independent results. + tag: [v0.5.9, v0.5.11] steps: - uses: actions/checkout@v5 with: diff --git a/sdk/compat/README.md b/sdk/compat/README.md index be35cc94e..2a8669ca9 100644 --- a/sdk/compat/README.md +++ b/sdk/compat/README.md @@ -17,7 +17,7 @@ their own test suites against that simulator. ``` sdk/compat/run-compat-tests.sh v0.5.11 -sdk/compat/run-compat-tests.sh v0.5.10 v0.5.11 # one simulator, both tags +sdk/compat/run-compat-tests.sh v0.5.9 v0.5.11 # one simulator, both tags ``` The released SDKs come from `git worktree add `, so they are the published @@ -32,14 +32,52 @@ behaviour, and the break is invisible. A released client cannot be edited to accommodate a change, so it fails when the surface moves — which is what a real deployed 0.5.x application would do. +## What this does not catch + +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, exactly as JSON and protobuf intend: +the suites stay green. That is not a hypothetical — the comment at the top of +`agent_rpc.proto` records that the surface acquired `GpuInfo`, `AttestGpu` and +an `AttestArgs` field between v0.5.11 and 0.6.0, which is the drift this job +would have slept through. + +Additions are caught one layer down, by the descriptor-digest test in +`dstack/guest-agent/rpc/tests/frozen_surface.rs`: it hashes every frozen +service's method list and the full field list of every message they reach, so a +*wire-compatible* addition fails CI even though no client would notice. + +Neither check subsumes the other. The digest pins the shape and cannot see +behaviour changing underneath a stable shape; this job exercises behaviour and +cannot see the shape growing. Both are required, and a change that trips +neither is what "frozen" is allowed to mean. + The suites run the same four languages `sdk/run-tests.sh` runs — Rust, Go, Python, JS — including their purely local tests (compose hashing, env encryption, signature verification vectors). Those need no agent and should pass unchanged; they are not skipped just because they are not client calls. -CI runs one tag per matrix job (`.github/workflows/sdk-compat.yaml`). Passing -several tags in one local invocation builds and starts the simulator once and -shares a Cargo target directory across them. +CI runs one tag per matrix job (`.github/workflows/sdk-compat.yaml`), over +`v0.5.11` — the tag the freeze is defined against — and `v0.5.9`, the newest +tag whose SDKs actually differ from it. Check before adding a tag: several +release tags share an SDK tree byte for byte (`git rev-parse v0.5.10:sdk +v0.5.11:sdk` prints one hash twice), and a pair like that runs the same suites +twice and reports the agreement as two independent results. + +Passing several tags in one local invocation builds and starts the simulator +once and shares a Cargo target directory across them. + +The JS leg is the one that is not hermetic: the released `sdk/js` ships a +`bun.lockb` but no `package-lock.json`, and pins `typescript` and `@types/node` +at `latest`, so `npm install` resolves fresh every run. A red JS suite here can +mean an npm publish rather than agent drift — check the diff before believing +it. The Rust, Go and Python legs are pinned by `Cargo.lock`, `go.sum` and +`pdm.lock`. + +Both scripts use the sockets and the binary under `sdk/simulator/`, so running +this and `sdk/run-tests.sh` at the same time in one checkout will have them +delete each other's sockets and fail to overwrite a running binary. Use +separate checkouts, or run them one after the other. Two things `sdk/run-tests.sh` does are deliberately left out: `pdm run check` (it lints the released SDK's source with today's ruff and mypy, which says diff --git a/sdk/compat/run-compat-tests.sh b/sdk/compat/run-compat-tests.sh index 5301d1e53..7ae32b8ee 100755 --- a/sdk/compat/run-compat-tests.sh +++ b/sdk/compat/run-compat-tests.sh @@ -21,7 +21,11 @@ # # Usage: sdk/compat/run-compat-tests.sh [...] # e.g. sdk/compat/run-compat-tests.sh v0.5.11 -# sdk/compat/run-compat-tests.sh v0.5.10 v0.5.11 # one simulator, both tags +# sdk/compat/run-compat-tests.sh v0.5.9 v0.5.11 # one simulator, both tags +# +# Any tag works, but check that the pair you pick differs: several release tags +# share an SDK tree byte for byte (v0.5.10 and v0.5.11 do), and running both +# reports one result twice. # # CI runs one tag per matrix job. Passing several tags locally builds and starts # the simulator once and shares one Cargo target directory across them. @@ -80,21 +84,40 @@ usage() { # --------------------------------------------------------------------------- # All four lists are empty, and that is the result, not an oversight: the -# v0.5.10 and v0.5.11 suites pass in full against the 0.6.0 agent, so the freeze -# currently holds with no exceptions. Two 0.6.0 changes were expected to land -# here and did not: +# released suites pass in full against the 0.6.0 agent, so the freeze currently +# holds with no exceptions. Two 0.6.0 changes were expected to land here and +# did not -- but for different reasons, and only one of them is reassuring: # -# - `EmitEvent` always fails now (CHANGELOG 0.6.0, Removed: "runtime RTMR3 -# events are system-owned"). Rust, Go and JS never tested it. Python's -# `test_emit_event` asserts HTTP 400 whenever DSTACK_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. -# - `GetQuote` is Intel TDX only now (CHANGELOG 0.6.0, Changed). The simulator +# - `GetQuote` is Intel TDX only now (CHANGELOG, Changed). The simulator # serves a TDX quote, so it answers, which is what the released suites -# assert. +# assert. Genuinely covered: the suites exercise the path and agree. +# - `EmitEvent` always fails now (CHANGELOG, Removed: "runtime RTMR3 events +# are system-owned"). Rust, Go and JS never tested it. Python's +# `assert_emit_event_behavior` asserts HTTP 400 whenever +# DSTACK_SIMULATOR_ENDPOINT is set, because the simulator had no RTMR to +# extend at v0.5.11 either -- so it asserts 400 whether the method works or +# is a stub, and it would pass either way. Not covered. The absence of a +# skip entry here says nothing about `EmitEvent`; do not read it as +# evidence. +# +# Both point at CHANGELOG.md's `[Unreleased]` section -- 0.6.0 is not cut yet, +# so there is no `## [0.6.0]` heading to cite. # # Entries with spaces must be quoted; a bare word list splits on them. +# +# The four lists do NOT share matching semantics, so an entry cannot be moved +# between them unchanged: +# +# RUST_SKIP literal substring of the full test path (`cargo test --skip`). +# Reaches `cargo test` only -- the two `cargo run --example` +# invocations below cannot be skipped. +# GO_SKIP REGEXP over the test name (`go test -skip`), joined with `|`. +# PYTHON_SKIP literal nodeid prefix (`pytest --deselect`); pytest matches +# with `startswith`, so `::test_foo` also takes `::test_foo_bar`. +# JS_SKIP REGEXP, woven into one negative-lookahead `--testNamePattern`. +# +# The two regexp lists interpolate entries unescaped: a `(`, `+` or `.` in an +# entry changes its meaning. Escape it, or the skip silently widens. # Rust: `cargo test -- --skip `, matched against the full test path. RUST_SKIP=( diff --git a/sdk/simulator/.gitignore b/sdk/simulator/.gitignore index fd1ce3bc3..94410ebfa 100644 --- a/sdk/simulator/.gitignore +++ b/sdk/simulator/.gitignore @@ -2,3 +2,4 @@ dstack-simulator dstack-guest-agent *.lock *.log +.simulator-logs-printed diff --git a/sdk/simulator/lifecycle.sh b/sdk/simulator/lifecycle.sh index 2b16be273..9969b4223 100644 --- a/sdk/simulator/lifecycle.sh +++ b/sdk/simulator/lifecycle.sh @@ -35,7 +35,20 @@ simulator_stop() { rm -f "$DSTACK_SOCKET" "$TAPPD_SOCKET" "$GUEST_SOCKET" "$EXTERNAL_SOCKET" } +# Printed at most once per run. The callers set `-E`, so an ERR trap propagates +# into every function and subshell: without a guard, a failure inside a suite +# dumps the same 100 lines on the way out of each frame, and the test failure an +# operator came to read ends up scrolled off the top. +# +# The marker is a file, not a variable, precisely because the duplicate print +# comes from a subshell -- an assignment there would not be visible to the +# parent that prints second. +SIMULATOR_LOGS_PRINTED_MARKER="$SIMULATOR_DIR/.simulator-logs-printed" + simulator_print_logs() { + if ! (set -o noclobber; : >"$SIMULATOR_LOGS_PRINTED_MARKER") 2>/dev/null; then + return 0 + fi if [[ -f "$SIMULATOR_LOG" ]]; then echo "Last simulator logs:" tail -100 "$SIMULATOR_LOG" || true @@ -70,7 +83,7 @@ simulator_build() { # have set, and building in the wrong directory is not a failure worth # discovering three steps later. cd "$SIMULATOR_DIR" || exit 1 - ./build.sh + ./build.sh || exit 1 ) } @@ -82,6 +95,7 @@ simulator_start() { "$TAPPD_SOCKET" \ "$GUEST_SOCKET" \ "$EXTERNAL_SOCKET" \ + "$SIMULATOR_LOGS_PRINTED_MARKER" \ "$SIMULATOR_LOG" export DSTACK_SIMULATOR_ENDPOINT="$DSTACK_SOCKET" @@ -89,12 +103,25 @@ simulator_start() { simulator_build - # `exec` matters: without it bash keeps the subshell around as a parent of - # the simulator -- it only elides the fork for a subshell's last command - # when no traps are installed, and the callers install several. `$!` would - # then be the subshell, `simulator_stop` would kill that and leave the - # simulator orphaned, holding the binary open so the next run's build.sh - # fails to overwrite it with "Text file busy". + # `exec` matters: without it `$!` is the subshell rather than the simulator, + # so `simulator_stop` kills the wrapper and leaves the simulator orphaned, + # holding its binary open until the next run's build.sh fails to overwrite + # it with "Text file busy". + # + # It takes both of the conditions below to lose the pid, which is why this + # is easy to get wrong by testing only one of them (measured, bash 5.2): + # + # traps body `$!` is + # none single command the command (bash collapses `( cmd ) &`) + # none cd; command the command (last-command exec applies) + # set single command the command (collapsed before traps matter) + # set cd; command THE SUBSHELL <- this script + # + # A trap that must still run after the last command is what disables the + # exec-in-place, and a body that does something first is what stops the + # whole subshell being collapsed instead. This function has a `cd` and its + # callers install four traps, so `exec` is the only thing making `$!` the + # simulator. ( cd "$SIMULATOR_DIR" || exit 1 exec ./dstack-simulator >"$SIMULATOR_LOG" 2>&1 From d5bb68bcbc0412c474f76a9eca95ba8bf1c5f8b4 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 24 Aug 2026 19:55:28 -0700 Subject: [PATCH 4/5] fix(dstack): source the shared simulator lifecycle in run-tests.sh 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. --- dstack/run-tests.sh | 78 +++++++------------------------------------- sdk/compat/README.md | 6 ++-- 2 files changed, 15 insertions(+), 69 deletions(-) diff --git a/dstack/run-tests.sh b/dstack/run-tests.sh index 4fb8dbd94..2e9038117 100755 --- a/dstack/run-tests.sh +++ b/dstack/run-tests.sh @@ -8,76 +8,20 @@ set -Eeuo pipefail CORE_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd -P)" REPO_ROOT="$(cd "$CORE_DIR/.." && pwd -P)" -SIMULATOR_DIR="$REPO_ROOT/sdk/simulator" -SIMULATOR_LOG="$SIMULATOR_DIR/dstack-simulator.log" -DSTACK_SOCKET="$SIMULATOR_DIR/dstack.sock" -TAPPD_SOCKET="$SIMULATOR_DIR/tappd.sock" -GUEST_SOCKET="$SIMULATOR_DIR/guest.sock" -EXTERNAL_SOCKET="$SIMULATOR_DIR/external.sock" -SIMULATOR_PID="" -cleanup() { - if [[ -n "${SIMULATOR_PID:-}" ]]; then - kill "$SIMULATOR_PID" 2>/dev/null || true - wait "$SIMULATOR_PID" 2>/dev/null || true - fi - rm -f "$DSTACK_SOCKET" "$TAPPD_SOCKET" "$GUEST_SOCKET" "$EXTERNAL_SOCKET" -} +# The third runner sharing `sdk/simulator/`: same binary, same four sockets. +# It used to transcribe the lifecycle instead of sourcing it, and inherited the +# lost-pid bug documented in `simulator_start` -- cleanup killed the wrapper and +# left the simulator holding its binary open, so the next run's `build.sh` hit +# "Text file busy". Sourcing keeps the fix in one place. +# shellcheck source=../sdk/simulator/lifecycle.sh +# shellcheck disable=SC1091 # the hook runs without -x, so it cannot follow this +source "$REPO_ROOT/sdk/simulator/lifecycle.sh" -print_simulator_logs() { - if [[ -f "$SIMULATOR_LOG" ]]; then - echo "Last simulator logs:" - tail -100 "$SIMULATOR_LOG" || true - fi -} +trap 'simulator_print_logs' ERR +trap simulator_stop EXIT INT TERM -wait_for_socket() { - local socket_path="$1" - local name="$2" - - for _ in {1..100}; do - if [[ -S "$socket_path" ]]; then - return 0 - fi - if [[ -n "${SIMULATOR_PID:-}" ]] && ! kill -0 "$SIMULATOR_PID" 2>/dev/null; then - echo "Simulator exited before $name socket became ready." - print_simulator_logs - return 1 - fi - sleep 0.2 - done - - echo "Timed out waiting for $name socket at $socket_path" - print_simulator_logs - return 1 -} - -trap 'print_simulator_logs' ERR -trap cleanup EXIT INT TERM - -rm -f \ - "$DSTACK_SOCKET" \ - "$TAPPD_SOCKET" \ - "$GUEST_SOCKET" \ - "$EXTERNAL_SOCKET" \ - "$SIMULATOR_LOG" -( - cd "$SIMULATOR_DIR" - ./build.sh -) - -( - cd "$SIMULATOR_DIR" - ./dstack-simulator >"$SIMULATOR_LOG" 2>&1 -) & -SIMULATOR_PID=$! -echo "Simulator process (PID: $SIMULATOR_PID) started." - -wait_for_socket "$DSTACK_SOCKET" "dstack" -wait_for_socket "$TAPPD_SOCKET" "tappd" - -export DSTACK_SIMULATOR_ENDPOINT="$DSTACK_SOCKET" -export TAPPD_SIMULATOR_ENDPOINT="$TAPPD_SOCKET" +simulator_start echo "DSTACK_SIMULATOR_ENDPOINT: $DSTACK_SIMULATOR_ENDPOINT" echo "TAPPD_SIMULATOR_ENDPOINT: $TAPPD_SIMULATOR_ENDPOINT" diff --git a/sdk/compat/README.md b/sdk/compat/README.md index 2a8669ca9..bfc8738de 100644 --- a/sdk/compat/README.md +++ b/sdk/compat/README.md @@ -74,8 +74,10 @@ mean an npm publish rather than agent drift — check the diff before believing it. The Rust, Go and Python legs are pinned by `Cargo.lock`, `go.sum` and `pdm.lock`. -Both scripts use the sockets and the binary under `sdk/simulator/`, so running -this and `sdk/run-tests.sh` at the same time in one checkout will have them +Three runners share the sockets and the binary under `sdk/simulator/`: this +one, `sdk/run-tests.sh` and `dstack/run-tests.sh` (the last is what `rust.yml` +invokes). All three `rm -f` the same four socket paths on cleanup and rebuild +the same binary, so running any two at once in one checkout will have them delete each other's sockets and fail to overwrite a running binary. Use separate checkouts, or run them one after the other. From 377d445e3bfeabdc5695228f17a948ed6a7f9917 Mon Sep 17 00:00:00 2001 From: Kevin Wang Date: Mon, 24 Aug 2026 20:08:06 -0700 Subject: [PATCH 5/5] ci(sdk): compat-test v0.5.8 instead of v0.5.9, and record what it catches 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. --- .github/workflows/sdk-compat.yaml | 24 +++- sdk/compat/README.md | 64 ++++++++++- sdk/compat/run-compat-tests.sh | 177 +++++++++++++++++++++--------- 3 files changed, 204 insertions(+), 61 deletions(-) diff --git a/.github/workflows/sdk-compat.yaml b/.github/workflows/sdk-compat.yaml index e3fe2deec..e4e4efe2d 100644 --- a/.github/workflows/sdk-compat.yaml +++ b/.github/workflows/sdk-compat.yaml @@ -31,12 +31,24 @@ jobs: # Each tag is an independent claim; one failing should not hide the other. fail-fast: false matrix: - # v0.5.11 is the tag the freeze is defined against, and v0.5.9 is the - # newest tag whose SDKs actually differ from it. Not v0.5.10, the - # obvious other choice: `git rev-parse v0.5.10:sdk v0.5.11:sdk` returns - # one hash, so that pair would run the same suites twice and report the - # agreement as two independent results. - tag: [v0.5.9, v0.5.11] + # v0.5.11 is the tag the freeze is defined against, and v0.5.8 is the + # newest tag whose SDK *clients and suites* actually differ from it. + # + # The bar is a different client, not a different tree. v0.5.10's sdk/ + # tree is the same object as v0.5.11's (`git rev-parse v0.5.10:sdk + # v0.5.11:sdk` prints one hash twice) and v0.5.9 differs from it by a + # single line -- a reqwest dependency spec in sdk/rust/Cargo.toml, with + # every client and test source in all four languages byte-identical. + # Either pair runs the same suites twice and reports one agreement as + # two independent results, at the cost of a second uncached + # four-language build. v0.5.8 is the first tag going back that carries + # a genuinely different client (26 files), and it earns its slot: it is + # the only released client that still asserts `EmitEvent` succeeds, so + # it is the only one that catches 0.6.0 removing it. v0.5.11's three + # assertions about that method were all rewritten in v0.5.9..v0.5.11 + # to tolerate failure under a simulator endpoint, so the v0.5.11 leg + # passes them without exercising anything. See sdk/compat/README.md. + tag: [v0.5.8, v0.5.11] steps: - uses: actions/checkout@v5 with: diff --git a/sdk/compat/README.md b/sdk/compat/README.md index bfc8738de..d1a40817d 100644 --- a/sdk/compat/README.md +++ b/sdk/compat/README.md @@ -17,7 +17,7 @@ their own test suites against that simulator. ``` sdk/compat/run-compat-tests.sh v0.5.11 -sdk/compat/run-compat-tests.sh v0.5.9 v0.5.11 # one simulator, both tags +sdk/compat/run-compat-tests.sh v0.5.8 v0.5.11 # one simulator, both tags ``` The released SDKs come from `git worktree add `, so they are the published @@ -58,11 +58,18 @@ encryption, signature verification vectors). Those need no agent and should pass unchanged; they are not skipped just because they are not client calls. CI runs one tag per matrix job (`.github/workflows/sdk-compat.yaml`), over -`v0.5.11` — the tag the freeze is defined against — and `v0.5.9`, the newest -tag whose SDKs actually differ from it. Check before adding a tag: several -release tags share an SDK tree byte for byte (`git rev-parse v0.5.10:sdk -v0.5.11:sdk` prints one hash twice), and a pair like that runs the same suites -twice and reports the agreement as two independent results. +`v0.5.11` — the tag the freeze is defined against — and `v0.5.8`, the newest +tag whose SDK clients and suites actually differ from it. + +Check before adding a tag, and check the *sources*, not the tree hash. Several +release tags share an `sdk/` tree byte for byte (`git rev-parse v0.5.10:sdk +v0.5.11:sdk` prints one hash twice), and a tag can differ by a hash while +carrying an identical client: `v0.5.9` differs from `v0.5.11` by one line, a +reqwest dependency spec in `sdk/rust/Cargo.toml`, with every client and test +source in all four languages unchanged. Either one runs the same suites twice +and reports one agreement as two independent results, at the cost of a second +uncached four-language build. `git diff --stat v0.5.11 -- sdk/` is the +check worth running. Passing several tags in one local invocation builds and starts the simulator once and shares a Cargo target directory across them. @@ -93,6 +100,51 @@ The script carries a per-language skip list. Every entry names a behaviour 0.6.0 deliberately changed on the frozen surface, with a pointer to where that decision is recorded — a `CHANGELOG.md` entry, or `docs/guest-api-v1.md`. +Lists are keyed by tag (`set_skips_for_tag`), because a skip is a claim about +one released client rather than about the frozen surface in general. The two +tags in the matrix disagree about the same method, under the same test name, so +a global list could not express it. + +The list has entries for exactly one thing, and reading them is the fastest way +to see what this job is for. **v0.5.11 skips nothing** — every released test and +example passes. **v0.5.8 skips four**, all `EmitEvent`, which 0.6.0 removed (`CHANGELOG.md`, +`[Unreleased]` / Removed: "runtime RTMR3 events are system-owned and cannot be +extended by apps"): + +| Skipped at v0.5.8 | What it asserted | Against a 0.6.0 agent | +| --- | --- | --- | +| `tests/test_client.py::test_emit_event` | "This should not raise an error" | `HTTPStatusError` 400 | +| `tests/test_client.py::test_sync_emit_event` | same, sync client | `HTTPStatusError` 400 | +| `dstack_client_usage` (Rust example) | step 4 calls `emit_event(...).await?` | example exits non-zero | +| `tests/test_client.py::test_emit_event_validation` | empty event name raises `ValueError` | still passes — see below | + +The first three are the one sanctioned break on the frozen surface, now +recorded rather than assumed. The fourth is collateral and is listed only +because pytest deselects by nodeid *prefix*: an entry for `::test_emit_event` +takes `::test_emit_event_validation` with it whether or not it is named. Naming +it keeps the list equal to what the run actually skips — otherwise pytest +reports `3 deselected` against two entries and nobody can see the difference. +It costs nothing: it raises before a request is built and never contacts the +agent, and the same client-side validation still runs in the Rust, Go and JS +suites. + +It is also the whole argument for the tag pair. Every v0.5.11 assertion about +`EmitEvent` is vacuous under a simulator endpoint: Go and JS never test it, +`assert_emit_event_behavior` asserts HTTP 400 whether the method works or is a +stub, and the example swallows the error when `DSTACK_SIMULATOR_ENDPOINT` is +set. Those three accommodations were added across v0.5.9..v0.5.11 (`fix(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** — which is exactly what it did before v0.5.8 was +added. + +Examples skip at whole-binary granularity, since `cargo run --example` has no +name filter — so an entry must also say what the skip costs. For +`dstack_client_usage`, nothing: its other four steps (`Info`, `GetKey`, +`GetQuote`, `GetTlsKey`) are each covered by `tests/test_client.rs`, which runs +unskipped in the same leg. + **A growing skip list is the failure signal, not the fix.** The list existing at all is a small admission that the freeze has exceptions; every addition to it enlarges that admission. When an old suite fails, there are exactly two diff --git a/sdk/compat/run-compat-tests.sh b/sdk/compat/run-compat-tests.sh index 7ae32b8ee..32ebc4f70 100755 --- a/sdk/compat/run-compat-tests.sh +++ b/sdk/compat/run-compat-tests.sh @@ -21,11 +21,12 @@ # # Usage: sdk/compat/run-compat-tests.sh [...] # e.g. sdk/compat/run-compat-tests.sh v0.5.11 -# sdk/compat/run-compat-tests.sh v0.5.9 v0.5.11 # one simulator, both tags +# sdk/compat/run-compat-tests.sh v0.5.8 v0.5.11 # one simulator, both tags # -# Any tag works, but check that the pair you pick differs: several release tags -# share an SDK tree byte for byte (v0.5.10 and v0.5.11 do), and running both -# reports one result twice. +# Any tag works, but check that the pair you pick differs in the client, not +# just in the tree hash: v0.5.10 and v0.5.11 share an sdk/ tree byte for byte, +# and v0.5.9 differs from v0.5.11 only by a dependency spec in +# sdk/rust/Cargo.toml. Running either pair reports one result twice. # # CI runs one tag per matrix job. Passing several tags locally builds and starts # the simulator once and shares one Cargo target directory across them. @@ -76,72 +77,144 @@ trap cleanup EXIT INT TERM usage() { echo "usage: ${BASH_SOURCE[0]} [...]" >&2 - echo " e.g. ${BASH_SOURCE[0]} v0.5.10 v0.5.11" >&2 + echo " e.g. ${BASH_SOURCE[0]} v0.5.8 v0.5.11" >&2 } # --------------------------------------------------------------------------- # Skip lists -- see the policy at the top of this file. # --------------------------------------------------------------------------- -# All four lists are empty, and that is the result, not an oversight: the -# released suites pass in full against the 0.6.0 agent, so the freeze currently -# holds with no exceptions. Two 0.6.0 changes were expected to land here and -# did not -- but for different reasons, and only one of them is reassuring: +# The lists are keyed by tag, and that is not incidental. A skip is a claim +# about one released client, not about the frozen surface in general, and the +# two tags in the matrix disagree about the same method: `test_emit_event` +# exists under that name at both, but at v0.5.8 it asserts the call succeeds +# and at v0.5.11 it asserts HTTP 400. A single global entry would silence both +# and hide which one was a real break. +# +# What the current entries say: of the two 0.6.0 changes to the frozen surface, +# one is genuinely agreed on and the other is the single sanctioned break. # # - `GetQuote` is Intel TDX only now (CHANGELOG, Changed). The simulator # serves a TDX quote, so it answers, which is what the released suites -# assert. Genuinely covered: the suites exercise the path and agree. +# assert. Genuinely covered at both tags: they exercise the path and agree. # - `EmitEvent` always fails now (CHANGELOG, Removed: "runtime RTMR3 events -# are system-owned"). Rust, Go and JS never tested it. Python's -# `assert_emit_event_behavior` asserts HTTP 400 whenever -# DSTACK_SIMULATOR_ENDPOINT is set, because the simulator had no RTMR to -# extend at v0.5.11 either -- so it asserts 400 whether the method works or -# is a stub, and it would pass either way. Not covered. The absence of a -# skip entry here says nothing about `EmitEvent`; do not read it as -# evidence. +# are system-owned"). Caught only at v0.5.8, whose entries are below. +# Every v0.5.11 assertion about it is vacuous under a simulator endpoint: +# Go and JS never test it; `assert_emit_event_behavior` asserts 400 +# whenever DSTACK_SIMULATOR_ENDPOINT is set, so it holds whether the method +# works or is a stub; and the example swallows the error under the same +# condition. Those three accommodations were added in v0.5.9..v0.5.11 +# ("fix(ci): restore simulator test stability") to make the suite pass +# against a simulator that could not extend an RTMR. v0.5.8 predates them +# and still asserts the call succeeds, which is why it is in the matrix. # # Both point at CHANGELOG.md's `[Unreleased]` section -- 0.6.0 is not cut yet, # so there is no `## [0.6.0]` heading to cite. # -# Entries with spaces must be quoted; a bare word list splits on them. -# -# The four lists do NOT share matching semantics, so an entry cannot be moved +# The lists do NOT share matching semantics, so an entry cannot be moved # between them unchanged: # -# RUST_SKIP literal substring of the full test path (`cargo test --skip`). -# Reaches `cargo test` only -- the two `cargo run --example` -# invocations below cannot be skipped. -# GO_SKIP REGEXP over the test name (`go test -skip`), joined with `|`. -# PYTHON_SKIP literal nodeid prefix (`pytest --deselect`); pytest matches -# with `startswith`, so `::test_foo` also takes `::test_foo_bar`. -# JS_SKIP REGEXP, woven into one negative-lookahead `--testNamePattern`. +# RUST_SKIP literal substring of the full test path +# (`cargo test --skip`). Reaches `cargo test` only. +# RUST_EXAMPLE_SKIP exact example name. `cargo run --example` has no name +# filter, so the unit is the whole example binary rather +# than one call inside it -- say what the skip costs. +# GO_SKIP REGEXP over the test name (`go test -skip`), joined +# with `|`. +# PYTHON_SKIP literal nodeid prefix (`pytest --deselect`); pytest +# matches with `startswith`, so `::test_foo` also takes +# `::test_foo_bar` -- name the test exactly. +# JS_SKIP REGEXP, woven into one negative-lookahead +# `--testNamePattern`, matched as a substring of the full +# test name with `describe` prefixes included. # # The two regexp lists interpolate entries unescaped: a `(`, `+` or `.` in an # entry changes its meaning. Escape it, or the skip silently widens. +# +# Entries with spaces must be quoted; a bare word list splits on them. -# Rust: `cargo test -- --skip `, matched against the full test path. -RUST_SKIP=( -) - -# Go: `go test -skip `, matched against the test name. -GO_SKIP=( -) - -# Python: `pytest --deselect ::`. Matched as a nodeid prefix, so -# `::test_foo` also deselects `::test_foo_bar` -- name the test exactly. -PYTHON_SKIP=( -) +# The Rust examples the compat run drives, in order. They are client exercises +# too: they walk the agent end to end the way a README reader would, which is +# how the v0.5.8 break below was found. +RUST_EXAMPLES=(tappd_client_usage dstack_client_usage) + +# Sets the five lists for one tag. Called once per tag, before its suites run; +# every list is reset here, so a tag with no entries clears the previous tag's. +set_skips_for_tag() { + RUST_SKIP=() + RUST_EXAMPLE_SKIP=() + GO_SKIP=() + PYTHON_SKIP=() + JS_SKIP=() + + case "$1" in + v0.5.8) + # `EmitEvent` was removed in 0.6.0 -- CHANGELOG.md, + # `[Unreleased]` / Removed: "runtime RTMR3 events are system-owned + # and cannot be extended by apps". v0.5.8 is the newest released + # client that still expects it to work -- in Python, which asserts + # the call succeeds, and in a Rust example, which propagates the + # error with `?`. + # + # This is what a sanctioned break looks like from the outside: a + # released client called a method that no longer exists and cannot + # be edited to stop. These entries record that the break is + # deliberate; they do not make the client work. + + # The first two assert the call raises nothing ("This should not + # raise an error"); both now raise HTTPStatusError 400. + # + # The third is collateral, not a break, and it is listed because + # pytest deselects by nodeid *prefix*: an entry for + # `::test_emit_event` takes `::test_emit_event_validation` with it + # whether or not it is named here. Listing it keeps this list equal + # to what the run actually skips -- otherwise pytest reports "3 + # deselected" against two entries and the difference is invisible. + # It costs nothing: it asserts client-side rejection of an empty + # event name, raises before any request is built, and never + # contacts the agent. The equivalent client-side validation still + # runs in the Rust, Go and JS suites. + PYTHON_SKIP=( + "tests/test_client.py::test_emit_event" + "tests/test_client.py::test_sync_emit_event" + "tests/test_client.py::test_emit_event_validation" + ) + + # `dstack_client_usage` step 4 calls `emit_event(...).await?`, so + # the whole example exits non-zero. Cost of skipping the binary: + # nothing that is not still checked -- its other four steps (Info, + # GetKey, GetQuote, GetTlsKey) are each covered by + # `tests/test_client.rs`, which runs unskipped in this same leg. + RUST_EXAMPLE_SKIP=(dstack_client_usage) + ;; + v0.5.11) + # Nothing skipped: every released test and example passes against + # the 0.6.0 agent. Note what that does and does not mean -- see the + # `EmitEvent` note above for the three assertions that pass here + # without exercising anything. + ;; + esac +} -# JS: vitest has no negative name filter, so the entries are woven into one -# negative-lookahead `--testNamePattern`. They are matched as substrings of the -# full test name, `describe` prefixes included. -JS_SKIP=( -) +# Exact-match membership test: `[[ $x == $y ]]` on each element rather than a +# substring search over a joined string, so an entry `foo` cannot also skip +# `foobar`. +contains_element() { + local needle="$1" + shift + local item + for item in "$@"; do + [[ "$item" == "$needle" ]] && return 0 + done + return 1 +} run_rust_suite() { local sdk_root="$1" + local tag="$2" local skip_args=() local pattern + local example for pattern in "${RUST_SKIP[@]+"${RUST_SKIP[@]}"}"; do skip_args+=(--skip "$pattern") @@ -152,10 +225,14 @@ run_rust_suite() { cd "$sdk_root/rust" export CARGO_TARGET_DIR="$COMPAT_CARGO_TARGET_DIR" cargo test -- --show-output "${skip_args[@]+"${skip_args[@]}"}" - # The examples are client exercises too: they drive the agent end to end - # the way a README reader would. - cargo run --example tappd_client_usage - cargo run --example dstack_client_usage + for example in "${RUST_EXAMPLES[@]}"; do + if contains_element "$example" \ + "${RUST_EXAMPLE_SKIP[@]+"${RUST_EXAMPLE_SKIP[@]}"}"; then + echo "--- skipping example $example at $tag (see set_skips_for_tag)" + continue + fi + cargo run --example "$example" + done ) } @@ -242,7 +319,9 @@ run_tag() { echo "# simulator: $DSTACK_SIMULATOR_ENDPOINT" echo "############################################################" - run_rust_suite "$worktree/sdk" + set_skips_for_tag "$tag" + + run_rust_suite "$worktree/sdk" "$tag" run_go_suite "$worktree/sdk" run_python_suite "$worktree/sdk" run_js_suite "$worktree/sdk"