fix(sessions): prevent PID-reuse from terminating unrelated processes on Windows

[wyongzhi]

Problem

Addresses the wrong-kill hazard from #347.

On Windows, kill_process_tree discovered the process tree by walking toolhelp (pid, ppid) pairs with no validation. th32ParentProcessID is a birth record (never updated when the parent exits) and Windows reuses PIDs aggressively, so a live unrelated process whose long-dead creator had the canceled subprocess's PID gets misattributed as a descendant and TerminateProcess'd. This is consistent with the observed CI failure in aws-deadline/deadline-cloud-worker-agent (differential cancel test run dies instantly after the kill log line, with the step exiting with the hardcoded TerminateProcess(h, 1) exit code and no pytest summary).

Two adjacent windows have the same shape: killing by raw PID after validation (collect-to-kill TOCTOU), and spawn_delayed_terminate moving a raw PID into a detached task that can fire after the Child handle is dropped and the PID recycled.

Fix

Mirrors the PID-reuse guard psutil's Process.children() applies in the Python implementation, which the original port dropped, then closes the two adjacent windows:

1. Edge validation: collect_tree_validated accepts a parent-child edge only when the child's creation time is >= its recorded parent's (under a forward-moving clock, a true child is never older than its parent; a stale edge always is). Fail closed: unreadable creation time excludes the candidate. Returns ValidatedProcess { pid, creation_time }.
2. Checked termination: kill_process_checked opens ONE handle (PROCESS_QUERY_LIMITED_INFORMATION | PROCESS_TERMINATE), revalidates the creation time on that same handle, and only then calls TerminateProcess on the same handle. Mismatch/unreadable: skip with a diagnostic log.
3. Delayed terminate: root identity is captured synchronously at schedule time (while the caller still holds the live Child handle, so the kernel pins the PID) and the delayed kill is gated on an identity match.

The pure graph logic is gated #[cfg(any(windows, test))] so its unit tests run on every platform.

Testing

• 10 new deterministic unit tests via an injectable (pid, ppid, ctime) snapshot: stale-edge rejection, equal-timestamp acceptance (pins >=), subtree pruning below a rejected edge, equal-time cycle termination via the visited set, unreadable-ctime fail-closed paths, and kill-time identity mismatch rejection. PID reuse itself cannot be reproduced deterministically, hence the injectable seam.
• Full workspace: cargo test --release --workspace all passing; cargo clippy --release --all-targets -- -D warnings clean; cargo fmt --check clean; windows-msvc cross-check via cargo xwin check passes locally. Existing Windows cancellation integration tests exercise the new path in this repo's Windows CI leg.

Residual risk (documented in code, out of scope here)

• Under-kill only, never wrong-kill: descendants that detach from the ppid chain or are spawned between snapshot and kill can be missed; system clock step-backs can make a true child fail the >= check; protected (PPL) processes whose creation time cannot be read are skipped.
• The structural fix for under-kill is assigning spawned processes to a Job Object (TerminateJobObject), as already done for the cross-user helper in fix(sessions): tie helper grandchildren to a Windows Job Object; harden CI #203 — proposed as the follow-up in Bug: (openjd-sessions))kill_process_tree can terminate unrelated processes due to PID reuse in Windows #347.

[github-actions]

The child_ct >= parent.creation_time invariant catches stale-parent edges, but it cannot catch the PID reuse that happens between snapshot_process_parents() and the creation_time(child) callback — and that case fails open, not closed:

1. Snapshot records (C, P) — genuine at snapshot time.
2. C exits and its PID is immediately reused by an unrelated process U.
3. creation_time(C) now returns Us creation time, which is newer than Ps, so the edge passes the >= check.
4. U is pushed as a ValidatedProcess, and kill_process_checked happily confirms the identity it just recorded for U and terminates it.

So the collection stage can admit an unrelated live process, and the kill-time revalidation cannot help because it validates against the already-poisoned recorded time. The doc comment claims the failure mode is "a leaked (skipped) descendant, never terminating an unrelated process" — that holds for the clock-skew case discussed, but not for this one.

There is a cheap upper bound that closes it: any process that was in the snapshot must have been created before the snapshot was taken. Capturing GetSystemTimeAsFileTime() immediately before CreateToolhelp32Snapshot and rejecting any node whose creation time is greater than that stamp prunes exactly the reused-after-snapshot PIDs, and is testable through the existing callback-injection seam (pass the stamp in as a parameter).

[wyongzhi]

Good catch — this direction did fail open. Fixed in 9a8f6e9: a wall-clock stamp is now captured immediately before the toolhelp snapshot and threaded into collect_tree_validated as snapshot_time; any node (root included) whose freshly read creation time is newer than the stamp is rejected, since a genuine snapshot member must predate the snapshot. Covered by rejects_candidate_created_after_snapshot and rejects_root_created_after_snapshot.

[github-actions]

The captured ct is used only as a gate — it is never threaded into kill_process_tree, so the root itself is still killed on whatever creation time is read after the delay:

• process_identity_matches(ct, process_creation_time(pid)) opens a handle, reads, and closes it.
• kill_process_tree(pid) then independently calls process_creation_time(root_pid) inside collect_tree_validated and records that value as the root ValidatedProcess.
• kill_process_checked compares against that freshly-read value, which it trivially matches.

So the schedule-time identity is discarded before the actual terminate, and any reuse landing between the gate check and the tree walk is invisible. Since the entire purpose of capturing root_identity is to survive the delay, it should be an input to the kill: e.g. give kill_process_tree an expected_root_ct: Option<u64> and have collect_tree_validated reject the root outright when the read value differs, so the root ValidatedProcess carries the schedule-time identity rather than a re-read one.

[wyongzhi]

Agreed — the gate discarded the identity it was supposed to carry. Fixed in 9a8f6e9: kill_process_tree now takes expected_root_ct: Option; the delayed path passes its schedule-time capture directly (separate gate removed), and collect_tree_validated returns empty when the re-read root ctime differs, so the root ValidatedProcess carries the schedule-time identity into kill_process_checked.

[github-actions]

This fallback is fail-open on exactly the input the rest of the change treats as the danger signal. collect_tree_validated returns empty only when the root creation time is unreadable, and the most common reason for that on a PID you just scheduled a kill for is that the process exited — after which the PID may already belong to something else. kill_process(root_pid) then terminates whatever now holds that number, with no identity check at all.

It is worst on the delayed path: spawn_delayed_terminate with root_identity == None reaches here after the full grace period has elapsed, i.e. the widest possible reuse window, and issues an unvalidated raw-PID kill.

process_creation_time uses PROCESS_QUERY_LIMITED_INFORMATION, which is granted far more liberally than PROCESS_TERMINATE — so if the open for query fails, the open for terminate would very likely have failed too and the fallback buys almost nothing on the legitimate access-denied (PPL) case it is justified by. Given the stated fail-closed posture, returning without killing (and logging) seems more consistent than a blind terminate.

[wyongzhi]

Agreed, including the access-rights point — QUERY_LIMITED_INFORMATION failing makes a successful TERMINATE-only open implausible, so the fallback's only reachable effect was the wrong-kill. Fixed in 9a8f6e9: the empty-collection path now logs and returns without killing, and the unused raw kill_process is deleted. Fail closed throughout.

[github-actions]

Minor: the ctimes map can never produce a cache hit, so it is dead weight. visited.insert(child) on line 626 short-circuits with continue on every repeat encounter of a PID, and root_pid is pre-seeded in visited too — so control only reaches this match for a child that has never been looked up before. Every iteration takes the None arm.

The map (and the HashMap import) can be dropped in favour of calling creation_time(child) directly, which also removes the misleading "caching callback results" comment.

[github-actions]

A root that has already exited now leaks the whole descendant subtree.

