Track cancelled question invocations through process closure

Author: mchwang

docs/implementation/read-only-review.md

@@ -92,3 +92,7 @@ PR #11 review round 3 reproduced retrying a locally active job after its persist
 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.
 
 PR #11 round 5 confirmed the shutdown fix and made the timeout-summary concern concrete: the CLI adapter replaced the abort reason with generic cancellation. Direct adapter regressions reproduced this for timeout and shutdown reasons. The adapter now preserves Error-valued abort reasons; provider launch errors remain sanitized.
+
+PR #11 round 6 reproduced releasing a concurrency slot while a cancelled invocation was still unsettled. Cancellation now persists the visible failure promptly but retains the job until the invocation settles; retries remain blocked and shutdown awaits it. The CLI adapter defers rejection and temporary-directory cleanup until child close, including abort and output-limit failures. Regressions cover late settlement and abort-error-before-close ordering. Injected agents must settle after cancellation; production adapters terminate with SIGKILL and await closure.
+
+The review's polling-efficiency observation is tracked separately in issue #12: answer polling currently rebuilds the full review, and the follow-up will measure and remove that work while preserving snapshot metadata and stale-response protection.

runner/question-agent.ts

@@ -18,11 +18,11 @@ export function cliQuestionAgent(provider: Provider): QuestionAgent {
       const stdout=await new Promise<string>((resolve,reject)=>{
         const env={...process.env};delete env.CLAUDECODE;delete env.NODE_OPTIONS;
         const child=spawn(provider,agentArguments(provider),{cwd,env,stdio:['pipe','pipe','pipe'],signal,killSignal:'SIGKILL'});
-        const chunks:Buffer[]=[];let bytes=0,diagnostic='';
-        child.stdout.on('data',(chunk:Buffer)=>{bytes+=chunk.length;if(bytes>1024*1024){child.kill('SIGKILL');reject(new Error('Agent output exceeded its limit.'));}else chunks.push(chunk);});
+        const chunks:Buffer[]=[];let bytes=0,diagnostic='';let failure:Error|undefined;
+        child.stdout.on('data',(chunk:Buffer)=>{bytes+=chunk.length;if(bytes>1024*1024){failure ??= new Error('Agent output exceeded its limit.');child.kill('SIGKILL');}else chunks.push(chunk);});
         child.stderr.on('data',(chunk:Buffer)=>{diagnostic=(diagnostic+chunk.toString()).slice(-2000);});
-        child.on('error',error=>reject(signal.aborted && signal.reason instanceof Error ? signal.reason : new Error(signal.aborted?'Agent cancelled.':`Could not start ${provider}. Check that its CLI is installed and signed in. (${error.name})`)));
-        child.on('close',code=>code===0?resolve(Buffer.concat(chunks).toString('utf8')):reject(new Error(`${provider} exited with status ${code}. Check its login and usage limits.${/auth|login|sign.in/i.test(diagnostic)?' Authentication may be required.':''}`)));
+        child.on('error',error=>{failure = signal.aborted && signal.reason instanceof Error ? signal.reason : new Error(signal.aborted?'Agent cancelled.':`Could not start ${provider}. Check that its CLI is installed and signed in. (${error.name})`);});
+        child.on('close',code=>failure?reject(failure):code===0?resolve(Buffer.concat(chunks).toString('utf8')):reject(new Error(`${provider} exited with status ${code}. Check its login and usage limits.${/auth|login|sign.in/i.test(diagnostic)?' Authentication may be required.':''}`)));
         child.stdin.on('error',()=>{});child.stdin.end(prompt);
       });
       if(provider==='claude') {

runner/questions.ts

@@ -34,19 +34,24 @@ export class Questions {
     this.service.store.beginAnswer(this.service.config.identity,id,attempt,provider??undefined);
     if(this.running.size>=2){this.service.store.finishAnswer(this.service.config.identity,id,attempt,{status:'failed',error:'Two questions are already running. Retry when one finishes.'});return;}
     const timeout=setTimeout(()=>controller.abort(new Error('Agent timed out. Try again.')),120_000);
+    let invocation: Promise<string> | undefined;
     const done=(async()=>{
       try {
         if(!agent) throw new Error('Choose a question agent in Settings, then retry.');
         const aborted = new Promise<never>((_,reject)=>controller.signal.addEventListener('abort',()=>reject(controller.signal.reason),{once:true}));
-        const text=await Promise.race([agent(questionPrompt(view,note),controller.signal),aborted]);
+        invocation = agent(questionPrompt(view,note),controller.signal);
+        const text=await Promise.race([invocation,aborted]);
         if(typeof text!=='string'||!text.trim()||text.length>24000) throw new Error('Agent returned an empty or oversized answer.');
         this.service.store.finishAnswer(this.service.config.identity,id,attempt,{status:'complete',text:text.trim()});
       } catch(error) {
         this.service.store.finishAnswer(this.service.config.identity,id,attempt,{status:'failed',error:(error instanceof Error?error.message:'Agent failed.').slice(0,1000)});
       } finally {clearTimeout(timeout);}
     })();
-    this.running.set(id,{controller,done});
-    void done.finally(()=>this.running.delete(id));
+    const settled = done.finally(async () => {
+      await invocation?.catch(() => {});
+      this.running.delete(id);
+    });
+    this.running.set(id,{controller,done:settled});
   }
   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));}
 }

