Skip to content

Commit 96dd3cb

Browse files
mchwangclaude
andcommitted
F1c: shutdown wiring and /api/runner
Store write gate with a one-time shutdown capability for settling coordinators; ShuttingDownError maps to 503 and BadRequest to 400; the merge coordinator rethrows ShuttingDownError instead of swallowing it and settles irreversible merges through the capability; the runner rejects admission at step 1; admitted requests drain (AGENTS.md) and the gate closes after the drain; idle keep-alive sockets are closed during shutdown; /api/runner status and replayable cancel-attempt, retry and cancel-task. Corrects the contract's shutdown steps 1 and 3 to follow the AGENTS.md drain rule. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 parent f1458a9 commit 96dd3cb

8 files changed

Lines changed: 374 additions & 42 deletions

File tree

‎docs/implementation/runner-lifecycle.md‎

Lines changed: 8 additions & 8 deletions
Large diffs are not rendered by default.

‎runner/coordinator.ts‎

Lines changed: 14 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import { identityKey, type PlanIdentity } from '../core/identity.ts';
22
import { captureInvocation, type InvocationHandle, type InvocationInput, type InvocationResult, type StopReason, type TaskClone } from '../agents/contract.ts';
33
import type { AttemptRecord, Store } from './store.ts';
4-
import { ATTEMPT_PHASES, GuardRefusal, WRITABLE_KINDS, bounded, sameContext, type AttemptKind, type Classification, type FirstReason } from './lifecycle.ts';
4+
import { ATTEMPT_PHASES, GuardRefusal, ShuttingDownError, WRITABLE_KINDS, bounded, sameContext, type AttemptKind, type Classification, type FirstReason, type ShutdownCapability } from './lifecycle.ts';
55

