Skip to content

refactor(ios-runner): give runner sessions a lifecycle state - #2673

Merged
thymikee merged 1 commit into
mainfrom
t3code/finish-2662-adversarial-review
Sep 19, 2026
Merged

thymikee merged 1 commit into
mainfrom
t3code/finish-2662-adversarial-review

Conversation

@thymikee

@thymikee thymikee commented Sep 18, 2026 •

Copy link
Copy Markdown
Member

Summary

Needed for #2662. 34 files, with no CLI, Node, or MCP surface change.

A runner session's ready boolean conflated "has answered" with "process exists". Sessions now move through starting, ready, draining, and stopped, while readRunnerSessionLiveness is the sole reader combining that state with the process probe. Startup, reuse, repeated stop/invalidate, abort, detach, recycle, late answers, and status consumers now use one model.

The new lifecycle and readiness tests drive production routes and check transition order, one disposal, draining refusal, abort/detach cleanup, and recycling after process death.

Validation

  • Head: 4b77eeefe32de5d6c237c162ac61deda53e8c74c.
  • pnpm check:affected --run: 365 files and 2,436 tests passed.
  • pnpm check:xctest-selection: passed; TypeScript-only.
  • iPhone 17 Pro simulator (iOS 26.2), --debug: startup ran uptime and answered as command runner-f2b89e7f-a63b-4ec4-9acd-baa27bc1982c with sessionReady=false; retained close emitted ios_runner_idle_stop after the 2-second window and ios_runner_session_invalidated appeared zero times. A second open started and answered a fresh runner.
  • Current and main device status --json outputs are identical after normalizing session, UDID, workspace, state directory, PID, and timestamp.
  • Required CI checks on this head passed.

@github-actions

github-actions Bot commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.60 MB 4.60 MB +756 B
Package (unpacked) 4.60 MB 4.60 MB +756 B
Package (download) 1.37 MB 1.37 MB +326 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 17.7 ms 17.7 ms +0.0 ms
CLI --help 50.2 ms 48.9 ms -1.4 ms

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed 6e3db18. The state change itself looks right: advanceRunnerSessionState is the only writer, a late answer cannot revive a draining or stopped session, a repeated teardown skips only the re-signal and never the cleanup, and observeRunnerSession still reports the old alive value. What is missing is tests on the production routes.

