Skip to content

fix(cli): renders on Windows stop cancelling when an old parent's pid is reused - #5358

Merged
miguel-heygen merged 2 commits into
mainfrom
fix/windows-ancestor-pid-reuse
Oct 9, 2026
Merged

miguel-heygen merged 2 commits into
mainfrom
fix/windows-ancestor-pid-reuse

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 9, 2026 •

Copy link
Copy Markdown
Collaborator

What changes for a user

On Windows, a local render could cancel itself a moment after starting, or partway through, with Render cancelled: render_cancelled_parent_exited, while the terminal that started it was still open. Renders now keep going unless a real parent exits.

Why

At start, a render records its chain of parent processes and cancels if any of them exits. On Windows it builds that chain from one Win32_Process listing by following each process's parent pid. Windows never reparents an orphan, so a parent pid can point at a parent that died long ago, and Windows may have handed that pid to an unrelated process. The walk followed it past the real chain onto that unrelated process; when it exited, the render cancelled.

recordsToAncestors now stops when a "parent" was created after its child, which can only mean its pid was reused. On macOS the ps listing carries no start time and Linux does not build this list at all, so nothing changes there.

This is the failure behind intermittent Windows render verification reds on main, the merge queue and unrelated PRs since the check was added.

Limits

The check trusts process creation times. If the system clock steps backwards between a parent's start and its child's, a real parent can look newer and the walk stops there. That render then stops watching its parents from that point up. A backwards step after the child starts can still let a reused pid look older, which is the old behaviour for that render, no worse.

Tests

  • orphanCleanup.windowsHide.test.ts: with 400 → 300 → 200 → 999 where 999 started after 200, the chain is 300, 200. Without the fix it also includes 999, and the test fails. A parent created in the same tick as its child stays an ancestor (fails if the check becomes >=).
  • The orphan-cleanup, process-tree and render-cancellation unit tests (28) pass three runs in a row; tsc --noEmit clean for the CLI.

Standalone on purpose: one root in the CLI's process-ancestry walk, unrelated to other open changes.

@miguel-heygen
miguel-heygen marked this pull request as ready for review October 9, 2026 17:51
@github-actions

github-actions Bot commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 2059 (base branch 2059), smooth 1542 of those

The gate passes.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Quarantined, measured but not gated (0)

@jrusso1020 jrusso1020 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Approve at 066eb11e. The walk now stops at a "parent" created after its child, and only a reused pid can look like that. Real parents are always created before their children, so the real chain is kept, and a terminal that really closes still cancels the render.

Checked

  • The cause: on Windows the ancestor chain comes from one Win32_Process listing, following ParentProcessId. Windows doesn't reparent orphans, so a dead parent's pid can belong to an unrelated, newer process. The old walk followed it, and the render cancelled when that process exited.
  • The guard is complete for reuse. A reused pid's new process can only start after the original parent died. That parent was alive when it created the child, so the new process always starts after the child, and parent.startedAt > child.startedAt catches it. The one exception is a clock step, which the body already covers under Limits.
  • The time source: the listing uses CreationDate.ToFileTimeUtc(), which is an integer tick count. So /^\d+$/ plus BigInt parses it, and comparing BigInts avoids float precision loss.
  • A missing or unparseable time leaves startedAt undefined, and the guard does nothing. That fails toward the old behaviour (watch the parent), never toward dropping a real one.
  • Closing the terminal still cancels:
    • The truncated chain only loses entries above a reused pid, which were never real ancestors.
    • The shell and every real parent below it stay watched.
    • Closing the console also delivers SIGHUP, which aborts on its own.
  • Scope: recordsToAncestors runs only for the off-Linux snapshot, so Linux is untouched. macOS reparents orphans to launchd, so a dead parent's pid isn't followed there.

Nits (non-blocking)

  • The body says the macOS ps listing "carries no start time". lstart is a start time; it's only a string. The conclusion still holds, because macOS reparents orphans.
  • Pre-existing, opposite direction: off Linux the watchdog only checks isAlive(pid), never identity (renderCancellation.ts:125-133). On Windows, if the real parent exits and its pid is reused within the 2 s poll, the render keeps running orphaned until that new process exits. The snapshot already records windows:<creation> identities, so comparing them in the off-Linux check would close this the same way Linux does.

Tests:

  • orphanCleanup (all three files), renderCancellation.test.ts and processTree.test.ts pass 28/28.
  • renderCancellation.process.test.ts fails 4 cases on this box, both at this head and at its base, so it's environmental; CI runs it.
  • Mutants:
    • > → >= is caught by the same-tick test.
    • Dropping the guard is caught.
    • Never parsing startedAt is caught.
  • Every check on this head passed before this review was posted.

— Rames

@miguel-heygen
miguel-heygen added this pull request to the merge queue Oct 9, 2026
Merged via the queue into main with commit ce359d9 Oct 9, 2026
152 checks passed
@miguel-heygen
miguel-heygen deleted the fix/windows-ancestor-pid-reuse branch October 9, 2026 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants