[janicduplessis]

Description

On the Windows native loop, stim stop sometimes fails while it shuts down the owned emulator: the teardown's adb devices times out after 15 s. This happened in three of the four Windows loop attempts on 10-20, and #3963 records the same signature on main fff0bfb.

The adb server is blocked, not slow. stop kills Metro but leaves the adb reverse that android set up, and the debug app keeps reconnecting through it. The host server connects each device-opened stream with a blocking loopback connect() on its single event thread (service_to_fd). On Windows a refused connect takes about 2 s, so the retries starve every other adb client. Linux refuses at once.

Controlled check with the real fixture app on the Windows runner (run 39489996200, run 39490067435):

• Metro killed, reverse left in place: adb devices took 17978 ms, then 13045 ms.
• adb reverse --remove tcp:8082 first, then the same kill: adb devices stayed at 31-135 ms for 30 s.

Other measurements

• A refused TcpClient.Connect to 127.0.0.1:8082 took 2005-2466 ms, five times in a row (run 39485502554).
• Six parallel device-side connects through a reverse to a closed host port made one adb devices take 11627 ms, against 34 ms before (run 39489996200).

Solution

Right before stop signals Metro (the verified supervisor, or a verified dev server the supervisor left behind), and when it finds nothing listening on the port, it removes the reverses that point each owned, running emulator at the workspace's Metro port. When stop refuses to signal an unverified process, Metro keeps running, so the reverses stay. After that the app's reconnects are refused on the device and never reach the host server. Ownership is checked through the emulator's own console (emu avd name must name the recorded AVD), which does not go through the adb server. Only reverses whose host side is this workspace's Metro port are removed, including the tcp:8081 fallback launchAndroidApp can add. Every call is quiet and bounded: 10 s for the console probe, 5 s for each reverse call. The removal is best effort: if it fails, the existing teardown and its failure reporting run unchanged, and the device record is kept.

The Windows lane also uploads the adb server's log (D:\e\adb.log; the server writes it to %TEMP%).

Not covered: this runs only when stop itself stops Metro for an owned emulator. When Metro dies some other way while the reverse stays (a crash, ports stop, a physical --device phone, a non-owned emulator), the adb server on Windows can stall the same way until the app or device stops.

Test plan

• sim-android.test.ts: only reverses to the Metro port are listed and removed (the fallback tcp:8081 -> 8082 entry included, an unrelated tcp:9000 and a localabstract: entry left alone), and no reverse command runs when the console names another AVD.
• stop.test.ts: for owned emulators in the default and a named slot, the removal runs before the supervisor or a verified leftover dev server is signalled, and when nothing listens on the port. Nothing is removed when the supervisor or the dev server it left cannot be verified, and a non-owned emulator never gets a reverse command.
• Real tool, Windows (invariant 9): the stop debug log records emu avd name, reverse --list and reverse --remove exiting ok in 29-57 ms before the supervisor phase, in both worktrees.
• Windows loop on this fix (Windows-only plan plus the adb log upload): green twice, run 39486537927 and run 39490059086. The bug is intermittent, so the Metro-kill comparison above is the evidence for the mechanism.

Fixes #4101

[janicduplessis]

Fresh review: one actionable finding, the rest checks out.

Actionable

1. packages/stim-cli/src/commands/stop.ts:522 removes the reverses before resolveSupervisorTarget (line 525) runs. When the target is unverified, stop refuses to signal the supervisor, keeps the port, and leaves the devices alone (line 651, "left alone (something is still running)"). guide/errors.ts (~1409-1418) documents that refusal as leaving things in place. With this change the Metro reverse is already gone at that point and Metro is still running, so the app loses its connection to Metro during a stop that reports it stopped nothing. stim reload android would put the reverse back, but nothing tells the user to run it. Fix: resolve target first, then call removeOwnedEmulatorReverses unless target.status === 'unverified', then go on to stopSupervisor. The new ordering test still holds (the reverse calls still come before 'supervisor'). This gate can't cover the timeout and failed cases, because the removal has to happen before SIGTERM, and those cases don't need it.

Checked, no change needed

