v0.6.1 — Dual timeout for stuck agents#3
Conversation
Replace the single 30-min timer with two independent timers: - Idle timeout (AGENT_IDLE_TIMEOUT, default 10 min): reset on any stdout activity. Agent with no stdout for 10 min is considered stuck → killed. Catches the hung-waiting-on-API pattern that caused 30-40 min hangs. - Absolute ceiling (AGENT_ABSOLUTE_TIMEOUT, default 45 min): never resets. Hard cap regardless of activity. Prevents runaway agents producing garbage output from running forever. Both timers log their reason (idle/absolute) for easier post-mortems. Both values are configurable via env vars. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis change replaces a single 30-minute timeout with two concurrent timers for agent containers: an idle timeout (default 10 min, resets on stdout activity) and an absolute ceiling (default 45 min, never resets). Config, runner logic, tests, changelog, and package version updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Runner
participant Agent as Agent Process
participant IdleTimer as Idle Timer
participant AbsoluteTimer as Absolute Timer
participant Logger as Logger
Runner->>Agent: start agent
Runner->>IdleTimer: start (resets on stdout)
Runner->>AbsoluteTimer: start (never resets)
Agent-->>Runner: stdout data
Runner->>IdleTimer: reset
Note right of AbsoluteTimer: continues counting
IdleTimer->>Runner: idle timeout elapsed (if no stdout)
Runner->>Logger: log "terminated (idle)"
Runner->>Agent: SIGTERM / SIGKILL (if needed)
AbsoluteTimer->>Runner: absolute timeout elapsed (if reached)
Runner->>Logger: log "terminated (absolute)"
Runner->>Agent: SIGTERM / SIGKILL (if needed)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/container-runner.ts (1)
437-441:⚠️ Potential issue | 🟡 MinorError message reports
configTimeoutbut could be misleading.When the idle timeout fires, the error message reports
configTimeout(potentially 30min) rather than the actual idle timeout (10min) that triggered the kill. This could confuse post-mortem analysis.Proposed fix
Consider tracking which timeout fired and using the appropriate duration in the error message:
+ const actualTimeout = hadStreamingOutput ? absoluteTimeoutMs : idleTimeoutMs; resolve({ status: 'error', result: null, - error: `Agent timed out after ${configTimeout}ms`, + error: `Agent timed out after ${actualTimeout}ms (${hadStreamingOutput ? 'absolute' : 'idle'})`, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/container-runner.ts` around lines 437 - 441, The error message uses configTimeout even when the idle timer caused the kill; update the timeout-handling in the function around the resolve({...}) call so it tracks which timer fired (e.g., set a variable/enum like timeoutType or lastTimeout and store the duration that triggered the stop) and then use that tracked value and label in the error string instead of always using configTimeout; locate the idle timeout handler and the config timeout handler in container-runner (references: configTimeout, idleTimeout, and the resolve({ status: 'error', result: null, error: ... }) invocation) and set the appropriate duration/label before calling resolve so the message reflects the actual timeout that fired.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/container-runner.ts`:
- Around line 437-441: The error message uses configTimeout even when the idle
timer caused the kill; update the timeout-handling in the function around the
resolve({...}) call so it tracks which timer fired (e.g., set a variable/enum
like timeoutType or lastTimeout and store the duration that triggered the stop)
and then use that tracked value and label in the error string instead of always
using configTimeout; locate the idle timeout handler and the config timeout
handler in container-runner (references: configTimeout, idleTimeout, and the
resolve({ status: 'error', result: null, error: ... }) invocation) and set the
appropriate duration/label before calling resolve so the message reflects the
actual timeout that fired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 313b8cee-2b5f-483f-a113-b03b715556a6
📒 Files selected for processing (5)
CHANGELOG.mdpackage.jsonsrc/config.tssrc/container-runner.test.tssrc/container-runner.ts
Track timeoutReason ('idle' | 'absolute') and use the corresponding
duration in the error string and log, instead of always reporting
configTimeout regardless of which timer fired.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/container-runner.ts (1)
426-449:⚠️ Potential issue | 🟠 MajorDon’t turn a timed-out streamed run into
success.
hadStreamingOutputonly means one marker pair parsed, not that a terminal success was seen. If the agent streams partial output and then hits a timeout, this branch resolves{ status: 'success' }, so the timeout disappears insrc/index.ts:306-336andsrc/task-scheduler.ts:230-265, which only inspect the finaloutput.status/error.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/container-runner.ts` around lines 426 - 449, The current timeout branch treats hadStreamingOutput as a terminal success (in the if (hadStreamingOutput) block) which hides timeouts; update that branch so after logging you wait for outputChain to finish but then resolve with a timeout error (status: 'error', include error message `Agent timed out after ${timeoutMs}ms (${timeoutLabel})`) instead of returning status: 'success'; preserve newSessionId in the error payload if needed and keep the existing logger.info call but do not convert a partial streamed run into success—use the same error shape used in the non-streaming timeout branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/container-runner.ts`:
- Around line 364-368: The code currently uses Math.max(configTimeout,
AGENT_ABSOLUTE_TIMEOUT) so any smaller AGENT_ABSOLUTE_TIMEOUT (or smaller
group.containerConfig.timeout) is ignored; change absoluteTimeoutMs to use
Math.min(configTimeout, AGENT_ABSOLUTE_TIMEOUT) and then compute idleTimeoutMs
as Math.min(AGENT_IDLE_TIMEOUT, absoluteTimeoutMs) (referencing
absoluteTimeoutMs, idleTimeoutMs, configTimeout, AGENT_ABSOLUTE_TIMEOUT,
AGENT_IDLE_TIMEOUT) so both absolute and idle ceilings can be lowered by config
or env.
- Around line 370-382: The timeout handler killOnTimeout should be made one-shot
and should track real process exit via the child process 'close' event instead
of relying on ChildProcess.killed; update killOnTimeout (and any surrounding
scope) to guard with a boolean like timeoutHandled so only the first invocation
sets timedOut and timeoutReason and performs agentProcess.kill('SIGTERM'), then
attach a one-time agentProcess.once('close', ...) listener to detect actual exit
and only then (if still running) escalate to SIGKILL after a 15s timer; remove
checks against agentProcess.killed and ensure the 'close' handler reads the
preserved timeoutReason to log the final outcome.
---
Outside diff comments:
In `@src/container-runner.ts`:
- Around line 426-449: The current timeout branch treats hadStreamingOutput as a
terminal success (in the if (hadStreamingOutput) block) which hides timeouts;
update that branch so after logging you wait for outputChain to finish but then
resolve with a timeout error (status: 'error', include error message `Agent
timed out after ${timeoutMs}ms (${timeoutLabel})`) instead of returning status:
'success'; preserve newSessionId in the error payload if needed and keep the
existing logger.info call but do not convert a partial streamed run into
success—use the same error shape used in the non-streaming timeout branch.
| const killOnTimeout = (reason: 'idle' | 'absolute') => { | ||
| timedOut = true; | ||
| timeoutReason = reason; | ||
| logger.error( | ||
| { group: group.name, processName }, | ||
| 'Agent timeout, killing process', | ||
| { group: group.name, processName, reason }, | ||
| reason === 'idle' | ||
| ? 'Agent idle timeout — no stdout for too long, killing process' | ||
| : 'Agent absolute timeout — hard ceiling reached, killing process', | ||
| ); | ||
| agentProcess.kill('SIGTERM'); | ||
| setTimeout(() => { | ||
| if (!agentProcess.killed) { | ||
| agentProcess.kill('SIGKILL'); | ||
| } | ||
| if (!agentProcess.killed) agentProcess.kill('SIGKILL'); | ||
| }, 15000); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Node.js, what does ChildProcess.killedmean? Does it becometruewhenkill() successfully sends a signal, or only after the child process has actually exited?
💡 Result:
In Node.js, ChildProcess.killed is a boolean property that becomes true when the subprocess.kill method is called successfully on that specific ChildProcess instance (i.e., when the underlying kill operation returns success, such as libuv error code 0). It does not become true if the process is killed externally (e.g., via process.kill(child.pid) or OS kill command), nor does it wait for the child process to actually exit. The property indicates that a kill signal was sent via the ChildProcess's kill method, not that the process has terminated. The child may continue running after killed is set to true, until it handles the signal and exits. To check if the process has exited, use the 'exit' or 'close' events, child.exitCode !== null, or signalCode. kill itself returns true if the signal send succeeded (ESRCH if already dead), but killed is set only on success before exit.
Citations:
- 1:
childProcess.killedshould betrueafterprocess.kill(childProcess.pid)is called nodejs/node#27490 - 2: https://bun.com/reference/node/child_process/ChildProcessByStdio/killed
- 3: https://github.com/nodejs/node/blob/main/lib/internal/child_process.js
- 4: https://runebook.dev/en/articles/node/child_process/subprocesskilled
🏁 Script executed:
cd /repo && sed -n '350,420p' src/container-runner.tsRepository: b1rdmania/ghostclaw
Length of output: 118
🏁 Script executed:
pwd && find . -name "container-runner.ts" -type f 2>/dev/null | head -5Repository: b1rdmania/ghostclaw
Length of output: 107
🏁 Script executed:
sed -n '350,420p' ./src/container-runner.tsRepository: b1rdmania/ghostclaw
Length of output: 2452
🏁 Script executed:
sed -n '400,430p' ./src/container-runner.tsRepository: b1rdmania/ghostclaw
Length of output: 1205
Make timeout termination one-shot and track real process exit.
ChildProcess.killed becomes true when kill() successfully sends the signal, not when the child has exited. The 15s fallback check if (!agentProcess.killed) will always skip SIGKILL since killed was already set to true by the initial SIGTERM call—even if the process is still running. Use the 'close' event to track actual termination.
Additionally, killOnTimeout() is re-entrant: both the idle and absolute timers can fire independently and call it, allowing the second call to overwrite timeoutReason before the 'close' handler reads it.
Suggested fix
+ let exited = false;
+
const killOnTimeout = (reason: 'idle' | 'absolute') => {
+ if (timedOut) return;
timedOut = true;
timeoutReason = reason;
+ clearTimeout(idleTimer);
+ clearTimeout(absoluteTimer);
logger.error(
{ group: group.name, processName, reason },
reason === 'idle'
@@ -381,7 +388,7 @@
);
agentProcess.kill('SIGTERM');
setTimeout(() => {
- if (!agentProcess.killed) agentProcess.kill('SIGKILL');
+ if (!exited) agentProcess.kill('SIGKILL');
}, 15000);
};
@@ -399,6 +406,7 @@
agentProcess.on('close', (code) => {
+ exited = true;
clearTimeout(idleTimer);
clearTimeout(absoluteTimer);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const killOnTimeout = (reason: 'idle' | 'absolute') => { | |
| timedOut = true; | |
| timeoutReason = reason; | |
| logger.error( | |
| { group: group.name, processName }, | |
| 'Agent timeout, killing process', | |
| { group: group.name, processName, reason }, | |
| reason === 'idle' | |
| ? 'Agent idle timeout — no stdout for too long, killing process' | |
| : 'Agent absolute timeout — hard ceiling reached, killing process', | |
| ); | |
| agentProcess.kill('SIGTERM'); | |
| setTimeout(() => { | |
| if (!agentProcess.killed) { | |
| agentProcess.kill('SIGKILL'); | |
| } | |
| if (!agentProcess.killed) agentProcess.kill('SIGKILL'); | |
| }, 15000); | |
| let exited = false; | |
| const killOnTimeout = (reason: 'idle' | 'absolute') => { | |
| if (timedOut) return; | |
| timedOut = true; | |
| timeoutReason = reason; | |
| clearTimeout(idleTimer); | |
| clearTimeout(absoluteTimer); | |
| logger.error( | |
| { group: group.name, processName, reason }, | |
| reason === 'idle' | |
| ? 'Agent idle timeout — no stdout for too long, killing process' | |
| : 'Agent absolute timeout — hard ceiling reached, killing process', | |
| ); | |
| agentProcess.kill('SIGTERM'); | |
| setTimeout(() => { | |
| if (!exited) agentProcess.kill('SIGKILL'); | |
| }, 15000); | |
| }; | |
| // ... (additional code between killOnTimeout and the close handler) | |
| agentProcess.on('close', (code) => { | |
| exited = true; | |
| clearTimeout(idleTimer); | |
| clearTimeout(absoluteTimer); | |
| // ... (rest of close handler) | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/container-runner.ts` around lines 370 - 382, The timeout handler
killOnTimeout should be made one-shot and should track real process exit via the
child process 'close' event instead of relying on ChildProcess.killed; update
killOnTimeout (and any surrounding scope) to guard with a boolean like
timeoutHandled so only the first invocation sets timedOut and timeoutReason and
performs agentProcess.kill('SIGTERM'), then attach a one-time
agentProcess.once('close', ...) listener to detect actual exit and only then (if
still running) escalate to SIGKILL after a 15s timer; remove checks against
agentProcess.killed and ensure the 'close' handler reads the preserved
timeoutReason to log the final outcome.
- 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 participant
Problem
Agents were hanging for 30-40 minutes before being killed. The single 30-min timer only reset on complete output marker pairs — a genuinely stuck agent (hung waiting on an API, no stdout at all) sat there until the full timer expired.
Solution
Two independent timers replace the single one:
AGENT_IDLE_TIMEOUT)AGENT_ABSOLUTE_TIMEOUT)Both are configurable via env vars. Both log their reason (
idlevsabsolute) for easier post-mortems.What changes
src/config.ts— two new exported constantssrc/container-runner.ts—idleTimer(reset on stdout data) +absoluteTimer(never reset) replace the old singletimeoutsrc/container-runner.test.ts— updated tests + new "stdout resets idle timer" test (449 passing)Test plan
/updatein Telegram — bot pulls and rebuildsidleorabsoluteas reason on any timeouts.env— can override withAGENT_IDLE_TIMEOUT=300000(5 min) orAGENT_ABSOLUTE_TIMEOUT=3600000(60 min) if needed🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Chores