Unify subprocess lifecycle around a single session-owned abort signal#164
Merged
Conversation
Extracts the SIGTERM->wait->SIGKILL tail (currently duplicated inline in xctest-agent.ts) into a shared terminate() helper. Removes spawn.ts's module-level activeChildProcesses set and installProcessCleanup(), which installed a second, competing SIGINT/SIGTERM handler that raw-kills every tracked child in parallel with the session's own graceful dispose. The session's structured dispose (packages/jest/src/harness-session.ts) already reaches every long-lived child, making this global net a redundant backstop that races the same class of bug #162 fixed. Relocates the @clack/prompts raw-mode workaround into an exported ensureSignalsDeliverable(), to be called once from the session.
…time No signature change. Clarifies that init.signal aborts on session dispose, not on a readiness timeout, so platform runners thread it into long-lived child processes rather than treating it as scoped to the init() call.
…oller Collapses the two competing AbortControllers (setup-phase setupController and plugin-only pluginAbortController) into a single sessionController that lives for the whole session: early SIGTERM/SIGINT abort it during setup, and dispose()'s finally aborts it during teardown. The plugin manager and platform init both now observe this one signal. withPlatformReadyTimeout no longer combines the readiness timeout into the signal passed to platform init — init receives the real session-lifetime signal, and a timeout race throws PlatformReadyTimeoutError without silently swapping in a synthetic abort reason. The outer setup catch now aborts sessionController so an abandoned init call left running past a readiness timeout is still cancelled cooperatively.
createAndroidAppSession now accepts the session-lifetime init.signal and combines it with its own logcatAbortController via AbortSignal.any, so the logcat stream aborts on session teardown even before dispose() runs. No change to the .kill()-free abort mechanism from #162.
…build Threads init.signal into createIosAppSession (terminates the --console launch process on session teardown) and into createXCTestAgentController (stops the XCTest agent on the same signal). Replaces the inlined SIGTERM->wait->SIGKILL tail in xctest-agent.ts's stopProcess() with the new shared terminate() helper, keeping the graceful client.shutdown() request in front of it unchanged. Also replaces the app-session launch process's raw .kill() with terminate().
Both runners previously ignored HarnessPlatformInitOptions.signal entirely (void init). Web now closes the Playwright browser on abort; Vega now disposes the current app session (stopping its poll loop and the app) on abort. Both still expose the same behavior through their normal dispose() paths.
Exports withPlatformReadyTimeout for direct testing and adds unit tests for its two behaviors: the real session signal reaches platform init unmodified, and a readiness timeout throws PlatformReadyTimeoutError without forging an abort on the session signal. Adds signal-abort coverage for createIosAppSession (terminates the launch process) and createAndroidAppSession (combines with logcatAbortController via AbortSignal.any). Adds the nx release version plan (patch) covering the touched packages.
Self-review follow-up: removing spawn.ts's per-spawn setRawMode(false) reset (previous commit) dropped the only thing that undid @Clack's raw-mode side effect for callers outside the jest harness session. The init wizard spawns subprocesses (platform installers) while a clack spinner is active, so without this it could stop responding to Ctrl+C after the first spinner runs. Calls the same ensureSignalsDeliverable() the session uses.
Opus review caught a real regression: the force-kill setTimeout was never cleared when the process exited gracefully before forceAfterMs, leaving a ref'd timer alive for the full duration and delaying process/worker exit (e.g. every iOS app-session dispose held the event loop open for up to 2s). delay() now returns a cancel() the winning race branch calls. Also wraps both kill() calls in try/catch, matching the defensiveness the old inline iOS code had around its .kill() call.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Manual verification (real Android/iOS emulators, SIGINT mid-run) surfaced that the session-lifetime abort listener added in a prior commit re-ran stop() a second time on every teardown: normal disposal already calls xctestAgent.dispose() explicitly, and sessionController.abort() always fires afterwards regardless of reason, re-triggering the abort listener. The second call was a functional no-op (agentProcess/agentClient already null) but logged a spurious "Stopping XCTest agent session" line every time. Guards dispose() with the same disposed-flag idempotency pattern already used by AppSession implementations elsewhere in the codebase.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What is this?
Removes a redundant, competing subprocess-shutdown mechanism so that Harness has exactly one authority for tearing down child processes (emulators, simulators, logcat, the iOS XCTest agent, browsers) when a test session ends or is interrupted.
How does it work?
The jest harness session already owned a structured, ordered
dispose()driven by anAbortController. Separately,packages/tools/src/spawn.tsinstalled its own globalSIGINT/SIGTERMhandlers that raw-killed every tracked child process in parallel with that graceful dispose — the same class of race a prior fix (#162) had to patch for Android logcat specifically, just not everywhere else.This change removes that global net entirely and makes the session's single
AbortController(sessionController) the only signal handlers in the process, for its whole lifetime — early SIGINT/SIGTERM during setup, anddispose()'s teardown afterward.HarnessPlatformInitOptions.signalis now documented and used as session-lifetime (not scoped to platform init), and is threaded into every platform runner's long-lived children:AbortSignal.any.--consolelaunch process and stops the XCTest agent.A platform readiness timeout no longer forges an abort reason onto the session signal — it races independently and, on timeout, the session's own abort cancels the abandoned init call.
The SIGTERM → wait → SIGKILL tail that used to live only inside iOS's XCTest agent code is now a shared
terminate()helper inpackages/tools, reused by both the agent and the app-session launch process.Why is this useful?
Two independent teardown paths racing on every signal was a source of exactly the bug class #162 already had to fix once — this removes the redundant path instead of patching it platform-by-platform. It also makes the abort contract explicit and consistent across all four platform runners, so a new runner only has to observe one signal correctly instead of guessing whether a second, implicit cleanup mechanism might also be reaching in.