mirror of
https://github.com/CopilotKit/CopilotKit.git
synced 2026-09-14 16:26:20 +08:00
Jordan Ritter
8433f5b118
## The race, with evidence Three PRs merged within 34 seconds on 2026-07-26. `Showcase: Build & Push` has **no concurrency group** (deliberately), so all three ran simultaneously and raced to push the same `:latest` tags. | run | commit | PR | start → end | |---|---|---|---| | `30190815370` | `7b28934387` | #6162 | 06:18:05 → 06:29:44 | | `30190823203` | `59f275eedc` | #6161 | 06:18:21 → 06:29:44 | | `30190831480` | `db75a04837` | #6158 | 06:18:39 → **06:29:13** ← newest, finished FIRST | The newest commit finished first, so the older builds overwrote its `:latest`. Per-service job completion times — older beating newer on every shared slot: | service | newer (`db75a04837`) | older (`59f275eedc`) | older won by | |---|---|---|---| | `shell-dashboard` | 06:25:33 | 06:25:34 | +1s | | `showcase-harness` | 06:27:51 | 06:27:55 | +4s | | `shell` | 06:27:17 | 06:27:28 | +11s | | `shell-dojo` | 06:25:10 | 06:25:26 | +16s | **All three runs reported `success`.** Staging served pre-#6158 code while CI, the redeploy gate and deploy verification all looked clean. Same failure class as #6171: a success that doesn't mean what it says. ## What I verified in YAML vs took on trust Verified by reading the files / querying the API: - **Tagging** — `showcase_build.yml` pushed `:latest` **and** `:${{ github.sha }}` in one `depot/build-push-action` step, in both the `build` and `build-starters` matrices. **A per-commit sha tag already existed**; confirmed in GHCR (`showcase-shell-dashboard` has digests tagged `db75a04837…`, `59f275eedc…`, `d28384a2eb…`). - **Nothing serialized the pushes.** No concurrency group; confirmed the header comment states this is intentional. - **Deploy consumes `:latest`** — `verify-railway-image-refs.ts` is the SSOT assertion: staging is `ghcr.io/copilotkit/<repo>:latest` (mutable), **prod is `ghcr.io/copilotkit/<repo>@sha256:<digest>` (already immutably pinned)**. So this race is **staging-only**; prod was never exposed. - **`Showcase: Verify Deploy` structurally cannot catch it.** It is a health probe; it asserts no digest or commit provenance anywhere. Its #6171 per-commit concurrency key is about *which run verifies*, not *what image is running*. A stale-but-healthy service passes. - **The racing runs build DISJOINT service sets** (see below) — I pulled the actual job lists. Taken on trust: nothing material. The issue description matched the API on every point I checked. ## Why NOT a concurrency group `detect-changes` builds a **per-push, path-filtered** matrix, so concurrent runs build overlapping but **non-identical** service sets: - `7b28934387` → ag2, agno, built-in-agent, claude-sdk-python, claude-sdk-typescript, crewai-crews, langgraph-fastapi, langgraph-python, langroid, llamaindex, mastra, pydantic-ai, shell-docs, spring-ai, strands (**15**) - `db75a04837` → crewai-crews, llamaindex, shell, shell-dashboard, shell-docs, shell-dojo, showcase-harness (**7**) `cancel-in-progress: true` would have cancelled the `7b28934387` run and the ~10 services **only it builds would never have shipped** — trading a stale-image bug for a never-shipped bug. Per [GitHub's docs](https://docs.github.com/en/actions/reference/workflows-and-actions/workflow-syntax#concurrency), `cancel-in-progress: false` is no better: *"Any previously pending job or workflow in the concurrency group will be canceled"* — with three rapid merges the **middle** commit's build is dropped outright. GitHub also does not guarantee FIFO ordering of queued runs. Concurrent runs here are **not redundant**, so they must not be cancelled. **Re the #6171 interaction:** this design does **not** use `cancel-in-progress`, so no build is ever superseded-and-cancelled and the cancelled-slot notifier is never tripped by this change. That interaction stays theoretical — deliberately. ## The design Make the one shared mutable resource monotonic instead of serializing the fleet. 1. The build step pushes **only** the immutable `:<sha>` tag, plus an `org.opencontainers.image.revision` label. 2. A guard resolves the commit behind the current `:latest`, asks GitHub `compare/<theirs>...<ours>`, and advances `:latest` (registry-side retag, no pull) **unless ours is `behind`** — i.e. `:latest` already holds a descendant and moving it would roll staging back. **Fails open by design.** No `:latest` yet, unlabelled legacy image, unreachable API, diverged history → advance. A stuck `:latest` is the very failure being fixed, so it declines only on *positive proof* of regression. **Placement:** in `redeploy-staging` / `redeploy-staging-starters`, immediately before the Railway pull that consumes `:latest` — not per build slot. Those jobs already have Node (build slots do **not**, so per-slot would mean an unpinned `npx tsx` fetch on ~50 parallel runners), and deciding right before the pull makes the window as narrow as possible. The image list is the **same matrix ∩ build-success intersection** that decides what gets redeployed, so a failed build can never move a tag. Also adds `showcase_build.yml` to `showcase_validate.yml`'s trigger paths — the new test asserts against that file's live text, and without the path a PR re-adding `:latest` would never run the test that catches it. ## Tradeoffs / what stays open - **Residual sub-second TOCTOU.** GHCR has no compare-and-swap on tags, so two runs reading `:latest` simultaneously could still both advance. This narrows the window from the whole build (~10 min) to inspect→retag. Fully closing it means retiring the mutable staging tag and pinning staging to digests the way prod already is — a change to the Railway image-ref SSOT contract, not a workflow change. **Recommended follow-up.** - **The guard is inert for one build per image.** Today's `:latest` images carry no labels (verified: `showcase-shell-dashboard:latest` has no `Labels` at all), so the first post-merge build fails open and advances unconditionally — same as today. Protection starts from the second build of each image. - Failure to retag exits non-zero, redding the redeploy job and stopping the deploy. That is intended: redeploying against a tag that did not move is exactly the silent false green being fixed. ## Proof **Red/green on the live YAML.** `advance-latest-tag.test.ts` parses the real `showcase_build.yml` (extending #6171's `redeploy-guard.test.ts` pattern). Reverting the workflow to its pre-fix state: **14 failed / 23 passed**. With the fix: **37 passed**. Full `showcase/scripts` suite: **2382 passed, 73 files**. **The load-bearing predicate, verified live against the real incident commits:** ``` compare/db75a04837...59f275eedc => behind (older run arriving late → DECLINE) compare/59f275eedc...db75a04837 => ahead (newer run → advance) compare/db75a04837...db75a04837 => identical ``` **Label reading, verified against a real multi-platform registry image** — `docker buildx imagetools inspect ghcr.io/astral-sh/uv:latest` piped through `extractRevisionLabel()` returns `3010295ae7ff572de459987ad70db315a62ecd61`, matching `jq` exactly. The platform-keyed shape is handled. **Shell/jq transforms** exercised directly, including the empty-CSV edge case (empty → empty, step skipped by its `if:`). **Lint:** `actionlint` finding counts byte-identical to the pre-change baseline (no new findings; the 11 pre-existing are unrelated). `zizmor --min-severity low` with the repo config: **no findings**. **Typecheck:** both new files are in `showcase/scripts/tsconfig.json`'s include set and produce **zero** errors. Worth stating plainly: `nx run-many -t check-types` **does not reach `showcase/scripts`** — the project isn't in the nx graph and has no `check-types` target (there are 9 pre-existing type errors in sibling files, which is how I confirmed it). So the typecheck above is mine, not CI's. The *tests* are gated: `showcase_validate.yml` runs bare `pnpm exec vitest run` in `showcase/scripts`, which auto-discovers the new file. ### What I could NOT prove **I did not construct a real concurrent race on scratch branches.** Doing it faithfully needs two builds pushing the same GHCR repo with controlled finish ordering, which means merging to `main` — the only branch the build workflow triggers on. No run IDs for a live race demonstration; I am not implying one. Unproven until this runs on main: that `docker buildx imagetools create` retags cleanly under the runner's GHCR credentials, and that `npx tsx` behaves in the redeploy jobs (it is already the established invocation there — `redeploy-env.ts` — so this is low risk, not zero). ## Normal single-merge builds are unaffected No concurrency group is added, so nothing queues or cancels. A lone merge finds `:latest` at its own parent → `ahead` → advances, exactly as before. Cost is one `imagetools inspect` + one `gh api` + one registry-side retag per built service, in a job that already exists — no extra job, no extra checkout, no change to build parallelism. --- Branched from `db75a04837`; #6156/#6159 landed after, so this will need main merged in before it goes green. Probable conflict with the concurrent `git lfs pull` work in `showcase_validate.yml` — my edit there is only the top-level `on: paths:` list, so it should merge cleanly, but flagging it.