fix(runner): close two timing windows in the capability-live suspend path (#13143)
## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The Paperclip Runner manages live agent sessions and their suspend path. > - A turn-timeout promise could reject before `reconcileActiveTurn()` attached its handler. > - A short provider-drain budget could reject a valid suspend on a loaded continuous-integration host. > - This pull request closes both timing windows and adds deterministic regression tests. > - The benefit is a fail-closed suspend path that does not report false failures under load. ## Linked Issues or Issue Description **What happened?** Capability-live tests failed under load. A turn-timeout promise could raise an unhandled rejection during a slow interrupt round trip. An idle provider drain could also exceed its one-second proof budget during suspend. **Expected behavior** The suspend path must observe turn-timeout rejections and allow enough time for one provider command round trip. It must still fail closed when the runner does not prove durable suspension. **Steps to reproduce** 1. Run the capability-live tests on a loaded four-vCPU continuous-integration host. 2. Delay a `turn/interrupt` reply beyond the turn timeout. 3. Close a live session and observe the suspend barrier. **Paperclip version or commit** `01b442b2926ac2010a2fcfbda6592802472ef07f` **Deployment mode** Built from source with the Paperclip Runner test suite. ## What Changed - Attach the turn-timeout rejection handler inside `armTurnWaiter()` at promise creation. - Remove the redundant per-call-site guard in `sendMessage()`. - Use a uniform five-second provider-drain proof budget, capped by the outer preparation deadline. - Remove the unused boolean return from the provider-turn-stop helper. - Add deterministic tests for the delayed interrupt and the short close grace period. ## Verification - `npx vitest run packages/paperclip-runner/src/live/live-session.test.ts` passed locally with zero skipped tests. - `npx vitest run packages/paperclip-runner/src/live/runnerd-codex-transport.test.ts` passed locally with zero skipped tests. - The two new tests appeared in the local run output and were not skipped. - The package type-check passed locally. - GitHub Actions must confirm the full continuous-integration suite, including `Verify Paperclip Runner`. ## Risks - The provider-drain wait now allows up to five seconds before the outer deadline caps it. - The fail-closed suspend barrier remains unchanged. - The test suite still depends on the Rust runner binary for capability-live tests. ## Model Used OpenAI Codex, GPT-5. The runtime provided tool use and code execution. The runtime did not provide a context-window value. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes: #` / `Refs: #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change (e.g. `docs/...`, `fix/...`) and contains no internal Paperclip ticket id or instance-derived details - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [x] I will address all Greptile and reviewer comments before requesting merge --------- Co-authored-by: Paperclip <noreply@paperclip.ing>
This commit is contained in:
parent
2a05b5ed34
commit
e25a6b797f
|
|
@ -66,6 +66,8 @@ interface FakeProviderState {
|
|||
usageRunDelta: Record<string, unknown> | null;
|
||||
onTurnStart?: () => Promise<void>;
|
||||
onUsage?: (queue: AsyncNotifications, turnId: string) => void | Promise<void>;
|
||||
/** Delays the fake `turn/interrupt` reply, to model a slow transport round trip. */
|
||||
interruptDelayMs: number;
|
||||
}
|
||||
|
||||
class FakeCapabilityCodexTransport implements CodexAppServerTransport {
|
||||
|
|
@ -132,6 +134,9 @@ class FakeCapabilityCodexTransport implements CodexAppServerTransport {
|
|||
return { turn: { id: turnId, status: "inProgress" } };
|
||||
}
|
||||
if (method === "turn/interrupt") {
|
||||
if (this.state.interruptDelayMs > 0) {
|
||||
await new Promise((resolve) => setTimeout(resolve, this.state.interruptDelayMs));
|
||||
}
|
||||
const turnId = String(params.turnId);
|
||||
this.state.turns.set(turnId, "interrupted");
|
||||
this.notificationsQueue.push({
|
||||
|
|
@ -318,6 +323,7 @@ function providerState(): FakeProviderState {
|
|||
holdAfterTool: false,
|
||||
closeError: null,
|
||||
usageRunDelta: null,
|
||||
interruptDelayMs: 0,
|
||||
};
|
||||
}
|
||||
|
||||
|
|
@ -1312,6 +1318,47 @@ describe("Capability live runnerd and Codex session", () => {
|
|||
});
|
||||
});
|
||||
|
||||
it("does not raise an unhandled rejection when interrupt() outlasts the turn timeout during reconcileActiveTurn", async () => {
|
||||
const state = providerState();
|
||||
const store = new InMemoryCapabilityLiveSessionStore();
|
||||
const firstService = new CapabilityLiveSessionService({
|
||||
store,
|
||||
transportFactory: fakeTransportFactory(state),
|
||||
});
|
||||
const first = await firstService.create({
|
||||
runId: "run-slow-interrupt-reconcile",
|
||||
sessionId: "session-slow-interrupt-reconcile",
|
||||
attemptId: "attempt-slow-interrupt-killed",
|
||||
turnTimeoutMs: 20,
|
||||
});
|
||||
state.holdAfterTool = true;
|
||||
const killedTurn = captureTurnRejection(first.sendMessage("Apply idempotent progress once."));
|
||||
await vi.waitFor(async () => {
|
||||
expect((await store.load(first.id))?.mockState).toContain("progress-governed-once");
|
||||
});
|
||||
await state.transports[0]!.close();
|
||||
await expect(killedTurn).resolves.toMatchObject({ message: expect.stringContaining("timed out") });
|
||||
|
||||
const resumedService = new CapabilityLiveSessionService({
|
||||
store,
|
||||
transportFactory: fakeTransportFactory(state),
|
||||
});
|
||||
const resumed = await resumedService.resume({
|
||||
sessionId: first.id,
|
||||
attemptId: "attempt-slow-interrupt-resumed",
|
||||
resumeOf: "attempt-slow-interrupt-killed",
|
||||
});
|
||||
|
||||
// Make the fake transport's turn/interrupt reply outlast the 20 ms turn
|
||||
// timeout. reconcileActiveTurn() awaits interrupt() first, so the turn
|
||||
// waiter's timer can reject before interrupt() resolves. The rejection
|
||||
// handler must already be in place at that moment; otherwise Node
|
||||
// reports an unhandled rejection and Vitest fails the whole file, even
|
||||
// though the assertion below is correct.
|
||||
state.interruptDelayMs = 200;
|
||||
await expect(resumed.reconcileActiveTurn()).rejects.toThrow(/timed out/);
|
||||
});
|
||||
|
||||
it("persists a resumed turnTimeoutMs override so a later resume that omits it keeps the value", async () => {
|
||||
const state = providerState();
|
||||
const store = new InMemoryCapabilityLiveSessionStore();
|
||||
|
|
|
|||
|
|
@ -1550,7 +1550,6 @@ export class CapabilityLiveSession {
|
|||
// Arm the provider timeout only after bounded preflight succeeds. The
|
||||
// admission token excludes concurrent sends before this point.
|
||||
const terminal = this.#armTurnWaiter();
|
||||
void terminal.catch(() => undefined);
|
||||
try {
|
||||
response = await admission.transport.request("turn/start", {
|
||||
threadId: this.#providerThreadId,
|
||||
|
|
@ -1996,7 +1995,7 @@ export class CapabilityLiveSession {
|
|||
|
||||
#armTurnWaiter(): Promise<Omit<CapabilityLiveTurnResult, "snapshot">> {
|
||||
if (this.#turnWaiter !== null) throw new Error("Capability live session already has a turn waiter");
|
||||
return new Promise<Omit<CapabilityLiveTurnResult, "snapshot">>((resolve, reject) => {
|
||||
const terminal = new Promise<Omit<CapabilityLiveTurnResult, "snapshot">>((resolve, reject) => {
|
||||
const timer = setTimeout(() => {
|
||||
const waiter = this.#turnWaiter;
|
||||
this.#turnWaiter = null;
|
||||
|
|
@ -2011,6 +2010,13 @@ export class CapabilityLiveSession {
|
|||
}, this.#config.turnTimeoutMs);
|
||||
this.#turnWaiter = { resolve, reject, timer, assistantText: "", draftId: null };
|
||||
});
|
||||
// A caller may await this promise only after another `await` of its own
|
||||
// (see `reconcileActiveTurn`). The timer above can reject before that
|
||||
// point, so attach a no-op handler here, at creation, on every call
|
||||
// site. `.catch()` returns a new promise; the original stays rejected
|
||||
// and a later `await terminal` still observes it.
|
||||
terminal.catch(() => undefined);
|
||||
return terminal;
|
||||
}
|
||||
|
||||
/** Interrupts and durably reconciles a checkpointed active turn after restart. */
|
||||
|
|
|
|||
|
|
@ -7341,6 +7341,47 @@ it("probes an exact-authority resume and confirms its live provider identity", a
|
|||
}
|
||||
}, 30_000);
|
||||
|
||||
it("still fails closed when a real close grace period cannot fit a durable suspension round trip", async () => {
|
||||
const stateDirectory = await mkdtemp(
|
||||
join(tmpdir(), "runnerd-close-grace-too-small-"),
|
||||
);
|
||||
const identity = {
|
||||
runnerInstanceId: "runner-close-grace-too-small",
|
||||
environmentLeaseId: "lease-close-grace-too-small",
|
||||
runId: "run-close-grace-too-small",
|
||||
normalizedSessionId: "session-close-grace-too-small",
|
||||
turnId: "turn-close-grace-too-small",
|
||||
itemId: "item-close-grace-too-small",
|
||||
};
|
||||
const bundle = createCapabilityRunnerdCodexTransport({
|
||||
runnerBinary: defaultCapabilityRunnerdBinary(),
|
||||
codexCommand: fakeCodex,
|
||||
codexArgs: fakeCodexArgs(stateDirectory),
|
||||
stateDirectory,
|
||||
// No real durable command round trip can complete this fast. A wider
|
||||
// budget for the provider-drain proof must not turn this barrier into
|
||||
// one that always passes; it still needs the actual proof to arrive.
|
||||
closeGraceMs: 1,
|
||||
lifecyclePolicy: { mode: "per_turn", idleTimeoutMs: null },
|
||||
prpIdentity: identity,
|
||||
});
|
||||
bundle.transport.setServerRequestHandler(async () => ({
|
||||
success: true,
|
||||
contentItems: [],
|
||||
}));
|
||||
try {
|
||||
await bundle.transport.request("thread/start", {
|
||||
cwd: tmpdir(),
|
||||
dynamicTools: [],
|
||||
});
|
||||
await expect(bundle.transport.close()).rejects.toThrow(
|
||||
"runner did not durably suspend before checkpoint",
|
||||
);
|
||||
} finally {
|
||||
await rm(stateDirectory, { recursive: true, force: true });
|
||||
}
|
||||
}, 30_000);
|
||||
|
||||
it("cold-restores a suspended provider session under its durable run binding", async () => {
|
||||
const stateDirectory = await mkdtemp(join(tmpdir(), "runnerd-cold-attach-"));
|
||||
const tracePath = join(stateDirectory, "provider-trace.ndjson");
|
||||
|
|
|
|||
|
|
@ -3828,9 +3828,7 @@ class DurablePrpCodexTransport implements CodexAppServerTransport {
|
|||
}
|
||||
}
|
||||
|
||||
async #stopActiveProviderTurnBeforeSuspend(
|
||||
deadline: number,
|
||||
): Promise<boolean> {
|
||||
async #stopActiveProviderTurnBeforeSuspend(deadline: number): Promise<void> {
|
||||
const state = this.#providerDrainState();
|
||||
const core = this.#core;
|
||||
const inferredActiveProviderTurnId =
|
||||
|
|
@ -3848,7 +3846,7 @@ class DurablePrpCodexTransport implements CodexAppServerTransport {
|
|||
activeProviderTurnId === null ||
|
||||
core === null
|
||||
) {
|
||||
return false;
|
||||
return;
|
||||
}
|
||||
const commandId = `command_close_stop_${randomUUID().replaceAll("-", "")}`;
|
||||
core.queueCommand(
|
||||
|
|
@ -3866,19 +3864,18 @@ class DurablePrpCodexTransport implements CodexAppServerTransport {
|
|||
this.#diagnostic(
|
||||
`stopped active provider turn ${activeProviderTurnId} before runner suspension`,
|
||||
);
|
||||
return true;
|
||||
return;
|
||||
}
|
||||
if (command !== undefined && command.status !== "pending") {
|
||||
this.#diagnostic(
|
||||
`provider turn stop ${command.status} before runner suspension`,
|
||||
);
|
||||
return false;
|
||||
return;
|
||||
}
|
||||
if (await this.#runnerHasExited()) return false;
|
||||
if (await this.#runnerHasExited()) return;
|
||||
await new Promise((resolveWait) => setTimeout(resolveWait, 5));
|
||||
}
|
||||
this.#diagnostic("provider turn stop timed out before runner suspension");
|
||||
return false;
|
||||
}
|
||||
|
||||
async #drainSettledProviderEventsBeforeSuspend(
|
||||
|
|
@ -3982,13 +3979,15 @@ class DurablePrpCodexTransport implements CodexAppServerTransport {
|
|||
this.#pumpEventsSafely();
|
||||
await new Promise((resolveWait) => setTimeout(resolveWait, 5));
|
||||
}
|
||||
const stoppedActiveTurn =
|
||||
await this.#stopActiveProviderTurnBeforeSuspend(preparationDeadline);
|
||||
// The drain always needs one real command round trip to the runner
|
||||
// process, whether or not a turn was active: stopping an active
|
||||
// turn only changes how much trailing event traffic that round
|
||||
// trip may need to carry. Give both cases the same budget so a
|
||||
// slow-but-idle runner is not held to a tighter deadline than a
|
||||
// runner that just stopped a turn.
|
||||
await this.#stopActiveProviderTurnBeforeSuspend(preparationDeadline);
|
||||
providerDrained = await this.#drainSettledProviderEventsBeforeSuspend(
|
||||
Math.min(
|
||||
stoppedActiveTurn ? 5_000 : 1_000,
|
||||
Math.max(0, preparationDeadline - Date.now()),
|
||||
),
|
||||
Math.min(5_000, Math.max(0, preparationDeadline - Date.now())),
|
||||
);
|
||||
}
|
||||
// Local durable roots are reused too. Process exit alone cannot prove
|
||||
|
|
|
|||
Loading…
Reference in New Issue