refactor(ios): name the runner session xctestrun as a process-identity contract - #2987
Conversation
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/contracts/src/runner-session-artifact.ts">
<violation number="1" location="packages/contracts/src/runner-session-artifact.ts:57">
P3: The contract's sanitizer is private, yet the writer at `packages/platform-apple/src/runner/runner-artifact-env.ts:48` still spells the same rule itself (`suffix.replaceAll(/[^a-zA-Z0-9._-]/g, '_')`). Two copies of the `[a-zA-Z0-9._-]` character set now govern on-disk session names; they're identical and the writer's pass is idempotent today, but any future edit to one copy silently renames sessions while the matchers keep the old bytes — exactly the drift this PR exists to prevent. Export `sanitizeRunnerSessionNameField` and have the writer consume it, or delete the writer's re-sanitize pass.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
|
|
||
| /** Characters a session name may carry; anything else is flattened, as the filesystem writer does. */ | ||
| function sanitizeRunnerSessionNameField(value: string): string { | ||
| return value.replaceAll(/[^a-zA-Z0-9._-]/g, '_'); |
There was a problem hiding this comment.
P3: The contract's sanitizer is private, yet the writer at packages/platform-apple/src/runner/runner-artifact-env.ts:48 still spells the same rule itself (suffix.replaceAll(/[^a-zA-Z0-9._-]/g, '_')). Two copies of the [a-zA-Z0-9._-] character set now govern on-disk session names; they're identical and the writer's pass is idempotent today, but any future edit to one copy silently renames sessions while the matchers keep the old bytes — exactly the drift this PR exists to prevent. Export sanitizeRunnerSessionNameField and have the writer consume it, or delete the writer's re-sanitize pass.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/contracts/src/runner-session-artifact.ts, line 57:
<comment>The contract's sanitizer is private, yet the writer at `packages/platform-apple/src/runner/runner-artifact-env.ts:48` still spells the same rule itself (`suffix.replaceAll(/[^a-zA-Z0-9._-]/g, '_')`). Two copies of the `[a-zA-Z0-9._-]` character set now govern on-disk session names; they're identical and the writer's pass is idempotent today, but any future edit to one copy silently renames sessions while the matchers keep the old bytes — exactly the drift this PR exists to prevent. Export `sanitizeRunnerSessionNameField` and have the writer consume it, or delete the writer's re-sanitize pass.</comment>
<file context>
@@ -0,0 +1,62 @@
+
+/** Characters a session name may carry; anything else is flattened, as the filesystem writer does. */
+function sanitizeRunnerSessionNameField(value: string): string {
+ return value.replaceAll(/[^a-zA-Z0-9._-]/g, '_');
+}
+
</file context>
There was a problem hiding this comment.
Fixed at 0683a3d, and the file this comment sits on no longer exists. There is one sanitizeRunnerSessionNameField in packages/platform-apple/src/runner/runner-artifact-env.ts: the suffix builder applies it, and prepareXctestrunWithEnv joins the stem to the already-sanitized suffix through the same function instead of re-spelling the character set.
|
Reviewed at 25f05bd. The rename to a process-identity contract looks correct. For realistic device ids, the runner argv and the disposal Before this goes further, one design question. The new contracts subpath is justified, because A pre-existing gap inside the invariant this PR names: a detached lease rewrites Smaller notes, not blocking: the writer still re-sanitizes the suffix with its own regex (runner-artifact-env.ts#L48); the round-trip test comment claims more than it pins, because a stem or prefix rename would pass on both sides (runner-xctestrun.test.ts#L366); and the dead Smoke Tests, Repo Guards, Coverage and Integration were still running. They all exercise this diff, so a failure there is likely related until shown otherwise. No live simulator run is needed: argv and |
…identity contract A runner launch is killed by matching `xcodebuild`'s argv with `pkill -f`, and that argv carries the per-session xctestrun's filename. The filename is therefore a process-identity contract, but it was spelled three times: inline in the session, inline in disposal, and again in the daemon-client timeout sweep. Disposal escaped the device id for regex without flattening it the way the writer's filesystem sanitization does, so a device id needing flattening built a cleanup pattern that could not match the file the writer had already written. Name it once. `runner-artifact-env.ts` now owns the stem, the suffix field order, the sanitization, and the `pkill -f` pattern derived from those same parts, and the writer, the session, and disposal all go through it. The name lives in the writer rather than a shared contracts module because the timeout sweep must not follow a rename: it ships separately, cannot know which version named a timed-out launch, and has to keep matching the names older writers used. So the sweep keeps a pinned literal, and its test proves those bytes still select both the owner-token and the pre-owner-token spelling. Placing the name in the module the Apple façades already evaluate also holds the eager-closure budget (#1960) at zero new edges. Verified byte-for-byte against the previous patterns for concrete, host-style, and flattened device ids. The only divergence is the flattened case, where the old pattern failed to match its own file. Also record three tooling constraints read from `xcodebuild(1)` and `simctl help`, and checked on Xcode 27.1: only `TEST_RUNNER_`-prefixed names cross into the test runner; `simctl launch --stdout/--stderr` resolve their paths inside the device's data container, so `--console-pty` is the console mode this host path can use; and xcodebuild re-synthesizes the attachment-lifetime keys from its own defaults, since a plan setting `keepNever` still builds an xctestrun carrying `SystemAttachmentLifetime=deleteOnSuccess`.
25f05bd to
0683a3d
Compare
…se recorded A detached lease rewrites its owner token to `detached-<token>` while the xcodebuild it handed over keeps running under the name the writer gave it, so the token-derived pkill pattern named a file that never existed and the launch survived the reclaim. Lease-backed cleanup now follows the recorded xctestrun basename, which is exactly the argv the launch carries; the device sweep stays for the no-lease case, and the token branch of the old builder is removed.
|
Good call — the smaller shape was the right one, and it also subsumed the detached-lease gap. Pushed as f1e27f1. The contracts subpath is gone entirely. Lease-backed disposal now matches the recorded basename, exactly as you sketched: the adapter takes a target instead of
That builder returns Because nothing reads the owner token anymore, I deleted the token branch of the old builder rather than leaving it as an unreached path. Detached-lease gap is closed here, not in a follow-up. Two tests:
Your smaller notes:
Gates at f1e27f1: |
|
Two reach differences from the lease shape, worth stating explicitly since the byte-parity review was about what the pattern selects: The lease pattern is now port-exact. The old token-prefix pattern ( The device id no longer appears in the lease pattern (it still scopes the sweep). It is inside the basename, so the device is still selected; the difference is only for a lease whose |
…ilds it Driving `runnerLeaseCleanupAdapter` directly proves the pkill bytes disposal issues select the launch a lease names, without session-startup choreography. Moves the detached-lease coverage out of the aggregated session suite, which the test-file size ratchet forbids growing.
|
One correction to my earlier comment: I cited
Verified the relocated test still earns its place: swapping the path builder for the device sweep in |
|
This is ready for human review. CI is green, all 19 checks pass at a11fba3, including the Coverage lane that runs the test-file size ratchet, so no failures need attribution. The delta answers the earlier evidence-pending state: the detached-lease gap is closed, and reuse and teardown ownership are unchanged, so the same lease owner and the same two adapter callers still apply. I did not run a live xcodebuild to confirm the pkill matches a real detached process; I checked the pattern by reading it as an ERE-escaped literal of a basename limited to [a-zA-Z0-9._-] against the fixture bytes, and confirmed that literal matches the JS RegExp the tests use. The claim that port-exact narrowing drops no live orphan is reasoned rather than exhaustively traced: a same-token launch on another port whose lease was overwritten by a later launch from the same daemon would not be reached by the reclaim pkill, and while I found no production path that leaves such a launch alive, I did not trace every startup-failure branch. I also did not run the author's own mutation checks (stem rename, swapping the path builder for the device sweep) locally. Not blocking: the stale-lease assertion at packages/platform-apple/src/runner/tests/runner-session.test.ts#L787 could be tightened to pin the full target including xctestrunPath the way it used to pin ownerToken, so a dropped xctestrunPath falling back to the device sweep would still fail it, and the test name at packages/platform-apple/src/runner/tests/runner-artifact-env.test.ts#L29 could be corrected to say the recorded basename is what gets selected rather than claiming released daemons get pkilled by the token prefix — both can be taken or left. |
thymikee
added
the
ready-for-human
|
1 participant
Summary
Follow-up to #2963 from the same inspection pass: what does runner process cleanup actually depend on, and is it declared anywhere?
A runner launch is killed by matching
xcodebuild's argv withpkill -f, and that argv carries the per-session.xctestrunfilename. The filename is a process-identity contract, but it was spelled three times: inline in the session, inline in disposal, and again in the daemon-client timeout sweep. Disposal escaped the device id for regex without flattening it the way the writer's filesystem sanitization does, so a device id needing flattening built a pattern that could not match the file the writer had already written.runner-artifact-env.tsnow owns that name: the stem, the suffix field order, one sanitizer, and twopkill -fbuilders. The writer and the session go through the suffix builder; the sanitizer is shared rather than re-spelled.Two patterns, because the two callers know different things:
RunnerXcodebuildCleanupTarget(runner-lease.ts) carries the lease'sxctestrunPath, and the pattern is that basename. This closes a gap the reviewer found:buildDetachedRunnerLeaserewritesownerTokentodetached-<token>while the handed-over xcodebuild keeps running under the name the writer gave it, so the token-derived pattern named a file that never existed and only the pid-tree kill covered that launch.session-<device>-[0-9]).Nothing derives a launch name from the owner token anymore, so the token branch of the old builder is gone rather than left unreached.
The name lives in the writer, not a shared contracts module, because the timeout sweep must not follow a rename: it ships separately and has to keep matching names older writers used, so it keeps a pinned literal. That also keeps the eager-closure budget (#1960) at zero new edges — the first shape of this PR added a
contractsmodule and the gate rejected it on four entries.Also records three tooling constraints from
xcodebuild(1)/simctl help, checked on Xcode 27.1: onlyTEST_RUNNER_-prefixed names cross into the test runner;simctl launch --stdout/--stderrresolve inside the device's data container; and xcodebuild re-synthesizes attachment lifetimes from its own defaults (plan setskeepNever, built xctestrun carriesSystemAttachmentLifetime=deleteOnSuccess).13 files. Production +131/−20, tests +272/−11.
Validation
Head
a11fba324:pnpm check:affected --runpasses (3031 tests, 429 files): eager-closure budgets green, layering guard OK,check:fallow --base origin/mainclean in changed files, and the test-file size ratchet green —runner-session.test.tsis back at its merge-base length. Also passing:typecheck,lint,format:check.apple-runnerproject 631 tests andunit-coredaemon-client + eager-closure 805 tests green.killRunnerXcodebuildProcessesfailsa leased cleanup selects the launch named by the artifact the lease recordedinrunner-disposal.test.ts, which drives the production adapter and asserts the emittedpkillbytes against the launch argv.runner-xctestrun.test.tsis what binds that literal to what the writer actually emits today.xcodebuild .*AgentDeviceRunner\.env\.session-is asserted to still match both owner-token and pre-owner-token (session-<device>-<port>, shipped through v0.17.0) names.unit-ci, integration, and Swift runner lanes stay GitHub-authoritative.