Skip to content

Commit 4bdbaf4

Browse files
committed
Close question admission before draining active jobs
1 parent 5d15f75 commit 4bdbaf4

3 files changed

Lines changed: 16 additions & 1 deletion

File tree

‎docs/implementation/read-only-review.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -88,3 +88,5 @@ PR #11 review round 1 reproduced a stale-poll race: a held question-list respons
8888
PR #11 review round 2 confirmed the poll fix and raised an outdated-retry issue in its summary. A browser regression reproduced the retry button on an older snapshot; such unanswered questions now instruct the reviewer to ask again against current code. Completed historical answers retain their outdated label. The summary's timeout-reporting concern supplied no concrete failure; timeout and interrupted-lease regressions remain in the validation suite.
8989

9090
PR #11 review round 3 reproduced retrying a locally active job after its persisted lease expires (for example after a clock jump). The manager now rejects a retry while that note is in its running map, before touching the persisted attempt. The regression advances wall time without advancing the deadline timer and checks the original attempt, single invocation, and shutdown cancellation remain intact.
91+
92+
PR #11 review round 4 reproduced new question work entering during shutdown. Questions now marks itself closing synchronously before cancelling/draining jobs; start rejects before any store write or agent invocation, during and after shutdown. The regression failed before the guard and checks that the unstarted note has no attempt recorded.

‎runner/questions.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,10 +18,12 @@ export function questionPrompt(view: ReturnType<ReviewService['load']>, note: Re
1818
}
1919
export class Questions {
2020
private running = new Map<string, {controller:AbortController;done:Promise<void>}>();
21+
private closing = false;
2122
private service: ReviewService;
2223
private agent?: QuestionAgent;
2324
constructor(service: ReviewService, agent?: QuestionAgent) { this.service=service; this.agent=agent; }
2425
start(id: string, view: ReturnType<ReviewService['load']>) {
26+
if (this.closing) throw new Error('Server is stopping. Reconnect before asking again.');
2527
if (this.running.has(id)) throw new Error('Agent is already answering this question.');
2628
const note = view.notes.find(n=>n.id===id && n.kind==='question');
2729
if (!note) throw new Error('Question not found.');
@@ -46,5 +48,5 @@ export class Questions {
4648
this.running.set(id,{controller,done});
4749
void done.finally(()=>this.running.delete(id));
4850
}
49-
async close() {for(const job of this.running.values())job.controller.abort(new Error('Server stopped. Retry the question.'));await Promise.all([...this.running.values()].map(job=>job.done));}
51+
async close() {this.closing = true;for(const job of this.running.values())job.controller.abort(new Error('Server stopped. Retry the question.'));await Promise.all([...this.running.values()].map(job=>job.done));}
5052
}

‎test/questions.test.ts‎

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,3 +56,14 @@ it('does not replace a locally running job when its persisted lease expires',asy
5656
expect(calls).toBe(1);expect(service.store.getReviewNotes(service.config.identity)[0]!.answer!.attempt).toBe(original);
5757
await manager.close();expect(signal?.aborted).toBe(true);expect(service.store.getReviewNotes(service.config.identity)[0]!.answer?.error).toMatch(/Server stopped/);
5858
});
59+
it('rejects new work as soon as shutdown begins',async()=>{
60+
const service=fixture(),first=question(service),second=question(service);let calls=0;
61+
const manager=new Questions(service,()=>{calls++;return new Promise(()=>{});});managers.push(manager);
62+
manager.start(first.createdNoteId!,first);
63+
const closing=manager.close();
64+
expect(()=>manager.start(second.createdNoteId!,second)).toThrow(/stopping/);
65+
await closing;
66+
expect(()=>manager.start(second.createdNoteId!,second)).toThrow(/stopping/);
67+
expect(calls).toBe(1);
68+
expect(service.store.getReviewNotes(service.config.identity).find(note=>note.id===second.createdNoteId)?.answer).toBeUndefined();
69+
});

0 commit comments

Comments
 (0)