Skip to content

test(apple-runner): pin exchange session view and accounting - #3006

Merged
thymikee merged 3 commits into
codex/2967-command-exchangefrom
codex/2967-runner-exchange
Sep 28, 2026
Merged

thymikee merged 3 commits into
codex/2967-command-exchangefrom
codex/2967-runner-exchange

Conversation

@thymikee

@thymikee thymikee commented Sep 28, 2026 •

Copy link
Copy Markdown
Member

Summary

Closes #2967. Final layer, based on #3012: narrow the exchange input to a named session view and pin command settlement plus awaited fatal invalidation with the real HTTP fake. The session owner keeps registration, leases, disposal, and fatal invalidation; runner-session.ts is 956 lines (1,533 before the stack). This 3-file layer has production +41/−15 (net +26), tests +186, and 242 rename-aware gross diff lines.

At 5894447ee, restore the original readiness-preflight exemption for activate, terminate, and targetReset even while the session is starting. The preceding version added a startup wait on physical XCTest open/close that #2967 did not authorize. A real HTTP fake now asserts each exempt command is sent once without an uptime probe. Earlier busy-stamp and fatal-invalidation review fixes remain.

Validation

pnpm format, focused exchange/readiness/retry tests (60/60), and exact-head pnpm check:affected --run at 5894447ee passed (format, lint, typecheck, layering, fallow, build, 380 related files / 2,580 tests). The fatal-answer and thrown-fatal tests previously failed when their production await was removed. Current-head CI is pending.

A live cold open --debug on a physical XCTest backend remains unverified: the connected iPhone is CoreDevice (devicectl sees it; xctrace lists it offline), and the available AWS Device Farm iOS path is WebDriver. The new behavior on exempt commands now matches main; the previous head's simulator prepare ios-runner passed but did not exercise physical XCTest open.

@github-actions

github-actions Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.88 MB 4.88 MB +48 B
Package (unpacked) 4.88 MB 4.88 MB +48 B
Package (download) 1.46 MB 1.46 MB +12 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 21.0 ms 22.1 ms +1.1 ms
CLI --help 60.9 ms 57.6 ms -3.3 ms

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 10 files

Reply with feedback, questions, or to request a fix.

Fix all with cubic | Re-trigger cubic

Comment thread packages/platform-apple/src/runner/__tests__/runner-exchange.test.ts Outdated
@thymikee
thymikee force-pushed the codex/2967-runner-exchange branch from 2be6259 to 7a113cc Compare September 28, 2026 10:27
@thymikee
thymikee changed the base branch from main to codex/2967-command-exchange September 28, 2026 10:27
@thymikee
thymikee added this pull request to stack #3013 September 28, 2026 10:27
@thymikee thymikee changed the title refactor(apple-runner): separate command exchange from session ownership test(apple-runner): pin exchange session view and accounting Sep 28, 2026
@thymikee

Copy link
Copy Markdown
Member Author

Reviewed at 7a113cc. The fatal-invalidation test at runner-exchange.test.ts line 75 does not pin the await it is meant to pin. The callback runs await Promise.resolve(); order.push(reason), and that microtask finishes before the test resumes whether or not settleRunnerAnsweredExchange (runner-exchange.ts line 170) awaits invalidateFatalSession. So the order is ['ax_failed', 'returned'] in both cases. If the await is dropped later, the test stays green and a caller could reuse a session whose kill has not finished. Could the callback return a deferred promise that the test controls, so the test can assert the exchange is still pending before it releases the deferred? Please check that the test fails when the await at line 170 is removed, and add the same check for the thrown-fatal branch at line 127.

Not blocking: sessionFor() at line 16 could reuse makeRunnerSession() from runner-session-fixtures.ts, and the "settles only its answer" test would show "only" better with two earlier abandoned charges.

CI: the non-passing checks were cancelled by a newer push or are still queued. Nothing has failed yet.

@thymikee

Copy link
Copy Markdown
Member Author

Addressed at a7a7301. The fatal-answer and thrown-fatal tests now hold owner invalidation behind a controlled promise and assert the exchange remains pending. I replaced the production await locally to check the regression: both tests failed, then passed again with the await restored. The same commit carries the readiness probe's busy stamp and gives first-session exempt commands a startup preflight. Exact-head affected gate passed (380 files, 2,579 tests); new-head CI is running.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No issues found across 3 files

Re-trigger cubic

@thymikee

Copy link
Copy Markdown
Member Author

