Skip to content

fix(runtime): resolve each lazily imported platform module exactly once - #2327

Merged
thymikee merged 1 commit into
fix/main-managed-allocation-move-mergefrom
fix/2314-memoize-platform-runtime-lazy-imports
Sep 6, 2026
Merged

fix(runtime): resolve each lazily imported platform module exactly once#2327
thymikee merged 1 commit into
fix/main-managed-allocation-move-mergefrom
fix/2314-memoize-platform-runtime-lazy-imports

Conversation

@thymikee

@thymikee thymikee commented Sep 6, 2026

Copy link
Copy Markdown
Member

Fixes #2314.

What was happening

session-open-runtime.test.ts > open --metro-port alone stays host-ambiguous on a physical Android device failed the hermetic-signal guard, but only when vitest related scheduled it in a worker under load:

This test tried to signal 2 process(es) this vitest worker did not spawn.
spawn pkill -TERM -f xcodebuild.*test-without-building.*AgentDeviceRunner\.env\.session-sim-1-[0-9]

The Android test never touches an Apple runner. The pkill came from two tests earlier in the same file, and reached the host through a Vitest mocking race.

Each port on createAppleApplicationTools() opened its own import(...) of the same specifier — seven of them for @agent-device/platform-apple/runner/operations alone. In production those duplicates are equivalent, because the module loader caches. Under Vitest they are not: while a vi.mock factory is still awaiting importOriginal(), a second dynamic import of that id resolves to the UNMOCKED module.

The open path makes that overlap routine — packages/platform-apple/src/lifecycle.ts deliberately leaves the iOS runner prewarm unawaited (recordUnawaited). So:

  1. open applies launch-only flags… (iOS simulator sim-1) schedules prewarm fix: skill should work, even if the npm package is not installed #1. Its dynamic import triggers the file's vi.mock factory, which awaits importOriginal() — the whole real Apple runner graph, slow under a loaded worker.
  2. open --metro-port alone defaults the host to 127.0.0.1 on an iOS simulator schedules prewarm Update README with correct GitHub link #2 while that factory is still in flight. It gets the real module.
  3. The real prewarmIosRunnerSessionprepareLocalIosRunnerensureRunnerSessionprepareRunnerLeaseForStartup, which finds an empty lease and spawns pkill -f xcodebuild...session-sim-1-[0-9] against the developer's own process table.
  4. That lands during the next test, and the guard names it.

That is the guard doing exactly its job: a unit test really was reaching for foreign processes.

The fix

Each specifier now has one memoized loader, and the ports reach their module only through it — so the second resolution the escape needs no longer exists. Same idiom already used in src/platform-runtime.ts and packages/platform-android/src/index.ts. Applied to the Apple tools, the Android tools, and loadAndroidMechanics, which had the same duplicated-lazy-import shape.

No production behavior changes: Node's loader already returned the same cached module for the duplicate imports.

Regression test

src/__tests__/platform-runtime-apple-application-tools.test.ts drives two runner ports concurrently and asserts both reach the mock. It fails deterministically without the fix:

AssertionError: expected "vi.fn()" to be called 1 times, but got 0 times

It uses the two shutdown ports, whose real implementations are no-ops on an empty session map, so a regression run fails on the assertion without doing host process work.

Validation

  • Repro command from the issue, 6 consecutive runs: 0 refusals, 231/231 files (run 6 had one unrelated waitForFileContent timeout in android-lifecycle.test.ts under contention; it passes on its own). Before the fix it reproduced in 1 of the first runs each time I tried.
  • pnpm test:unit — 1203 files, 9011 passed.
  • pnpm lint, pnpm format:check, pnpm typecheck, pnpm check:layering, pnpm check:di-seams, pnpm depgraph:test, pnpm check:gate-manifest, pnpm check:fallow — all green.

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.49 MB 4.49 MB +93 B
Package (unpacked) 4.49 MB 4.49 MB +93 B
Package (download) 1.33 MB 1.33 MB +218 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.3 ms 26.5 ms +0.2 ms
CLI --help 78.6 ms 80.0 ms +1.3 ms

@thymikee
thymikee force-pushed the fix/2314-memoize-platform-runtime-lazy-imports branch from 807fbbc to 52d65cc Compare September 6, 2026 06:40
@thymikee

thymikee commented Sep 6, 2026

Copy link
Copy Markdown
Member Author

Rebased onto 6e22e266d7. The three red checks on the previous run were not from this branch — main is broken independently, and #2328 fixes it. Until that merges, this PR's Typecheck / Repo Guards / Integration Tests will keep failing on:

src/daemon/managed-device-allocation/lease-admission.ts(19,8): error TS2307:
  Cannot find module './record-validation.ts'

@thymikee

thymikee commented Sep 6, 2026

Copy link
Copy Markdown
Member Author

No code findings in the loader change reviewed at 807fbbc. The shared lazy imports preserve runtime behavior, and the concurrency test covers the reported mock escape without risking host process cleanup. CI on that commit was blocked by the managed-allocation move and an iOS fixture startup timeout. The head changed to 52d65cc during review, so this assessment does not yet cover the updated commit.

The application-tools ports each opened their own `import(...)` of the same
specifier — seven of them for `@agent-device/platform-apple/runner/operations`
alone. In production those duplicates are equivalent, because the loader caches.
Under Vitest they are not: while a `vi.mock` factory is still awaiting
`importOriginal()`, a second dynamic import of that id resolves to the UNMOCKED
module.

The open path makes that overlap routine — it deliberately leaves the iOS runner
prewarm unawaited — so a unit test that mocks the runner could still reach the
real one. `session-open-runtime.test.ts` did: its two iOS-simulator cases
scheduled two prewarms, the second one bypassed the mock, and the real local
XCTest runner started for device `sim-1`. Its stale-process cleanup then spawned
`pkill -f xcodebuild...session-sim-1-[0-9]` against the developer's own process
table, which the hermetic-signal guard refused and reported against whichever
test happened to be running when it landed — the unrelated
`open --metro-port alone stays host-ambiguous on a physical Android device`.

Each specifier now has one memoized loader, and the ports reach their module
only through it, so the second resolution the escape needs no longer exists.

Fixes #2314
@thymikee
thymikee force-pushed the fix/2314-memoize-platform-runtime-lazy-imports branch from 52d65cc to 89e2f86 Compare September 6, 2026 07:08
@thymikee
thymikee changed the base branch from main to fix/main-managed-allocation-move-merge September 6, 2026 07:08
@thymikee

thymikee commented Sep 6, 2026

Copy link
Copy Markdown
Member Author

Stacked on #2328 so this branch's CI runs against a repaired tree — main itself is red (Cannot find module './record-validation.ts', see #2328), which is where every failure on the previous run came from. Base retargets back to main automatically when #2328 merges.

The one failure that was not inherited — the Android smoke wait text "Alert result: cancelled" — passed on re-run; it was a live-emulator flake.

@thymikee

thymikee commented Sep 6, 2026

Copy link
Copy Markdown
Member Author

Looks good at 89e2f86. The shared lazy imports preserve initialization order and the regression covers concurrent callers. All checks are green. Please merge #2328 first; this PR is based on it.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Sep 6, 2026
@thymikee
thymikee merged commit ed5e973 into main Sep 6, 2026
18 checks passed
@thymikee
thymikee deleted the fix/2314-memoize-platform-runtime-lazy-imports branch September 6, 2026 07:51
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-09-06 07:52 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.

session-open-runtime.test.ts leaks an Apple runner pkill into the next test

1 participant