• Ownership: the removal runs only for owned records with a recorded consolePort, and only after emu avd name on that console names the recorded AVD. It removes only tcp: device ports whose host side is this workspace's Metro port. Non-owned emulators and physical devices are untouched. This is not a device teardown action (invariant 4), and it adds no new way to decide ownership.
• Failure handling: runOperationFileQuiet turns ordinary failures into null, and reverseDevicePortsTo handles null and \r\n. Cancellation and WorkspaceProcessCleanupError propagate, which matches every other contextual probe. Calls are bounded: the console probe at 10 s (EMU_AVD_NAME_TIMEOUT_MS), each reverse call at 5 s. The PR body's "every call ... bounded at 5 s" is slightly off for the console probe.
• stop --slot <name> passes metroPort: null, so nothing is removed. That is correct because Metro keeps running in that case.
• Existing callers: I ran the full unit suite with ANDROID_HOME pointing at a fake SDK whose adb logs its arguments. No reverse or emu avd name call reached the real tool. The only logged calls were two track-devices calls from unrelated tests. Three tests failed: hosted-ios-client and hosted-log-discovery pass when rerun alone, and scripts/agent-benchmark/macos-processes.test.mjs also fails alone on this machine (load average ~170). None of them touches this diff.
• Tests: both can disagree with the implementation. The sim-android case fails if the console gate or the port filter is dropped. The stop case fails if the owned check is dropped (the console stub would name Pixel_8), if the removal moves after the supervisor kill, or if the slot records are skipped. Optional: one case where the listing returns null and the supervisor is still signalled would test the best-effort claim directly.
• Comment in devices/android.ts: it names the external source (adb services.cpp service_to_fd) and the concrete constraint, and it links the issue. It meets the comment policy.
• Workflow: D:\e\adb.log matches the job's TEMP: D:\e, where adb writes its server log. It is a separate chore: commit.
• Guides and website: no topic describes what stop does to reverses, and no command, default, or remedy changed. No guide edit is needed.
• pnpm run format:check, pnpm run lint and pnpm run typecheck pass. Targeted stop.test.ts and sim-android.test.ts pass.

Non-blocking

• The "Not covered" list in the PR body names phones and Metro crashes. It could also name a non-owned emulator that android launched against: that emulator gets the same reverse and the same Windows stall. This is a note for the body, not a scope change.

[janicduplessis]

Fresh review: one actionable finding at ae02fc19a. The new gate covers the unverified supervisor refusal, but stop has a second refusal path that still loses the reverses.

Actionable

1. packages/stim-cli/src/commands/stop.ts:976 returns early only for unverified. When the supervisor is stale (dead) but the state records a serverPid and serverProcessToken, stopMetro checks the dev server it left behind. When inspectIdentity returns 'unknown', stop.ts:927-931 returns refused ("refusing to signal it"). Then stop.ts:610-612 sets stillHolding, and the devices are "left alone (something is still running)". So Metro may still be running, stop reports a refusal, and the emulator's reverse to Metro is already gone. This is the same failure the previous review found, reached a different way. Two possible fixes:

   • (a) The refusal can be computed before the supervisor phase: target.status === 'stale' plus a synchronous inspectIdentity({ pid: serverPid, processToken: serverProcessToken }) returning 'unknown'. Skip the removal in that case too.
   • (b) Move the removal to the two places where stop signals Metro: in stopSupervisor just before killGroup, and in stopMetro after identity === 'same' and before signalServer. Then the removal runs only after stop has decided to signal. The diff is larger, but no refusal path can be missed.

   Add a matching test case: isAlive false (so resolveSupervisorTarget returns stale at supervisor/ownership.ts:50), state with serverPid/serverProcessToken, inspectIdentity returning 'unknown' for the server, an owned emulator record, and an assertion that no reverse call runs. The existing 'a recorded dev server with %s identity that stays up keeps its record and the port' table (stop.test.ts:449) already sets up this state.

   This does not cover the supervisor timeout/failed and dev-server failed cases, and it doesn't need to. In those cases the removal has to come before SIGTERM, and stop was already trying to stop Metro.

Checked, no change needed

