diff --git a/plugins/provider-pi/src/bridge/bridge.lifecycle.test.ts b/plugins/provider-pi/src/bridge/bridge.lifecycle.test.ts index 0342cf8c34..f99d52ae52 100644 --- a/plugins/provider-pi/src/bridge/bridge.lifecycle.test.ts +++ b/plugins/provider-pi/src/bridge/bridge.lifecycle.test.ts @@ -133,22 +133,21 @@ it("discard ends the child and removes the session file", async () => { }, 90_000); it("a failed construction leaves no child", async () => { - // A provider the catalog does not know passes the up-front check (pi may - // serve providers the catalog omits); the fake then starts on its default - // model, the bridge sees the mismatch after get_state, retries the spawn - // through the transient-auth window, and fails. + // Retry policy has deterministic unit coverage. This process-level case + // needs one real child to prove the bridge kills a failed construction. + vi.stubEnv("FAKE_PI_EXIT_BEFORE_FIRST_RESPONSE", "1"); const response = await harness.request((nextId += 1), "thread/start", { threadId: "thr_lc_failed", cwd: harness.workspaceDir, instructionMode: "append", - options: { ...FULL_PERMISSION_OPTIONS, model: "other-provider/no-such-model" }, + options: FULL_PERMISSION_OPTIONS, }); expect(response.error).toMatchObject({ - message: expect.stringContaining('did not start with model "other-provider/no-such-model"'), + message: expect.stringContaining("pi exited"), }); const log = harness.readProcessLog(); - expect(log.spawned.length).toBeGreaterThan(1); - await expectEveryChildGone(log.spawned.length); + expect(log.spawned).toHaveLength(1); + await expectEveryChildGone(1); }, 90_000); it("the fork helper child exits once the fork is done", async () => { @@ -212,7 +211,7 @@ async function expectScratchFilesGone(): Promise { } } -it("a child's tool and prompt files go with the child, for a released thread and for every attempt of a failed construction", async () => { +it("a child's tool and prompt files go with the child after release and failed construction", async () => { // The bridge process outlives every pi child it spawns, so a file written // for one child must not wait for the bridge's own temp dir to be removed. await harness.startThread("thr_lc_scratch", { @@ -234,18 +233,19 @@ it("a child's tool and prompt files go with the child, for a released thread and await expectEveryChildGone(1); await expectScratchFilesGone(); - // Every retry of the transient-auth window wrote its own set; each went - // with its detached child. + // A construction that exits before readiness still owns the files the + // bridge prepared for it, and its exit removes them. + vi.stubEnv("FAKE_PI_EXIT_BEFORE_FIRST_RESPONSE", "1"); const failed = await harness.request((nextId += 1), "thread/start", { threadId: "thr_lc_scratch_failed", cwd: harness.workspaceDir, instructionMode: "append", - options: { ...FULL_PERMISSION_OPTIONS, instructions: "be brief", model: "other-provider/no-such-model" }, + options: { ...FULL_PERMISSION_OPTIONS, instructions: "be brief" }, }); expect(failed.error).toBeDefined(); const log = harness.readProcessLog(); - expect(log.spawned.length).toBeGreaterThan(2); - await expectEveryChildGone(log.spawned.length); + expect(log.spawned).toHaveLength(2); + await expectEveryChildGone(2); await expectScratchFilesGone(); }, 60_000); diff --git a/plugins/provider-pi/src/bridge/fake-pi-rpc.mjs b/plugins/provider-pi/src/bridge/fake-pi-rpc.mjs index 8bd294ed29..822e4d82e2 100644 --- a/plugins/provider-pi/src/bridge/fake-pi-rpc.mjs +++ b/plugins/provider-pi/src/bridge/fake-pi-rpc.mjs @@ -48,6 +48,8 @@ * - Fault knobs for the bridge's own tests: FAKE_PI_SPAWN_COUNTER_FILE counts * spawns across processes and FAKE_PI_MISMATCH_FIRST_SPAWN=1 makes only the * first spawn ignore `--model` (a transient model mismatch); + * FAKE_PI_EXIT_BEFORE_FIRST_RESPONSE=1 exits after recording the spawn but + * before importing the extension or reading a command; * FAKE_PI_NO_SESSION_START=1 never emits session_start to the extension (so * no `ready`); FAKE_PI_DROP_STEER_AT_END=1 ends a run with a queued steer * still queued; FAKE_PI_STREAMING_AFTER_END=1 reports isStreaming after a @@ -134,6 +136,9 @@ process.on("SIGTERM", () => { if (hangOnClose) return; exit(); }); +if (process.env.FAKE_PI_EXIT_BEFORE_FIRST_RESPONSE === "1") { + exit(); +} const MODELS = [ { diff --git a/plugins/provider-pi/src/bridge/rpc-session.retry.test.ts b/plugins/provider-pi/src/bridge/rpc-session.retry.test.ts new file mode 100644 index 0000000000..98765a9967 --- /dev/null +++ b/plugins/provider-pi/src/bridge/rpc-session.retry.test.ts @@ -0,0 +1,28 @@ +import { expect, it, vi } from "vitest"; +import { runPiTransientAuthConstruction } from "./rpc-session.js"; + +it("exhausts the initial construction and eight transient-auth retries deterministically", async () => { + const errors = Array.from( + { length: 9 }, + (_, index) => new Error(`mismatch ${index + 1}`), + ); + const attempt = vi.fn(async () => ({ + ok: false as const, + error: errors[attempt.mock.calls.length - 1]!, + })); + const discardFailedAttempt = vi.fn(); + const waitBeforeRetry = vi.fn(async () => undefined); + + await expect( + runPiTransientAuthConstruction({ + attempt, + discardFailedAttempt, + isClosed: () => false, + waitBeforeRetry, + }), + ).rejects.toBe(errors[8]); + + expect(attempt).toHaveBeenCalledTimes(9); + expect(discardFailedAttempt).toHaveBeenCalledTimes(8); + expect(waitBeforeRetry).toHaveBeenCalledTimes(8); +}); diff --git a/plugins/provider-pi/src/bridge/rpc-session.ts b/plugins/provider-pi/src/bridge/rpc-session.ts index 6881d4d80b..79ff75a226 100644 --- a/plugins/provider-pi/src/bridge/rpc-session.ts +++ b/plugins/provider-pi/src/bridge/rpc-session.ts @@ -119,6 +119,38 @@ const CHANNEL_REQUEST_TIMEOUT_MS = 30_000; /** How long an `agent_end` waits for the extension's in-process leaf report. */ const AGENT_END_LEAF_TIMEOUT_MS = 5_000; +type PiSessionConstructionOutcome = { ok: true } | { ok: false; error: Error }; + +function waitForPiTransientAuthRetry(): Promise { + return new Promise((resolve) => + setTimeout(resolve, PI_TRANSIENT_AUTH_RETRY_DELAY_MS), + ); +} + +/** + * Retry the transient model-resolution window without coupling its policy to + * child construction. Tests inject synchronous attempts and waits; production + * supplies real child construction and teardown. + */ +export async function runPiTransientAuthConstruction(args: { + attempt: () => Promise; + discardFailedAttempt: () => void; + isClosed: () => boolean; + waitBeforeRetry: () => Promise; +}): Promise { + for (let attempt = 0; ; attempt += 1) { + const outcome = await args.attempt(); + if (outcome.ok) { + return; + } + if (attempt >= PI_TRANSIENT_AUTH_MAX_RETRIES || args.isClosed()) { + throw outcome.error; + } + args.discardFailedAttempt(); + await args.waitBeforeRetry(); + } +} + export interface PiRpcSessionState { model?: { provider?: string; id?: string; contextWindow?: number }; thinkingLevel?: string; @@ -198,30 +230,21 @@ export class PiRpcSession { * resolved its model): respawn a few times before failing. */ async start(): Promise { - for (let attempt = 0; ; attempt += 1) { - const outcome = await this.spawnAndVerify(); - if (outcome.ok) { - return; - } - if (attempt >= PI_TRANSIENT_AUTH_MAX_RETRIES || this.closed) { - throw outcome.error; - } - // The failed attempt's child is detached before it is killed: its exit - // (async, possibly after the next attempt spawned) must neither report - // a session error for a thread still under construction nor touch the - // next child's readiness, leaf waiter, or processing state. - const failed = this.child; - this.child = undefined; - failed?.kill(); - await new Promise((resolve) => - setTimeout(resolve, PI_TRANSIENT_AUTH_RETRY_DELAY_MS), - ); - } + await runPiTransientAuthConstruction({ + attempt: () => this.spawnAndVerify(), + discardFailedAttempt: () => { + // A retried child is detached before it is killed: its late lines and + // exit must not touch the next child's state or report a session error. + const failed = this.child; + this.child = undefined; + failed?.kill(); + }, + isClosed: () => this.closed, + waitBeforeRetry: waitForPiTransientAuthRetry, + }); } - private async spawnAndVerify(): Promise< - { ok: true } | { ok: false; error: Error } - > { + private async spawnAndVerify(): Promise { const toolsFilePath = join( this.options.scratchDir, `pi-tools-${process.pid}-${Date.now()}-${Math.random().toString(16).slice(2)}.json`,