66
/** What F's host-side preparation hands to D's start call. */
77
export interface PreparedAttempt {
@@ -58,10 +58,15 @@ export class RunnerCoordinator {
5858
#store: Store; #deps: RunnerDeps; #limits: SlotLimits;
5959
#jobs = new Map<string, Job>(); #markers = new Map<string, Marker>();
6060
#closing = false;
61-
constructor(store: Store, deps: RunnerDeps, limits: SlotLimits = { writable: 1, readOnly: 1 }) {
61+
/** Settlement writes run with the shutdown capability, so they still land after the write gate closes. */
62+
#write: <T>(fn: () => T) => T;
63+
constructor(store: Store, deps: RunnerDeps, limits: SlotLimits = { writable: 1, readOnly: 1 }, capability?: ShutdownCapability) {
6264
if (![limits.writable, limits.readOnly].every(n => Number.isSafeInteger(n) && n >= 1)) throw new Error('Slot limits must be positive integers.');
6365
this.#store = store; this.#deps = deps; this.#limits = limits;
66+
this.#write = capability ? fn => capability.run(fn) : fn => fn();
6467
}
68+
/** Shutdown step 1: reject admission synchronously, in the same turn as the server flag and the Store gate. */
69+
rejectAdmission(): void { this.#closing = true; }
6570
get closing(): boolean { return this.#closing; }
6671
#now(): number { return this.#deps.now?.() ?? Date.now(); }
6772
#used(group: Group): number {
@@ -75,7 +80,7 @@ export class RunnerCoordinator {
7580
* a refused transaction releases the reservation in the same turn.
7681
*/
7782
start(identity: PlanIdentity, request: StartRequest): AttemptRecord {
78-
if (this.#closing) throw new GuardRefusal('The runner is shutting down.');
83+
if (this.#closing) throw new ShuttingDownError();
7984
const key = identityKey(identity);
8085
if (this.#jobs.has(key)) throw new GuardRefusal('An attempt is already active for this task.');
8186
const marker = this.#markers.get(key);
@@ -90,7 +95,8 @@ export class RunnerCoordinator {
9095
catch (error) { this.#jobs.delete(key); throw error; }
9196
job.attemptId = attempt.id; job.attempt = attempt;
9297
this.#arm(job, attempt);
93-
job.done = this.#run(job, attempt).catch(error => this.#unexpected(job, error));
98+
// Start after the caller's transaction commits: a rolled-back admission must not leave a job running.
99+
job.done = Promise.resolve().then(() => this.#run(job, attempt)).catch(error => this.#unexpected(job, error));
94100
return attempt;
95101
}
96102
/** Retry is a new attempt bound to the latest failed or cancelled one. */
@@ -136,7 +142,7 @@ export class RunnerCoordinator {
136142
if (job.firstReason) { job.handle?.cancel(D_REASON[job.firstReason]); return false; }
137143
job.firstReason = reason;
138144
try {
139-
if (job.attemptId && !this.#store.recordFirstReason(job.identity, job.attemptId, reason)) {
145+
if (job.attemptId && !this.#write(() => this.#store.recordFirstReason(job.identity, job.attemptId, reason))) {
140146
// Another writer (for example cancel task) recorded a reason first; adopt the durable one.
141147
const durable = this.#store.getAttempt(job.identity, job.attemptId).firstReason;
142148
if (durable) job.firstReason = durable;
@@ -163,6 +169,7 @@ export class RunnerCoordinator {
163169
}
164170
async #run(job: Job, attempt: AttemptRecord): Promise<void> {
165171
try {
172+
if (!this.#store.getAttempts(job.identity).some(row => row.id === attempt.id)) return; // admission was rolled back
166173
let prepared: PreparedAttempt;
167174
try { prepared = await this.#deps.prepare(attempt, job.controller.signal); }
168175
catch (error) { return await this.#endBeforeLaunch(job, attempt, this.#preparationDetail(job, error)); }
@@ -182,7 +189,7 @@ export class RunnerCoordinator {
182189
} catch (error) { return await this.#endBeforeLaunch(job, attempt, { detail: `Launch failed: ${message(error)}` }); }
183190
job.handle = handle;
184191
let running: boolean | undefined;
185-
try { running = this.#store.markRunning(job.identity, attempt.id); } catch { running = undefined; }
192+
try { running = this.#write(() => this.#store.markRunning(job.identity, attempt.id)); } catch { running = undefined; }
186193
if (running === undefined) {
187194
// A storage error, not a stop: keep ownership until D settles, then hold the slot under a marker.
188195
handle.cancel('capture-failure');
@@ -219,7 +226,7 @@ export class RunnerCoordinator {
219226
}
220227
#settle(job: Job, s: { stopReason?: StopReason; exitCode: number | null; signal: string | null; valid: boolean; result?: unknown; detail?: string }): Classification | undefined {
221228
try {
222-
return this.#store.settleAttempt(job.identity, job.attemptId, { ...s, firstReason: job.firstReason });
229+
return this.#write(() => this.#store.settleAttempt(job.identity, job.attemptId, { ...s, firstReason: job.firstReason }));
223230
} catch {
224231
// The row's outcome is unknown: hold the slot until startup recovery reconciles it.
225232
this.#markers.set(job.key, { group: job.group, attemptId: job.attemptId, reason: 'result-not-saved' });

‎runner/lifecycle.ts‎

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,11 +29,13 @@ const UUID_V4 = /^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f
2929
/** Attempt, action and allocation IDs are lowercase UUID v4s; anything else is refused before use. */
3030
export function isUuidV4(value: unknown): value is string { return typeof value === 'string' && UUID_V4.test(value); }
3131
export function assertUuidV4(value: unknown, name: string): asserts value is string {
32-
if (!isUuidV4(value)) throw new GuardRefusal(`${name} must be a lowercase UUID v4.`);
32+
if (!isUuidV4(value)) throw new BadRequest(`${name} must be a lowercase UUID v4.`);
3333
}
3434

3535
/** A guard refused the action. Refusals are definite outcomes and are recorded for replay. */
3636
export class GuardRefusal extends Error {}
37+
/** A malformed request, refused before any transaction and never recorded. The server maps it to HTTP 400. */
38+
export class BadRequest extends GuardRefusal {}
3739
/** Reusing an action ID for a different request. */
3840
export class ActionIdReused extends GuardRefusal {}
3941
/**
@@ -44,6 +46,10 @@ export class RefusalWithEffect extends GuardRefusal {
4446
readonly effect: () => void;
4547
constructor(message: string, effect: () => void) { super(message); this.effect = effect; }
4648
}
49+
/** A Store write after shutdown began. The server maps it to HTTP 503, never to the 409 used for review errors. */
50+
export class ShuttingDownError extends Error { constructor() { super('The review server is shutting down.'); } }
51+
/** Lets settling coordinator code write after the gate closes. Only the server hands it out, and never to HTTP handlers. */
52+
export interface ShutdownCapability { run<T>(fn: () => T): T }
4753

4854
export function bounded(reason: string): string {
4955
const text = reason.trim() || 'No reason given.';

‎runner/merge.ts‎

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import type { ReviewService } from './review.ts';
22
import { mergeActionResponse, type MergeAttempt } from './store.ts';
3-
import { ActionIdReused, GuardRefusal, MERGEABLE_STATUSES, assertUuidV4 } from './lifecycle.ts';
3+
import { ActionIdReused, GuardRefusal, MERGEABLE_STATUSES, ShuttingDownError, assertUuidV4, type ShutdownCapability } from './lifecycle.ts';
44
import { MergeSubmissionError, type MergeGateway, type MergeQueueGateway, type MergeQueueObservation, type MergeResult, type RemoteMergeState } from '../github/merge.ts';
55

66
type ReviewView = ReturnType<ReviewService['load']>;
@@ -42,9 +42,12 @@ export class MergeCoordinator {
4242
readonly service: ReviewService;
4343
readonly gateway: MergeGateway;
4444
readonly operationTimeoutMs: number;
45-
constructor(service: ReviewService, gateway: MergeGateway, operationTimeoutMs = 14_000) {
45+
/** Settlement of an irreversible merge keeps its writes after the Store gate closes; request-path reconciliation does not. */
46+
#settle: <T>(fn: () => T) => T;
47+
constructor(service: ReviewService, gateway: MergeGateway, operationTimeoutMs = 14_000, capability?: ShutdownCapability) {
4648
if (!Number.isSafeInteger(operationTimeoutMs) || operationTimeoutMs < 1 || operationTimeoutMs > 14_000) throw new Error('Invalid merge operation deadline.');
4749
this.service = service; this.gateway = gateway; this.operationTimeoutMs = operationTimeoutMs;
50+
this.#settle = capability ? fn => capability.run(fn) : fn => fn();
4851
}
4952

5053
#attempt(): MergeAttempt | null {
@@ -135,6 +138,7 @@ export class MergeCoordinator {
135138
async displayStatus(view = this.service.load(), signal?: AbortSignal): Promise<MergeStatus | MergeUnavailableStatus> {
136139
try { return await this.status(view, false, signal); }
137140
catch (error) {
141+
if (error instanceof ShuttingDownError) throw error;
138142
if (signal?.aborted) throw signal.reason;
139143
return { available: true, ready: false, action: null, blockers: [{ code: 'github', message: `Could not read GitHub merge state. ${error instanceof Error ? error.message : 'Unknown error.'}` }], remote: null, queue: this.#queueStatus() };
140144
}
@@ -282,9 +286,10 @@ export class MergeCoordinator {
282286
// report that action as failed; the durable submitting record is recoverable by polling.
283287
let applied = true;
284288
try {
285-
applied = queueAttempt.kind === 'queue'
286-
? this.service.store.queueMergeAttempt(this.service.config.identity, queueAttempt.id, result.url)
287-
: this.service.store.finishMergeAttempt(this.service.config.identity, queueAttempt.id, { state: 'merged', url: result.url });
289+
const attempt = queueAttempt;
290+
applied = this.#settle(() => attempt.kind === 'queue'
291+
? this.service.store.queueMergeAttempt(this.service.config.identity, attempt.id, result.url)
292+
: this.service.store.finishMergeAttempt(this.service.config.identity, attempt.id, { state: 'merged', url: result.url }));
288293
} catch {}
289294
// Not applied: a concurrent poll changed the attempt first. A committed failure is the answer, not this success.
290295
if (!applied) {
@@ -302,15 +307,15 @@ export class MergeCoordinator {
302307
if (error instanceof CommittedFailure) throw error;
303308
// GitHub's refusal is definite only once the attempt's failed state (and the click's saved answer) is durable.
304309
let refusalSaved = false;
305-
if (queueAttempt) try {
310+
if (queueAttempt) try { const attempt = queueAttempt; this.#settle(() => {
306311
const message = error instanceof Error ? error.message : 'GitHub merge submission outcome is unknown.';
307312
if (error instanceof MergeSubmissionError && error.outcome === 'refused') {
308-
refusalSaved = this.service.store.finishMergeAttempt(this.service.config.identity, queueAttempt.id, {
313+
refusalSaved = this.service.store.finishMergeAttempt(this.service.config.identity, attempt.id, {
309314
state: 'failed', reason: message,
310315
requiresFreshReview: /head (?:branch |commit )?(?:was )?(?:modified|changed)|does not match.*head|stale review/i.test(message),
311316
});
312-
} else this.service.store.recordMergeAttemptDiagnostic(this.service.config.identity, queueAttempt.id, message);
313-
} catch {}
317+
} else this.service.store.recordMergeAttemptDiagnostic(this.service.config.identity, attempt.id, message);
318+
}); } catch {}
314319
// A definite refusal before admission is this click's outcome; a resend replays it. Aborts, the deadline and
315320
// shutdown applied nothing for a passing reason, so the same click may be sent again.
316321
if (!queueAttempt && !signal.aborted && action) {
@@ -360,6 +365,7 @@ export class MergeCoordinator {
360365
this.#publishQueueObservation(attempt, observation);
361366
return this.#queueStatus();
362367
} catch (error) {
368+
if (error instanceof ShuttingDownError) throw error;
363369
if (signal.aborted) throw signal.reason;
364370
const message = error instanceof Error ? error.message : 'Could not read the merge queue.';
365371
if (/head changed after review/i.test(message)) {

‎runner/questions.ts‎

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import { QuestionWorker } from './question-agent.ts';
44
import { LeftoverLedger } from './question-leftovers.ts';
55
import { StopError, type QuestionScope } from './question-container.ts';
66
import type { ReviewNote } from './store.ts';
7+
import type { ShutdownCapability } from './lifecycle.ts';
78
export type QuestionAgent = (prompt: string, signal: AbortSignal, scope?: QuestionScope, timeoutMs?: number) => Promise<string>;
89
const QUESTION_TIMEOUT_MS = 120_000;
910
const SHUTDOWN_SETTLE_MS = 20_000;
@@ -26,8 +27,10 @@ export class Questions {
2627
private service: ReviewService;
2728
private agent?: QuestionAgent;
2829
private worker: QuestionWorker;
29-
constructor(service: ReviewService, agent?: QuestionAgent) {
30-
this.service=service; this.agent=agent;
30+
/** Settlement writes (finishAnswer after abort) keep working after the Store write gate closes. */
31+
private write: <T>(fn: () => T) => T;
32+
constructor(service: ReviewService, agent?: QuestionAgent, capability?: ShutdownCapability) {
33+
this.service=service; this.agent=agent; this.write = capability ? fn => capability.run(fn) : fn => fn();
3134
// Beside the review database's canonical path, so a restart of the same review finds what an earlier session left.
3235
this.worker=new QuestionWorker(undefined,LeftoverLedger.forDatabase(service.config.database));
3336
}
@@ -65,9 +68,9 @@ export class Questions {
6568
invocation = agent(questionPrompt(view,note),controller.signal,scope,QUESTION_TIMEOUT_MS);
6669
const text=await Promise.race([invocation,aborted]);
6770
if(typeof text!=='string'||!text.trim()||text.length>24000) throw new Error('Agent returned an empty or oversized answer.');
68-
this.service.store.finishAnswer(this.service.config.identity,id,attempt,{status:'complete',text:text.trim()});
71+
this.write(()=>this.service.store.finishAnswer(this.service.config.identity,id,attempt,{status:'complete',text:text.trim()}));
6972
} catch(error) {
70-
this.service.store.finishAnswer(this.service.config.identity,id,attempt,{status:'failed',error:(error instanceof Error?error.message:'Agent failed.').slice(0,1000)});
73+
try { this.write(()=>this.service.store.finishAnswer(this.service.config.identity,id,attempt,{status:'failed',error:(error instanceof Error?error.message:'Agent failed.').slice(0,1000)})); } catch {}
7174
} finally {clearTimeout(timeout);}
7275
})();
7376
const settled = done.finally(async () => {

‎runner/store.ts‎

Lines changed: 17 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { importPlan, applySuggestion, assertEditReply, type Plan, type PlanConte
66
import type { Approval, SegmentChoice } from '../core/approvals.ts';
77
import type { InvocationContext, StopReason } from '../agents/contract.ts';
88
import {
9-
ATTEMPT_PHASES, CLOSED_STATUSES, MERGEABLE_STATUSES, DEFAULT_TASK_BUDGET_MS, FIRST_REASONS, GuardRefusal, ActionIdReused, RefusalWithEffect, MAX_RESULT_BYTES, TASK_STATUSES, TERMINAL_STATES,
9+
ATTEMPT_PHASES, BadRequest, CLOSED_STATUSES, MERGEABLE_STATUSES, ShuttingDownError, type ShutdownCapability, DEFAULT_TASK_BUDGET_MS, FIRST_REASONS, GuardRefusal, ActionIdReused, RefusalWithEffect, MAX_RESULT_BYTES, TASK_STATUSES, TERMINAL_STATES,
1010
assertUuidV4, bounded, classifySettlement, requestHash, sameContext,
1111
type AttemptKind, type AttemptState, type Classification, type FirstReason, type Settlement, type TaskStatus,
1212
} from './lifecycle.ts';
@@ -121,13 +121,26 @@ export class Store {
121121
}
122122
close(): void { this.#db.close(); }
123123
#get(sql: string, ...args: SQLInputValue[]) { return this.#db.prepare(sql).get(...args); }
124-
#run(sql: string, ...args: SQLInputValue[]) { return this.#db.prepare(sql).run(...args); }
124+
#run(sql: string, ...args: SQLInputValue[]) { this.#checkWrite(); return this.#db.prepare(sql).run(...args); }
125+
// Shutdown write gate (runner-lifecycle.md, "Shutdown" step 1).
126+
#gateClosed = false; #privileged = 0; #capabilityIssued = false;
127+
#checkWrite(): void { if (this.#gateClosed && this.#privileged === 0) throw new ShuttingDownError(); }
128+
/** Issued once, to the server, which hands it only to coordinators' settlement and close code. */
129+
shutdownCapability(): ShutdownCapability {
130+
if (this.#capabilityIssued) throw new Error('The shutdown capability was already issued.');
131+
this.#capabilityIssued = true;
132+
return Object.freeze({ run: <T>(fn: () => T): T => { this.#privileged++; try { return fn(); } finally { this.#privileged--; } } });
133+
}
134+
/** Shutdown step 1: from now on, every write without the capability throws ShuttingDownError. Reads still work. */
135+
closeWrites(): void { this.#gateClosed = true; }
136+
get writesClosed(): boolean { return this.#gateClosed; }
125137
#depth = 0;
126138
/** The user action whose transaction is open, so its events can prove they belong to it. */
127139
#action: { key: string; actionId: string } | null = null;
128140
/** Nested calls join the outer transaction, so a user action can wrap existing Store methods atomically. */
129141
#transaction<T>(fn: () => T): T {
130142
if (this.#depth > 0) { this.#depth++; try { return fn(); } finally { this.#depth--; } }
143+
this.#checkWrite();
131144
this.#db.exec('BEGIN IMMEDIATE'); this.#depth = 1;
132145
try { const result = fn(); this.#db.exec('COMMIT'); return result; }
133146
catch (error) { this.#db.exec('ROLLBACK'); throw error; }
@@ -816,8 +829,8 @@ export class Store {
816829
return { response: value, replayed: false };
817830
});
818831
} catch (error) {
819-
const storage = (error as { code?: string }).code === 'ERR_SQLITE_ERROR';
820-
if (!replaying && !storage && !(error instanceof ActionIdReused) && this.#depth === 0) {
832+
const storage = (error as { code?: string }).code === 'ERR_SQLITE_ERROR' || error instanceof ShuttingDownError;
833+
if (!replaying && !storage && !(error instanceof ActionIdReused) && !(error instanceof BadRequest) && this.#depth === 0) {
821834
const message = error instanceof Error ? bounded(error.message) : 'Refused.';
822835
this.#transaction(() => {
823836
if (!this.#get('SELECT 1 FROM user_actions WHERE plan_key=? AND action_id=?', key, action.actionId)) record({ ok: false, error: message });

0 commit comments

Comments
 (0)