Skip to content

Commit cb7c7aa

Browse files
b1rdmaniaclaude
andcommitted
fix(timeout): apply CodeRabbit review fixes to dual timeout
- Math.min for both timeouts so config can lower below the defaults (absoluteTimeoutMs = min(configTimeout, AGENT_ABSOLUTE_TIMEOUT), idleTimeoutMs = min(AGENT_IDLE_TIMEOUT, absoluteTimeoutMs)) - One-shot kill guard: timeoutHandled boolean prevents double-fire when idle and absolute timers both trigger - Timeout always resolves as error, even when hadStreamingOutput is true — hides the timeout from callers; index.ts outputSentToUser guard already prevents incorrect cursor rollback in this case - Update test: 'absolute timeout after output' now asserts error status Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
1 parent 0b6d33f commit cb7c7aa

2 files changed

Lines changed: 33 additions & 30 deletions

File tree

src/container-runner.test.ts

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ describe('container-runner timeout behavior', () => {
118118
vi.useRealTimers();
119119
});
120120

121-
it('absolute timeout after output resolves as success', async () => {
121+
it('absolute timeout after output resolves as error', async () => {
122122
const onOutput = vi.fn(async () => {});
123123
const resultPromise = runContainerAgent(
124124
testGroup,
@@ -146,8 +146,9 @@ describe('container-runner timeout behavior', () => {
146146
await vi.advanceTimersByTimeAsync(10);
147147

148148
const result = await resultPromise;
149-
expect(result.status).toBe('success');
150-
expect(result.newSessionId).toBe('session-123');
149+
// Timeout always resolves as error — index.ts outputSentToUser guard prevents cursor rollback
150+
expect(result.status).toBe('error');
151+
expect(result.error).toContain('timed out');
151152
expect(onOutput).toHaveBeenCalledWith(
152153
expect.objectContaining({ result: 'Here is my response' }),
153154
);

src/container-runner.ts

Lines changed: 29 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -361,13 +361,13 @@ export async function runContainerAgent(
361361
let timeoutReason: 'idle' | 'absolute' | null = null;
362362
let hadStreamingOutput = false;
363363
const configTimeout = group.containerConfig?.timeout || CONTAINER_TIMEOUT;
364-
const idleTimeoutMs = Math.min(
365-
AGENT_IDLE_TIMEOUT,
366-
Math.max(configTimeout, AGENT_ABSOLUTE_TIMEOUT),
367-
);
368-
const absoluteTimeoutMs = Math.max(configTimeout, AGENT_ABSOLUTE_TIMEOUT);
364+
const absoluteTimeoutMs = Math.min(configTimeout, AGENT_ABSOLUTE_TIMEOUT);
365+
const idleTimeoutMs = Math.min(AGENT_IDLE_TIMEOUT, absoluteTimeoutMs);
369366

367+
let timeoutHandled = false;
370368
const killOnTimeout = (reason: 'idle' | 'absolute') => {
369+
if (timeoutHandled) return;
370+
timeoutHandled = true;
371371
timedOut = true;
372372
timeoutReason = reason;
373373
logger.error(
@@ -377,8 +377,15 @@ export async function runContainerAgent(
377377
: 'Agent absolute timeout — hard ceiling reached, killing process',
378378
);
379379
agentProcess.kill('SIGTERM');
380+
agentProcess.once('close', () => {
381+
// Process already dead, nothing more to do
382+
});
380383
setTimeout(() => {
381-
if (!agentProcess.killed) agentProcess.kill('SIGKILL');
384+
try {
385+
agentProcess.kill('SIGKILL');
386+
} catch {
387+
// already dead
388+
}
382389
}, 15000);
383390
};
384391

@@ -423,30 +430,25 @@ export async function runContainerAgent(
423430
? `idle timeout (${idleTimeoutMs}ms no stdout)`
424431
: `absolute timeout (${absoluteTimeoutMs}ms ceiling)`;
425432

426-
if (hadStreamingOutput) {
427-
logger.info(
428-
{ group: group.name, processName, duration, code, timeoutReason },
429-
`Agent timed out after output (${timeoutLabel})`,
430-
);
431-
outputChain.then(() => {
432-
resolve({
433-
status: 'success',
434-
result: null,
435-
newSessionId,
436-
});
437-
});
438-
return;
439-
}
440-
441433
logger.error(
442-
{ group: group.name, processName, duration, code, timeoutReason },
443-
`Agent timed out with no output (${timeoutLabel})`,
434+
{
435+
group: group.name,
436+
processName,
437+
duration,
438+
code,
439+
timeoutReason,
440+
hadStreamingOutput,
441+
},
442+
`Agent timed out (${timeoutLabel})`,
444443
);
445444

446-
resolve({
447-
status: 'error',
448-
result: null,
449-
error: `Agent timed out after ${timeoutMs}ms (${timeoutLabel})`,
445+
outputChain.then(() => {
446+
resolve({
447+
status: 'error',
448+
result: null,
449+
error: `Agent timed out after ${timeoutMs}ms (${timeoutLabel})`,
450+
newSessionId,
451+
});
450452
});
451453
return;
452454
}

0 commit comments

Comments
 (0)