fix(cli): auto-resume network reconnects#10358
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| ).catch(() => false) | ||
| } | ||
|
|
||
| async function resume(input: { requestID: QuestionID }) { |
There was a problem hiding this comment.
WARNING: resume() does not propagate or respect the abort signal from the original ask() call. While the current implicit guard (if (!req || !req.info.restored) return after looking up the pending map) correctly prevents double-reply when the request has already been rejected/aborted, the 10-second setTimeout keeps a live promise in flight even after the user has already cancelled (Esc). This is not a crash, but it means a dangling async task exists for 10 seconds after every abort.
Consider passing the abort signal through to resume() and using it to short-circuit the wait:
async function resume(input: { requestID: QuestionID; abort: AbortSignal }) {
await new Promise<void>((resolve) => {
const id = setTimeout(resolve, RESUME_MS)
input.abort.addEventListener("abort", () => { clearTimeout(id); resolve() }, { once: true })
})
if (input.abort.aborted) return
// ... rest
}| setCountdown(10) | ||
| return | ||
| } | ||
| const restored = props.request.time.restored ?? Date.now() |
There was a problem hiding this comment.
SUGGESTION: The restored timestamp fallback ?? Date.now() creates a subtle discrepancy: if props.request.time.restored is undefined (e.g. the TUI received the restored=true event before the time.restored field was applied), the countdown is calculated from the client's current time rather than the server's authoritative restore timestamp. This could show a full 10-second countdown even when the server's auto-resume will fire in less than 10 seconds.
Since time.restored is set atomically alongside restored = true in restore() on the server side, and the TUI sync now sets both fields in the same event handler, this fallback should rarely trigger — but when it does it will desync the displayed countdown from the actual auto-resume. Consider logging a warning or asserting the value is present rather than silently falling back.
| globalThis.setTimeout = timer | ||
| }) | ||
|
|
||
| function manual() { |
There was a problem hiding this comment.
SUGGESTION: The manual() helper only intercepts setTimeout (used by resume()), but does not intercept setInterval or clearTimeout. If resume() ever changes to use setInterval internally, the mock won't capture it. The approach works for the current implementation, but it's worth noting this is testing an implementation detail of resume() (its use of setTimeout) rather than observable behavior.
Also, globalThis.setTimeout is patched at the module level, so if tests run concurrently (Bun runs test files in parallel workers), the global mutation could leak between test files. The afterEach restore mitigates leaks within this file, but concurrent tests in the same worker could still interfere. This is acceptable for the current test setup, just worth being aware of.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge All 4 issues from the previous review have been resolved. The latest commit ( Files Reviewed (5 core files + formatting)
Reviewed by claude-sonnet-4.6 · 165,883 tokens Review guidance: REVIEW.md from base branch |
1 participant
Context
CLI network reconnects currently require users to manually press Enter after connectivity returns, which blocks unattended workflows. This changes the reconnect flow so users still have a short cancellation window, but interrupted turns resume automatically once the network is back.
Implementation
Network waits now record an authoritative restore timestamp and auto-reply after 10 seconds unless the user cancels. The TUI shows the countdown from that server timestamp, Enter still resumes immediately, and Esc still stops the turn.
Network detection was broadened to cover additional transient socket/browser-style errors and aggregate causes, and recovery probing now uses short HTTPS checks instead of a single DNS lookup. The provider connection timeout is also increased to 3 minutes.
Screenshots
How to Test
Network reconnectedwith a 10 second retry countdown.bun test ./test/session/network.test.ts ./test/kilocode/run-network.test.tsfrompackages/opencode/.bun run typecheckfrompackages/opencode/.Get in Touch