From 3776e44144c832361ce93ae5feb19b99a2ab8220 Mon Sep 17 00:00:00 2001 From: JUN Date: Thu, 17 Sep 2026 16:53:38 +0900 Subject: [PATCH] ci(windows): the batch leg is one test run, so it must not queue against itself dev went red on windows 4/9 of dispatch 35191675127, and the cause is mine. #4851 replaced the Windows leg's single `bun test` invocation with 25 sequential batch invocations. tests/preload.ts takes a user-scoped machine-local lock, and line 101 makes it win32-only, so Linux and macOS have run this same batch runner unqueued for a long time while Windows had never batched at all. The lock joins workers that share a run ID and blocks anything with a different one, so each batch now queued behind the previous batch's stragglers: [test] bare Bun worker 2548 is waiting for test run pid 7272 to release the user lock. ##[warning]Bun test process timed out after 480s in shard 4/9 batch 6/25. ##[error]Shard 4/9 batch 6: every file passed alone The first file of that batch then ran in 11.25s during attribution. The eight minutes were queue, not work, which is why a larger bound would only have hidden it for longer. scripts/test-run-lock.ts already names this case in its own timeout message: set OCX_TEST_NO_QUEUE=1 only when overlapping test runners are intentional. A dedicated CI job running its own batches back to back is one logical test run, so the batch step now sets it. What the queue protects against - an unrelated second suite stacking load on a developer's machine - cannot happen in that job, and each batch still creates its isolated home and arms the live-home and service-manager guards before it would have reached the lock. The lock's own unit tests now pin an explicitly queued environment, so the workflow bypass can never silently turn their acquisitions into no-ops. That was the real risk in disabling a guard by environment variable. The 480-second batch bound and the 30-minute job ceiling are unchanged. No local suite, focused test, typecheck, build, or install was run. --- .github/workflows/ci.yml | 8 ++++++ structure/ops/docs-and-release.md | 14 ++++++---- structure/overview.md | 4 ++- .../ci-workflows/ci-crash-disposition.test.ts | 9 ++++++- tests/ci-workflows/ci-workflows.test.ts | 4 +++ tests/ci-workflows/test-runner.test.ts | 27 +++++++++++++++---- 6 files changed, 54 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 5fc5188498..a4684d392d 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -833,12 +833,20 @@ jobs: # only 0.106-0.168s of wrapper overhead per process: at most ~2.1s against the margin. # A timeout or crash still fixes the shard red before singleton attribution. scope=all # preserves the full Windows suite; Linux keeps its correctly sized 12-file/120s defaults. + # + # The preload's Windows-only user lock serializes separate test runners on one machine. + # These batches are already one dedicated job's sequential pieces, so treating each Bun + # process as a competing runner can queue batch N+1 behind a straggler from batch N until + # this step's 480s process bound fires without running a test. Disable that outer queue for + # this step only. Every process still creates its own isolated home and arms the live-home + # and service-manager guard before the lock boundary. shell: bash env: TEST_SHARD: ${{ matrix.shard }}/9 BUN_TEST_FILE_SCOPE: all BUN_TEST_BATCH_SIZE: "6" BUN_TEST_BATCH_TIMEOUT_SECONDS: "480" + OCX_TEST_NO_QUEUE: "1" run: bash scripts/ci/run-bun-test-batches.sh "$TEST_SHARD" - name: CLI help smoke diff --git a/structure/ops/docs-and-release.md b/structure/ops/docs-and-release.md index 1bb34e4dc4..65f519d38b 100644 --- a/structure/ops/docs-and-release.md +++ b/structure/ops/docs-and-release.md @@ -89,7 +89,7 @@ Those controls still have no owner, so there is no image-publish workflow or off | Workflow | Trigger | Purpose | | --- | --- | --- | -| `.github/workflows/ci.yml` | Any `pull_request`; runtime/package `push` to `main`/`preview`/`dev`; manual dispatch | Linux runs four suite shards plus `gates`; macOS runs two shards. Windows runs nine shards only on manual dispatch with `lane=all` (or empty), not on push events. Linux runs at-most-12-file processes with a 120-second process bound; Windows uses measured six-file/480-second processes and all-file scope so its full-suite contract is unchanged. No lane retries: a test failure, a process timeout and a Bun runtime crash each fail their job on the first occurrence. Aggregate `ci` is event-aware — it derives which jobs this event requested and requires `success` from each of them and `skipped` from the rest, and on a `lane=all` dispatch it reads the run's own job list and requires nine concrete successful `windows N/9` results. `npm-global-smoke` remains GitHub-hosted because it mutates the global package prefix. | +| `.github/workflows/ci.yml` | Any `pull_request`; runtime/package `push` to `main`/`preview`/`dev`; manual dispatch | Linux runs four suite shards plus `gates`; macOS runs two shards. Windows runs nine shards only on manual dispatch with `lane=all` (or empty), not on push events. Linux runs at-most-12-file processes with a 120-second process bound; Windows uses measured six-file/480-second processes and all-file scope so its full-suite contract is unchanged. The dedicated Windows batch step sets `OCX_TEST_NO_QUEUE=1` because its sequential processes are one logical runner; each process still creates an isolated home and arms the test guards before the lock boundary. No lane retries: a test failure, a process timeout and a Bun runtime crash each fail their job on the first occurrence. Aggregate `ci` is event-aware — it derives which jobs this event requested and requires `success` from each of them and `skipped` from the rest, and on a `lane=all` dispatch it reads the run's own job list and requires nine concrete successful `windows N/9` results. `npm-global-smoke` remains GitHub-hosted because it mutates the global package prefix. | | `.github/workflows/dev-version-bump.yml` | Manual dispatch with an intended version and `pre-move` or `repair` mode | Opens the reviewed pull request that moves `dev` past a release target. The default `pre-move` mode runs before promotion and publication; explicit `repair` mode retains the post-publish catch-up path. It is neither called by `release.yml` nor triggered by publication. | | `.github/workflows/release.yml` | Manual dispatch only | npm publish/dry-run workflow. It requires successful Cross-platform CI for the exact `GITHUB_SHA`, requires `dev` to outrank the target, then checks the target against the freshly fetched global tag set before publish or dry-run. | | `.github/workflows/deploy-docs.yml` | `push` to `main` touching `docs-site/**` or the workflow, or manual dispatch | Build and publish the Astro/Starlight docs site to GitHub Pages. | @@ -299,10 +299,14 @@ aggregate green `ci` check on those events legitimately includes a deliberate Wi Nothing in the workflow retries. Linux and Windows use `scripts/ci/run-bun-test-batches.sh`, but each lane owns its measured process shape: Linux keeps the default twelve files and 120 seconds; Windows uses six files and 480 seconds. Windows selects all test families, while Linux leaves the -storage-policy and api-usage families to its dedicated jobs. A test failure, a process timeout -and a Bun runtime crash each fail their job on the first occurrence; the batch runner still sweeps a -crashed or timed-out batch one file per process, but only to attribute a failure the shard has -already taken. The aggregate `ci` gate derives, from the event and the `changes` outputs, which +storage-policy and api-usage families to its dedicated jobs. The Windows step disables the +user-scoped test-run queue with `OCX_TEST_NO_QUEUE=1`: the batches already run sequentially in one +dedicated job, and queueing a new batch behind a surviving process from the preceding batch spends +the process timeout without executing tests. The per-process home isolation and live-home/service +manager guards remain active because the preload installs them before the lock boundary. A test +failure, a process timeout and a Bun runtime crash each fail their job on the first occurrence; the +batch runner still sweeps a crashed or timed-out batch one file per process, but only to attribute a +failure the shard has already taken. The aggregate `ci` gate derives, from the event and the `changes` outputs, which jobs this run actually requested, then requires `success` from every one of them and `skipped` from every job the event did not request — so a job that was requested and never started can no longer report as a deliberate skip. On a `lane=all` dispatch the gate additionally reads the diff --git a/structure/overview.md b/structure/overview.md index fb5d97ba2e..3b076ea986 100644 --- a/structure/overview.md +++ b/structure/overview.md @@ -110,7 +110,9 @@ CI enumerates that domain layout through `scripts/ci/run-bun-test-batches.sh`. I scope and 12-file/120-second process shape leave the dedicated Linux storage-policy and api-usage jobs out of the general shards. The manual Windows matrix selects all-file scope and overrides the process shape to six files and 480 seconds, so batching changes process size without changing the -platform suite's file set. The workflow contract and process bounds live in +platform suite's file set. Its dedicated batch step sets `OCX_TEST_NO_QUEUE=1`: those sequential +processes are one logical runner, while each process still installs its own isolated home and test +guards. The workflow contract and process bounds live in [`ops/docs-and-release.md`](ops/docs-and-release.md#cross-platform-ci). Two invariants are stated here without a binding, and `grace.unboundInvariants` in diff --git a/tests/ci-workflows/ci-crash-disposition.test.ts b/tests/ci-workflows/ci-crash-disposition.test.ts index b87e4eeefb..13085c8eae 100644 --- a/tests/ci-workflows/ci-crash-disposition.test.ts +++ b/tests/ci-workflows/ci-crash-disposition.test.ts @@ -138,7 +138,7 @@ const FAKE_BUN = [ 'for arg in "$@"; do', ' case "$arg" in *.test.ts) files=$((files + 1)) ;; esac', "done", - "printf '%s|%s\\n' \"$files\" \"$*\" >> \"$FIXTURE_CALLS\"", + "printf '%s|%s|%s\\n' \"$files\" \"${OCX_TEST_NO_QUEUE:-}\" \"$*\" >> \"$FIXTURE_CALLS\"", 'if [ "$files" -le 1 ]; then', " exit 0", "fi", @@ -185,6 +185,7 @@ function runBatches( CI: "true", BUN_TEST_BATCH_SIZE: "3", BUN_TEST_FILE_SCOPE: fileScope, + OCX_TEST_NO_QUEUE: "1", OPENCODEX_BUN_PATH: join(binDirectory, "bun"), FIXTURE_MODE: mode, FIXTURE_CALLS: calls, @@ -207,6 +208,8 @@ const batchCalls = (result: RunnerResult): string[] => result.calls.filter(call => !call.startsWith("1|")); const singletonCalls = (result: RunnerResult): string[] => result.calls.filter(call => call.startsWith("1|")); +const noQueueFlags = (result: RunnerResult): string[] => + result.calls.map(call => call.split("|")[1] ?? ""); describe.skipIf(process.platform !== "linux")("the Linux batch runner, executed", () => { test("a clean run is green and runs each batch exactly once", () => { @@ -214,6 +217,7 @@ describe.skipIf(process.platform !== "linux")("the Linux batch runner, executed" expect(`status:${run.status}`, run.output).toBe("status:0"); expect(batchCalls(run)).toHaveLength(2); expect(singletonCalls(run)).toEqual([]); + expect(noQueueFlags(run)).toEqual(["1", "1"]); expect(run.calls.some(call => call.includes(DEDICATED_FILE))).toBe(false); }, SPAWN_BUDGET_MS); @@ -249,6 +253,9 @@ describe.skipIf(process.platform !== "linux")("the Linux batch runner, executed" // This is the exact case the deleted "a timeout may still recover" contract pinned green. expect(`status:${run.status}`, run.output).toBe("status:124"); expect(singletonCalls(run)).toHaveLength(FIRST_BATCH.length); + // The bypass reaches both the primary process and every attribution process; + // otherwise a survivor from the failed process can queue the diagnostic sweep too. + expect(new Set(noQueueFlags(run))).toEqual(new Set(["1"])); expect(run.output).toContain("every file passed alone"); expect(run.output).not.toContain("continuing"); }, SPAWN_BUDGET_MS); diff --git a/tests/ci-workflows/ci-workflows.test.ts b/tests/ci-workflows/ci-workflows.test.ts index 0eb242705f..ca137e17a1 100644 --- a/tests/ci-workflows/ci-workflows.test.ts +++ b/tests/ci-workflows/ci-workflows.test.ts @@ -209,6 +209,10 @@ describe("GitHub Actions hardening", () => { expect(windowsTest?.env?.BUN_TEST_FILE_SCOPE).toBe("all"); expect(windowsTest?.env?.BUN_TEST_BATCH_SIZE).toBe("6"); expect(windowsTest?.env?.BUN_TEST_BATCH_TIMEOUT_SECONDS).toBe("480"); + // The 25 sequential Bun processes are one logical runner. On Windows the preload's + // machine-local queue can otherwise hold batch N+1 behind a straggler from batch N + // until the process bound fires without executing a test. + expect(windowsTest?.env?.OCX_TEST_NO_QUEUE).toBe("1"); expect(windowsTest?.run).toBe('bash scripts/ci/run-bun-test-batches.sh "$TEST_SHARD"'); expect(ci.jobs?.["platform-windows"]?.name).toBe(`windows \${{ matrix.shard }}/${windowsShards.length}`); expect(workflow).toContain(`shards=${windowsShards.length}`); diff --git a/tests/ci-workflows/test-runner.test.ts b/tests/ci-workflows/test-runner.test.ts index eaa711fa81..70dbaa68e4 100644 --- a/tests/ci-workflows/test-runner.test.ts +++ b/tests/ci-workflows/test-runner.test.ts @@ -773,6 +773,11 @@ describe("bun test argv", () => { }); describe("bun test user lock", () => { + // Lock behavior must not inherit a workflow-level opt-out. The Windows batch leg + // intentionally sets OCX_TEST_NO_QUEUE for its outer processes, while these unit + // cases exercise the queued implementation itself. + const queuedTestEnv: NodeJS.ProcessEnv = {}; + test("distinct POSIX users receive distinct temp-runtime locks", () => { const common = { env: {}, tempDir: "/tmp", hostName: "builder-1", platform: "linux" as const }; const alice = resolveDefaultTestRunLockPath({ @@ -1163,8 +1168,9 @@ describe("bun test user lock", () => { const root = mkdtempSync(join(tmpdir(), "opencodex-test-lock-")); const lockPath = join(root, "suite.lock"); try { - const owner = await acquireTestRunLock({ runId: "suite-a", lockPath, pollMs: 5, maxWaitMs: 50 }); - const sibling = await acquireTestRunLock({ runId: "suite-a", lockPath, pollMs: 5, maxWaitMs: 50 }); + const options = { runId: "suite-a", lockPath, pollMs: 5, maxWaitMs: 50, env: queuedTestEnv }; + const owner = await acquireTestRunLock(options); + const sibling = await acquireTestRunLock(options); expect(owner.acquired).toBe(true); expect(sibling.acquired).toBe(false); sibling.release(); @@ -1180,12 +1186,15 @@ describe("bun test user lock", () => { const root = mkdtempSync(join(tmpdir(), "opencodex-test-lock-")); const lockPath = join(root, "suite.lock"); try { - const owner = await acquireTestRunLock({ runId: "wrapped", lockPath, pollMs: 5, maxWaitMs: 50 }); + const owner = await acquireTestRunLock({ + runId: "wrapped", lockPath, pollMs: 5, maxWaitMs: 50, env: queuedTestEnv, + }); expect(owner.owner).not.toBeNull(); const sibling = await acquireTestRunLock({ runId: "wrapped", lockPath, joinExistingOwnerToken: owner.owner!.token, + env: queuedTestEnv, }); expect(sibling.acquired).toBe(false); const wrongToken = owner.owner!.token === "57f44b0e-b750-4bd2-b23d-4a035e75da18" @@ -1196,6 +1205,7 @@ describe("bun test user lock", () => { runId: "wrapped", lockPath, joinExistingOwnerToken: wrongToken, + env: queuedTestEnv, })).rejects.toThrow("refusing to create or reclaim"); owner.release(); @@ -1204,6 +1214,7 @@ describe("bun test user lock", () => { runId: "wrapped", lockPath, joinExistingOwnerToken: owner.owner!.token, + env: queuedTestEnv, })).rejects.toThrow("refusing to create or reclaim"); expect(existsSync(lockPath)).toBe(false); } finally { @@ -1221,8 +1232,11 @@ describe("bun test user lock", () => { lockPath, pollMs: 5, maxWaitMs: 50, + env: queuedTestEnv, + }); + const replacement = await acquireTestRunLock({ + runId: "stale", lockPath, pollMs: 5, maxWaitMs: 50, env: queuedTestEnv, }); - const replacement = await acquireTestRunLock({ runId: "stale", lockPath, pollMs: 5, maxWaitMs: 50 }); expect(replacement.acquired).toBe(true); stale.release(); expect(existsSync(lockPath)).toBe(true); @@ -1237,13 +1251,16 @@ describe("bun test user lock", () => { const root = mkdtempSync(join(tmpdir(), "opencodex-test-lock-")); const lockPath = join(root, "suite.lock"); try { - const owner = await acquireTestRunLock({ runId: "live", lockPath, pollMs: 5, maxWaitMs: 50 }); + const owner = await acquireTestRunLock({ + runId: "live", lockPath, pollMs: 5, maxWaitMs: 50, env: queuedTestEnv, + }); let waits = 0; await expect(acquireTestRunLock({ runId: "blocked", lockPath, pollMs: 5, maxWaitMs: 20, + env: queuedTestEnv, onWait: () => { waits += 1; }, })).rejects.toThrow("timed out"); expect(waits).toBe(1);