diff --git a/docs/testing.md b/docs/testing.md index 4095c42e61..a02ac87b38 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -493,10 +493,17 @@ The standing rule is unchanged and now has two precedents: an overrun with NO su regression to fix in the lane, not a budget to raise. The optimization to reach for first is still per-file cost — each file re-pays module import and provisions its own disposable PostgreSQL database, so consolidating files is worth more than touching any assertion. + +FNXC:PipelineSmoke 2026-08-25-06:55: +Re-baselined 150s -> 175s, again for attributable growth: a 7th file (the dedicated Code Review +remediation drive) plus S05 extended to `builtin:coding-ideas-v2`, one of the longest scenarios in +the matrix. Five consecutive runs measured 140.1s, 143.8s, 146.7s, 147.0s and 148.4s — green against +the old 150s ceiling, but with under 2s of headroom, which is a flake waiting to happen rather than +a passing lane. Third precedent for the same rule: growth must be nameable, or it is a regression. --> -The declared budget is **150 seconds**, rounded up from a measured 124,950ms full-matrix run -(6 files, 87 tests) after `builtin:coding-ideas-v2` was added to 17 scenarios and multi-repository -workspace coverage was introduced. The wrapper enforces it for every run; an overrun is a result +The declared budget is **175 seconds**, rounded up from a measured 148,434ms slowest full-matrix +run (7 files, 90 tests) after the Code Review remediation drive was added and S05 was extended to +`builtin:coding-ideas-v2`. The wrapper enforces it for every run; an overrun is a result to investigate, never a reason to hide a regression behind unbounded timeouts. Use `--repeat=10` for the reproducibility proof, `--json` for machine output, and `--budget-ms=` only for loud diagnostic measurement. The normalized report lists diff --git a/packages/engine/src/__tests__/pipeline-smoke/_pipeline-harness.ts b/packages/engine/src/__tests__/pipeline-smoke/_pipeline-harness.ts index d2ec3755e7..00fffb267d 100644 --- a/packages/engine/src/__tests__/pipeline-smoke/_pipeline-harness.ts +++ b/packages/engine/src/__tests__/pipeline-smoke/_pipeline-harness.ts @@ -20,7 +20,7 @@ import { import type { SharedPgTaskStoreHarness } from "../../../../core/src/__test-utils__/pg-test-harness.js"; import { ProjectEngine } from "../../project-engine.js"; import { TaskExecutor } from "../../executor.js"; -import { activeSessionRegistry } from "../../agents/active-session-registry.js"; +import { activeSessionRegistry, executingTaskLock } from "../../agents/active-session-registry.js"; import { resetMockScripts } from "../../providers/mock-provider.js"; import { WorktreePool } from "../../worktree/worktree-pool.js"; import { acquireTaskWorktree, type AcquireTaskWorktreeResult } from "../../worktree/worktree-acquisition.js"; @@ -99,6 +99,18 @@ let fixtureEnvironmentTail: Promise = Promise.resolve(); type FixtureEnvironmentRelease = () => void; +/* +FNXC:PipelineSmoke 2026-08-25-06:40: +Task ids are unique per PROCESS, not per harness. The counter used to live on the instance and reset +with it, so every test's first task was `FN-182-S05-1` — and the engine's process-wide state +(`executingTaskLock`, `activeSessionRegistry.pathsForTask`, worktree registrations) is keyed by task +id. A straggler from the previous test therefore answered for the NEXT test's identically-named +task, handing it a worktree under the previous fixture. The executor correctly refused that path +(`outside_worktrees_dir`) and failed a task that had done nothing wrong. Colliding ids also defeat +the teardown drain, which can only wait for ids it can tell apart. +*/ +let pipelineTaskSerial = 0; + async function acquireFixtureGlobalHome(fixture: PipelineGitFixture): Promise { let releaseQueue: (() => void) | undefined; const previous = fixtureEnvironmentTail; @@ -173,10 +185,12 @@ export class PipelineSmokeHarness { readonly centralCore: CentralCore; private executor: PipelineGraphExecutor | undefined; private authoritativeSeamsObserved = false; - private serial = 0; + /* Serial lives on the module, not the instance — see `pipelineTaskSerial`. */ private manualHoldTaskIds = new Set(); private readonly promptRevisions = new Map(); private readonly mockScriptStates = new Map(); + /** Every task this harness created, so teardown can wait for their execution to actually stop. */ + private readonly createdTaskIds = new Set(); /** Newest non-empty branch per task; a workspace row only gains one at acquisition. */ private readonly scriptedBranches = new Map(); @@ -362,6 +376,7 @@ export class PipelineSmokeHarness { consults. Both are process-global, so the damage lands on whichever file runs next. */ await this.engine.stop(); + await this.drainInFlightExecution(); activeSessionRegistry.clear(); this.mockScriptStates.clear(); this.scriptedBranches.clear(); @@ -375,6 +390,34 @@ export class PipelineSmokeHarness { } } + /* + FNXC:PipelineSmoke 2026-08-25-06:05: + WAIT for in-flight execution to stop before tearing the fixture down; do not merely forget it. + `ProjectEngine.stop()` clears timers but does not await a task execution already in progress, and + teardown then called `activeSessionRegistry.clear()`, which HIDES a live session rather than + ending it. The surviving execution keeps a reference to THIS fixture's directory, so when the next + test in the file installs a fresh fixture the straggler creates a worktree under the OLD one and + writes that path onto the new test's task row. The next executor correctly refuses it + (`outside_worktrees_dir`), retries, exhausts its budget, and fails a task that never did anything + wrong — a failure that reproduced only under full-lane timing, which is what made it look flaky. + `executingTaskLock` is the process-wide truth for "this task is inside execute()", so drain it. + The wait is bounded and throws on expiry rather than proceeding: a straggler that outlives the + budget is a real defect, and a silent continue would restore exactly the leak this removes. + */ + private async drainInFlightExecution(): Promise { + const deadline = Date.now() + 30_000; + for (;;) { + const busy = [...this.createdTaskIds].filter( + (taskId) => executingTaskLock.has(taskId) || activeSessionRegistry.pathsForTask(taskId).length > 0, + ); + if (busy.length === 0) return; + if (Date.now() > deadline) { + throw new Error(`Pipeline smoke teardown timed out waiting for in-flight execution: ${busy.join(", ")}`); + } + await new Promise((resolve) => setTimeout(resolve, 25)); + } + } + async freshTask(taskId: string): Promise { (this.store as TaskStore & { taskCache?: Map }).taskCache?.delete(taskId); const task = await this.store.getTask(taskId); @@ -430,7 +473,8 @@ export class PipelineSmokeHarness { if (!ir) throw new Error(`Missing workflow ${workflowId}`); const creation = resolveCreationColumn(ir)?.id; const { holdColumn, wipColumn, reviewColumn, completeColumn } = resolvedColumns(ir); - const taskId = `FN-182-${options.idPrefix ?? "SMOKE"}-${++this.serial}`.replace(/[^A-Za-z0-9-]/g, "-"); + const taskId = `FN-182-${options.idPrefix ?? "SMOKE"}-${++pipelineTaskSerial}`.replace(/[^A-Za-z0-9-]/g, "-"); + this.createdTaskIds.add(taskId); const column = options.initialColumn === "creation" ? creation : holdColumn; if (!column) throw new Error(`${workflowId} has no creation column`); diff --git a/packages/engine/src/__tests__/pipeline-smoke/_pipeline-scenarios.ts b/packages/engine/src/__tests__/pipeline-smoke/_pipeline-scenarios.ts index 1b015acb12..ecf7ba7894 100644 --- a/packages/engine/src/__tests__/pipeline-smoke/_pipeline-scenarios.ts +++ b/packages/engine/src/__tests__/pipeline-smoke/_pipeline-scenarios.ts @@ -93,7 +93,7 @@ export const PIPELINE_SCENARIOS: readonly PipelineScenario[] = [ { id: "S05", title: "Code review revisions require a current approval", - workflows: ["builtin:coding-ideas", "builtin:coding"], + workflows: ["builtin:coding-ideas", "builtin:coding-ideas-v2", "builtin:coding"], expectedTerminal: "merged-done", variants: ["revise-twice"], arrange: PIPELINE_SCENARIO_DRIVERS.s05Arrange, diff --git a/scripts/run-pipeline-smoke.mjs b/scripts/run-pipeline-smoke.mjs index 33136587c1..3e4ffd753a 100644 --- a/scripts/run-pipeline-smoke.mjs +++ b/scripts/run-pipeline-smoke.mjs @@ -17,13 +17,18 @@ export const ENGINE_DIR = join(REPO_ROOT, "packages", "engine"); export const PIPELINE_SMOKE_PROJECT = "engine-pipeline-smoke"; export const PIPELINE_SMOKE_SCENARIO_COUNT = 19; /* -FNXC:PipelineSmoke 2026-08-23-20:49: -The completed harness now drives actual planning, graph execution, worktree acquisition, and -review sessions instead of seeded lifecycle rows. Warm remediation measurements were 53,378ms and -60,459ms; round the observed slowest run to a 70-second declared budget rather than letting a -real pipeline execution fail on an obsolete pre-production-chain ceiling. +FNXC:PipelineSmoke 2026-08-25-06:55: +Re-baselined 150s -> 175s for measured workload growth, not to hide a regression. Two additions: +the dedicated Code Review remediation drive (a 7th file, which re-pays module import and provisions +its own disposable PostgreSQL database), and S05 extended to `builtin:coding-ideas-v2` 2014 a +revise-twice scenario that is among the longest in the matrix. Five consecutive full runs measured +140.1s, 143.8s, 146.7s, 147.0s and 148.4s against the old 150s ceiling: green, but with under 2s of +headroom, which is a flake waiting to happen rather than a passing lane. +The standing rule is unchanged and now has three precedents: an overrun with NO attributable growth +is a regression to fix in the lane, never a budget to raise. Per-file cost remains the first +optimization to reach for. */ -export const PIPELINE_SMOKE_DURATION_BUDGET_MS = 150_000; +export const PIPELINE_SMOKE_DURATION_BUDGET_MS = 175_000; export const DEFAULT_REPORT_PATH = join(ENGINE_DIR, ".pipeline-smoke-report.json"); /*