creation_time(root_pid) fails once the root has terminated (OpenProcess by PID fails for a dead process even while the Child handle is still held), so this returns empty and kill_process_tree logs and returns without killing anything.

That is the common case on the delayed-terminate path: NotifyThenTerminate gives the root a grace period, the root usually exits during it, and what remains is exactly the orphaned grandchildren this function exists to reap — including the MSYS2 sh.exe orphans holding inherited pipe handles that the drain-deadline comment in the read loop describes. Those orphans still record th32ParentProcessID == root_pid in the snapshot, so the pre-PR collect_tree found and killed them. After this change they are silently left running.

Note this fires even in the Some(ct) branch of spawn_delayed_terminate, where a pinned schedule-time identity is available — the early return happens before expected_root_ct is consulted at all.

A fix that keeps the fail-closed property: when expected_root_ct is Some(exp) and the fresh root read is None, the root is gone but exp is still a trustworthy lower bound for its descendants. Seed the walk with ValidatedProcess { pid: root_pid, creation_time: exp } instead of bailing (kill_process_checked will no-op on the dead root anyway). Descendants then still have to satisfy child_ct >= exp && child_ct <= snapshot_time, so a reused root PID and its unrelated children are still rejected.

Separately, the doc comment above ("callers fall back to killing just the root by other means") no longer matches the caller — kill_process_tree kills nothing at all on the empty return.

[wyongzhi]

Agreed on the regression and the fix direction — reaping orphans after the root exits is this walk's purpose, and the early return fired before expected_root_ct was consulted. One correction to the premise: OpenProcess on a terminated process still succeeds while an open handle keeps the object alive (zombie PIDs stay reserved), so the fresh read only fails after the Child handle is dropped — which is exactly the delayed-path timing, so the conclusion stands where it matters. Fixed in 41db905: with a pinned Some(exp) and a None fresh read, the walk is seeded with ValidatedProcess { root_pid, exp }. This reopens a documented residual: a freed PID's fossil edges can include children of an interim holder with ctimes inside [exp, snapshot_time]; bounded, no worse than the pre-validation walk, and structurally eliminated only by the Job Object rework tracked in #347.

[github-actions]

An intermediate node that has already exited prunes its entire live subtree.

Because the BFS only descends from nodes already pushed into result, a child skipped here is never enumerated as a parent, so every live descendant beneath it becomes unreachable. prunes_entire_subtree_below_rejected_edge covers this for a stale edge (a genuinely wrong edge, correct to prune), but the same pruning happens for the benign and much more common case: the intermediate process simply exited between the snapshot and the creation_time read, so OpenProcess fails and creation_time returns None.

Concretely, cmd.exe -> sh.exe -> renderer.exe: sh.exe exits promptly on the notify, and the surviving renderer.exe is the process that actually needs killing. Pre-PR collect_tree reached it because sh.exe was in result purely by PID; now sh.exe is dropped and renderer.exe is never visited. Reaping orphans whose intermediate shell has died is the main reason this tree walk exists, so this is not a rare corner.

Unlike the parent-identity check, nothing is actually being protected here: a dead PID cannot itself be terminated, and each descendant is independently revalidated against snapshot_time and re-checked in kill_process_checked before any TerminateProcess. Consider keeping the node traversable with the parent's creation time as its lower bound — e.g. push it with creation_time: parent.creation_time so its children must still satisfy >= parent_ct && <= snapshot_time — rather than dropping the branch. A reused-PID impostor at that slot would read a ctime newer than snapshot_time and be rejected on its own merits.

[github-actions]

The comment above is not accurate, and the gap it papers over is the exact reuse window this PR is closing elsewhere.

"with no pinned identity, collect_tree_validated fails closed and kills nothing whenever the root stays unreadable" — but on this branch the root usually does not stay unreadable. collect_tree_validated reads the root fresh after the sleep and gates it only on read_root_ct <= snapshot_time, and snapshot_time is captured at kill time, i.e. after the delay. A process that acquired pid by reuse during the sleep was created before that snapshot, so it passes the gate, becomes the validated root, and is terminated — along with any of its children, which now satisfy >= impostor_ct.