Reviewed at a7a7301. This follows up on the earlier review at 7a113cc (#3006 (comment)); the tests and code layout look cleaner now, but the behavior-change question from that round is still open.

Main skips the preflight for exempt commands in every session state (runner-session.ts:1353 on main). This delta adds a startup uptime preflight, bounded by min(startupTimeout, command deadline), before a first activate/terminate on a starting session (https://github.com/callstack/agent-device/blob/a7a7301/packages/platform-apple/src/runner/runner-exchange.ts#L413). In production this is the route an XCTest-backend physical-device open/close takes (physical-device-control.ts:206/221); the simulator targetReset path is mostly gated by hasLiveRunnerSession (lifecycle.ts:268-273) and doesn't reach this branch. So does a cold open or close on a physical device now wait for startup instead of sending once, and can it fail as a readiness-preflight error when the command deadline is shorter than startup? Nothing on record shows which outcome happens, since the only live run so far is a simulator prepare ios-runner. Please run one live cold open <bundleId> on an XCTest-backend physical iOS device with no warm runner, using --debug (the AWS Device Farm recipe works for this). The ndjson needs to show ios_runner_readiness_preflight with reason=startup before the activate send, and the open itself needs to succeed.

Not blocking, and can be taken or left: the mirror-present runnerMainThreadBusy stamp block is now duplicated at runner-exchange.ts:161-164 and :331-334 and could go through one mirrorRunnerMainThreadBusy(session, data) helper or a single post-parse step; the thrown-fatal test at runner-exchange.test.ts:186 ends on a bare assert.rejects(exchange) and could validate the RUNNER_WEDGED code instead; and since issue #2967 asks the stack to keep existing outcomes, and this PR's title says test(...) while runner-exchange.ts:410 carries two behavior changes the body already discloses, a fix(...) title or a body note naming both fixes would keep the squash history honest.

I didn't run the new tests or the await-removal mutation myself; the regression read comes from reading the code. I also didn't check whether main's cold xctest-backend activate actually failed before this change or was rescued by a recovery path, so the size of the user-visible fix is unknown. And I'm relying on the Swift runner stamping every transport response (RunnerTests+Transport.swift:261) without having run it, so a real uptime reply carrying the stamp is an assumption, not something I confirmed live.

Both Smoke Tests jobs are still queued or in progress and neither has failed yet. The iOS smoke job goes through executeRunnerExchange and the readiness preflight, which is exactly the code this delta touches, so a later failure there would not be clear of this diff.

Next step: run a live cold-runner first activate on an XCTest-backend physical iOS device and show the startup preflight firing followed by a successful open.

@thymikee
thymikee force-pushed the codex/2967-runner-exchange branch from a7a7301 to 57678db Compare September 28, 2026 15:36
@thymikee
thymikee force-pushed the codex/2967-runner-exchange branch from 57678db to 7b2b1c5 Compare September 28, 2026 16:54
@thymikee

Copy link
Copy Markdown
Member Author

Reviewed at 7b2b1c5. The evidence gap from the earlier review (57678db) is still open.

canSkipReadySessionPreflightForExemptCommand now runs the readiness preflight before a first activate or terminate on a starting session, where main skipped the preflight for exempt commands in every session state: https://github.com/callstack/agent-device/blob/7b2b1c5/packages/platform-apple/src/runner/runner-exchange.ts#L471-L476. The XCTest-backend open and close on physical devices (activateXctestDeviceApp and terminateXctestDeviceApp) use this exact route. So a cold open <bundleId> on a physical iOS device with no warm runner now waits for a startup preflight bounded by min(startup timeout, command deadline). If the command deadline is shorter than startup, an open that used to succeed could now fail. Can you add a live cold-runner open <bundleId> --debug on an XCTest-backend physical iOS device, showing ios_runner_readiness_preflight with reason=startup before the activate send and the open succeeding?

CI is still waiting on one Smoke Tests run; the cancelled jobs are from before the restack. Smoke Tests uses simulators only, so a green run does not cover the physical-device path above.

@thymikee

Copy link
Copy Markdown
Member Author

Addressed at 5894447ee. #2967 is a behavior-preserving split, so I removed the new startup preflight for exempt commands. activate, terminate, and targetReset now skip readiness preflight in both starting and ready sessions, as on main. The real HTTP fake asserts one command send and no uptime request for each; focused exchange/readiness/retry tests passed (60/60), and the exact-head affected gate passed (2,580 tests).

I cannot claim the requested live physical XCTest run. The connected iPhone is CoreDevice (devicectl sees it; xctrace lists it offline), and the available AWS Device Farm iOS route is WebDriver. The unverified startup-wait behavior is no longer in this PR. Current-head CI is pending.

@thymikee
thymikee merged commit 8fe03de into main Sep 28, 2026
19 checks passed
@thymikee
thymikee deleted the codex/2967-runner-exchange branch September 28, 2026 18:01
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-28 18:03 UTC

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.

refactor(apple-runner): separate command exchange from session resource ownership

1 participant