Skip to content

fix: bound Windows ACP shutdown and retain failed cleanup - #430

Closed
slashdevcorpse wants to merge 4 commits into
LodyAI:mainfrom
slashdevcorpse:fix/windows-process-tree-cleanup
Closed

slashdevcorpse wants to merge 4 commits into
LodyAI:mainfrom
slashdevcorpse:fix/windows-process-tree-cleanup

Conversation

@slashdevcorpse

@slashdevcorpse slashdevcorpse commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor

Related issue

Refs #429

Problem / pressure

Windows auxiliary ACP shutdown can kill only its wrapper. Fallback sandbox cleanup accepts failed taskkill exits, force-exit waiting is unbounded, and session/terminal shutdown can hide errors or discard ownership before cleanup succeeds. A stalled terminal disposal can also prevent the process-kill phase from running, and the outer CLI deadline could previously expire before session cleanup reached forced termination.

Summary

  • Add bounded hidden taskkill /PID /T cleanup with optional /F, validated helper result, and timer/listener cleanup; await auxiliary ACP root exit and coalesce its cleanup.

  • Retain ChildProcess ownership in the sandbox, avoid targeting known-exited roots, attempt every root, and aggregate failures.

  • Bound graceful/forced session exit waits and terminal disposal (30 seconds); continue process termination after disposal failure while retaining a failed result and sharing still-pending disposal on retry.

  • Retain failed sessions in manager cleanup, including late root-exit events; remove only the exact successfully terminated instance and preserve replacements.

  • Terminal release now awaits observed exit, escalates live roots when necessary, reports failures, retains retry state, and coalesces concurrent release. Actual close reporting does not wait for resource-limit inspection.

  • Add Windows-only OS-handle verification for a real wrapper/child/grandchild tree and deterministic failure/retry tests.

  • Share one session termination attempt, upgrade concurrent force requests, and reject replacement or new admission during shutdown.

  • Reserve 15 seconds for graceful shutdown, 15 seconds for forced cleanup, and a separate bounded telemetry flush. Force all retained workspace and preparing-session owners concurrently even when graceful drains stall.

Replacement native stack

Native GitHub stack #6 in slashdevcorpse/Lody, created with gh stack link:

  1. [1/3] fix: bound Windows ACP shutdown and retain failed cleanup slashdevcorpse/Lody#2 — bounded shutdown and retained cleanup ownership; base main.
  2. [2/3] fix: protect background sessions and record resource history slashdevcorpse/Lody#3 — background-aware eviction and bounded resource history; base fix/windows-process-tree-cleanup.
  3. [3/3] fix: own Windows process trees with native job supervision slashdevcorpse/Lody#4 — native Windows Job Object ownership; base fix/background-session-resource-history.

Replaces the closed cross-fork #430 and #435. Each PR has an incremental diff against its predecessor. GitHub native stacks require all branches in the same repository, so this stack is in the contributor fork. It has not merged or landed upstream; #429 remains open.

Before / after

Before After
Windows auxiliary cleanup signals only the wrapper Requests recursive cleanup and awaits helper success plus root exit
Failed taskkill or terminal kill can look successful Errors propagate and failed cleanup retains ownership
Forced exit or terminal disposal can stall shutdown Bounded waits allow process cleanup to proceed and report incomplete shutdown
Manager clears every session after cleanup attempts Successful exact instances are removed; failures remain retryable
Terminal release fabricates a SIGTERM exit Waiters receive observed process exit only

