Skip to content

fix(win): escalate stubborn agent PTY tree-kills on worktree delete - #10638

Closed
innocarpe wants to merge 2 commits into
stablyai:mainfrom
innocarpe:fix/hermes-worktree-delete-pty
Closed

innocarpe wants to merge 2 commits into
stablyai:mainfrom
innocarpe:fix/hermes-worktree-delete-pty

Conversation

@innocarpe

@innocarpe innocarpe commented Jul 26, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Nested agent TUIs (Hermes, etc.) can survive the first ConPTY kill / single taskkill, so worktree delete fails with “Failed to physically stop every PTY”.
  • Retry tree-kill when the root is still alive; re-escalate mid-wait during immediate physical stop.

Fixes

Closes #10475

Test plan

  • windows-process-tree-kill / local-pty-provider / session force-kill escalation tests
  • Manual Windows: idle Hermes worktree → right-click Delete succeeds

ELI5

On Windows, nested agent processes sometimes survived the first kill, blocking worktree delete. Tree-kill is retried and escalated so stubborn PTYs actually die and delete can finish.

Hermes (and similar idle agent TUIs) can keep the ConPTY console non-empty
after the first bare kill, so destructive worktree removal fails with
"Failed to physically stop every PTY for worktree".

- Retry taskkill /T /F once when the root is still alive after the first attempt
- Re-tree-kill when immediate/destructive teardown joins a graceful stop
- Mid-wait force escalation inside the physical-exit budget (Windows taskkill /
  POSIX SIGKILL)

Closes stablyai#10475
@coderabbitai

coderabbitai Bot commented Jul 26, 2026 •

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Windows process-tree termination now checks root liveness and can retry taskkill after a configurable delay. Daemon sessions perform platform-specific force-kill escalation during physical-exit waits. Local PTY shutdown adds a configurable escalation grace period, staged exit waiting, and descendant-tree force-kill handling for Windows and agent sessions. Tests cover retry behavior, session exit transitions, PTY cleanup, and updated force-kill call counts.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning The description covers summary and tests, but it omits Screenshots, AI Review Report, Security Audit, and Notes required by the template. Add the missing template sections and expand Testing to the checklist format, including screenshots or 'No visual change' and the required review/security notes.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address #10475 by escalating Windows PTY tree-kills so idle Hermes worktrees can be deleted successfully.
Out of Scope Changes check ✅ Passed The modified code and tests all support the Windows PTY termination fix and do not appear unrelated to the issue.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title matches the main change: Windows PTY tree-kill escalation for stubborn agent processes during worktree delete.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 5


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 2ef14ddf-20b0-4068-811c-7507b29822ab

📥 Commits

Reviewing files that changed from the base of the PR and between c4b7aed and 536dbf4.

📒 Files selected for processing (7)
  • src/main/daemon/session-force-kill-escalation.test.ts
  • src/main/daemon/session.ts
  • src/main/daemon/terminal-host.test.ts
  • src/main/providers/local-pty-provider.test.ts
  • src/main/providers/local-pty-provider.ts
  • src/main/windows-process-tree-kill.test.ts
  • src/main/windows-process-tree-kill.ts

Comment thread src/main/daemon/session.ts
Comment thread src/main/daemon/session.ts Outdated
Comment thread src/main/providers/local-pty-provider.ts
Comment thread src/main/providers/local-pty-provider.ts Outdated
Comment thread src/main/windows-process-tree-kill.ts
…#10475)

Address CodeRabbit: tree-kill-only mid-wait on Windows, floor post-
escalation waits, join in-flight shutdown if tree-kill throws, escalate
only agent/immediate paths, and share a single taskkill wall-clock budget
across the live-root retry.
@innocarpe

Copy link
Copy Markdown
Contributor Author

Sync update (f965a79fc)

Address CodeRabbit: tree-kill-only mid-wait on Windows; floor post-escalate waits; join shutdown if tree-kill throws; escalate only agent/immediate; shared taskkill budget for retry

@nwparker nwparker added the bug Something isn't working label Jul 27, 2026
@AmethystLiang
AmethystLiang requested a review from OrcaWin July 28, 2026 16:50
@nwparker

Copy link
Copy Markdown
Contributor

Closing — superseded, though the core observation was correct and did get fixed.

ConPTY forceKill is a no-op after the first bare kill (nodePtyKillIssued), so stubborn nested agents that ignore soft stop must be re-tree-killed on escalate.

That is right, and it is the same thing @hanbong5938 found in #9854. It is fixed in #15886: forceKill() on Windows now terminates the pane's Win32 job object instead of returning early. The job is exact — it names only the tree Orca created — so no re-taskkill, no identity probe, and no escalation ladder is needed. #15886 also wires the job into the daemon teardown path, which is where worktree delete actually runs (measured in #11047).

The mid-wait re-escalation and the separate escalation grace window are not carried over. They existed because the first kill could silently do nothing; with the job it either terminates the tree or reports unavailable, so a second round of the same call has nothing new to try. If a tree survives a TerminateJobObject we would want to know that directly rather than retry into it.

Thanks — the diagnosis held up.

@nwparker

Copy link
Copy Markdown
Contributor

Superseded by the job objects in #15755/#15886, which fix the forceKill no-op this PR diagnosed.

@nwparker nwparker closed this Aug 22, 2026
@nwparker

Copy link
Copy Markdown
Contributor

Landed: #15886 is merged (ecfc547ece7).

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

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: Right-click delete fails for Hermes worktrees — PTY process does not respond to SIGTERM

3 participants