root_identity is None for two distinct reasons and they need different handling:

• the root had already exited at schedule time — its PID is then free for reuse, so this is precisely when a fresh unpinned read is untrustworthy;
• PROCESS_QUERY_LIMITED_INFORMATION was denied (protected/PPL process) — still no pinned identity, and the process can exit and have its PID reused during the delay.

In both cases there is no identity to validate against, so the safe action is to not walk the tree at all rather than to walk it with an unpinned root. Suggest returning early here (log and skip) instead of calling kill_process_tree(pid, None). None is a meaningful signal on the immediate send_terminate path (the live Child handle pins the PID there), but on the delayed path it means "identity unknown", and reusing the same sentinel for both conflates them.

[github-actions]

The stamp is captured before CreateToolhelp32Snapshot, so the stated invariant does not actually hold: "anything genuinely present was created before this instant" is false for any process created during the snapshot call itself. CreateToolhelp32Snapshot(TH32CS_SNAPPROCESS) plus the Process32FirstW/Process32NextW walk take a non-trivial amount of time on a busy host (it enumerates every process on the system), and a process created in that interval can legitimately appear in pairs with creation_time > stamp.

collect_tree_validated then rejects it on the child_ct <= snapshot_time check even though it is a genuine child — and per the subtree-pruning behaviour, its own descendants go with it. The processes most likely to land in that window are the newest, fastest-forking children, which are exactly the ones a kill needs to catch.

Capturing the stamp after the enumeration completes fixes both halves: it is a sound upper bound (every process in the snapshot was created before the snapshot finished, hence before an after-stamp), and it stops rejecting legitimate snapshot members. The reuse window it leaves open shrinks from "duration of the whole snapshot walk" to "the few instructions between the last Process32NextW and the stamp".

[github-actions]

This single OpenProcess requests PROCESS_QUERY_LIMITED_INFORMATION | PROCESS_TERMINATE, where the removed kill_process requested PROCESS_TERMINATE alone. OpenProcess is all-or-nothing on the requested mask, so any process whose DACL grants terminate but not query-limited-info now fails to open and is skipped entirely — a kill that previously succeeded now silently does not happen. Same-user processes with default DACLs grant both, so this is narrow, but tree descendants are not necessarily same-user or default-DACL.

Using one handle for both the identity read and the terminate is the right design (it is what actually closes the check-to-kill window), so this is a deliberate trade rather than a bug. It is worth making explicit though: the fail-closed rationale in kill_process_tree argues the other direction ("PROCESS_QUERY_LIMITED_INFORMATION is granted more liberally than PROCESS_TERMINATE, so anything we could validate we could also have terminated"), which holds for the two masks requested separately but not for the combined request made here.

[github-actions]

On the immediate send_terminate path this snapshot_time bound on the ROOT can turn a cancellation into a complete no-op, with no fallback.

send_terminate passes expected_root_ct = None, so the root lands in the (None, Some(rc)) arm. If rc > snapshot_time the function returns empty, kill_process_tree logs and returns, and nothing at all is killed — not the root, not any descendant. The old code always attempted TerminateProcess on the root. So a timed-out or cancelled action leaves its whole process tree running, terminate_sent is set but c.wait() never reaps anything, and the session hangs on a live tree.

The trigger is a backward wall-clock adjustment between the root's creation and the snapshot: NTP step-back, host/guest time re-sync after a VM resume or live migration, or a manual clock change. FILETIME creation times are wall-clock, and system_time_as_filetime_ticks reads the same wall clock, so the comparison inverts. The doc block discusses clock step-back only for the >= parent-edge check, where the consequence is a leaked descendant; here the consequence is strictly worse and is not mentioned.

