Two AcpAdapterV2 teardown tests fail on Windows (taskkill one-shot + session poison timeout) #17011

Activity

1. juliusmarminge commented on Oct 8, 2026

   

   juliusmarminge

   on Oct 8, 2026

   Member

   More actions

   Note

   Grok responding on behalf of Julius.

   Thanks for the clean repro on two commits. Checked against main at ff3a04a8ebaaf6bb74b8b5b996013b82e044c700. Not reproduced on Windows here; everything below is from reading the code.

   Summary: both look like test-only problems. The tests fake processGroupPlatform: "win32" on a Linux host, but parts of the teardown path and the fixtures depend on the real host platform. CI only runs Ubuntu (e.g. ci.yml#L233), and the Windows lane is manual-only and notes that the suite doesn't pass on Windows yet, so neither test has run on a real Windows host in CI. Both tests and the teardown code they exercise arrived with #2829.

   1. Windows teardown is one-shot explicitly with independent finalizer cleanup — mechanism confirmed in code

   • The test routes taskkill to a recording stub and everything else to the real spawner (L12301-L12307), sets processGroupPlatform: "win32" (L12319), calls terminateProcessGroup twice concurrently, then closes the scope and asserts the count is still 1 (L12326-L12331).
   • The explicit path is one-shot via Effect.cached (L1734-L1736) and uses options.processGroupPlatform to choose taskkill (L1706-L1708).
   • The scope finalizer is a separate effect that does not go through that cache, and it branches on HostProcessPlatform (the real process.platform by default, hostProcess.ts#L7-L12), not on options.processGroupPlatform (L1737-L1752):
     • On a Windows host it runs terminateWindowsProcessTreeWithTaskkill(spawner, …), using the same stubbed spawner, so the stub records a second taskkill → expected 2 to equal 1.
     • On a Linux host (CI) it falls through to signalOwnedProcessGroup("SIGKILL"), which never touches the taskkill stub, so the count stays 1.

   So the finalizer runs independently of the one-shot guard by design (that's what the test name says), and the second assertion only holds on a non-Windows host. In production on Windows this means one extra taskkill /T /F on the same PID at scope close after an explicit teardown, with any failure swallowed and logged (L1749-L1750). That matches the "independent finalizer" intent and isn't a functional bug from what the code shows.

   2. poisons the session when hard teardown defects and blocks replacement work — timeout cause is a hypothesis

   Confirmed from code:

   • The test enables T3_ACP_EMIT_RUNNING_COMMAND_THEN_HANG with a PID file (L12483, L12486). The mock agent then runs bash -c 'sleep 120 & … printf "%s %s\n" "$$" "$child" > "$1"' via NodeChildProcess.spawn("bash", …), with no error listener (acp-mock-agent.ts#L1352-L1355, L1375-L1377).
   • The test's first wait spins on that PID file with no timeout (L12534-L12536). Later waits are unbounded too: Deferred.await(teardownStarted) (L12554), Fiber.join(interruptFiber) (L12568), and the session/request_permission Stream.runHead (L12580-L12588). By contrast, waitForProcesses/waitForProcessesToExit are capped at 2s (L277-L289), so a PID mismatch alone would show up as an assertion failure, not a 60s timeout.
   • On a Windows host the final Scope.close goes through the real taskkill (finalizer above), not the test's injected windowsProcessTreeTerminator (L12475-L12481). The test then expects the bash/sleep PIDs it read to be gone, checked with process.kill(pid, 0) (L12644-L12650).

   Hypothesis (not verified on Windows): the fixture assumes a POSIX bash/sleep whose $$/$! are host PIDs. If bash on your PATH is missing or is something else (for example WSL's bash.exe, where a Windows temp path may not resolve), the PID file stays empty and the test spins at L12534 until the 60s timeout. Even with Git Bash, whether its $$/$! match Windows PIDs is unverified, so later assertions might fail too.

   Precedent: this file already guards several real-process fixture tests to Linux hosts with if ((yield* HostProcessPlatform) !== "linux") return; (L1722, L1843, L1928).

   Possible fixes (for whoever picks this up)

   • Test 1: make the last assertion host-aware: expect 2 when HostProcessPlatform is win32, since the finalizer is meant to be independent. Alternatively, build the runtime with a pinned HostProcessPlatform and assert on the finalizer path separately.
   • Test 2: add the same Linux-host guard as the tests above, since it depends on POSIX bash fixtures. Separately, bounding the L12534 wait (e.g. Effect.timeoutOption plus a clear assertion message) would turn a missing fixture into a fast, readable failure on any platform.
   • Longer term, these belong with the effort to get the suite green on the manual Windows lane.

   No open PR touches these lines today.

   @arslnysuf since you offered to re-run: could you share the output of where.exe bash and where.exe sleep in the shell you run vp test from? That should confirm or rule out the PID-file hypothesis for test 2.
2. juliusmarminge

   added

   bugSomething is broken or behaving incorrectly.Something is broken or behaving incorrectly.

   via-triageFiled through npx t3 triageFiled through npx t3 triage

   on Oct 8, 2026