Skip to content

fix(macos): allow XCTest teardown before runner kill#1206

Merged
thymikee merged 2 commits into
callstack:mainfrom
lott-ai:fix/macos-screensaver-runner-teardown
Jul 11, 2026
Merged

fix(macos): allow XCTest teardown before runner kill#1206
thymikee merged 2 commits into
callstack:mainfrom
lott-ai:fix/macos-screensaver-runner-teardown

Conversation

@lott-ai

@lott-ai lott-ai commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

Fixes #1205

Summary

  • stage macOS runner shutdown as SIGINT, wait, SIGTERM, wait, then SIGKILL only if still necessary
  • preserve the existing immediate cancellation behavior for iOS, tvOS, and runner preparation processes
  • cover normal macOS teardown, TERM escalation, KILL escalation, and non-macOS behavior with focused unit tests

Problem

XCTest disables the host screen saver while macOS UI automation is running. The runner abort path previously sent SIGINT, SIGTERM, and SIGKILL back-to-back, which could terminate xcodebuild before XCTest restored the screen-saver permission.

When a client disconnected during an in-flight request, macOS could therefore remain at setScreenSaverCanRun: 0. Dock still recognized the configured hot-corner action, but loginwindow refused to start the screen saver.

Implementation

macOS runner disposal now:

  1. sends SIGINT and waits up to five seconds for XCTest teardown;
  2. sends SIGTERM only to runners that remain alive and waits another two seconds; and
  3. sends SIGKILL only to runners still alive after both grace periods.

The wait helper now reports whether the runner exited and clears its timeout when the test process settles. Graceful-shutdown fallback also reuses the staged macOS path without waiting for the same process twice.

The conservative delay is isolated to macOS because it protects host-wide screen-saver state. Other Apple targets retain their previous immediate abort behavior.

Validation

Automated checks:

pnpm exec vitest run --project unit-core src/platforms/apple/core/__tests__/runner-disposal.test.ts
pnpm check:affected --base origin/main --run

All four focused regression tests and all nine affected checks passed.

Manual fault-path validation on macOS:

  1. Installed the package built from this branch as the active global agent-device CLI.

  2. Started a TextEdit snapshot so the macOS XCTest runner was active.

  3. Terminated the client while the request was in flight.

  4. Confirmed the runner process was cleaned up and no automation marker remained.

  5. Confirmed the same xcodebuild process restored the loginwindow state:

    setScreenSaverCanRun to 0
    setScreenSaverCanRun to 1
    

The interrupted runner cleaned up in approximately 1.6 seconds, without requiring TERM or KILL escalation.

@thymikee

Copy link
Copy Markdown
Member

Review found one P2 scope gap: HTTP/proxy macOS disconnects still do not invoke runner abort. shouldAbortIosRunnerSessionsOnDisconnect gates abort to platform === "ios" or replay/test, so a macOS JSON-RPC/proxy snapshot disconnect marks cancellation but never enters the new staged SIGINT -> TERM -> KILL disposal path. The staged helper/tests themselves are sound and the manual screen-saver 0 -> 1 evidence supports the socket/manual path, but #1205 explicitly covers client/daemon interruption and remote Mac clients remain exposed.

Extend disconnect abort routing to the macOS runner without changing iOS/tvOS cancellation semantics, add an HTTP disconnect regression, and provide SHA-pinned evidence for that boundary. Not auto-fixing because this is an external contributor branch.

@lott-ai

lott-ai commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

@thymikee fixed in ee736a6.

macOS HTTP/proxy disconnects now route through runner abort and reach the staged SIGINT -> SIGTERM -> SIGKILL disposal path. The new HTTP JSON-RPC snapshot disconnect regression passes, and the disposal tests pin existing immediate cancellation behavior for both iOS and tvOS.

pnpm check:affected --base origin/main --run checks passed locally including coverage and provider integration.

@thymikee

Copy link
Copy Markdown
Member

Reviewed — solid, well-scoped fix. 👍

The staged SIGINT → wait 5s → SIGTERM → wait 2s → SIGKILL teardown for macOS is correct: escalation only ever targets survivors, and the conservative delay is properly isolated to macOS active sessions — prep processes and iOS/tvOS keep immediate cancellation, so the added latency only applies where it protects host screen-saver state. The disposeRunnerSession branching (graceful-success / graceful-fallback / non-graceful) lines up correctly with the new processExitHandled short-circuit. Good catch on the waitForRunnerProcessExit timer leak too — clearing it in finally and returning an exit boolean is a genuine improvement beyond the bug fix. Test coverage across the four signal paths plus the HTTP-disconnect integration test is thorough.

Non-blocking nits (all optional):

  • processExitHandled reads a bit ambiguously — something like handledByStagedInterrupt would make "kill already fully escalated, skip the generic wait+kill" clearer.
  • Awaited callers like stopAllIosRunnerSessions (daemon shutdown) now block up to ~7s when a macOS runner is active. Bounded and intended, but a one-line note would keep it from reading as a hang.
  • The two .filter(isMacOs) passes in abortRunnerSessionsAndPrepProcesses could be a single partition — trivial.

Good to merge as-is.

@thymikee
thymikee merged commit 7571b22 into callstack:main Jul 11, 2026
17 checks passed
@thymikee

Copy link
Copy Markdown
Member

Thank you so much @lott-ai and congrats on first contribution! Hopefully more to come :D

@lott-ai
lott-ai deleted the fix/macos-screensaver-runner-teardown branch July 11, 2026 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

macOS: interrupted XCTest runner can leave the screen saver disabled

2 participants