• Ownership: only owned records with avdName and consolePort, only after emu avd name on that console names the recorded AVD (the same rule teardown's resolveOwnedAvdSerial uses), and only tcp: device ports whose host side is this workspace's Metro port. Physical and non-owned devices are not touched. Nothing is deleted or killed, and there is no new ownership rule, so review: needs-human does not apply.
• Failure behaviour: runOperationFileQuiet returns null for ordinary failures and timeouts, and reverseDevicePortsTo handles null. Abort and WorkspaceProcessCleanupError propagate. The CLI stop passes no operation, so only the API path (api.ts:515) can abort here, and that stops before anything is signalled. Each call has a bound: 10 s for the console probe, 5 s for each reverse call.
• stop --slot: runStop passes metroPort: null, so the gate returns. Metro is shared across slots, so that is correct.
• Tests: the [9999, []] row fails if the unverified gate is dropped or moved after the target check. The 8083 row fails if the owned check, the slot iteration, or the before-supervisor ordering is lost. The sim-android pair fails if the console gate or the host-port filter is dropped. Each expected result can disagree with the implementation.
• Comment in devices/android.ts: it names the external constraint (adb services.cpp service_to_fd, blocking loopback connect on Windows) and links the issue. That meets the comment policy.
• Workflow: D:\e\adb.log matches the job's TEMP, and it is in its own chore: commit.
• pnpm run lint, pnpm run typecheck, pnpm run format:check pass. stop.test.ts (90 tests) and the sim-android cases pass.

The PR stays draft without review: ai-approved until the finding is addressed.

[janicduplessis]

Fresh review: two actionable findings at 8684c7530. The refusal paths from the earlier reviews are fixed. The new problems are a cancellation window in stopSupervisor and one untested call site.

Refusal paths: all keep the reverses now

• unverified supervisor: no releaseReverses call on that branch (stop.ts:541-545), and the metro phase is skipped.
• Dev server left by a stale supervisor whose identity is unknown: stopMetro returns refused at stop.ts:933-937, before either release call.
• Port held by a server Stim does not manage (not-managed): returns at stop.ts:962-969 with no release.
• Stale supervisor whose server is gone/different: falls through to resolveMetro and releases only if nothing is listening. That is correct.
• Supervisor timeout/failed and dev-server failed: the removal has to happen before SIGTERM, and in these cases stop was already stopping Metro. Same conclusion as the earlier reviews.

Actionable

1. packages/stim-cli/src/commands/stop.ts:860-862: releaseReverses() runs after requestDevServerStop writes the "stim stop" request and before killGroup. Before this PR nothing could throw in that window. killGroup sits in a catch-all, and when it fails, withdrawDevServerStopRequest runs. releaseReverses rethrows aborts and WorkspaceProcessCleanupError (through runOperationFileQuiet), and it can take 10 s + 5 s per reverse per emulator. The API stop passes its operation (api.ts:515). If that stop is cancelled during the removal, stopSupervisor exits without signalling and without withdrawing the request. The only other code that clears the key is the supervisor's own finish (supervisor/run.ts:241,258), which uses it to attribute the exit. A later idle stop, crash or external signal is then recorded as "stop requested by stim stop (pid ...)", and metroLastStop reports the wrong cause. Fix: move await releaseReverses() above the requestDevServerStop call (release, then request, then kill). The removal still runs before SIGTERM. The new ordering test and 'stim stop names itself before it signals, and withdraws that when the signal fails' still hold.

2. packages/stim-cli/src/commands/stop.ts:940: deleting this call leaves the whole suite green. This branch is the dev server that a dead supervisor left behind, one of the three cases the fix covers, so it needs a regression check under the repo's test bar. One more row in the new stop.test.ts table covers it: state: { pid: 4242, port: 8083, serverPid: 4243, serverProcessToken: 'server-token' }, with the seams defaults (isAlive false makes the supervisor stale, inspectIdentity returns 'same'), a signalServer override that pushes 'server' and returns true, and an expected [...REVERSES_REMOVED, 'server']. It fails if the call is dropped or moved after the signal.

Checked, no change needed

• Ownership: the removal runs only for slot records with owned, avdName and consolePort. It runs only after adb -s emulator-<port> emu avd name names the recorded AVD; the adb client sends that command to the console port, not through the server. It removes only tcp: device ports whose host side is this workspace's Metro port, and the non-owned Pixel_8 slot never gets a call. Nothing is deleted or killed, there is no new ownership rule, and devices/teardown.ts is not bypassed (invariants 2 and 4). review: needs-human does not apply.
• Failure behaviour: ordinary failures and timeouts become null and are skipped. \r\n listings are trimmed. The CLI path passes no operation, so it cannot abort there. Apart from finding 1, aborts propagate the same way they do from the other phases.
• stop --slot: runStop sets metroPort: null, so removeOwnedEmulatorReverses returns at once. That is correct because the workspace Metro keeps running.
• Single exec wrapper: the calls go through runOperationFileQuiet and getExecutor().
• Tests: the unverified row fails if the gate moves before the target check. The unknown dev-server row is reached through a stale target, stopMetro and refused, so it fails if the release moves above the identity check. The verified row fails if the owned check, the slot iteration or the before-supervisor ordering is lost. The sim-android pair fails without the console gate or the host-port filter.
• Comment policy: the JSDoc on removeOwnedEmulatorMetroReverses names the external constraint (adb services.cpp service_to_fd, slow refused loopback connect on Windows) and links the issue. No other comments were added.
• Workflow: the Windows job sets TEMP: D:\e (e2e-native.yml:530), and the adb server writes adb.log to %TEMP%. The change is in its own chore: commit and touches no secrets.
• pnpm run lint, pnpm run typecheck and pnpm run format:check pass. stop.test.ts (92 tests) and the two removeOwnedEmulatorMetroReverses cases pass.

The PR stays draft without review: ai-approved until both findings are addressed.

[janicduplessis]

Fresh review: clear at da5ee3166. All four earlier findings are resolved, and the rest of the diff checks out.

Earlier findings

1. Unverified supervisor: releaseReverses is only built at stop.ts:530 and only called from stopSupervisor and stopMetro. The unverified branch (stop.ts:541-545) calls neither, and supervisorHandled skips the metro phase. Resolved; the an unverified supervisor row expects [].
2. Unverifiable leftover dev server: stopMetro returns refused at stop.ts:933-937 before either release call. The not-managed return (stop.ts:962-969) does not release either. Resolved; the an unverifiable dev server left by the supervisor row expects [].
3. Cancellation window: await releaseReverses() is now the first statement of stopSupervisor (stop.ts:858), before requestDevServerStop. An abort or WorkspaceProcessCleanupError from the removal now exits before any stop request is written, so no stale "stim stop" attribution can be left behind. In stopMetro the removal also runs before signalServer, and nothing is recorded before it. Resolved.
4. Verified leftover dev server: the a verified dev server left by the supervisor row expects [...REVERSES_REMOVED, 'server']. It reaches stopMetro through a stale supervisor (isAlive false) and inspectIdentity 'same' for pid 4243. It fails if the call at stop.ts:940 is dropped or moved after signalServer. Resolved.

Checked, no change needed

• Ownership (invariants 2 and 4): the removal touches only slot records with owned, avdName and consolePort, and only after emu avd name on that console names the recorded AVD. It removes only tcp: device ports whose host side is this workspace's Metro port. It never deletes, kills or boots anything, adds no new ownership rule, and does not go around devices/teardown.ts. The non-owned Pixel_8 slot in the test never gets a command. review: needs-human does not apply.
• Failure behaviour: ordinary failures and timeouts become null through runOperationFileQuiet and are skipped. Only aborts and cleanup errors propagate, and with finding 3 fixed they propagate before anything is signalled or recorded. Each call has a bound: 10 s for the console probe, 5 s for each reverse call, killed with SIGKILL.
• Remaining accepted gap: supervisor failed/timeout and dev-server failed remove the reverses even though Metro may still be up. The removal has to happen before SIGTERM, and in those cases stop had already decided to stop Metro. The earlier reviews reached the same conclusion.
• stop --slot: runStop passes metroPort: null (stop.ts:327), so removeOwnedEmulatorReverses returns at once and the shared Metro keeps its reverses.
• Single exec wrapper: the calls go through runOperationFileQuiet and getExecutor(). Real-tool evidence for the three adb calls is in the PR body (invariant 9). The argument lists did not change in the later commits.
• Tests: each row's expected result can disagree with the implementation. Together they fail on a dropped owned check, a skipped slot iteration, a removal after the signal, a removal on either refusal path, or a dropped console gate or host-port filter.
• Comment policy: the only new comment is the JSDoc on the exported removeOwnedEmulatorMetroReverses. It names the external constraint (adb services.cpp service_to_fd, slow refused loopback connect on Windows) and links the issue.
• Workflow: D:\e\adb.log matches the job's TEMP. It is in its own chore: commit, adds only an artifact path, and touches no secrets.
• PR body: matches the diff, including the refusal cases and the non-owned emulator in "Not covered".
• pnpm run lint, pnpm run typecheck, pnpm run format:check pass. stop.test.ts (93 tests) and the two removeOwnedEmulatorMetroReverses cases pass.

Non-blocking

• No test pins finding 3's ordering (removal before requestDevServerStop). A test would have to abort the operation during the removal and assert that no stop request is left. That is optional; the ordering is easy to see at stop.ts:858-861.
• In stopMetro's identity === 'same' branch, the "sending SIGTERM to dev server" line (stop.ts:938) is reported before await releaseReverses() (stop.ts:940). stopSupervisor releases before its matching line. If the removal aborts there, stderr names a signal that was never sent. Swapping the two lines would make the order match. Nothing else changes, including the tests.