Skip to content

Commit 5d15f75

Browse files
committed
Reject retries while the original question job is active
1 parent 8e06335 commit 5d15f75

3 files changed

Lines changed: 13 additions & 0 deletions

File tree

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -86,3 +86,5 @@ Selection actions now appear in a bordered floating toolbar beside the visible s
8686
PR #11 review round 1 reproduced a stale-poll race: a held question-list response hid a question submitted after the poll began. Review actions and refreshes now advance a generation counter; earlier poll successes and errors are ignored. The browser regression failed with one visible note instead of two before the fix.
8787

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.
89+
90+
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.

‎runner/questions.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ export class Questions {
2222
private agent?: QuestionAgent;
2323
constructor(service: ReviewService, agent?: QuestionAgent) { this.service=service; this.agent=agent; }
2424
start(id: string, view: ReturnType<ReviewService['load']>) {
25+
if (this.running.has(id)) throw new Error('Agent is already answering this question.');
2526
const note = view.notes.find(n=>n.id===id && n.kind==='question');
2627
if (!note) throw new Error('Question not found.');
2728
if (note.snapshotId!==view.snapshot.id || note.revision!==view.plan.revision) throw new Error('This question belongs to an older review. Ask again against the current code.');

‎test/questions.test.ts‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -46,3 +46,13 @@ it('times out an unresponsive agent and allows expired pending attempts to be re
4646
service.store.beginAnswer(service.config.identity,asked.createdNoteId!,'interrupted');await vi.advanceTimersByTimeAsync(125001);service.store.beginAnswer(service.config.identity,asked.createdNoteId!,'replacement');service.store.finishAnswer(service.config.identity,asked.createdNoteId!,'interrupted',{status:'complete',text:'Old answer'});expect(service.store.getReviewNotes(service.config.identity)[0]!.answer?.attempt).toBe('replacement');
4747
} finally {vi.useRealTimers();}
4848
});
49+
it('does not replace a locally running job when its persisted lease expires',async()=>{
50+
const service=fixture(),asked=question(service);let calls=0,signal:AbortSignal|undefined;
51+
const manager=new Questions(service,(_prompt,currentSignal)=>{calls++;signal=currentSignal;return new Promise(()=>{});});managers.push(manager);
52+
manager.start(asked.createdNoteId!,asked);
53+
const original=service.store.getReviewNotes(service.config.identity)[0]!.answer!.attempt;
54+
const later=Date.now()+130000;vi.spyOn(Date,'now').mockReturnValue(later);
55+
expect(()=>manager.start(asked.createdNoteId!,asked)).toThrow(/already answering/);
56+
expect(calls).toBe(1);expect(service.store.getReviewNotes(service.config.identity)[0]!.answer!.attempt).toBe(original);
57+
await manager.close();expect(signal?.aborted).toBe(true);expect(service.store.getReviewNotes(service.config.identity)[0]!.answer?.error).toMatch(/Server stopped/);
58+
});

0 commit comments

Comments
 (0)