Skip to content

Commit 35d819f

Browse files
committed
Preserve timeout and shutdown reasons in question adapters
1 parent 4bdbaf4 commit 35d819f

3 files changed

Lines changed: 21 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
@@ -90,3 +90,5 @@ PR #11 review round 2 confirmed the poll fix and raised an outdated-retry issue
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.
9191

9292
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.
93+
94+
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.

‎runner/question-agent.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ export function cliQuestionAgent(provider: Provider): QuestionAgent {
2121
const chunks:Buffer[]=[];let bytes=0,diagnostic='';
2222
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);});
2323
child.stderr.on('data',(chunk:Buffer)=>{diagnostic=(diagnostic+chunk.toString()).slice(-2000);});
24-
child.on('error',error=>reject(new Error(signal.aborted?'Agent cancelled.':`Could not start ${provider}. Check that its CLI is installed and signed in. (${error.name})`)));
24+
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})`)));
2525
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.':''}`)));
2626
child.stdin.on('error',()=>{});child.stdin.end(prompt);
2727
});

‎test/question-agent.test.ts‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
import { EventEmitter } from 'node:events';
2+
import { afterEach, expect, it, vi } from 'vitest';
3+
import { spawn } from 'node:child_process';
4+
import { cliQuestionAgent } from '../runner/question-agent.ts';
5+
vi.mock('node:child_process',()=>({spawn:vi.fn()}));
6+
afterEach(()=>vi.clearAllMocks());
7+
it.each(['Agent timed out. Try again.','Server stopped. Retry the question.'])('preserves the cancellation reason: %s',async message=>{
8+
vi.mocked(spawn).mockImplementation(((_command:unknown,_args:unknown,options:{signal:AbortSignal})=>{
9+
const child=Object.assign(new EventEmitter(),{stdout:new EventEmitter(),stderr:new EventEmitter(),stdin:{on:vi.fn(),end:vi.fn()},kill:vi.fn()});
10+
options.signal.addEventListener('abort',()=>child.emit('error',new Error('The operation was aborted')),{once:true});
11+
return child;
12+
}) as unknown as typeof spawn);
13+
const controller=new AbortController();const answer=cliQuestionAgent('codex')('Question',controller.signal);
14+
const result=answer.catch(error=>error);
15+
await vi.waitFor(()=>expect(spawn).toHaveBeenCalledOnce());
16+
controller.abort(new Error(message));
17+
expect((await result).message).toBe(message);
18+
});

0 commit comments

Comments
 (0)