fix(kiro): improve timeout handling with activity-based idle detection #145
Conversation
- Increase total timeout from 10s to 20s to handle slow cold starts - Add 5s idle timeout that resets when output is received - Use readabilityHandler callbacks for non-blocking output tracking - Only throw timeout error if no output was ever received - Accumulate output progressively via thread-safe ActivityState class This fixes timeouts on first run when LoginShellPathCache needs to capture the user's PATH, which combined with kiro-cli startup time could exceed the previous 10s limit. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0bb8dd860
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| while process.isRunning, Date() < deadline { | ||
| // Idle timeout: if we got output but then it went silent | ||
| if state.hasReceivedOutput, | ||
| Date().timeIntervalSince(state.lastActivityAt) >= idleTimeout | ||
| { | ||
| // Process went idle after producing output - likely done or stuck | ||
| break | ||
| } | ||
| Thread.sleep(forTimeInterval: 0.1) | ||
| } | ||
|
|
||
| // Clean up handlers | ||
| stdoutPipe.fileHandleForReading.readabilityHandler = nil | ||
| stderrPipe.fileHandleForReading.readabilityHandler = nil | ||
|
|
||
| if process.isRunning { | ||
| process.terminate() | ||
| process.waitUntilExit() | ||
| continuation.resume(throwing: KiroStatusProbeError.timeout) | ||
| return | ||
| // Only throw timeout if we never got output | ||
| if !state.hasReceivedOutput { | ||
| continuation.resume(throwing: KiroStatusProbeError.timeout) | ||
| return |
There was a problem hiding this comment.
The new idle-timeout path breaks out of the loop and then terminates the process, but only throws a timeout when no output was received. If kiro-cli prints an initial banner and then takes >5s before producing the full /usage output (e.g., slow network after startup), this will kill the process and still return partial stdout/stderr as a “successful” result. That makes runUsageCommand/parse operate on incomplete data and can surface bogus usage or parse errors without any timeout signal. Consider treating an idle-timeout termination as a timeout error (or at least checking for a clean exit) instead of returning partial output.
Useful? React with 👍 / 👎.
1 participant
Summary
Problem
On first run, CodexBar's
LoginShellPathCachespawns an interactive login shell to capture the user's PATH (including~/.local/binwherekiro-clilives). This adds ~2 seconds of latency. Combined withkiro-cli's own startup time, it could exceed the previous 10-second timeout.Solution
Implemented a hybrid timeout approach similar to
TTYCommandRunner:readabilityHandlercallbacks for non-blocking output monitoringTest plan
KiroStatusProbeTestspasskiro-cliinstalled and logged in🤖 Generated with Claude Code