From a9114a8317aa8b8f917b5010b5ecdedd111e9c68 Mon Sep 17 00:00:00 2001 From: Jordan Ritter Date: Tue, 16 Jun 2026 20:07:18 -0700 Subject: [PATCH] fix(showcase/harness): scope --isolate rebuild to target slug only (A21) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause ---------- Both the shell `cmd_up` (showcase/bin/showcase line 98) and the harness auto-start path (showcase/harness/src/cli/lifecycle.ts up() line 268) emit `docker compose up -d --build` with NO positional service filter after `up`. Under `docker compose` semantics, an unfiltered `--build` rebuilds EVERY service in every active profile — for `--isolate` runs that means the target slug PLUS aimock + pocketbase + dashboard + harness-pool-worker all rebuild on every isolated test invocation. Concurrent `--isolate` runs (one per `SHOWCASE_ISO_SLOT=`) contend on the shared BuildKit instance, serializing what should be parallel work. This is the BuildKit-contention stall that has been blocking csdkts (A20) when it runs alongside other concurrent isolated cells: a small slug rebuild waits on every other slot's full-stack rebuild. Fix --- Scope `--build` to the targeted slug(s) by passing them as positional service args AFTER `up`. Compose then rebuilds ONLY those services and falls through to cached images for infra services. First-time bootstrap still works because `--build` only FORCES a rebuild of services with an existing image — missing images are built automatically by compose. Two call sites: - showcase/bin/showcase (user-facing `showcase up ...`) - showcase/harness/src/cli/lifecycle.ts (npx tsx test path, the one actually triggered under `--isolate`) When no slugs are provided (infra-only bring-up), the blanket `--build` is preserved so first-time infra bootstrap rebuilds whatever is missing. `--rebuild` regression: the `--rebuild` flag goes through `rebuild()`, not `up()`, and that path is unchanged — it still force-rebuilds and force-recreates every targeted slug as before. Red→Green proof --------------- 3 new vitest cases in showcase/harness/src/cli/lifecycle.test.ts assert the compose argv shape: - up(["langgraph-python"]): slug positional after `up` (was: missing) - up([]): no slug positional (infra-only blanket) - up(["a","b"]): both slugs as positionals after `up` Pre-fix: 2 failed | 4 passed (6) Post-fix: 0 failed | 6 passed (6) The compose-argv assertion is the deterministic proof of behavior change. Wall-clock timing improvement on concurrent --isolate runs follows mechanically from the compose semantics: with the fix, slot N's BuildKit work no longer blocks slot M's because they only rebuild their respective slug images, not the shared infra stack. (cherry picked from commit 8a47dbb0d2a2fe72e21c09a824485e1bb5afc463) --- showcase/bin/showcase | 23 +++-- showcase/harness/src/cli/lifecycle.test.ts | 101 ++++++++++++++++++++- showcase/harness/src/cli/lifecycle.ts | 22 ++++- 3 files changed, 136 insertions(+), 10 deletions(-) diff --git a/showcase/bin/showcase b/showcase/bin/showcase index 02e2fdcaeb..fd44000d6f 100755 --- a/showcase/bin/showcase +++ b/showcase/bin/showcase @@ -67,12 +67,14 @@ cmd_up() { local pool_count="${HARNESS_POOL_COUNT:-1}" export HARNESS_POOL_COUNT="$pool_count" - # Split args: bare slugs → `--profile `; flags (-*) → pass through. - # `--dev` is intercepted (not passed to compose): it layers the dev overlay - # (docker-compose.dev.yml) which bind-mounts integration source + overrides - # the run command to a hot-reload entrypoint — fast iteration without an - # image rebuild. The built-image mode (no --dev) stays the faithful default. + # Split args: bare slugs → `--profile ` + build target; flags (-*) → + # pass through. `--dev` is intercepted (not passed to compose): it layers + # the dev overlay (docker-compose.dev.yml) which bind-mounts integration + # source + overrides the run command to a hot-reload entrypoint — fast + # iteration without an image rebuild. The built-image mode (no --dev) stays + # the faithful default. local profile_args=() + local build_targets=() local passthrough_args=() local dev_mode=false local arg @@ -83,6 +85,7 @@ cmd_up() { passthrough_args+=("$arg") else profile_args+=(--profile "$arg") + build_targets+=("$arg") fi done @@ -93,10 +96,18 @@ cmd_up() { fi info "Launching fleet topology: control-plane + ${pool_count} worker(s) + PocketBase + demos + aimock (HARNESS_POOL_COUNT=${pool_count})" + # Scope `--build` to the targeted slug(s) when provided. Without a positional + # service filter, `up -d --build` rebuilds EVERY service in every active + # profile (infra + slug profiles), which serializes BuildKit and stalls + # concurrent `--isolate` runs (issue #5495, A21). When no slugs are given + # (infra-only bring-up), the blanket `--build` is preserved so first-time + # bootstrap still builds whatever infra services are missing. + # # bash 3.2 (macOS default) errors on `${arr[@]}` for an EMPTY array under # `set -u`; the `${arr[@]+...}` guard expands to nothing when unset/empty. $compose_cmd --profile infra ${profile_args[@]+"${profile_args[@]}"} up -d --build \ - --scale harness-pool-worker="$pool_count" ${passthrough_args[@]+"${passthrough_args[@]}"} + --scale harness-pool-worker="$pool_count" ${passthrough_args[@]+"${passthrough_args[@]}"} \ + ${build_targets[@]+"${build_targets[@]}"} } cmd_down() { diff --git a/showcase/harness/src/cli/lifecycle.test.ts b/showcase/harness/src/cli/lifecycle.test.ts index ada4a1d234..a599eaaf1c 100644 --- a/showcase/harness/src/cli/lifecycle.test.ts +++ b/showcase/harness/src/cli/lifecycle.test.ts @@ -1,4 +1,4 @@ -import { describe, it, expect, vi, beforeEach } from "vitest"; +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; // --------------------------------------------------------------------------- // Hoisted mocks — vi.hoisted() runs before vi.mock factories so the spies @@ -19,15 +19,23 @@ vi.mock("node:child_process", () => ({ vi.mock("node:fs", () => { // stageSharedModules() short-circuits when INTEGRATIONS_DIR is absent. + // readFileSync is consulted by loadPortsFile() to map slugs → host ports + // for health checks; provide stub ports for any slug the up() tests use. const api = { existsSync: (...args: unknown[]) => existsSyncMock(...args), readdirSync: vi.fn(() => []), - readFileSync: vi.fn(() => "{}"), + readFileSync: vi.fn(() => + JSON.stringify({ + "langgraph-python": 10001, + a: 10002, + b: 10003, + }), + ), }; return { default: api, ...api }; }); -import { rebuild } from "./lifecycle.js"; +import { rebuild, up } from "./lifecycle.js"; /** Pull out the compose argv (after the `-f ` prefix) for each call. */ function composeCalls(): string[][] { @@ -92,3 +100,90 @@ describe("rebuild() — targeted slugs", () => { expect(recreateCall).toContain("b"); }); }); + +describe("up() — scoped --build (A21)", () => { + // Stub healthCheck transitively by making fetch unreachable but compose succeed. + // up() throws on unhealthy services, so we mock the health-check path via + // network fetch failing — but the IMPORTANT assertion is the compose argv + // captured by composeCalls() BEFORE health checks run. + // To avoid up() throwing before we can assert, mock fetch globally to resolve OK. + const _origFetch = globalThis.fetch; + beforeEach(() => { + globalThis.fetch = vi.fn(() => + Promise.resolve({ ok: true, status: 200 } as Response), + ) as unknown as typeof fetch; + }); + afterEach(() => { + globalThis.fetch = _origFetch; + }); + + /** + * The compose argv shape we want: + * [--profile infra --profile ... up -d --build ...] + * + * Without slug positionals AFTER `--build`, compose rebuilds EVERY service + * in every active profile (infra + slug profiles). With slug positionals, + * compose rebuilds ONLY the named services and uses cached images for + * everything else — that is the A21 fix. + * + * `--profile ` arguments do NOT scope the build target on their own; + * docker compose only treats positional args after `up` as the service + * filter for `--build`. So the test asserts the slug appears AFTER the `up` + * keyword (as a positional), not merely anywhere in the argv. + */ + function positionalsAfterUp(argv: string[]): string[] { + const upIdx = argv.indexOf("up"); + if (upIdx < 0) return []; + const out: string[] = []; + for (let i = upIdx + 1; i < argv.length; i++) { + const tok = argv[i]; + // Skip flags and their values. `-d`/`--build` take no value; `--progress` + // takes a value. Keep it simple — the only flags we emit here are + // `-d`, `--build`, `--progress plain`. + if (tok === "-d" || tok === "--build") continue; + if (tok === "--progress") { + i++; // skip the value + continue; + } + if (tok.startsWith("-")) continue; + out.push(tok); + } + return out; + } + + it("scopes --build to the targeted slug (does NOT rebuild infra services)", async () => { + await up(["langgraph-python"]); + + const upCall = composeCalls().find( + (argv) => argv.includes("up") && argv.includes("--build"), + ); + expect(upCall).toBeDefined(); + // The slug must appear as a positional AFTER `up` so compose treats it as + // the build target filter — not merely as a `--profile ` argument + // (profiles don't scope --build). + expect(positionalsAfterUp(upCall!)).toEqual(["langgraph-python"]); + }); + + it("with no slugs (infra-only), --build remains blanket (first-time bootstrap)", async () => { + await up([]); + + const upCall = composeCalls().find( + (argv) => argv.includes("up") && argv.includes("--build"), + ); + expect(upCall).toBeDefined(); + // No slug positional after `up`: compose builds whatever is missing in the + // infra profile (cached images skipped, fresh ones built). This preserves + // first-time bootstrap behaviour when no infra images exist yet. + expect(positionalsAfterUp(upCall!)).toEqual([]); + }); + + it("multi-slug: scopes --build to ALL targeted slugs", async () => { + await up(["a", "b"]); + + const upCall = composeCalls().find( + (argv) => argv.includes("up") && argv.includes("--build"), + ); + expect(upCall).toBeDefined(); + expect(positionalsAfterUp(upCall!).sort()).toEqual(["a", "b"]); + }); +}); diff --git a/showcase/harness/src/cli/lifecycle.ts b/showcase/harness/src/cli/lifecycle.ts index 6402d4e085..7dcf82733e 100644 --- a/showcase/harness/src/cli/lifecycle.ts +++ b/showcase/harness/src/cli/lifecycle.ts @@ -265,7 +265,27 @@ export async function up( } const verboseFlag = opts?.verbose ? ["--progress", "plain"] : []; - const args = [...profileArgs, "up", "-d", "--build", ...verboseFlag]; + // Scope `--build` to the targeted slug(s) when provided. Without a + // positional service filter, `up -d --build` rebuilds EVERY service in + // every active profile (infra + slug profiles), which serializes BuildKit + // and stalls concurrent `--isolate` runs — slot N's full-stack rebuild + // contends with slot M's rebuild on the same daemon (issue #5495, A21). + // + // With positional slugs after `up`, compose only rebuilds the named + // services; everything else uses cached images and falls through to plain + // start. First-time bootstrap (no infra images) still works because + // compose builds missing images automatically — `--build` only FORCES a + // rebuild of services that already have an image. For a forced full + // rebuild, callers should use `rebuild()` (the `--rebuild` flag path). + const buildTargets = slugs.length > 0 ? slugs : []; + const args = [ + ...profileArgs, + "up", + "-d", + "--build", + ...verboseFlag, + ...buildTargets, + ]; log.info("starting services", { slugs: slugs.length ? slugs : ["infra"] }); compose(...args);