Worth noting that the bound buys nothing on this path. Its stated purpose is catching a PID reused between the snapshot and the identity read — but on the immediate path the caller still holds the live Child handle, which the code itself argues pins the root PID against reuse (see the comment on send_terminate and on spawn_delayed_terminate). The root cannot be a reused PID here, so rc > snapshot_time can only ever be a false positive. Applying the bound to descendants only, or treating a root that post-dates the snapshot as root_ct = rc rather than as a hard abort, would keep the reuse protection where it matters without making a legitimate kill vanish.

[github-actions]

This arm creates a coverage hole that defeats the delayed terminate's primary purpose, and the design's own reasoning argues against it.

On the delayed path there are three outcomes for the root after the grace period:

root state after grace	fresh read	behavior
still alive	Some(exp)	walk, kill tree
exited, PID unassigned	None	seed with exp, reap orphans ((Some(exp), None) arm below)
exited, PID reassigned	Some(other)	rc != exp → return empty, kill nothing

The third row is the problem. The root exiting during the grace period is the expected case — that is what NotifyThenTerminate grants the grace period for. Whether the freed PID happened to get reassigned in the interim is pure chance, and it decides between "reap the orphaned descendants" and "leak the entire tree." The orphaned grandchildren are exactly what this code exists to clean up, and they are equally identifiable in both cases: they are validated against >= exp and <= snapshot_time, and the root node itself carries exp so kill_process_checked will refuse to terminate the PID's new occupant regardless.

The residual risk is also identical between the two rows. The doc block already accepts fossil-edge risk for the (Some(exp), None) arm — "a freed PID's fossil edges can include children of an INTERIM holder of that PID whose creation times happen to land in [exp, snapshot_time]." A reassigned PID is that same interim-holder case, just observed one step later. Returning empty here does not avoid that risk class; it only avoids it for the subset of runs where the reassignment happened to be visible at read time.

Seeding with exp and walking on mismatch (instead of returning empty) would make the two exited cases consistent and close the leak. If the abort is deliberate, the reasoning should be stated, because as written the doc lists this arm as "closing the reuse window between an earlier identity read and this walk" — but the walk never terminates the root's PID on identity mismatch anyway, so there is no reuse window here for the abort to close.

Related: kill_process_tree's empty-tree log reads "root identity could not be established (process exited or protected)", which describes the (None, None) case. When this arm fires the cause is a mismatch — reused PID — and the message will point maintainers at the wrong diagnosis.

[github-actions]

The stated reason for taking the stamp AFTER the walk does not hold, and it costs a wider reuse window for nothing.

CreateToolhelp32Snapshot copies the process list into the snapshot object at call time; Process32FirstW/Process32NextW then iterate that copy, not live kernel state. A process forked during the enumeration is therefore not in pairs at all, so it can never reach the <= snapshot_time check — the false-rejection scenario the comment describes ("a genuine child forked DURING the enumeration is no longer falsely rejected") is not reachable, in either stamp placement.

What the placement does change is real: the stamp is now snapshot_creation + walk_duration rather than ~snapshot_creation, so every PID reused inside that walk window passes the bound instead of being rejected. On a busy host with hundreds of processes the toolhelp walk is the non-trivial part of this function, which is precisely the interval the comment concedes it is widening.

Capturing the stamp immediately after the CreateToolhelp32Snapshot call (before Process32FirstW) keeps the invariant sound — every listed process was created before the snapshot, hence before the stamp — and tightens the bound to the smallest correct value. If there is a reason to believe the toolhelp iteration is not snapshot-backed, that is worth stating explicitly, since the whole soundness argument for snapshot_time rests on it.

[github-actions]

This sentence is now wrong in both halves, and it is the one a reader is most likely to rely on when reasoning about the fail-closed guarantee.

"If the ROOT's creation time is unreadable we return an empty Vec" — only when expected_root_ct is None. With a pin, the (Some(exp), None) arm seeds the walk and proceeds, which is the case the rest of this doc block goes on to describe at length.

"(callers fall back to killing just the root by other means)" — no caller does. kill_process_tree logs and returns without killing anything, and its own comment explains why the fallback was deliberately removed. Leaving this parenthetical in place suggests a safety net that does not exist, which matters because it is the difference between "under-kill one descendant" and "kill nothing at all."

The same stale framing appears two paragraphs up: "an edge whose child creation time is unreadable is skipped, preferring to under-kill" — unreadable children are no longer skipped; they are recorded as traversable carrying the parent's bound. That is documented correctly further down, so this earlier line now contradicts it.

[github-actions]

The None branch below is reasoned about as "the process likely already exited," but at this point that is the one thing it cannot mean — which inverts the conclusion drawn from it.

This runs synchronously inside the stdout read loop, where child is still Some(c), so tokio holds an open handle to the process. An open handle keeps the kernel process object (and therefore the PID) valid, so OpenProcess by PID succeeds even if the process has already terminated but not been reaped — a dead-but-held process still reports its creation time via GetProcessTimes. That is the same pinning the doc comment relies on two lines up.

So process_creation_time returning None here means the open or the query was denied, not that the process is gone: a PPL/protected target, or a same-PID process the session cannot query. In that case the None arm logs and never walks the tree, so NotifyThenTerminate degrades to notify-only and the process tree is never terminated after the grace period. The previous code unconditionally called kill_process_tree. This is a live-process leak on exactly the targets least likely to respond to CTRL_BREAK on their own.

Two consequences worth separating:

• The log message will mislead whoever debugs it — it asserts an exit that did not happen, when the real cause is an access denial that is likely reproducible and fixable.
• If the intent is genuinely fail-closed here, the trade is heavier than the comment implies: it is not "skip one ambiguous kill," it is "silently never terminate this tree." A warn! rather than info! would at least surface it.

Note this also makes the reachability of (Some(exp), None) in collect_tree_validated narrower than its doc claims: on the delayed path the root's handle is released when the enclosing run_subprocess returns, so whether the post-grace read yields None (PID freed) or Some(other) (PID reassigned) depends on timing the code does not control — see my note on the rc != exp abort.

[github-actions]

This arm accepts an unreadable child with no validation at all, which lets a stale edge that the readable path would reject become a traversable node — and that can end in killing an unrelated process's children.

Concretely: PID X was freed by some process and later reassigned to our root R. A process Y forked by that earlier PID-X holder is still in the snapshot carrying th32ParentProcessID == R.pid, so it shows up as a child of R. Y is not ours.

• If Y is readable: Y.ctime < R.ctime (it predates R), the >= check rejects it, subtree pruned. This is the stale-edge guard working.
• If Y is unreadable: it is pushed unconditionally with creation_time = R.ctime. The >= check that would have caught it never runs, and it now carries a bound younger than its own true creation time. Y's genuine children — unrelated live processes — then pass >= R.ctime and <= snapshot_time and are terminated.

So the failure mode differs in kind from the readable path: for a readable child, unreadability of identity is what fails closed, but here the absence of identity is treated as a pass. The asymmetry is that None skips validation rather than substituting a conservative bound.

The doc block's residual note covers a related case ("a freed PID's fossil edges ... children of an INTERIM holder"), but it frames it as arising from seeding a dead root with exp. This instance needs neither a pinned identity nor a dead root — a live, freshly validated root on the immediate send_terminate path is enough, as long as one intermediate is unreadable. That is a broader reach than the note describes.

Keeping dead intermediates traversable to reach live grandchildren is a reasonable goal, but the bound it inherits should not be weaker than what a readable node at that position would have to satisfy. Using the snapshot's own th32ParentProcessID chain to require that the unreadable node's readable descendants still satisfy >= against the nearest readable ancestor would preserve the intent without dropping the stale-edge check. At minimum this vector deserves to be called out separately from the dead-root residual, since the mitigation reasoning ("bounded, no worse than the pre-validation walk") does not obviously transfer.

[github-actions]

This parenthetical is stale as of the last commit and now states the opposite of what the code does: snapshot_process_parents captures the stamp immediately after CreateToolhelp32Snapshot returns, before the enumeration walk. Its own doc explicitly calls the post-walk placement "an earlier version".

That matters beyond wording, because the soundness argument here is derived from the placement. The post-walk version gave the bound slack equal to the walk duration; the new placement removes that slack, so the justification for <= snapshot_time now rests entirely on "the snapshot froze the list at call time, therefore every listed process predates the call" rather than on "created before the enumeration walk finished". Both conclusions are true, but only the second is written here, and a reader auditing the tightened bound will check the wrong invariant.

Suggest replacing "(the stamp is captured after the walk)" with something like "(the stamp is captured immediately after CreateToolhelp32Snapshot, which froze the list, so every listed process predates it)".

[github-actions]

Moving the stamp from after the walk to here removes the one thing that was hiding a clock-resolution mismatch, so this commit makes a silent under-kill more likely rather than less.

collect_tree_validated claims the comparison is "exact" because both sides are FILETIME ticks on the same wall-clock base. Same base, yes — but not necessarily the same sampling resolution:

• std::time::SystemTime::now() on Windows is backed by GetSystemTimeAsFileTime, the coarse system time, which only advances on the clock tick (~15.6 ms by default). This doc comment itself asserts equivalence to that API, so the coarse behavior is the assumption being relied on.
• Process creation times from GetProcessTimes are observably finer-grained than a 15.6 ms tick on modern Windows.

If the stamp lags true "now" by up to a tick while creation times do not, then a descendant created inside the tick window immediately preceding CreateToolhelp32Snapshot reads child_ct > snapshot_time and is rejected at subprocess.rs:530 — a live descendant of a real cancel is skipped and leaks. The old post-walk placement happened to absorb this, because the enumeration walk (hundreds of processes) took long enough to cover the lag; the tightened placement gives up that slack. So the commit message's "for no benefit" holds for the reuse window, but the slack was doing something else that was not accounted for.

This is the burst-spawn shape, not an exotic one: a wrapper that spawns its real payload and gets cancelled milliseconds later is exactly the tree this code exists to reap.

The direction is still under-kill, which is the safe side, and it is silent — but a rejected child also prunes its entire subtree, so one lost edge can leak a whole branch. Options, roughly in order of cost: add an explicit tolerance of one clock tick when comparing against snapshot_time; or use GetSystemTimePreciseAsFileTime (which does need a windows feature, the thing this helper was written to avoid); or at minimum drop the word "exact" and record the granularity assumption, since a future reader will otherwise re-tighten this the same way.

[github-actions]

Scope note: the embedded cross-user helper still carries the pre-fix version of this exact code, so the PID-reuse kill this PR eliminates remains reachable on the cross-user path.

crates/openjd-sessions/src/helper/src/runner_win.rs has its own kill_process_tree (line 287) / collect_tree (line 303) / kill_process pair — a copy of what this diff replaces here. It has the cycle guard, but none of the creation-time validation: no >= parent edge check, no snapshot bound, no kill-time identity revalidation, and it terminates by raw PID.

That path is not a corner case. run_subprocess hard-rejects cross-user work (subprocess.rs:901), routing every cross-user action through the helper — so on Windows, cross-user is precisely the configuration that gets the unvalidated killer. It is also called from all three cancel sites in runner_win.rs (124, 167, 176), including the notify-then-terminate escalation after a grace period, which is the longest-window and highest-risk case: the helper reads child_pid once at spawn and force-kills that raw PID after the notify period, exactly the collect-to-kill window kill_process_checked was added to close.

Not necessarily in scope for this PR — the helper is a separate Cargo project with its own lockfile and independent CI, and issue #347's Job Object rework may subsume both copies. But the two implementations have now diverged in security behavior rather than just in style, and nothing in the code says so. Worth either porting the validation across or leaving a pointer in runner_win.rs noting that it is deliberately behind, so the gap is not mistaken for the helper simply not having been looked at.