The new tests in runner-session-types.test.ts (https://github.com/callstack/agent-device/blob/6e3db18/packages/platform-apple/src/runner/__tests__/runner-session-types.test.ts#L1) check the transition table and resolveRunnerSessionLiveness on hand-made {state} objects only. No test drives the sites that move or read the state: the early return in stopRunnerSessionInternal (https://github.com/callstack/agent-device/blob/6e3db18/packages/platform-apple/src/runner/runner-session.ts#L514) and invalidateRunnerSession, advance('draining') in disposeRunnerSession and abortRunnerSessionsAndPrepProcesses, advance('stopped') in cleanupRunnerSessionResources and detach, parseRunnerResponse on a draining session, and hasLiveSession skipping a draining session. I expect the suite would stay green if any one of these guards were deleted, but I did not try it. #2662 asks for one startup, one idle-stop and one recycle to go through the enum in the runner-session tests. Could runner-session.test.ts drive ensureRunnerSession from starting to ready, stop the same session twice and see one dispose and a final stopped, invalidate during a dispose, and send a late answer to a draining session that then stays draining with hasLiveIosRunnerSession false? Each test should go red when its guard is removed.

One design question: do the three liveness readers need to stay separate? getRunnerSessionSnapshot, readRunnerSessionLiveness and resolveRunnerSessionLiveness answer the same question. Would one readRunnerSessionLiveness(deviceId) that returns { sessionId, liveness }, also used by resolveReusableRunnerSession, do the job? It would remove the snapshot type and the direct isRunnerProcessAlive read (https://github.com/callstack/agent-device/blob/6e3db18/packages/platform-apple/src/runner/runner-session.ts#L337), where a draining session with a live process can still be handed out for reuse. That was also true before this PR, and today only shutdown reaches it. If you prefer to keep them separate, what would have to change first? The fixture churn across the test files follows from the rename and looks fine.

Not blocking: listRegisteredRunnerSessions() (https://github.com/callstack/agent-device/blob/6e3db18/packages/platform-apple/src/runner/runner-session.ts#L648) is a one-line wrapper with one caller, and the PR body says nothing changes in behavior, but recording start now refuses starting and draining sessions, and a draining session now takes the not-ready preflight path; one line in the body would help.

This changes how the iOS runner session starts, is reused and is disposed, so it needs live evidence. #2662 asks for the iOS simulator and macOS host lanes green on this commit, device status JSON for a running iOS session unchanged against main, and one simulator run of open, snapshot, close, then a second open, with --debug ndjson that shows ios_runner_session_startup, a first answer and one dispose, and no second ios_runner_session_invalidated for the same session id.

Smoke Tests was still running when I looked. This diff runs through its path (ensureRunnerSession, parseRunnerResponse, disposeRunnerSession, stopIosRunnerSession), so a failure there needs a look before it is called unrelated. There are no conflicts. The next step is the production-route tests and an answer to the design question, then the live evidence above.

@thymikee
thymikee force-pushed the t3code/finish-2662-adversarial-review branch from 6e3db18 to e559ccb Compare September 19, 2026 06:37
The session ready boolean mixed the runner answer with process existence, and three
readers combined them three ways. A session now records whether it has answered and
whether disposal has begun, while the process probe remains a separate fact. Both feed
one liveness answer and one idempotence guard. (#2662)
@thymikee
thymikee force-pushed the t3code/finish-2662-adversarial-review branch from e559ccb to 4b77eee Compare September 19, 2026 06:41
@thymikee

Copy link
Copy Markdown
Member Author

Pushed 4b77eeefe3 with CI green.

Design question. Yes. readRunnerSessionLiveness(deviceId) is now the one public reader, returning { sessionId, liveness } or null. RunnerSessionSnapshot and listRegisteredRunnerSessions() are gone. resolveReusableRunnerSession, hasLiveIosRunnerSession, observation, recycle, and the recording transport all read through it, so a live draining session can no longer be reused.

Production-route coverage. runner-session-lifecycle.test.ts records the transition sequence from the production entry points and covers:

  • ensureRunnerSession starting -> first answer -> ready;
  • idle stop draining -> stopped;
  • two stops: one disposal and final stopped;
  • concurrent stop/invalidate: no second disposal;
  • a late answer while draining: state remains draining and hasLiveIosRunnerSession is false;
  • abort: draining -> stopped;
  • detach: stopped without killing the runner;
  • dead process: gone, recycle to a fresh session;
  • draining session: reuse refuses and starts a fresh runner.

Readiness tests moved to runner-session-readiness.test.ts, reducing the giant file without changing its routes. The body now explicitly notes the record-start and preflight behavior differences.

Live evidence. On iPhone 17 Pro (iOS 26.2), open --debug recorded ios_runner_session_startup, then answered uptime as runner-f2b89e7f-a63b-4ec4-9acd-baa27bc1982c with sessionReady=false. Retained close scheduled a 2-second idle stop; the debug record then shows ios_runner_idle_stop, a graceful runner shutdown and simulator terminate, and ios_runner_session_invalidated occurred zero times. A second open started and answered a fresh runner. device status --json while a session was open is identical on this branch and main after normalizing session, UDID, workspace, state directory, PID, and timestamp.

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed 4b77eee. Both points from the earlier review are done. readRunnerSessionLiveness(deviceId) is now the only liveness reader, so a draining session with a live process is no longer reused. runner-session-lifecycle.test.ts drives the production entry points: startup to ready, idle stop, double stop, stop during invalidate, a late answer while draining, abort, detach and recycle. The readiness tests moved to runner-session-readiness.test.ts unchanged apart from the state rename.

The reported iPhone 17 Pro run reaches the changed routes: startup, the first answer, an idle stop with no invalidation, and a fresh reopen. It has no snapshot step, but the uptime answer covers the first-answer route, so I don't think another run is needed.

Two optional notes. The test "stop and invalidate do not start a second disposal" (https://github.com/callstack/agent-device/blob/4b77eee/packages/platform-apple/src/runner/__tests__/runner-session-lifecycle.test.ts#L265) stays green without the new guard in invalidateRunnerSession, because stopRunnerSessionInternal still blocks the second dispose; an assertion that only one ios_runner_session_invalidated diagnostic is emitted would cover it. The recycle-budget gate (https://github.com/callstack/agent-device/blob/4b77eee/packages/platform-apple/src/runner/runner-lifecycle.ts#L273) counts only gone and stopped as a fresh start, but reuse now also starts a fresh runner for draining, so a request that sees a draining session during shutdown can boot a runner without tryBeginRunnerRecycle. Should draining count there too?

The PR body does not yet have the line about recording start refusing starting and draining sessions, and draining sessions taking the not-ready preflight path.

All checks pass on 4b77eee and there are no conflicts. This is ready for a human review.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 19, 2026
@thymikee
thymikee merged commit 33d48a1 into main Sep 19, 2026
18 checks passed
@thymikee
thymikee deleted the t3code/finish-2662-adversarial-review branch September 19, 2026 10:07
@github-actions

Copy link
Copy Markdown
Contributor
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-19 10:08 UTC

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant