agentHost: await Codex process shutdown before directory cleanup - #337070
Closed
Christof Marti (chrmarti) wants to merge 1 commit into
Closed
Christof Marti (chrmarti) wants to merge 1 commit into
Christof Marti (chrmarti) wants to merge 1 commit into
Conversation
Adapt #335992 to current main, including its review follow-up for startup cleanup errors and shutdown budgets. Preserve the current throttled workspace-change handling while awaiting owned process-tree shutdown and retaining cleanup diagnostics. Based on 6b488c7 and 072087b. Co-authored-by: Dmitriy Vasyura <dmitriv@microsoft.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot started reviewing on behalf of
Christof Marti (chrmarti)
September 21, 2026 15:57
View session
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Cross-platform process lifecycle behavior remains high-risk and the described strict replay and Azure validations are still pending.
Review effort: Balanced
Findings: None
What changed in this PR
Hardens Agent Host teardown so Codex process trees fully stop before temporary resources are removed.
Changes:
- Adds bounded, awaitable process-tree shutdown and cleanup.
- Separates Agent Host shutdown into independently bounded phases.
- Expands lifecycle tests and teardown diagnostics.
| File | Description |
|---|---|
src/vs/base/node/processes.ts |
Adds bounded process-tree shutdown. |
src/vs/platform/agentHost/node/agentHostManagementService.ts |
Extends provider shutdown allowance. |
src/vs/platform/agentHost/node/agentHostServerMain.ts |
Reports shutdown failures through exit status. |
src/vs/platform/agentHost/node/agentHostService.ts |
Extends managed shutdown timeout. |
src/vs/platform/agentHost/node/agentHostShutdown.ts |
Introduces separate bounded shutdown phases. |
src/vs/platform/agentHost/node/codex/codexAgent.ts |
Awaits connection and resource cleanup. |
src/vs/platform/agentHost/node/codex/codexAppServerClient.ts |
Adds awaitable transport shutdown. |
src/vs/platform/agentHost/test/node/agentHostShutdown.test.ts |
Tests shutdown phases and budgets. |
src/vs/platform/agentHost/test/node/codex/codexAppServerClient.test.ts |
Tests client shutdown behavior. |
src/vs/platform/agentHost/test/node/codex/codexAppServerProcess.test.ts |
Covers real process-tree cleanup scenarios. |
src/vs/platform/agentHost/test/node/codex/codexCreateChat.test.ts |
Updates lifecycle test transports. |
src/vs/platform/agentHost/test/node/codex/codexModelRefresh.test.ts |
Adds startup and shutdown race coverage. |
src/vs/platform/agentHost/test/node/codex/codexPrewarmEviction.test.ts |
Updates mock transport shutdown. |
src/vs/platform/agentHost/test/node/e2e/harness/agentHostE2ETestHarness.ts |
Awaits server teardown before directory removal. |
src/vs/platform/agentHost/test/node/e2e/README.md |
Documents teardown behavior and diagnostics. |
src/vs/platform/agentHost/test/node/serverIntegrationTestHelpers.ts |
Uses process-tree shutdown and captures output. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Contributor
Screenshot ChangesBase: Changed (1)
|
| Before | After |
|---|---|
 |
 |
1 insignificant change(s) omitted (≤20 px, Δ≤2). See CI logs for details.
Collaborator
Author
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 free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Main-based adaptation of Dmitriy Vasyura (@dmitrivMS)'s #335992, including both the original fix and its review follow-up. This is not an independent implementation: the original commits are recorded in the commit message and Dmitriy Vasyura is credited as a co-author. The existing PR remains open and is not modified by this PR.
Investigated after the unrelated Windows / Electron failure on #337043: all 100 Codex E2E tests passed, but suite teardown failed to remove its isolated home. Current main stops the direct Codex child without awaiting its process tree and test teardown observes the agent-host parent's exit, which does not prove descendants have released filesystem handles. The job's aggregate error does not identify the exact locking process or nested filesystem errno, so that individual CI occurrence cannot be conclusively attributed from its logs alone.
Changes
killTreefor forced cleanup.The port preserves current main's newer
ThrottlerByKeyworkspace-change handling; that import was the only textual conflict when applying the two original commits.Validation
Local Windows, Electron 43.6.0, Codex 0.153.0:
npm run gulp compile-client: passed with 0 errors.Azure pipeline 111 build 475538 is running for Windows, Linux, and macOS x64 with publishing/release disabled, pinned to commit
9b149aee4c1e9a4238e8140df309d4c552344392. Cross-platform results are pending; this remains a draft.