test/browser/review.spec.ts

@@ -213,7 +213,7 @@ test('Clear selection removes native text selection as well as selected lines',a
  await expect(page.getByRole('group',{name:'Selected code'})).toBeHidden();
 });
 test('ignores question polls started before a newer submission',async({page})=>{
- const config=app.service.config;await app.close();app=await startServer(config,0,()=>new Promise(()=>{}));
+ const config=app.service.config;await app.close();app=await startServer(config,0,(_prompt,signal)=>new Promise((_,reject)=>signal.addEventListener('abort',()=>reject(signal.reason),{once:true})));
  await page.goto(app.url);await page.getByLabel('Question about this item').fill('First question');await page.getByRole('button',{name:'Ask agent',exact:true}).click();await expect(page.getByText('Agent · Answering…',{exact:true})).toBeVisible();
  let release!:()=>void,arrived!:()=>void;const held=new Promise<void>(r=>release=r),captured=new Promise<void>(r=>arrived=r);
  await page.route('**/api/questions',async route=>{const response=await route.fetch();arrived();await held;await route.fulfill({response});});

test/question-agent.test.ts

@@ -5,14 +5,19 @@ import { cliQuestionAgent } from '../runner/question-agent.ts';
 vi.mock('node:child_process',()=>({spawn:vi.fn()}));
 afterEach(()=>vi.clearAllMocks());
 it.each(['Agent timed out. Try again.','Server stopped. Retry the question.'])('preserves the cancellation reason: %s',async message=>{
+ let childProcess:EventEmitter;
  vi.mocked(spawn).mockImplementation(((_command:unknown,_args:unknown,options:{signal:AbortSignal})=>{
   const child=Object.assign(new EventEmitter(),{stdout:new EventEmitter(),stderr:new EventEmitter(),stdin:{on:vi.fn(),end:vi.fn()},kill:vi.fn()});
+  childProcess=child;
   options.signal.addEventListener('abort',()=>child.emit('error',new Error('The operation was aborted')),{once:true});
   return child;
  }) as unknown as typeof spawn);
  const controller=new AbortController();const answer=cliQuestionAgent('codex')('Question',controller.signal);
- const result=answer.catch(error=>error);
+ let settled=false;
+ const result=answer.catch(error=>error).finally(()=>{settled=true;});
  await vi.waitFor(()=>expect(spawn).toHaveBeenCalledOnce());
  controller.abort(new Error(message));
+ await new Promise(resolve=>setTimeout(resolve,20));expect(settled).toBe(false);
+ childProcess!.emit('close',null);
  expect((await result).message).toBe(message);
 });

test/questions.test.ts

@@ -10,6 +10,7 @@ import { agentArguments } from '../runner/question-agent.ts';
 vi.setConfig({testTimeout:15000});
 const roots:string[]=[], services:ReviewService[]=[], managers:Questions[]=[];
 afterEach(async()=>{for(const manager of managers.splice(0))await manager.close();services.splice(0).forEach(s=>s.close());roots.splice(0).forEach(root=>rmSync(root,{recursive:true,force:true}));vi.restoreAllMocks();});
+function waitForAbort(_prompt:string,signal:AbortSignal):Promise<string>{return new Promise((_,reject)=>signal.addEventListener('abort',()=>reject(signal.reason),{once:true}));}
 function fixture(){const root=mkdtempSync(join(tmpdir(),'codeboost-answers-'));roots.push(root);const service=new ReviewService(createDemo(join(root,'demo')));services.push(service);return service;}
 function question(service:ReviewService){const view=service.load();const segment=view.segments.find(s=>s.row==='P1'&&s.operation==='+')!;return service.act({action:'note',item:'P1',kind:'question',text:'Why cap the retry delay?',reference:{key:segment.key,start:segment.newLine,end:segment.newLine},token:view.token});}
 it('persists answers with plan, code, selected snippet and prior conversation context',async()=>{
@@ -30,7 +31,7 @@ it('fails visibly and retries without duplicating the question or accepting stal
  expect(service.store.getReviewNotes(service.config.identity)).toHaveLength(1);expect(service.store.getReviewNotes(service.config.identity)[0]!.answer!.text).toBe('Recovered answer');
 });
 it('prevents duplicate invocations and records interruption when the server stops',async()=>{
- const service=fixture(),asked=question(service);const manager=new Questions(service,()=>new Promise(()=>{}));managers.push(manager);manager.start(asked.createdNoteId!,asked);
+ const service=fixture(),asked=question(service);const manager=new Questions(service,waitForAbort);managers.push(manager);manager.start(asked.createdNoteId!,asked);
  expect(()=>manager.start(asked.createdNoteId!,asked)).toThrow(/already answering/);await manager.close();
  expect(service.store.getReviewNotes(service.config.identity)[0]!.answer?.error).toMatch(/Server stopped/);
 });
@@ -40,15 +41,15 @@ it('persists provider selection and restricts commands to fixed provider launch
  const codex=agentArguments('codex');expect(codex).toContain('read-only');expect(codex).toContain('features.shell_tool=false');expect(codex).toContain('features.plugins=false');
 });
 it('times out an unresponsive agent and allows expired pending attempts to be recovered',async()=>{
- const service=fixture(),asked=question(service);const manager=new Questions(service,()=>new Promise(()=>{}));managers.push(manager);
+ const service=fixture(),asked=question(service);const manager=new Questions(service,waitForAbort);managers.push(manager);
  vi.useFakeTimers();
  try {manager.start(asked.createdNoteId!,asked);await vi.advanceTimersByTimeAsync(120001);expect(service.store.getReviewNotes(service.config.identity)[0]!.answer?.error).toMatch(/timed out/);
  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');
  } finally {vi.useRealTimers();}
 });
 it('does not replace a locally running job when its persisted lease expires',async()=>{
  const service=fixture(),asked=question(service);let calls=0,signal:AbortSignal|undefined;
- const manager=new Questions(service,(_prompt,currentSignal)=>{calls++;signal=currentSignal;return new Promise(()=>{});});managers.push(manager);
+ const manager=new Questions(service,(_prompt,currentSignal)=>{calls++;signal=currentSignal;return waitForAbort(_prompt,currentSignal);});managers.push(manager);
  manager.start(asked.createdNoteId!,asked);
  const original=service.store.getReviewNotes(service.config.identity)[0]!.answer!.attempt;
  const later=Date.now()+130000;vi.spyOn(Date,'now').mockReturnValue(later);
@@ -58,7 +59,7 @@ it('does not replace a locally running job when its persisted lease expires',asy
 });
 it('rejects new work as soon as shutdown begins',async()=>{
  const service=fixture(),first=question(service),second=question(service);let calls=0;
- const manager=new Questions(service,()=>{calls++;return new Promise(()=>{});});managers.push(manager);
+ const manager=new Questions(service,(prompt,signal)=>{calls++;return waitForAbort(prompt,signal);});managers.push(manager);
  manager.start(first.createdNoteId!,first);
  const closing=manager.close();
  expect(()=>manager.start(second.createdNoteId!,second)).toThrow(/stopping/);
@@ -67,3 +68,17 @@ it('rejects new work as soon as shutdown begins',async()=>{
  expect(calls).toBe(1);
  expect(service.store.getReviewNotes(service.config.identity).find(note=>note.id===second.createdNoteId)?.answer).toBeUndefined();
 });
+it('keeps cancelled invocations tracked until they settle',async()=>{
+ const service=fixture(),asked=question(service);let settle!:(value:string)=>void;
+ const manager=new Questions(service,()=>new Promise(resolve=>settle=resolve));managers.push(manager);
+ vi.useFakeTimers();
+ try {
+  manager.start(asked.createdNoteId!,asked);await vi.advanceTimersByTimeAsync(120001);
+  expect(service.store.getReviewNotes(service.config.identity)[0]!.answer?.status).toBe('failed');
+  expect(()=>manager.start(asked.createdNoteId!,asked)).toThrow(/already answering/);
+  let closed=false;const closing=manager.close().then(()=>{closed=true;});
+  await Promise.resolve();await Promise.resolve();expect(closed).toBe(false);
+  settle('Late answer');await closing;expect(closed).toBe(true);
+  expect(service.store.getReviewNotes(service.config.identity)[0]!.answer?.status).toBe('failed');
+ } finally {settle('Cleanup');vi.useRealTimers();}
+});