Uh oh!
There was an error while loading. Please reload this page.
fix(copilot): recover from a dead spawned Copilot runtime process - #484
Merged
Conversation
Jason Robert (jrob5756) pushed a commit
that referenced
this pull request
Aug 24, 2026
Blocking fixes (PR #484 review): - _restart_spawned_runtime no longer publishes the rebuilt client until it has actually started: _client/_started are invalidated first, so a failed start() (e.g. OOM at spawn) leaves the provider correctly believing no client is started, instead of silently disabling dead-runtime recovery for the rest of the process. - The consecutive-restart cap is now checked before incrementing the counter and is never left stale: the cap can no longer be tripped after zero actual restarts, the giving-up message reports the real restart count, and close() resets the counter so a cached provider isn't permanently wedged after a workflow crash-loops once. - Replaced the unfalsifiable cap-message assertion in test_copilot_runtime_recovery.py with one that pins the rendered clause and asserts the cap actually prevents the next rebuild. - Added tests/test_providers/conftest.py: an autouse fixture clearing COPILOT_PROVIDER_RUNTIME_URL/TOKEN so the runtime-recovery tests pass regardless of the developer's/CI runner's environment. - Added a regression test covering the corrupted-state bug: when the rebuilt client's start() raises, _started must end up False and a later _ensure_client_started() must re-attempt start(). Recommendations applied: - _runtime_unavailable_error now distinguishes a confirmed-dead process (poll() returned an exit code) from a broken connection to a still- alive process, instead of always claiming the process "died" and suggesting NODE_OPTIONS. - Client teardown during restart, and session.disconnect() in the per-agent finally block, now log a warning on failure instead of silently swallowing the exception (a leaked child / stranded session is diagnostically useful, especially given this PR's own OOM focus). - The session.error ProviderError path is now also routed through dead- runtime classification when retryable, instead of always surfacing a generic "Copilot SDK error" message that hides an exit-code 137 OOM kill. - Narrowed _spawned_runtime_process's return type from Any | None to subprocess.Popen[bytes] | None, matching the isinstance check the body already performs and the SDK's own annotation. - Added a one-time warning when a spawned, started client has no usable _cli_process handle, so a future SDK rename surfaces instead of silently degrading recovery to a no-op. - Fixed the inverted _FakeClient docstring/comments describing mock auto-vivification as looking "live" when it in fact reads as dead. - Scoped the restart-counter-reset comment to agent execution (several auxiliary paths increment without resetting). - Updated CHANGELOG.md, docs/configuration.md and AGENTS.md to name the restart cap (2, fixed, non-configurable), correct the "endlessly retrying" overstatement, and scope the SDK-boundary claim to agent-execution; documented the _cli_process vs _process split. Recommendations skipped (not applied): #5 (_interrupted_session reset + disclosure wording), #6 (max_session pre-flight), #12 (Liveness enum), #13 (_RestartBudget value type), #17 (per-generation client tracking for parallel groups), #18 (additional missing tests beyond the one added for finding #1), #19 (collapsing except clauses), #20 (extracting shared helpers) -- all correctness-neutral hardening/refactors judged to grow the diff beyond what this pass should touch; pyproject.toml dependency cap was also left alone as an unrelated, broader change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Detect when the nested Copilot runtime subprocess has died (broken pipe/connection reset, or a check before sending an idle-recovery prompt) and transparently restart it on the next attempt instead of surfacing a confusing stuck-agent error. Externally-owned runtimes (runtime_url) are never restarted here -- that failure is reported as non-retryable so the owning orchestrator can act. A consecutive restart counter (reset on any successful SDK call) caps restart attempts so a runtime that keeps dying before ever succeeding fails fast rather than looping forever. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Blocking fixes (PR #484 review): - _restart_spawned_runtime no longer publishes the rebuilt client until it has actually started: _client/_started are invalidated first, so a failed start() (e.g. OOM at spawn) leaves the provider correctly believing no client is started, instead of silently disabling dead-runtime recovery for the rest of the process. - The consecutive-restart cap is now checked before incrementing the counter and is never left stale: the cap can no longer be tripped after zero actual restarts, the giving-up message reports the real restart count, and close() resets the counter so a cached provider isn't permanently wedged after a workflow crash-loops once. - Replaced the unfalsifiable cap-message assertion in test_copilot_runtime_recovery.py with one that pins the rendered clause and asserts the cap actually prevents the next rebuild. - Added tests/test_providers/conftest.py: an autouse fixture clearing COPILOT_PROVIDER_RUNTIME_URL/TOKEN so the runtime-recovery tests pass regardless of the developer's/CI runner's environment. - Added a regression test covering the corrupted-state bug: when the rebuilt client's start() raises, _started must end up False and a later _ensure_client_started() must re-attempt start(). Recommendations applied: - _runtime_unavailable_error now distinguishes a confirmed-dead process (poll() returned an exit code) from a broken connection to a still- alive process, instead of always claiming the process "died" and suggesting NODE_OPTIONS. - Client teardown during restart, and session.disconnect() in the per-agent finally block, now log a warning on failure instead of silently swallowing the exception (a leaked child / stranded session is diagnostically useful, especially given this PR's own OOM focus). - The session.error ProviderError path is now also routed through dead- runtime classification when retryable, instead of always surfacing a generic "Copilot SDK error" message that hides an exit-code 137 OOM kill. - Narrowed _spawned_runtime_process's return type from Any | None to subprocess.Popen[bytes] | None, matching the isinstance check the body already performs and the SDK's own annotation. - Added a one-time warning when a spawned, started client has no usable _cli_process handle, so a future SDK rename surfaces instead of silently degrading recovery to a no-op. - Fixed the inverted _FakeClient docstring/comments describing mock auto-vivification as looking "live" when it in fact reads as dead. - Scoped the restart-counter-reset comment to agent execution (several auxiliary paths increment without resetting). - Updated CHANGELOG.md, docs/configuration.md and AGENTS.md to name the restart cap (2, fixed, non-configurable), correct the "endlessly retrying" overstatement, and scope the SDK-boundary claim to agent-execution; documented the _cli_process vs _process split. Recommendations skipped (not applied): #5 (_interrupted_session reset + disclosure wording), #6 (max_session pre-flight), #12 (Liveness enum), #13 (_RestartBudget value type), #17 (per-generation client tracking for parallel groups), #18 (additional missing tests beyond the one added for finding #1), #19 (collapsing except clauses), #20 (extracting shared helpers) -- all correctness-neutral hardening/refactors judged to grow the diff beyond what this pass should touch; pyproject.toml dependency cap was also left alone as an unrelated, broader change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Jason Robert (jrob5756)force-pushed
the
fix/483-copilot-runtime-recovery
branch
from
August 24, 2026 16:07
2600d43 to
1aea23eCompareUh oh!
There was an error while loading. Please reload this page.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Detects when the nested Copilot runtime subprocess has died (broken pipe/connection reset, or a pre-flight check before sending an idle-recovery prompt) and transparently restarts it on the next attempt, instead of surfacing a confusing stuck-agent error.
runtime_url) are never restarted by Conductor — that failure is reported as non-retryable so the owning orchestrator can act.Testing
tests/test_providers/test_copilot_runtime_recovery.pyCloses#483