Test plan

  • 106 focused tests passed across session-manager, terminal-manager, session-terminate-cleanup, session-sandbox, windows-process-tree, acp-runner, and acp-capabilities.
  • Real Windows process-tree test passed separately (107 total): IPC readiness, detached descendants, and independently captured OS handles verify all three original processes exited.
  • Negative control: replacing recursive cleanup with wrapper-only root.kill correctly failed with an owned descendant still alive; test cleanup removed the synthetic processes, and restored helper passed again.
  • CLI typecheck, changed-file Prettier check, git diff --check, and root pnpm check:quick passed (lint zero errors plus i18n/import/platform/public boundary guards).
  • Authentication previously had 27 passes and four Windows path failures reproduced on the unpatched base.
  • Earlier full pnpm check stopped on unchanged shared local-cli-host-lease shutdown-control test (1022 passed, one failed, five skipped in shared). Full repository verification is not claimed green; GitHub checks must be evaluated for this head.
  • Additional shutdown controller/fleet/session integration tests cover coalescing, force upgrades, pending preparation, rejected admission, and failed workspace retries; 129 distinct focused tests have passed across the updated scope.
  • GitHub CI at 26b6ad7 passed Static checks, Tests, and Desktop E2E (smoke). Earlier local full-suite failures above remain recorded as historical local results.

Context handoff

Instructions for reviewing agents

  • Review focus: Trace Windows helper failures through Session and SessionManager; verify terminal real-exit reporting, the graceful-to-force deadline, coalesced force upgrades, closed admission, and late-exit retention.
  • Decisions to challenge: Cleanup failures now block shared-resource teardown; failed sessions remain retryable, while known-exited root PIDs never authorize a new kill.
  • Plausible failures / evidence gaps: The Windows test proves a controlled live-root tree, not native provider behavior or crash ownership; failed cleanup remains conservatively retained; native crash ownership is implemented in the third PR.

Authoring context

  • User goal / directives: Investigate suspected process retention and submit an upstream fix, including bounded disposal, failed-session retention, truthful terminal status, and actual Windows descendant verification.
  • Constraints / non-goals: Preserve unrelated work; no native Codex changes, global process scans, image-name killing, or claims that all subagent MCP retention is fixed.
  • Risk-bearing decisions: Cleanup now rejects instead of fabricating success; manager failures stop shared-resource teardown, retaining state for retry. Timed-out terminal disposal remains pending rather than launching duplicate disposal.
  • Destructive or irreversible behavior: Explicit shutdown terminates owned live roots and their descendants, including forced escalation. The integration test only kills its own synthetic processes through captured ownership.
  • Deliberately not done or tested: This first PR excludes Windows Job Objects and background eviction/resource history, which are separate dependent PRs. No real user sessions were terminated.
  • Unknowns / confidence: 129 distinct focused tests and real OS-handle verification support this shutdown slice; GitHub CI at 26b6ad7 is green. Native provider-specific residency remains outside that evidence, and crash ownership is handled by the third PR.

Copilot AI lite review requested due to automatic review settings September 6, 2026 04:14

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5536eaa9c2

ℹ️ 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".

Comment thread apps/cli/src/session/session.ts
@slashdevcorpse slashdevcorpse changed the title fix: await bounded Windows ACP process tree cleanup fix: bound Windows ACP shutdown and retain failed cleanup Sep 6, 2026

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8f71ec192f

ℹ️ 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".

Comment thread apps/cli/src/agent/acp-runner.ts
@slashdevcorpse

Copy link
Copy Markdown
Contributor Author

Closing to replace this cross-fork PR with a native three-PR GitHub stack in slashdevcorpse/Lody. GitHub native stacks require all branches in the same repository. The implementation is preserved on fix/windows-process-tree-cleanup; replacement links will follow. #429 remains open.

@slashdevcorpse

Copy link
Copy Markdown
Contributor Author

Native GitHub stack #5 in slashdevcorpse/Lody, created with gh stack link:

  1. [1/3] fix: bound Windows ACP shutdown and retain failed cleanup slashdevcorpse/Lody#2 — bounded shutdown and retained cleanup ownership; base main.
  2. [2/3] fix: protect background sessions and record resource history slashdevcorpse/Lody#3 — background-aware eviction and bounded resource history; base fix/windows-process-tree-cleanup.
  3. [3/3] fix: own Windows process trees with native job supervision slashdevcorpse/Lody#4 — native Windows Job Object ownership; base fix/background-session-resource-history.

Replaces the closed cross-fork #430 and #435. Each PR has an incremental diff against its predecessor. GitHub native stacks require all branches in the same repository, so this stack is in the contributor fork. It has not merged or landed upstream; #429 remains open.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants