fix: reduce post-completion idle watchdog and add cleanup timeouts to prevent Copilot CLI hang on exit#44254
Conversation
This comment has been minimized.
This comment has been minimized.
|
Hey A couple of things to address before this is ready for review:
When you're ready to fill in the implementation, here's a prompt you can use:
|
… prevent Copilot CLI hang on exit - Reduce SDK_POST_COMPLETION_IDLE_MS_DEFAULT from 5 minutes to 30 seconds so the watchdog fires quickly after the agent completes, well before the step timeout fires - Add 5-second cleanup timeout to session.disconnect() and client.stop() in the finally block so the driver does not hang when the SDK server is unresponsive after sendAndWait times out - Add regression test verifying the cleanup timeout prevents a hung client.stop() from blocking the exit path Closes #44243 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ess and log warning Address code review feedback: the cleanup helper now returns a boolean (true = settled in time, false = timed out) and logs a warning message when the timeout fires, making timeout conditions observable in logs. The `catch` blocks remain for best-effort semantics. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR prevents successful Copilot SDK-driven runs from being reported as failures due to the Copilot CLI hanging on exit, by tightening the post-completion watchdog and bounding cleanup operations that can otherwise block forever.
Changes:
- Reduced the default post-completion idle watchdog from 5 minutes to 30 seconds to beat typical GitHub Actions step timeouts.
- Added a per-cleanup 5s timeout wrapper around
session.disconnect()andclient.stop()to avoid indefinite hangs in thefinallyblock. - Added a regression test covering the “
client.stop()never resolves” hang scenario.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/copilot_sdk_session.cjs | Shortens the post-completion watchdog and adds timeout-bounded best-effort cleanup in the finally block. |
| actions/setup/js/copilot_sdk_driver.test.cjs | Adds a regression test ensuring cleanup timeout prevents hangs when client.stop() never resolves. |
Review details
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Low
|
@copilot please run the
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed in bbe84f5: the cleanup timeout helper now clears and unreferences its deadline timer when cleanup settles, and the JS regression tests cover the prompt-cleanup path. Local |
|
@copilot revert lock files |
|
\n@copilot please run the skill, address unresolved review comments, and rerun checks once the branch is up to date.
|
🤖 PR Triage
Score breakdown: Impact 38 + Urgency 22 + Quality 14 Assessment: Fixes Copilot CLI hang on exit — every successful run is marked as failed due to a watchdog timing race and blocking Next step: Confirm CI green, then fast-track merge.
|
|
@copilot run pr-finisher skill |
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done. I ran the pr-finisher pass on the current head, replied on the unresolved Copilot review thread, and revalidated locally with |
|
🎉 This pull request is included in a new release. Release: |
The Impeccable Skills Reviewer was completing its review successfully but the Copilot CLI process never exited, causing every run to be killed by the 15-minute hard step timeout — a false failure on otherwise successful reviews.
Root cause
Two compounding issues in
copilot_sdk_session.cjs:Watchdog too slow:
SDK_POST_COMPLETION_IDLE_MS_DEFAULTwas 5 minutes. If the agent finishes at T+10min with a 15-min step, the watchdog fires at T+15min — racing the timeout instead of beating it.client.stop()can hang: AftersendAndWaittimes out, thefinallyblock callsawait client.stop(). When the SDK server is unresponsive this never settles, blocking the process until the step timeout kills it.Changes
actions/setup/js/copilot_sdk_session.cjsReduce
SDK_POST_COMPLETION_IDLE_MS_DEFAULTfrom5 * 60 * 1000→30 * 1000. The watchdog guards against false positives via three conditions (output collected, no in-flight tool calls, not mid-inference turn), so 30 s is safe in practice.Add 5-second cleanup timeout to
session.disconnect()andclient.stop()in thefinallyblock. UsesPromise.raceso a hung server unblocks the driver within a bounded window, with a warning logged on timeout:actions/setup/js/copilot_sdk_driver.test.cjsclient.stop()never resolves" —client.stop()returns a promise that never settles; assertsexitCode: 0and wall-clock completion under 10 s.