mirror of
https://github.com/CopilotKit/CopilotKit.git
synced 2026-09-14 16:26:20 +08:00
fix(showcase/harness): scope --isolate rebuild to target slug only (A21)
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=<N>`) 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 <slug>...`) - 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)
This commit is contained in:
+17
-6
@@ -67,12 +67,14 @@ cmd_up() {
|
||||
local pool_count="${HARNESS_POOL_COUNT:-1}"
|
||||
export HARNESS_POOL_COUNT="$pool_count"
|
||||
|
||||
# Split args: bare slugs → `--profile <slug>`; 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 <slug>` + 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() {
|
||||
|
||||
@@ -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 <file>` 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 <slug>... up -d --build <slug>...]
|
||||
*
|
||||
* 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 <name>` 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 <slug>` 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"]);
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in New Issue
Block a user