fix(FN-186): give pipeline-smoke tasks process-unique ids and drain execution on teardown
S05 now covers builtin:coding-ideas-v2. 5/5 consecutive full lanes. Two harness defects, both of which made a CORRECT engine refusal look like a flake. 1. Task ids collided. The serial lived on the harness instance and reset with it, so every test's first task was `FN-182-S05-1`. The engine's process-wide state — `executingTaskLock`, `activeSessionRegistry.pathsForTask`, worktree registrations — is keyed by task id, so a straggler from the previous test answered for the NEXT test's identically-named task and handed it a worktree under the PREVIOUS fixture. The serial now lives on the module. 2. Teardown forgot in-flight work instead of waiting for it. `ProjectEngine.stop()` clears timers but does not await an execution already inside `execute()`, and the harness then called `activeSessionRegistry.clear()` — which hides a live session rather than ending it. `dispose()` now drains `executingTaskLock` and the registry for its own task ids, bounded, and THROWS on expiry: a straggler that outlives the budget is a real defect, and a silent continue would restore the leak. Throughout this, the product was right. The executor detected the foreign worktree, refused it (`outside_worktrees_dir`), retried, exhausted its budget and failed visibly. That refusal is the desired behaviour and was never the bug — the harness was manufacturing the condition. Budget re-baselined 150s -> 175s for attributable growth: a 7th file (the remediation drive) and S05 on V2, one of the longest scenarios. Five runs at 140.1-148.4s left under 2s of headroom against the old ceiling, which is a flake waiting to happen. The standing rule is unchanged and now has three precedents: growth must be nameable, or it is a regression to fix rather than a budget to raise. pnpm lint 0 errors, test:gate, verify:fast, engine-pipeline-smoke 90/90, and five consecutive full runs: 146.0s, 142.6s, 148.5s, 144.8s, 146.3s of the 175s budget.
This commit is contained in:
@@ -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=<n>` only for loud diagnostic measurement. The normalized report lists
|
||||
|
||||
@@ -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<void> = 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<FixtureEnvironmentRelease> {
|
||||
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<string>();
|
||||
private readonly promptRevisions = new Map<string, number>();
|
||||
private readonly mockScriptStates = new Map<string, { behavior: PipelineScriptedMergeBehavior; state: PipelineMockScriptState }>();
|
||||
/** Every task this harness created, so teardown can wait for their execution to actually stop. */
|
||||
private readonly createdTaskIds = new Set<string>();
|
||||
/** Newest non-empty branch per task; a workspace row only gains one at acquisition. */
|
||||
private readonly scriptedBranches = new Map<string, string>();
|
||||
|
||||
@@ -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<void> {
|
||||
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<Task> {
|
||||
(this.store as TaskStore & { taskCache?: Map<string, unknown> }).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`);
|
||||
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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");
|
||||
|
||||
/*
|
||||
|
||||
Reference in New Issue
Block a user