Skip to content

fix(process): verify Windows PTY descendant cleanup - #75113

Open
akivavh wants to merge 3 commits into
NousResearch:mainfrom
akivavh:fix/windows-pty-descendant-cleanup
Open

fix(process): verify Windows PTY descendant cleanup#75113
akivavh wants to merge 3 commits into
NousResearch:mainfrom
akivavh:fix/windows-pty-descendant-cleanup

Conversation

@akivavh

@akivavh akivavh commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Summary

  • snapshot the complete Windows PTY-owned PID/start-time tree before PTY teardown can erase parentage
  • terminate only the attributable tree with the existing Windows tree-kill primitive and verify every captured identity is gone before reporting killed
  • fail closed on identity mismatch, timeout, nonzero taskkill, or surviving descendants while preserving idempotent cleanup and existing browser cleanup callers

Verification

  • focused Windows process-registry regression: 10 passed
  • existing browser orphan-reaper and cleanup callers: 17 passed
  • canonical targeted process-registry run: 43 passed (four exact pre-existing Windows-red tests excluded and reproduced against the base)
  • native provider-free Windows probe: six attributable identities terminated; unrelated sentinel preserved during kill; repeated kill returned already_exited; simulated taskkill failure returned an error without PTY teardown
  • Ruff: pass
  • git diff --check: pass
  • Windows footgun classifier: zero findings on lines added by this change

Scope and limits

This PR repairs Hermes core PTY ownership and cleanup semantics. It does not use global image-name termination, add Job Objects, modify provider behavior, or claim live Claude/Remote Control certification. The unfiltered repository suite and remote CI remain to be evaluated by this PR.

@alt-glitch alt-glitch added type/bug Something isn't working P2 Medium — degraded but workaround exists comp/tools Tool registry, model_tools, toolsets platform/windows Native Windows-specific behavior or breakage sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows labels Jul 31, 2026
@akivavh akivavh changed the title fix(process): verify Windows PTY descendant cleanup Retracted Jul 31, 2026
@akivavh akivavh closed this Jul 31, 2026
@akivavh
akivavh deleted the fix/windows-pty-descendant-cleanup branch July 31, 2026 01:21
@akivavh
akivavh restored the fix/windows-pty-descendant-cleanup branch July 31, 2026 02:07
@akivavh akivavh changed the title Retracted fix(process): verify Windows PTY descendant cleanup Jul 31, 2026
@akivavh akivavh reopened this Jul 31, 2026
@akivavh
akivavh marked this pull request as ready for review July 31, 2026 02:11
@teknium1

Copy link
Copy Markdown
Contributor

Thanks for the focused Windows PTY cleanup work. I found no blocking issue in this sweep.

  • Current main directly terminates PTYs at tools/process_registry.py:1607-1613; it does not verify the Windows descendant tree.
  • This change snapshots and verifies identities in tools/process_registry.py:548-606, makes taskkill failures explicit at tools/process_registry.py:665-696, and delays PTY-handle teardown until verified cleanup at tools/process_registry.py:1693-1741.
  • The PR base cc4cab2f592e60a197e796506de9168f74baf3ea is the current main, so this is directly salvageable without rebase work.

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 31, 2026

@GottZ GottZ 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.

This was generated by AI during triage.

Summary

Five PRs are associated with this complex, but only #75113 addresses the reported Windows PTY descendant-cleanup failure: it captures attributable process identities before PTY teardown, terminates that tree, and verifies cleanup. The four closed, retracted PRs modify unrelated governance, image-ingest, or Discord behavior and do not address the process-registry cause.

Related pull requests

  • #25160 [closed] related — (+611/-0) — unrelated, retracted: This added an opt-in GitHub governance preflight around mutating tools, not Windows PTY ownership or descendant cleanup; although closed, it remains relevant here only to establish that it is not a competing fix.
  • #26104 [closed] related — (+278/-0) — unrelated, retracted: This reported successful image generation to an agent-ops ingest endpoint and did not touch process cleanup; although closed, it remains relevant only as an out-of-scope association.
  • #37676 [closed] related — (+185/-3) — unrelated, retracted: This converted Discord Markdown tables into aligned fenced text and has no overlap with the Windows PTY failure; although closed, it remains relevant only to exclude it from consolidation.
  • #37696 [closed] related — (+117/-3) — unrelated, retracted: This routed Discord streaming edits through thread metadata rather than the parent channel, without changing process ownership or termination; although closed, it remains relevant only to confirm that it is not a duplicate.
  • #75113 related — (+399/-21) — keep open with a salvage path: The diff directly addresses the cause by snapshotting the Windows PTY-owned PID/start-time tree before parentage is lost, making taskkill failures explicit, and withholding PTY teardown until captured identities are verified gone. This agrees with the keep_open maintainer-bot review, which identifies the salvageable core at tools/process_registry.py:548-606, tools/process_registry.py:665-696, and tools/process_registry.py:1693-1741 against current main behavior at tools/process_registry.py:1607-1613.

Suggested consolidation

Keep #75113 open with the concrete salvage path of retaining its attributable PID/start-time snapshot, explicit taskkill error handling, survivor verification, and delayed PTY-handle teardown. The maintainer-bot verdict reports no blocking issue and a current-main base, while #25160, #26104, #37676, and #37696 are already closed, retracted, and technically unrelated rather than duplicates; no merge recommendation is supported by the recorded lane evidence.

Cross-PR triage: Reviewed 5 pull requests and 0 issues in this complex. Diffs were read for 4 of 5 PRs (rest unavailable); Assessment working set: 50 kB of PR diffs, 1 kB of issue/PR text, 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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

Labels

comp/tools Tool registry, model_tools, toolsets P2 Medium — degraded but workaround exists platform/windows Native Windows-specific behavior or breakage sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:risk-platform-windows Sweeper risk: may break or behave differently on native Windows sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants