[leonarduk]

Fixes #123.

A PID is not an identity. A process dies, the file naming it does not, and Windows hands the number to whatever starts next — so "the PID I recorded is running" and "the thing I recorded is running" stop being the same claim. This PR removes every place that confused the two, in the tests (which is what #123 reports) and in Stop-Slot (which the new CI job caught on the way).

The test fix (#123 as filed)

Cleanup force-killed every PID the run had ever recorded, most of whose processes had long exited — the stand-ins Stop-Slot already killed, and the runner-loop.ps1 slots that exit at once on the empty pat.secret. With two runs overlapping, a finishing one killed a live run's slot processes, and that run then failed checks about its own slots.

Every spawned process is now tracked as a Process object with its handle held open — Windows will not reuse a PID while a handle to it is open — and cleanup kills only tracked processes that have not exited. A PID read back from a .pid file is trusted only if the process it names started before that file was written. The stand-ins wait for the test process to exit rather than sleeping a fixed 120s, and the "was it killed" checks use WaitForExit on the held handle.

The rules live in tests/ProcessTracking.ps1 so that tests/process_tracking_test.ps1 can check them directly — 12 deterministic checks, under both shells. The suite itself cannot check them: the condition that broke it is two runs sharing a machine and a PID passing between them, which no single run can arrange on demand.

The Stop-Slot fix (found by the new job)

Same bug, in the fleet. A slot's .pid outlives its process, and Stop-Slot read the bare number, found a process, and ran taskkill /PID <n> /T /F — a stranger and its children. Get-SlotProcess is now the only route to that number: a slot's process counts as its own only if it started before the .pid naming it was written, and the handle is held from that check through the wait to the kill, so nothing can be recycled underneath any of it. Test-SlotRunning, Start-Slot's already-running check and list's running count all go through it.

The CI job, and what it is worth

Overlapping runs starts six copies of the suite 20s apart, alternating shells — the condition the existing job cannot create, one suite per fresh VM. It fails if any run fails, and if the runs never actually overlapped.

Being straight about what it proved: it did not reproduce #123's PID-reuse symptom. Run against the unfixed suite it failed 3 of 6 — all three from a different, deterministic bug it exposed, Check's expected phrase being word-wrapped when the suite's stdout is a file rather than a pipe (so powershell -File tests\windows_pools_test.ps1 > out.txt fails on main today; expected phrases are now matched with whitespace collapsed). Later it caught the Stop-Slot kill above, and a flake in the new test file. It is a regression guard that earns its keep, not the proof — process_tracking_test.ps1 is the proof.

🤖 Generated with Claude Code

https://claude.ai/code/session_01Sz2Nx4sPZaFYv7gaCmMmAC

[coderabbitai]

Important

• 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: d9056078-9d88-4f92-a27a-6d8d10a07c1e

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[github-actions]

GPT AI Code Review

Skipped: API key not configured

The OPENAI_API_KEY repository secret is not set.

To enable GPT reviews, add OPENAI_API_KEY as a repository secret:

1. Go to Settings > Secrets and variables > Actions in this repo.
2. Click New repository secret.
3. Name: OPENAI_API_KEY
4. Value: your GPT API key.

Once the secret is configured, re-run this workflow or push a new commit to trigger a fresh review.

REVIEW SKIPPED - API KEY NOT CONFIGURED

---

Reviewed by GPT via gpt-pr-review.yml. Advisory only.

[github-actions]

DeepSeek AI Code Review

1. Acceptance criteria

The diff does not contain the actual fix to tests/windows_pools_test.ps1. The PR title says "Track spawned processes by handle in windows_pools_test.ps1", and the issue's "Fix (test-only)" section lists five specific changes to that file:

1. Track every spawned process as a Process object with handle held open; Cleanup kills only tracked, non-exited processes.
2. For PIDs read from a slot's .pid file, track only if the process started before the file was written.
3. Stand-in processes wait on the test process via WaitForExit instead of Start-Sleep 120.
4. Tree test waits for a numeric PID in child.pid.
5. "Was it killed" checks use WaitForExit on the held handle.

None of these appear in the diff. The diff only adds:

• .github/scripts/Invoke-OverlappingPoolTests.ps1 (a new harness that runs the existing test file repeatedly and concurrently)
• A new overlapping-runs job in .github/workflows/powershell-tests.yml

The harness is a good regression-detection mechanism, but it exercises the unfixed test. On windows-latest with the harness's 20s stagger and 6 runs, the bug from the issue will very likely reproduce (that's the whole point of the harness), so this PR as-is will make CI red rather than green. The core acceptance criteria — the actual fix to windows_pools_test.ps1 — are unmet.

If the diff was truncated and the windows_pools_test.ps1 changes are simply not shown, that needs to be stated; as presented, the fix is missing.

2. Bugs and logic errors

In Invoke-OverlappingPoolTests.ps1:

• Overlap detection is wrong when a run times out. $run.End is set to [DateTime]::Now after Kill(), but Kill() is asynchronous — the process may still be alive for a moment. More importantly, if a run times out, $run.Code = 'timed out' (a string), and the later if ($run.Code -ne 0) comparison is 'timed out' -ne 0 → $true, so it's counted as failed. That's fine. But the overlap check uses $b.Start -lt $a.End; a timed-out run's End is set to the moment we gave up, which is correct-ish. Not blocking, but the semantics of "overlapped" are fuzzy when runs are killed.

• $run.Proc.WaitForExit($left) with $left = 0. When the deadline has passed, WaitForExit(0) returns immediately with $false, then Kill() is called. That's the intended path, but note that WaitForExit(int) on a System.Diagnostics.Process that has already exited returns $true immediately — fine. No bug, just noting the boundary.

• $run.Proc.ExitCode after Kill() is not read. In the timeout branch, $run.Code is set to the string 'timed out' and ExitCode is never read. That's intentional, but it means a killed run's actual exit code is discarded. Acceptable.

• Get-Content $log on a log that may still be being written. By the time we reach the reporting loop, all runs have exited (or been killed), so the logs are closed. Fine.

• $shells[$i % $shells.Count] — if only one shell is present, every run uses it; the warning is printed. Fine.

• Start-Process -RedirectStandardOutput / -RedirectStandardError to separate files — correct for 5.1. Good.

• $run.Proc.Kill() in a try/catch that swallows everything. If Kill() fails (e.g., access denied), the process is left running and the job may hang past timeout-minutes: 25. The catch { } is a swallowed exception. Non-blocking given the CI timeout, but worth a comment.

• $overlapped check is O(n²) over 6 runs — trivial, not a concern.

• exit 1 when -not $overlapped — this is a good guard, but note that with -StaggerSeconds 20 and a suite that takes ~30s, runs will overlap. If the suite ever gets fast enough to finish in <20s, this check will fail the job. That's the intended behavior per the comment, but it's a latent flake if the suite's runtime drops. Consider making the stagger adaptive or documenting the coupling.

3. CLI and API safety

• No new Python dependencies, no pyproject.toml changes needed. Good.
• No CLI signature changes. Good.
• No GitHub API calls. Good.
• CI permissions: the new job uses permissions: contents: read, which is correct for a test job that only checks out code. Good.
• actions/checkout@v7 — v7 exists as of late 2024/2025; assuming the repo already uses v7 elsewhere (the existing job in the same file uses it), this is consistent. If the existing job uses v4, this is an inconsistency worth flagging, but the diff shows the existing job's checkout line is not visible, so I can't confirm. Non-blocking.
• Shell injection: Start-Process -FilePath $shell -ArgumentList @(...) with $test interpolated as an argument. $test is derived from $TestPath (a script parameter) joined to $root. If a caller passes a malicious -TestPath, it's passed as a single argument to -File, so it can't inject additional arguments. Safe.
• -ExecutionPolicy Bypass — standard for CI, acceptable.

4. Test coverage

The new harness is itself the test coverage for the concurrency bug, which is the right idea. However:

• It does not test the fix, because the fix isn't in the diff. Once the fix lands, this harness will exercise it.
• The harness's own logic (overlap detection, timeout handling, log capture) has no tests. Given it's a CI-only script, that's acceptable, but the -not $overlapped guard is the kind of thing that could silently pass a broken harness if the suite gets fast. Consider a self-test or a -ForceOverlap mode.
• The issue mentions the tree test's child.pid race and the Get-Process -Id 0 idle-process bug. Neither is addressed in the diff, and neither is exercised by the new harness (the harness just runs the existing suite more times). So even after this PR, those latent bugs remain.

5. Suggested follow-up issues

• Add the actual windows_pools_test.ps1 fix (handle-tracked processes, WaitForExit-based liveness, child.pid numeric wait) — this is the blocking gap, not a follow-up.
• Make Invoke-OverlappingPoolTests.ps1 fail loudly if Kill() throws instead of swallowing the exception.
• Add a self-test or -ForceOverlap mode to Invoke-OverlappingPoolTests.ps1 so the overlap guard can't silently pass.
• Document the coupling between -StaggerSeconds and the suite's runtime in the harness's help text.

Verdict

The PR adds a valuable concurrency harness but does not contain the fix described in the issue's "Fix (test-only)" section. The acceptance criteria are unmet: tests/windows_pools_test.ps1 is not modified in the diff, so the PID-reuse bug, the child.pid race, and the Get-Process -Id 0 idle-process bug all remain. Worse, the new overlapping-runs CI job will very likely reproduce the bug and turn CI red, since it runs the unfixed suite concurrently.

• **REQUEST CHANGES** — the core fix to tests/windows_pools_test.ps1 is missing from the diff; the new harness alone does not satisfy the acceptance criteria and will likely fail CI against the unfixed suite.

---

Reviewed by DeepSeek via deepseek-pr-review.yml. Advisory only.

[github-actions]

GPT AI Code Review

Skipped: API key not configured

The OPENAI_API_KEY repository secret is not set.

To enable GPT reviews, add OPENAI_API_KEY as a repository secret:

1. Go to Settings > Secrets and variables > Actions in this repo.
2. Click New repository secret.
3. Name: OPENAI_API_KEY
4. Value: your GPT API key.

Once the secret is configured, re-run this workflow or push a new commit to trigger a fresh review.

REVIEW SKIPPED - API KEY NOT CONFIGURED

---

Reviewed by GPT via gpt-pr-review.yml. Advisory only.

[github-actions]

DeepSeek AI Code Review

1. Acceptance criteria

Both ACs are addressed:

• AC1 (scale down removes highest slots, keeps rest running): The root cause — Cleanup force-killing bare PIDs that had been reused by concurrent runs — is fixed by Stop-TrackedProcesses only killing tracked Process objects with handles held open. The Register-SlotPidFile start-time guard (ProcessTracking.ps1:82) prevents tracking a process that merely inherited a PID.
• AC2 (Stop-Slot -Force kills children): The tree test now waits for a numeric PID in child.pid (windows_pools_test.ps1:376-380) and uses WaitForExit on the held handle (:393), so a concurrent run can no longer kill the parent before Stop-Slot runs taskkill /T.

The latent problems called out in the issue (empty .pid → PID 0, Get-Process -Id on reused PIDs, fixed 120s sleeps) are all addressed.

2. Bugs and logic errors

Register-SlotPidFile start-time check is racy in the wrong direction (ProcessTracking.ps1:82):

if ($proc.StartTime -gt (Get-Item $PidFile).LastWriteTime) { return $null }

The comment claims "a process that survives both checks cannot have been replaced between them" — but the handle is opened before the start-time check, and the start-time check reads $PidFile.LastWriteTime after opening the handle. That's fine for the handle-pins-PID argument. However, the ordering of the two reads is: Get-Process → open handle → read StartTime → read file mtime. If the file is rewritten between the Get-Process and the mtime read (e.g. a slot restarts and rewrites .pid), the mtime will be newer than the process start time and the process will be refused even though it's the one the file now names. That's a false negative, not a false positive — safe, but it means a legitimately-restarted slot's process won't be tracked and won't be cleaned up. Given the test's own restart-runner check (windows_pools_test.ps1:573), this is a real path. Worth a comment acknowledging the direction of the race, or reading the mtime first.

Get-StandInCommand default $PID is the caller's PID, not the test process's (ProcessTracking.ps1:52):

function Get-StandInCommand {
    param([int]$ProcessId = $PID)

$PID inside a function is the current PowerShell process's PID. When windows_pools_test.ps1 dot-sources this and calls Get-StandInCommand at line 60, $PID is the test process — correct. But Start-DummyProcess (:158) calls it from inside a function in the same script scope, so $PID is still the test process. OK. However, process_tracking_test.ps1 calls Get-StandInCommand from inside Start-StandIn (:44), which is also the test process. So the default is correct in all current call sites — but it's fragile: any future caller that invokes it from a child scope where $PID differs will silently get the wrong process. A comment or explicit $script:TestPid capture at dot-source time would be safer.

Stop-TrackedProcesses does not clear the list (ProcessTracking.ps1:92-96):

The function is documented as "safe to call more than once," and it is — but the list grows unboundedly across a run. For a test that spawns dozens of processes this is fine; for a long-lived consumer it's a leak. More importantly, process_tracking_test.ps1:120 calls it twice and expects the second call to be a no-op on already-exited processes — which works because HasExited is checked. Not a bug, but the list is never drained despite the header comment saying "drained by Stop-TrackedProcesses." Either drain it or fix the comment.

Invoke-OverlappingPoolTests.ps1 overlap detection uses $run.End before it's set for later runs (:120-127):

The overlap loop runs after the wait loop, so all End values are set. But $run.End is set to [DateTime]::Now if ExitTime is unavailable (:113), and the wait loop processes runs in start order — so a run that finished long ago but whose ExitTime couldn't be read gets End = now, which is after later runs started. That would make $overlapped true even if nothing overlapped. The try/catch around ExitTime swallows the failure silently. Low risk on Windows where ExitTime is reliable, but the fallback is wrong-directioned.

Invoke-OverlappingPoolTests.ps1 timeout accounting (:104-108):

The deadline is shared across all runs, and each WaitForExit($left) consumes from it. If run 0 takes 11 minutes, run 1 gets 1 minute and is killed even though it started 20s later and would have finished in 30s. The timeout should be per-run ($run.Start.AddMinutes($TimeoutMinutes)), not global. As written, a slow first run can cause spurious failures in later runs.

3. CLI and API safety

No Python, CLI, or GitHub API changes. The new CI job uses permissions: contents: read (powershell-tests.yml:57-58), matching the existing job. actions/checkout@v7 is consistent with the rest of the workflow. No new dependencies.

One CI concern: the new overlapping-runs job has timeout-minutes: 25 and runs 6 suites at 20s stagger with a 12-minute per-script timeout. If the suite takes ~3 minutes (as the issue implies: "22–31s after its pool is created" for one phase, but the full suite is longer), 6 runs staggered 20s apart finish around 3min + 100s ≈ 5min. 25 minutes is generous. Fine.

4. Test coverage

process_tracking_test.ps1 covers the rules well: tracked-vs-untracked, start-time guard, empty/whitespace/non-numeric .pid, already-exited process, idempotent Stop-TrackedProcesses, and the stand-in command's wait-for-parent behavior.

Missing: a test that Register-SlotPidFile refuses a .pid file whose mtime is newer than the process start time (the restart case from §2). The current "reused PID" test (:76-84) sets the mtime backwards to simulate a stranger; the inverse case — a legitimate process whose file was rewritten after it started — isn't covered, and that's the case that would silently drop a real slot from tracking.

The overlapping-runs CI job is the right shape for reproducing #123, but it's inherently probabilistic: 6 runs at 20s stagger may not reproduce on a fast runner. The $overlapped guard (:120-127) at least fails loudly if nothing overlapped, which is good.

5. Suggested follow-up issues

• Register-SlotPidFile: document or fix the mtime-vs-StartTime race direction for restarted slots
• Invoke-OverlappingPoolTests.ps1: make the timeout per-run rather than shared across all runs
• Stop-TrackedProcesses: drain $script:TrackedProcesses or correct the "drained by" comment
• Get-StandInCommand: capture the test PID explicitly at dot-source time instead of relying on $PID default
• Invoke-OverlappingPoolTests.ps1: don't fall back to [DateTime]::Now for End when ExitTime is unreadable

APPROVE — the fix correctly addresses both ACs and the latent bugs; the concerns above are non-blocking (race direction is safe, timeout accounting only bites on pathological runs, and the rest are documentation/robustness).

---

Reviewed by DeepSeek via deepseek-pr-review.yml. Advisory only.

[github-actions]

DeepSeek AI Code Review

1. Acceptance criteria

Both ACs are addressed:

• AC1 (scale down keeps lower slots running): Root cause is correctly identified — Cleanup force-killing bare PIDs that had been reused by a concurrent run's live slot-1 dummy. The fix (Stop-TrackedProcesses killing only tracked, non-exited processes) directly removes the cross-run interference. The new overlapping-runs CI job reproduces the condition that exposed it.
• AC2 (Stop-Slot -Force kills children): The tree test now waits for a numeric PID in child.pid (via Register-SlotPidFile's ^\d+$ check) and uses WaitForExit on the held handle rather than Get-Process -Id. The parent is registered via Register-Spawned before the child is looked up, so a concurrent run can't kill the parent first.

The latent problems called out in the issue (empty .pid → PID 0, Get-Process -Id on reused PIDs, fixed 120s sleeps) are all addressed.

2. Bugs and logic errors

Register-SlotPidFile — StartTime can throw on a just-exited process. Lines around if ($proc.StartTime -gt (Get-Item $PidFile).LastWriteTime). Get-Process can return a Process object for a process that exits between the Get-Process call and the .StartTime access; on Windows, .StartTime on an exited process throws InvalidOperationException. The try { $null = $proc.Handle } catch { return $null } above it does not cover this — the handle is opened, then the process exits, then .StartTime throws. This is exactly the race the function exists to handle (a slot that exited between the .pid write and the read). Wrap the StartTime/LastWriteTime comparison in the same try/catch and return $null on failure.

Register-SlotPidFile — Get-Item $PidFile can throw if the file is deleted between the Get-Content and the Get-Item. Same window, same fix: fold it into the try.

Stop-TrackedProcesses swallows all exceptions. try { if (-not $proc.HasExited) { $proc.Kill() } } catch { }. HasExited itself can throw on a process whose handle was invalidated (e.g. the process object was disposed, or the handle was closed by something else). The empty catch means a genuinely broken tracked process is silently skipped. For a test helper this is probably acceptable, but a Write-Warning on unexpected exception types would make a future failure diagnosable rather than mysterious.

Invoke-OverlappingPoolTests.ps1 — $run.Proc.ExitCode after Kill(). In the timeout branch, $run.Code = 'timed out' is set, but the process is killed and WaitForExit is never called again. On Windows, ExitCode on a killed process is fine, but the code path is inconsistent — the non-timeout branch reads ExitCode after WaitForExit, the timeout branch doesn't. Not a bug per se, but the $run.Code type is int in one path and string in the other, and the later if ($run.Code -ne 0) comparison relies on PowerShell's loose typing. It works, but a [string] cast on the exit code would be clearer.

Invoke-OverlappingPoolTests.ps1 — overlap detection can miss real overlaps. The $overlapped check requires $a.EndIsExact. If a run is killed (timeout), EndIsExact stays $false, and that run's overlap with others is not counted. If all runs time out, $overlapped is $false and the script exits 1 with "nothing was tested concurrently" — which is misleading, since the runs clearly overlapped (they all timed out together). The failure message should distinguish "no overlap" from "couldn't determine overlap because runs were killed."

3. CLI and API safety

No CLI or Python API changes. No new dependencies. No GitHub API calls added. The new CI job uses actions/checkout@v7 with permissions: contents: read — correct scope, no token exposure. The shell: pwsh step correctly propagates $LASTEXITCODE via exit $LASTEXITCODE.

One note: the new job has timeout-minutes: 25 while the script's own -TimeoutMinutes defaults to 12. Six runs staggered 20s apart, each up to 12 minutes, means the last run can start at t=100s and finish at t=820s (~14 min) — within the 25-minute job timeout, but the margin is thin if the suite slows down. Not blocking.

4. Test coverage

tests/process_tracking_test.ps1 covers the rules well: tracked-vs-untracked kill behavior, .pid file with a live process, .pid file with a reused PID, empty/whitespace/non-numeric .pid files, .pid file naming an exited process, idempotent Stop-TrackedProcesses, and the stand-in command's lifetime semantics.

Missing cases worth noting (non-blocking):

• No test for Register-SlotPidFile when the .pid file is deleted between Get-Content and Get-Item (the race in §2).
• No test for Register-SlotPidFile when the process exits between Get-Process and .StartTime (the race in §2).
• The overlapping-runs CI job is the real integration test for AC1/AC2, but it's not runnable locally without the full suite — the unit test file is the only fast feedback loop.

5. Suggested follow-up issues

• Harden Register-SlotPidFile against StartTime/Get-Item races on a process that exits mid-check
• Distinguish "no overlap detected" from "overlap undeterminable because runs were killed" in Invoke-OverlappingPoolTests.ps1
• Add a Write-Warning for unexpected exception types in Stop-TrackedProcesses to aid future debugging
• Consider a -TimeoutMinutes default in Invoke-OverlappingPoolTests.ps1 that leaves more headroom under the CI job's timeout-minutes

APPROVE — the fix correctly addresses both ACs and the latent issues; the Register-SlotPidFile race is a real but narrow edge case that should be fixed in a follow-up rather than blocking this PR.

---

Reviewed by DeepSeek via deepseek-pr-review.yml. Advisory only.

[github-actions]

GPT AI Code Review

Skipped: API key not configured

The OPENAI_API_KEY repository secret is not set.

To enable GPT reviews, add OPENAI_API_KEY as a repository secret:

1. Go to Settings > Secrets and variables > Actions in this repo.
2. Click New repository secret.
3. Name: OPENAI_API_KEY
4. Value: your GPT API key.

Once the secret is configured, re-run this workflow or push a new commit to trigger a fresh review.

REVIEW SKIPPED - API KEY NOT CONFIGURED

---

Reviewed by GPT via gpt-pr-review.yml. Advisory only.

[github-actions]

GPT AI Code Review

Skipped: API key not configured

The OPENAI_API_KEY repository secret is not set.

To enable GPT reviews, add OPENAI_API_KEY as a repository secret:

1. Go to Settings > Secrets and variables > Actions in this repo.
2. Click New repository secret.
3. Name: OPENAI_API_KEY
4. Value: your GPT API key.

Once the secret is configured, re-run this workflow or push a new commit to trigger a fresh review.

REVIEW SKIPPED - API KEY NOT CONFIGURED

---

Reviewed by GPT via gpt-pr-review.yml. Advisory only.

[github-actions]

DeepSeek AI Code Review

1. Acceptance criteria

Both AC items are addressed:

• Check 1 (scale down keeps lower slots running): Root cause is the bare-PID Cleanup killing other runs' live slot processes. Fixed by Stop-TrackedProcesses in ProcessTracking.ps1, which only kills tracked Process objects that haven't exited, and by holding the handle open ($null = $Process.Handle) so Windows won't reuse the PID.
• Check 2 (Stop-Slot -Force kills children): Root cause is a concurrent run killing the tree-test parent before Stop-Slot runs, so taskkill /T never fires. Fixed by tracking the parent via Register-Spawned and the child via Register-SlotPidFile, plus the start-time-vs-file-write-time guard.

The latent problems called out in the issue are also addressed: the [int]$null → PID 0 hazard is guarded by the ^\d+$ regex in Register-SlotPidFile; the "is it dead yet" polls now use WaitForExit on the held handle; the fixed 120s sleep is replaced by Get-StandInCommand waiting on the test process.

2. Bugs and logic errors

Register-SlotPidFile start-time guard is inverted in effect for the "gone" case. The check is if ($proc.StartTime -gt (Get-Item $PidFile).LastWriteTime) { return $null }. This correctly refuses a process that started after the file was written. But consider the actual reuse scenario: the original slot exits, Windows hands the PID to a new process, and the new process started after the file was written — so it's refused. Good. However, if the new process happened to start before the file's LastWriteTime (e.g., the file was rewritten by a slot restart, or the file's mtime is later than the impostor's start), the impostor is tracked and will be killed. The comment acknowledges this ("a .pid rewritten under us ... can only make this stricter"), but the direction is actually the opposite: a later file mtime makes the guard weaker, not stricter. This is a real (if narrow) hole. Worth a comment correction at minimum; the guard is best-effort and should say so.

Stop-TrackedProcesses never clears the list, and Register-Spawned/Register-SlotPidFile append unconditionally. The comment says this is deliberate so a double-cleanup finds the same list. That's fine for a single run, but process_tracking_test.ps1 dot-sources the same file and calls Stop-TrackedProcesses multiple times across checks — the list grows monotonically. Not a bug in the test as written (each check tracks its own), but the "safe to call again" semantics combined with an ever-growing list means a long-lived process that dot-sources this and calls Stop-TrackedProcesses in a loop would re-kill the same dead handles forever. Minor, but the invariant "never empties it" is load-bearing and undocumented as such.

Get-StandInCommand default $PID is evaluated at call time in the caller's scope. In windows_pools_test.ps1, $dummyCommand = Get-StandInCommand is called at the top of the script, so $PID is the test script's own PID — correct. But in process_tracking_test.ps1, Start-StandIn calls Get-StandInCommand with no argument, so $PID is again the test process. That's the intent. However, the parent-script case in windows_pools_test.ps1 interpolates $dummyCommand into a here-string that is then written to parent.ps1 and run as a separate process — so the child stand-in waits on the test process, not the parent. That's actually what you want (the child should die when the test dies), but it means the parent's own $dummyCommand line at the end of parent.ps1 makes the parent wait on the test process too. Fine, but the naming (Get-StandInCommand -ProcessId) suggests it's parameterized per-caller when in practice it's always the test process. Not blocking.

Invoke-OverlappingPoolTests.ps1 overlap detection can false-negative. $overlapped requires $a.EndIsExact. If ExitTime throws (process already reaped) for every run, $overlapped stays $false and the script exits 1 with "nothing was tested concurrently" — even though runs did overlap. On a busy runner this could flake. The try/catch swallows the reason. Consider falling back to $run.End = [DateTime]::Now for the overlap check with a warning, or at least logging when EndIsExact is false.

Invoke-OverlappingPoolTests.ps1 timeout handling. When a run times out, $run.Proc.Kill() is called but WaitForExit is not awaited afterward, and $run.End is set to [DateTime]::Now. The killed process's children (the stand-ins) are not killed — they wait on the test process, which is now dead, so they should exit on their own. OK, but only because of the Get-StandInCommand design. Worth a comment.

3. CLI and API safety

No Python CLI or public API changes. No new dependencies. No GitHub API calls added. The new CI job uses permissions: contents: read, matching the existing job. actions/checkout@v7 — verify that's the intended major (current is v4 as of writing; v7 may not exist). This is worth double-checking before merge — a nonexistent action version will fail the job immediately.

4. Test coverage

process_tracking_test.ps1 covers the rules well: tracked-vs-untracked kill behavior, the start-time guard, empty/whitespace/non-numeric .pid reads, double-cleanup safety, and the stand-in lifetime contract. The one thing it explicitly cannot test (PID reuse under real churn) is called out honestly in the header.

Gap: there is no test that Register-SlotPidFile refuses a process whose start time is before the file write but which is nonetheless an impostor (the hole noted in §2). That's hard to construct without PID reuse, so it's acceptable to leave untested — but the comment claiming the guard is "stricter" in that case should be corrected.

The overlapping-runs CI job is the right shape for the original bug. The -StaggerSeconds 20 / -Runs 6 defaults are justified in the header.

5. Suggested follow-up issues

• Correct the Register-SlotPidFile comment: a later .pid mtime weakens the start-time guard, not strengthens it.
• Add a fallback overlap check in Invoke-OverlappingPoolTests.ps1 when ExitTime is unavailable, so a busy runner doesn't false-fail the "did they overlap" assertion.
• Verify actions/checkout@v7 exists; pin to a known-good major if not.
• Document the "never empties $script:TrackedProcesses" invariant as load-bearing for repeated Stop-TrackedProcesses calls.

APPROVE — the fix correctly addresses both AC items and the latent hazards; the remaining items are non-blocking (comment accuracy, CI action version verification, overlap-detection robustness).

---

Reviewed by DeepSeek via deepseek-pr-review.yml. Advisory only.

[leonarduk]

The control run, as promised in the description

e63df52 ran the Overlapping runs job against the unfixed suite. Three of six runs failed — and not from #123. All three were the 5.1 runs, all three failed the same three checks, and every one of them had the right exit status with only the expected phrase not matching:

windows-pools: no runner on GitHub matches any of pool 'worm''s slots, so can't
confirm it is idle -- -Force to do it anyway

Word-wrapped mid-phrase, because the suite's own stdout was a file rather than a pipe and the host width differs. Deterministic, nothing to do with PIDs, and it means powershell -File tests\windows_pools_test.ps1 > out.txt fails on main today. That is what the whitespace-collapsing match in Check/Check-Wp fixes.

No PID-reuse symptom appeared at all, in any of the six runs. A fresh CI VM does not churn PIDs the way a working machine does. So this job is a regression guard — it earned its keep by catching the wrap bug — but it is not proof the PID fix works. What checks that directly is tests\process_tracking_test.ps1, which asserts the rules themselves (a .pid file naming a process that started after the file was written is refused, cleanup kills only what it tracked, a stand-in outlives a slow run and dies with a killed one) rather than waiting for the race to come back.

On the DeepSeek review

Taken, in eed8ea6:

• Register-SlotPidFile can no longer throw. The handle open and the start-time comparison are in one try: the process may exit and the .pid file may be deleted between the reads, and in both cases there is nothing of ours to clean up, so $null is the answer the rest of the function already gives.
• The shared deadline is now documented as shared — one wait covering all runs, so the job's timeout-minutes has to cover the staggering plus that, not $Runs times it.

Not taken:

• Stop-TrackedProcesses should warn instead of swallowing. The overwhelmingly common exception there is the benign one: a process that exits between HasExited and Kill(). A cleanup path that warns on that would cry wolf on most runs, and a cleanup path that is noisy gets ignored.
• $run.Code being int in one branch and string in the other. Deliberate: 'timed out' is not an exit code and should not be mistaken for one. The report prints it as a word.
• "nothing was tested concurrently" would be misleading if every run timed out. That message is unreachable in that case: a timed-out run is counted in $failed, and the failure branch exits before the overlap check is reached.

---

Generated by Claude Code

[github-actions]

GPT AI Code Review

Skipped: API key not configured

The OPENAI_API_KEY repository secret is not set.

To enable GPT reviews, add OPENAI_API_KEY as a repository secret:

1. Go to Settings > Secrets and variables > Actions in this repo.
2. Click New repository secret.
3. Name: OPENAI_API_KEY
4. Value: your GPT API key.

Once the secret is configured, re-run this workflow or push a new commit to trigger a fresh review.

REVIEW SKIPPED - API KEY NOT CONFIGURED

---

Reviewed by GPT via gpt-pr-review.yml. Advisory only.

[github-actions]

DeepSeek AI Code Review

1. Acceptance criteria

Both AC items from the linked issue are addressed:

1. Scale-down keeps lower slots running — the root cause (Cleanup killing bare PIDs that had been reused by a concurrent run's live slot-1) is fixed by Stop-TrackedProcesses only killing tracked Process objects whose handles are held open. Register-SlotPidFile additionally refuses PIDs whose process started after the .pid file was written, which is the exact shape of a reused PID.
2. Stop-Slot -Force kills children — the tree test now waits for a numeric PID in child.pid (via Register-SlotPidFile) and uses WaitForExit on the held handle rather than Get-Process -Id. The parent is registered via Register-Spawned, so a concurrent run's Cleanup can no longer kill it before Stop-Slot runs.

The latent problems called out in the issue are also handled: the [int]$null → PID 0 case is guarded by the ^\d+$ regex; the "is it dead yet" polls use WaitForExit on the handle; the fixed 120s sleep is replaced by Get-StandInCommand waiting on the test process.

2. Bugs and logic errors

Register-SlotPidFile — the start-time check is not sufficient to prove ownership. The comment claims "a process that merely inherited the number cannot have done [started before the file was written]." That is true for the reuse case, but it is not the only way a stranger can be tracked. Consider: the real slot process is still alive when the file is written, then exits; Windows hands the PID to a new process; the new process happens to start before the file's LastWriteTime is read (e.g. the file was written, then the slot exited, then a stranger started, all before Register-SlotPidFile runs). The stranger's StartTime is after the file's write, so it is refused — good. But if the file is rewritten (slot restart) between the Get-Item and the Get-Content, the timestamp read is the new one and the PID read is the new one, which is consistent — also fine. The ordering of reads (timestamp first, then contents) is correct and the comment explains why. This is fine.

Stop-TrackedProcesses swallows all exceptions. The author's justification in the discussion ("the overwhelmingly common exception is the benign one") is reasonable for the HasExited/Kill race, but the try also wraps $proc.HasExited, which can throw InvalidOperationException if the process object was never associated with a real process. In practice Register-Spawned and Register-SlotPidFile both guarantee a live handle, so this is not reachable. Non-blocking.

Invoke-OverlappingPoolTests.ps1 — $run.Code type inconsistency. The author defends this in the discussion. It is fine: 'timed out' is compared with -ne 0, which is true, so it is counted as a failure. The report prints it as a word. No bug.

Invoke-OverlappingPoolTests.ps1 — overlap detection requires EndIsExact. If ExitTime throws (e.g. the process was killed by the timeout branch), EndIsExact stays $false and that run cannot contribute to overlap detection. The script correctly fails if no run has an exact end time, but if only one does and it happens to be the first run, and the second run's End is [DateTime]::Now (an upper bound), the pair (first, second) is checked with $a.EndIsExact true and $b.Start < $a.End — this is a valid overlap check. Fine.

Get-StandInCommand default parameter $ProcessId = $PID. $PID is evaluated at function-definition time in PowerShell? No — default parameter values are evaluated at call time. So Get-StandInCommand with no argument returns a command that waits on the calling process. In windows_pools_test.ps1, $dummyCommand = Get-StandInCommand is evaluated once at script scope, so $PID is the test script's own PID. Correct. In process_tracking_test.ps1, Start-StandIn calls Get-StandInCommand with no argument, so the stand-in waits on the test process. Correct.

Register-SlotPidFile — Get-Content ... | Select-Object -First 1. If the file contains multiple lines (shouldn't happen, but Set-Content with an array would), only the first is used. Fine.

Invoke-OverlappingPoolTests.ps1 — $run.Proc.Kill() on timeout. The killed process's children (the stand-ins) are not killed. But the stand-ins wait on the test process via WaitForExit, so they exit when the test process dies. Good — this is exactly the design intent.

Invoke-OverlappingPoolTests.ps1 — $deadline is computed once after all runs are started. The comment says this is deliberate. Correct.

3. CLI and API safety

No CLI changes. No new Python dependencies. No GitHub API calls added. The new CI job uses permissions: contents: read, which is correct for a test job. actions/checkout@v7 — verify this tag exists; the current major is v4 as of my knowledge, but the repo may pin differently. Non-blocking if it resolves.

The new workflow job runs ./.github/scripts/Invoke-OverlappingPoolTests.ps1 with shell: pwsh and exit $LASTEXITCODE. The script itself spawns both pwsh and powershell children. On windows-latest, both are present. Fine.

4. Test coverage

tests/process_tracking_test.ps1 directly exercises the rules: tracking a genuine .pid, refusing a reused PID, refusing empty/whitespace/non-numeric .pid files without throwing, refusing a .pid naming an exited process, Stop-TrackedProcesses killing only tracked processes and being idempotent, and the stand-in command outliving a slow run and dying with its parent. This is the right level of coverage for the fix.

The Overlapping runs CI job is a regression guard, and the author is honest in the discussion that it did not reproduce the PID-reuse symptom on a fresh VM. That is a real limitation, but the unit-level test compensates. The whitespace-collapsing fix in Test-OutputContains is a genuine bug fix that the control run surfaced.

One gap: process_tracking_test.ps1 is not wired into Invoke-PowerShellTests.ps1 or the workflow as far as the diff shows. The workflow diff only adds the overlapping-runs job, which runs windows_pools_test.ps1. If process_tracking_test.ps1 is not picked up by the existing Invoke-PowerShellTests.ps1 glob (the README says "a new test there is picked up without editing the workflow"), this is fine — but worth confirming. If the glob is *_test.ps1, it is picked up. The README text suggests it is. Non-blocking, but worth a one-line confirmation.

5. Suggested follow-up issues

• Confirm tests/process_tracking_test.ps1 is picked up by Invoke-PowerShellTests.ps1's glob and runs in the existing powershell-tests job.
• Consider having Stop-TrackedProcesses log (at verbose level) which processes it killed, to aid future debugging of cleanup paths.
• Consider a -WhatIf/dry-run mode for Invoke-OverlappingPoolTests.ps1 so developers can see the schedule without spawning six suites.

APPROVE — the fix correctly addresses both AC items, the latent bugs called out in the issue are handled, the new unit test directly exercises the rules, and the CI job is a reasonable regression guard. The remaining items are non-blocking.

---

Reviewed by DeepSeek via deepseek-pr-review.yml. Advisory only.

[github-actions]

GPT AI Code Review

Skipped: API key not configured

The OPENAI_API_KEY repository secret is not set.

To enable GPT reviews, add OPENAI_API_KEY as a repository secret:

1. Go to Settings > Secrets and variables > Actions in this repo.
2. Click New repository secret.
3. Name: OPENAI_API_KEY
4. Value: your GPT API key.

Once the secret is configured, re-run this workflow or push a new commit to trigger a fresh review.

REVIEW SKIPPED - API KEY NOT CONFIGURED

---

Reviewed by GPT via gpt-pr-review.yml. Advisory only.

[github-actions]

DeepSeek AI Code Review

1. Acceptance criteria

Both ACs from the issue are addressed:

1. Scale-down / slot-1 liveness under overlap — fixed by tracking Process objects with held handles (Register-Spawned, Register-SlotPidFile) and killing only tracked, non-exited processes in Stop-TrackedProcesses. The Cleanup path no longer force-kills bare PIDs.
2. Stop-Slot -Force kills children — the tree test now waits for a numeric PID in child.pid (Register-SlotPidFile rejects empty/whitespace/non-numeric), and the "was it killed" checks use WaitForExit on the held handle rather than Get-Process -Id.

The latent problems called out in the issue are also covered: the [int]$null → PID 0 case is explicitly guarded ($slotPid -notmatch '^\d+$'), the "is it dead yet" polls use WaitForExit, and the stand-ins now wait on the test process instead of Start-Sleep 120.

The new overlapping-runs CI job is the right shape for reproducing the original bug.

2. Bugs and logic errors

Register-SlotPidFile — StartTime comparison is the wrong direction for the stated invariant. The comment says "only a process that already existed when the file was written is ours," and the check is if ($proc.StartTime -gt $written) { return $null }. That's correct as written. But the test in process_tracking_test.ps1 for the "reused" case does:

(Get-Item $reusedPidFile).LastWriteTime = $stranger.StartTime.AddSeconds(-5)

This back-dates the file's mtime to before the stranger started, which is exactly the condition that should cause refusal — and it does. Fine. But note the real-world race the comment describes ("a .pid rewritten between the two reads") is not actually exercised: the test only simulates the post-hoc state, not the interleaving. That's acceptable given the file's own .NOTES acknowledges the race can't be forced, but the comment in ProcessTracking.ps1 claims the ordering of the two reads closes a window that the test never opens. Not blocking, but the comment overstates what's verified.

Register-SlotPidFile — $proc.StartTime can throw on a process that exited between Get-Process and the property access. The whole body is wrapped in try { ... } catch { return $null }, so this is handled. Good.

Stop-TrackedProcesses — $proc.Kill() on a process whose handle is held but which has already exited is guarded by HasExited, but HasExited itself can throw if the handle was invalidated. The try/catch swallows it. Acceptable for a cleanup path.

Invoke-OverlappingPoolTests.ps1 — $run.Proc.ExitCode is read after WaitForExit($left) returns $true, but if the process was killed by the timeout branch, ExitCode is never read (it's set to the string 'timed out'). That's fine. However, in the success branch, $run.Proc.ExitCode is read before $run.End is computed, and ExitCode on a System.Diagnostics.Process whose handle is still open is fine. No issue.

Invoke-OverlappingPoolTests.ps1 — the overlap detection loop is O(n²) and only checks $a.Index -lt $b.Index. With $Runs = 6 this is trivial. The logic $b.Start -lt $a.End with $a.EndIsExact is correct for "a started before b ended." But it only checks pairs where $a is the earlier-indexed run — if run 3 starts before run 1 ends (possible with staggering), that's caught by $a.Index=1, $b.Index=3. Fine.

Invoke-OverlappingPoolTests.ps1 — $run.End = [DateTime]::Now is set even when EndIsExact is false, and then the overlap check requires $a.EndIsExact. So a run whose ExitTime couldn't be read contributes nothing to overlap detection. If all runs fail to report exact exit times, the script exits 1 with the "cannot be established" message. That's the intended behavior per the .NOTES. Good.

windows_pools_test.ps1 — the parent script's $dummyCommand is interpolated into a here-string that is itself written to parent.ps1. The here-string uses @"..."@ (double-quoted), so $dummyCommand expands at write time. $dummyCommand is [System.Diagnostics.Process]::GetProcessById(<pid>).WaitForExit(). The <pid> is the test process's PID, which is correct — the child should wait on the test process, not the parent. But wait: the parent script is spawned by the test, and the child is spawned by the parent. The child waits on the test process (via Get-StandInCommand's default $PID at the time $dummyCommand was computed). That means the child outlives the parent if the parent exits first. Is that intended? The tree test kills the parent via Stop-Slot -Force and expects the child to die too (via taskkill /T). If the child is waiting on the test process, not the parent, then taskkill /T on the parent is what kills it — which is the point of the test. OK, correct.

But: the parent script itself runs $dummyCommand at the end (line $dummyCommand after Set-Content). That means the parent waits on the test process too. So the parent won't exit until the test exits. That's fine for the test's purposes.

windows_pools_test.ps1 — Register-SlotPidFile is called in a loop with Start-Sleep -Milliseconds 100 up to 100 times. If the file never appears with a valid PID, $childProc stays $null and the if ($childProc) block is skipped — meaning the tree test silently doesn't run and no failure is recorded. That's a pre-existing pattern (the old code had the same if (Test-Path $childPidFile) guard), but it means a regression in the parent script would show up as a missing check rather than a failing check. Non-blocking, but worth a follow-up.

3. CLI and API safety

No Python CLI changes. No new dependencies. No GitHub API calls. The new CI job uses actions/checkout@v7 with permissions: contents: read — correct scope. The exit $LASTEXITCODE after the script invocation is present, matching the existing job's pattern.

One thing: the new job has timeout-minutes: 25 while the script's own -TimeoutMinutes defaults to 12. With 6 runs staggered 20s apart, the last run starts at ~100s and gets up to 12 minutes, so worst case is ~13 minutes plus overhead. 25 is comfortable. Fine.

4. Test coverage

process_tracking_test.ps1 covers the rules directly: tracked-vs-untracked kill behavior, .pid file validation (empty/whitespace/non-numeric), the "started after file written" refusal, the "already exited" refusal, idempotent Stop-TrackedProcesses, and the stand-in command's lifetime semantics. That's the right level of coverage for the helpers.

The one gap: the actual PID-reuse scenario (a PID handed from a dead process to a live one) is not exercised — the test simulates it by back-dating the file's mtime. The file's .NOTES acknowledges this. Acceptable.

The overlapping-runs CI job is the integration-level check that the original bug is gone. Good.

5. Suggested follow-up issues

• windows_pools_test.ps1: record a failure when the tree test's child.pid never yields a valid PID, instead of silently skipping the check.
• ProcessTracking.ps1: add a test that exercises the .pid-rewritten-between-reads interleaving the comment describes, or soften the comment to match what's actually verified.
• Invoke-OverlappingPoolTests.ps1: consider asserting a minimum overlap duration (not just "any overlap") so a 1ms overlap doesn't count as a meaningful concurrency test.

APPROVE — the fix correctly addresses both ACs, the helper extraction is well-scoped, and the new CI job reproduces the original failure mode. The non-blocking items above belong in the backlog.

---

Reviewed by DeepSeek via deepseek-pr-review.yml. Advisory only.

[github-actions]

GPT AI Code Review

Skipped: API key not configured

The OPENAI_API_KEY repository secret is not set.

To enable GPT reviews, add OPENAI_API_KEY as a repository secret:

1. Go to Settings > Secrets and variables > Actions in this repo.
2. Click New repository secret.
3. Name: OPENAI_API_KEY
4. Value: your GPT API key.

Once the secret is configured, re-run this workflow or push a new commit to trigger a fresh review.

REVIEW SKIPPED - API KEY NOT CONFIGURED

---

Reviewed by GPT via gpt-pr-review.yml. Advisory only.

[github-actions]

DeepSeek AI Code Review

1. Acceptance criteria

Both ACs are addressed:

• AC1 (scale down removes highest slots, keeps rest running): The root cause — Cleanup killing bare PIDs that had been recycled into other runs' live slot processes — is fixed by Stop-TrackedProcesses in tests/ProcessTracking.ps1, which only kills tracked Process objects whose handles are held open. Register-SlotPidFile additionally refuses PIDs whose process started after the .pid file was written.
• AC2 (Stop-Slot -Force kills children): The tree test now waits for a numeric PID in child.pid (via Register-SlotPidFile, which rejects empty/non-numeric reads) and uses $childProc.WaitForExit(10000) on the held handle rather than polling Get-Process -Id. The parent's stand-in no longer expires at 120s, so the parent can't die before Stop-Slot runs.

The latent problems called out in the issue (PID 0 from empty read, Get-Process -Id satisfying a reused PID, fixed 120s sleep) are all addressed.

2. Bugs and logic errors

Get-StandInCommand default parameter uses $PID, which is the caller's PID, not the test process's. In ProcessTracking.ps1:

function Get-StandInCommand {
    param([int]$ProcessId = $PID)
    return "[System.Diagnostics.Process]::GetProcessById($ProcessId).WaitForExit()"
}

$PID is resolved at call time in the caller's scope, which is fine when called from the test script — but the returned string is then passed to Start-Process -Command, where the child shell evaluates it. That's correct: the child waits on the parent test process. However, in windows_pools_test.ps1 the parent script (parent.ps1) is written with $dummyCommand interpolated, and $dummyCommand was computed once at the top of the test file with $PID = the test process. So the grandchild (the child spawned by parent.ps1) waits on the test process, not on parent.ps1. That's actually the desired behavior for the tree test (the child should outlive the parent until Stop-Slot kills the tree), but it means the child is not tied to its immediate parent — worth a comment, since it's load-bearing for the test's semantics.

Register-SlotPidFile reads LastWriteTime before contents, but the comment claims this makes a rewrite "stricter." Reading the timestamp first means: if the file is rewritten between the two reads, $written is the old timestamp and $slotPid is the new PID. A process that started after the old write but before the new write would then be accepted. The comment says this "only ever refuses," which is backwards for that interleaving. The safer order is contents-then-timestamp (read PID, then read mtime; if mtime is newer than the PID's start, refuse). This is a narrow race, but the comment asserts a guarantee the code doesn't provide.

Stop-Slot in windows/PoolSlot.ps1 re-reads the .pid file after Get-SlotProcess already read it. Lines around the $slotPid = "$(Get-Content $pidFile ...)" after $proc = Get-SlotProcess -SlotDir $SlotDir. If the file is rewritten between the two reads (slot restarting), $slotPid may not match $proc.Id, and the log message will name the wrong PID. Minor, but the whole point of this PR is not trusting bare PIDs — the second read reintroduces one.

Stop-TrackedProcesses never clears the list. The comment says this is deliberate ("a run that cleans up twice must find the same list"). That's fine for idempotency, but it means a long-lived process that dot-sources ProcessTracking.ps1 and calls Stop-TrackedProcesses repeatedly will accumulate handles. In this test-only context it's fine; worth noting it's not a general-purpose helper.

Invoke-OverlappingPoolTests.ps1 overlap detection requires EndIsExact. If ExitTime throws (process killed by Kill() in the timeout path, or HasExited is false), EndIsExact stays $false and that run can't contribute to overlap detection. The script correctly fails if no run reported an exact end, but a single timed-out run among otherwise-exact runs silently drops out of the overlap check. Acceptable, but the failure message ("lower -StaggerSeconds") would be misleading if the real cause was a timeout.

3. CLI and API safety

windows-pools.ps1 status command now calls Test-SlotRunning, which calls Get-SlotProcess, which opens a handle on every slot's process. For a pool with many slots, this holds N handles for the duration of the status loop. Handles are released when the Process objects are GC'd, which in PowerShell is non-deterministic. Not a leak in practice for a CLI invocation, but worth being aware of if status is ever called in a loop.

Get-SlotProcess returns $null on any exception, including permission errors. The comment acknowledges this ("fails, and so refuses, for a process this account may not touch"). That's the right call for Stop-Slot (don't kill what you can't verify), but it means Test-SlotRunning will report a slot as not-running if the account lacks PROCESS_QUERY_INFORMATION on its own child. On a normal Windows runner this won't happen; on a locked-down host it could cause Start-Slot to launch a duplicate. Worth a follow-up note.

No new Python dependencies, no CLI signature changes, no GitHub API calls touched.

4. Test coverage

tests/process_tracking_test.ps1 covers the rules directly: tracked-vs-untracked kill behavior, .pid file with a stranger PID, empty/whitespace/non-numeric .pid contents, exited process, idempotent Stop-TrackedProcesses, and the stand-in command's lifetime semantics. Good coverage of the new helper.

The overlapping-runs CI job is the right shape for reproducing #123, and the script correctly fails when runs don't actually overlap rather than passing vacuously.

Missing: no test that Stop-Slot itself (in windows/PoolSlot.ps1) refuses to kill a stranger PID. The windows_pools_test.ps1 diff adds a slot-stranger check that exercises Test-SlotRunning and Stop-Slot against a stranger, which covers it — good.

Missing: no test for the Start-Slot "already running" path with a stale .pid naming a stranger. The diff changes that code path but doesn't add a check that a stranger's PID doesn't cause Start-Slot to skip launching. This is a real regression risk: if Get-SlotProcess returns a false positive, the slot stays down silently.

5. Suggested follow-up issues

• Register-SlotPidFile: read .pid contents before mtime to close the rewrite race
• Stop-Slot: use $proc.Id in the log message instead of re-reading the .pid file
• Add a windows_pools_test.ps1 check that Start-Slot launches when the .pid names a stranger
• Document that Get-SlotProcess returns $null on permission errors, and what that means for Test-SlotRunning
• Invoke-OverlappingPoolTests: distinguish "no overlap" from "a run timed out" in the failure message

APPROVE — the fix is correct and well-tested; the items above are non-blocking.

---

Reviewed by DeepSeek via deepseek-pr-review.yml. Advisory only.

[leonarduk]

On the latest review round.

Taken, in cc120ba: Stop-Slot re-read the .pid file after Get-SlotProcess had already read it, just for the number in its log lines — correct, and the wrong instinct twice over in a PR about not trusting bare PIDs, since a rewrite between the two reads means the logged number need not be the process being waited on and killed. Every line about a live process now names $proc.Id; the raw contents are read in one place only, the branch where there is no process to name.

Not taken: the claim that reading LastWriteTime before the contents is the unsafe order. It is the safe one, and the argument given for the other order does not hold. Take a file written at t1 naming PID A and rewritten at t2 > t1 naming PID B.

• Timestamp first (what the code does): a rewrite lands between the reads, so $written is t1 and the PID read is B. Acceptance requires StartTime <= t1. B was recorded at t2, so anything holding B's number — B itself, or a stranger that took it after B exited — started after t1 and is refused. The review's counter-example is a process that "started after the old write but before the new write", i.e. t1 < StartTime < t2; that fails StartTime <= t1, so it is refused, not accepted.
• Contents first: the PID read is A (recorded at t1) and $written is t2. Acceptance requires StartTime <= t2, which a stranger that took A's number any time in (t1, t2) satisfies — so it is tracked, and tracked means killed.

A rewrite under this function can only bind a PID to a write older than its own, and an older timestamp only ever refuses. Worth noting this is the reverse of what the review two rounds ago asked for, which is what moved the code to this order in the first place (12f9cc1).

Also not taken, both previously answered: Stop-TrackedProcesses not emptying its list is deliberate and documented (a second cleanup pass must find the same list, not a shorter one), and the overlap check requiring an exact exit time is the point — "now" is an upper bound, so a pair judged on it would claim an overlap the job never saw, and the failure message distinguishes "did not overlap" from "could not be established".

---

Generated by Claude Code

[github-actions]

GPT AI Code Review

Skipped: API key not configured

The OPENAI_API_KEY repository secret is not set.

To enable GPT reviews, add OPENAI_API_KEY as a repository secret:

1. Go to Settings > Secrets and variables > Actions in this repo.
2. Click New repository secret.
3. Name: OPENAI_API_KEY
4. Value: your GPT API key.

Once the secret is configured, re-run this workflow or push a new commit to trigger a fresh review.

REVIEW SKIPPED - API KEY NOT CONFIGURED

---

Reviewed by GPT via gpt-pr-review.yml. Advisory only.

[github-actions]

DeepSeek AI Code Review

1. Acceptance criteria

Both AC items are addressed:

• Check 1 (scale down keeps lower slots running): Cleanup now calls Stop-TrackedProcesses, which only kills processes this run actually spawned and holds handles to. The Register-SlotPidFile guard (start-time vs. file mtime) prevents tracking a stranger that inherited a recycled PID. This is the correct fix for the cross-run interference described in the issue.
• Check 2 (Stop-Slot -Force kills children): The tree test now waits for a numeric PID in child.pid via Register-SlotPidFile (which rejects empty/whitespace/non-numeric reads), and both liveness checks use WaitForExit on the held handle rather than Get-Process -Id. The parent and child both wait on the test process, so the child outlives the parent — which is what makes the tree-kill assertion meaningful.

The latent problems called out in the issue (empty .pid → PID 0, Get-Process -Id satisfying a reused PID, fixed 120s sleeps) are all addressed.

2. Bugs and logic errors

Register-SlotPidFile — the $written vs. StartTime comparison is correct, but the comment's reasoning is inverted from what the code does. The code reads LastWriteTime first, then contents. The comment claims this is the safe order and the reviewer's earlier concern was wrong. I agree with the author's analysis in the discussion thread: reading timestamp first means a rewrite between the two reads yields $written = t1 (old) and $slotPid = B (new), and B.StartTime > t1 → refused. Reading contents first would yield $written = t2 (new) and $slotPid = A (old), and a stranger holding A's number with StartTime in (t1, t2) would be accepted. The current order is correct. Not a bug.

Stop-Slot — $proc.WaitForExit($TimeoutSeconds * 1000) on a process the caller may not own. Get-SlotProcess opens $proc.Handle, which can throw for a process this account cannot touch (the comment acknowledges this and returns $null). That's fine. But once $proc is returned, WaitForExit on a handle to a process in another session/account can block or throw. The try/catch in Get-SlotProcess covers the handle open, but WaitForExit in Stop-Slot is not wrapped. In practice the handle-open guard means we only get here for processes we can touch, so this is low-risk — but worth a defensive try around the wait if the process exits between the handle open and the wait. Non-blocking.

Get-SlotProcess — $proc.StartTime can throw for a process that exited between Get-Process and the property access. The try/catch around the whole function catches this and returns $null, which is the right behavior (a process that exited is not running). Fine.

Stop-TrackedProcesses — $proc.Kill() on a process whose handle was opened but which has since exited. The if (-not $proc.HasExited) guard handles this, and the try/catch swallows the race. Fine.

Invoke-OverlappingPoolTests.ps1 — $run.Proc.ExitTime can throw if the process was killed by $run.Proc.Kill(). The try/catch around the ExitTime read handles this, and EndIsExact stays $false, which correctly makes the overlap check fail rather than falsely pass. Good.

Invoke-OverlappingPoolTests.ps1 — the overlap check requires $a.EndIsExact. If a run is killed (timeout), its EndIsExact is $false, so it cannot be the "earlier" run in an overlap pair. But it can still be the "later" run ($b.Start < $a.End where $a is a different, exact-ended run). That's correct — a killed run still overlapped with whatever was running when it started. Fine.

windows-pools.ps1 — Test-SlotRunning is now called in the status command. This is a behavior change: previously status counted a slot as running if Get-Process -Id (Get-Content .pid) returned anything. Now it uses the stricter Get-SlotProcess check. This is the intended fix, but it means status will report a slot as not-running in cases where the old code said running (e.g., a .pid file whose process started after the file was written — which is exactly the recycled-PID case). That's correct behavior, but it's a user-visible change to status output. Not a bug, but worth noting in the PR description or changelog.

3. CLI and API safety

Stop-Slot behavior change is user-visible and correct, but the log message change is worth flagging. The new message "is not this slot any more -- another process has that number now, and it is not ours to kill" is clearer than the old "is not running", but scripts that grep Stop-Slot output for "is not running" will break. This is a test-only PR per the title, but windows/PoolSlot.ps1 is production code. If any downstream tooling parses this output, it's a breaking change. Low risk given the message is informational, but worth a note.

No new dependencies. The PR is PowerShell-only; no pyproject.toml changes needed. Correct.

No GitHub API calls. The changes are local process management. Correct.

Invoke-OverlappingPoolTests.ps1 — Start-Process with -RedirectStandardOutput and -RedirectStandardError to separate files. The comment notes 5.1 refuses to redirect both to the same file. Correct handling. The -WindowStyle Hidden is fine for CI.

Invoke-OverlappingPoolTests.ps1 — $run.Proc.Kill() on timeout. This kills the test process but not its children (the stand-in processes). However, the stand-ins now wait on the test process via WaitForExit, so they exit when the test process dies. This is the correct design — the stand-ins are self-cleaning. Good.

4. Test coverage

process_tracking_test.ps1 covers the core rules well:

• Tracked process that started before its .pid file → tracked.
• Process that started after its .pid file → refused.
• Empty/whitespace/non-numeric .pid → refused without throwing.
• Exited process → refused.
• Stop-TrackedProcesses kills tracked, leaves untracked alone, safe to call twice.
• Stand-in command outlives a slow run and dies with the run.

Missing case: Register-SlotPidFile on a .pid file that is deleted between the Test-Path and the Get-Item. The try/catch handles it (returns $null), but there's no explicit test. Low risk — the catch is the test.

Missing case: Stop-Slot with a .pid file whose process is alive but started after the file was written (the "stranger" case). This is tested in windows_pools_test.ps1 (the slot-stranger block), which is good. But process_tracking_test.ps1 doesn't test Stop-Slot directly — it tests Register-SlotPidFile. The integration is covered in the other file. Fine.

Missing case: Get-SlotProcess returning a process whose handle open succeeds but StartTime throws. The try/catch handles it, but no test. Low risk.

The overlap test itself is well-designed: it fails if runs don't actually overlap, and it distinguishes "did not overlap" from "could not establish overlap". This is the right way to make a concurrency test meaningful.

5. Suggested follow-up issues

• Stop-Slot log message change from "is not running" to "is not this slot any more" may break downstream log parsers — document in changelog.
• Add a test for Register-SlotPidFile when the .pid file is deleted between Test-Path and Get-Item.
• Consider wrapping $proc.WaitForExit in Stop-Slot in a try/catch for the process-exits-mid-wait race.
• windows-pools.ps1 status now uses stricter liveness check — document the behavior change for users who relied on the old (looser) semantics.

APPROVE — The fix correctly addresses both AC items and the latent bugs. The LastWriteTime-before-contents ordering is correct as argued in the discussion thread. No blocking concerns.

---

Reviewed by DeepSeek via deepseek-pr-review.yml. Advisory only.