fix(ios-runner): certify cached runner products by content, not by stats - #2991
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.
2 issues found across 27 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="scripts/write-xcuitest-cache-metadata.ts">
<violation number="1" location="scripts/write-xcuitest-cache-metadata.ts:58">
P3: The build-log verification cannot actually check the `-IDEPackageSupport*` recipe flags: `recordedRunnerBuildSettings` only matches keys starting with `[A-Z]`, so those two args are dropped from the diff silently. Since both flags are a single shared constant there is no live drift path, but the guarantee that the identity is verified against the log is incomplete; the comment labels it the whole recipe.</violation>
</file>
<file name="packages/platform-apple/src/runner/runner-cache.ts">
<violation number="1" location="packages/platform-apple/src/runner/runner-cache.ts:183">
P2: When `buildRunnerCacheArtifacts` refuses (escaping symlink, file over 128 MB, unreadable subtree), the writer still publishes identity-only metadata. The next run then evaluates `artifact_manifest_missing` and `ensureXctestrunUnderCacheLock` deletes the tree and rebuilds, producing the same refusal — so every runner launch on such a host rebuilds for minutes with no progress and no diagnostic beyond a generic miss. Publish the failure as a typed error instead of a silently uncertifiable state, or preserve the previous certified manifest, so the loop cannot recur.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| ): Promise<RunnerXctestrunCacheMetadata> { | ||
| const { device, derivedPath, buildLogPath } = parseWriterInvocation(args); | ||
| const metadata = resolveExpectedRunnerCacheMetadata(device, cwd); | ||
| requireRunnerBuildSettingsMatchBuildLog(metadata, buildLogPath); |
There was a problem hiding this comment.
P3: The build-log verification cannot actually check the -IDEPackageSupport* recipe flags: recordedRunnerBuildSettings only matches keys starting with [A-Z], so those two args are dropped from the diff silently. Since both flags are a single shared constant there is no live drift path, but the guarantee that the identity is verified against the log is incomplete; the comment labels it the whole recipe.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At scripts/write-xcuitest-cache-metadata.ts, line 58:
<comment>The build-log verification cannot actually check the `-IDEPackageSupport*` recipe flags: `recordedRunnerBuildSettings` only matches keys starting with `[A-Z]`, so those two args are dropped from the diff silently. Since both flags are a single shared constant there is no live drift path, but the guarantee that the identity is verified against the log is incomplete; the comment labels it the whole recipe.</comment>
<file context>
@@ -0,0 +1,79 @@
+): Promise<RunnerXctestrunCacheMetadata> {
+ const { device, derivedPath, buildLogPath } = parseWriterInvocation(args);
+ const metadata = resolveExpectedRunnerCacheMetadata(device, cwd);
+ requireRunnerBuildSettingsMatchBuildLog(metadata, buildLogPath);
+
+ const xctestrunPath = findRunnerXctestrun(derivedPath, device);
</file context>
There was a problem hiding this comment.
Fixed in 6d268ff. readRunnerBuildLogRecipe (runner-cache-metadata.ts:660) also captures the Command line invocation: line, and diffRunnerInvocationFlagsAgainstBuildLog (:717) requires every recorded arg that is not a KEY=VALUE setting — the -I package-sandbox disables — to appear on it. Confirmed those -IDEPackageSupportDisable* flags appear only on the invocation line, not in the settings block. Three cases in runner-cache-metadata.test.ts.
There was a problem hiding this comment.
1 existing issue remains and no new issues found across 22 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
|
Reviewed at b6bd6e2. The manifest approach is sound, but I can't call this ready to merge until the fail-closed launch case and the scope are settled. After a successful fresh xcodebuild, The gross diff is +3215/-1622, past the 1,000-line budget, and it bundles the content manifest with a separate source-identity change (the Not blocking: whether skipping digestion when size+mtime+ino match a same-process record is worth it given the ~23 ms per-launch cost can be taken or left, and it's worth asking whether the manifest module alone (without the recipe/facade widening) is enough for this PR to close on. The live evidence covers Xcode 26.2 reaching reuse_ready, then digest_mismatch after a one-byte flip, then refusal of an escaping symlink, but it doesn't say which platform or whether it went through the daemon's Smoke Tests is still running with no failure output yet, and it launches the Apple runner through |
A restored DerivedData tree was reuse-eligible when its cache metadata matched and every named product still existed with the size and mtime the metadata recorded. That signature survives patching a Mach-O in place, stripping a signature, or rewriting bytes through a restore that preserved stats, so a build could be blessed under an identity it was not compiled with. Reuse is now authorized by a content manifest: the writer digests every file under the cache root, records permission bits, and records symlink targets; the reader walks the same tree with the same traversal and refuses the first entry whose bytes, kind, mode, or target disagrees. Product discovery by directory scan is gone, along with the stat-only reuse path and the 'restore-key' cache kind it produced, because reuse cannot occur without a certified manifest. Source identity was equally narrow: only .swift/.m/.h files under apple/runner counted, so an edited .pbxproj, .xcscheme, or .xcworkspacedata reused a stale runner. Everything inside an Xcode package is now a build input, xcuserdata aside, and AGENT_DEVICE_XCUITEST_ARCHS reaches both the xcodebuild arguments and the cache identity. The build script and the metadata writer now emit their settings from one owner, and the writer checks them against what xcodebuild echoed in the build log before it certifies anything, so a drifted recipe cannot publish a manifest. Cache schema is 3; anything older rebuilds once.
…ublish it Declare the TypeScript build-settings emitter and cache-metadata writer as entries, replacing the ignore that hid the deleted .mjs writer from dead-code analysis.
The manifest trusted a lexical containment check, so a symlink whose target reached the cache root by `..` cancellation certified bytes the walk never digested, and a product root that was itself a link out of the cache was walked as though it were inside. Containment now resolves through the filesystem: a symlink is certified only when its target resolves inside one of the walked product roots, and a root that resolves outside the canonical cache root refuses certification outright. - report refusals on typed reasons (`root_escapes_cache`, `symlink_escapes_cache`, `root_unusable`) and surface them as a failed build instead of a silently uncertifiable publish; - validate the destination's platform clause by clause instead of scanning the whole destination for the substring `simulator`, and require the CI action's platform and destination inputs; - verify recorded build arguments against the invocation line in the build log and drop `.DS_Store` / `.xcuserstate` from the source fingerprint; - split the certification engine into `runner-artifact-manifest.ts` so the cache lifecycle module stays under the size ceiling.
…sure Splitting the manifest out of `runner-cache.ts` made three platform façades (`app-lifecycle`, `doctor`, `runner-operations`) evaluate one module more than the merge-base did, which ADR-0019's implementation-laziness rule refuses. `runner-artifact-manifest.ts` digests whole product trees, so nothing on the metadata path should pay to load it. Both entry points into it are already `async`, so the edge moves behind a function-scoped `await import` as the gate prescribes; the mismatch and refusal types stay type-only and cost nothing.
…estrun The second review round found two ways a manifest and its reader disagreed: - two `productPaths` that resolve to the same directory -- a direct bundle beside an in-cache alias -- each became a walked root, so the leaves were collected twice and the reader's walk called the second root's copies undeclared. `resolveWalkedRoots` now collapses equal canonical roots, not just nested ones; shared by both sides, so a manifest already written certifies the same way it is read. - the writer digested an in-cache symlink at the `.xctestrun` path while the reader lstats the named path and calls a symlink a kind change, so every such entry missed on sight. The writer now checks the named path is a regular file before digesting. Certification also moved ahead of the `reuse_ready` decision, so diagnostics cannot record a reuse that the gate then refused. The remaining findings were test honesty: fixtures assert the refusal their own contract promises, the foreign-artifacts reuse case carries real digests so only containment can fail it, the toolchain stub is registered by the fixture that needs it, and the Xcode-package comment sits above the predicate it documents.
b6bd6e2 to
7125529
Compare
|
Reviewed at 7125529. This fixes the reuse gap from the earlier review: certification now runs before reuse_ready and the writer refuses a symlinked .xctestrun, so a fresh build no longer silently trusts stats-only matches. I did not run the tests. That the new tests would fail without the fix comes from reading the pre-delta code at b6bd6e2, not from a mutation run. I did not execute the reverse-order nesting case in the fixture; that comes from tracing dedupeNestedPaths and resolveWalkedRoots, so is the ordering guarantee actually order-independent, or only order-independent for the cases the current fixtures exercise? The live-evidence gaps from the b6bd6e2 review (macOS or physical-device reuse through the daemon, digest cost on the event loop) are unchanged, and this delta does not touch them. The b6bd6e2 review's symlinked AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH example looks wrong on closer reading, since the cache root is canonicalised with realpath before containment is checked; fail-closed launch stays reachable through real escapes, unreadable subtrees, files over 128 MB, or sockets. Not blocking: the delta only dedupes roots kept earlier in lexical order rather than deduping the fully-resolved set (packages/platform-apple/src/runner/runner-artifact-manifest.ts#L178), the test fixtures re-export digestFile as digest (packages/platform-apple/src/runner/tests/runner-cache.fixtures.ts#L59), and the PR body says duplicated roots refuse certification when the code actually collapses them to one walk — all take-it-or-leave-it. Smoke Tests, Repo Guards, and Coverage were still running at review time; Smoke exercises this delta directly through ensureXctestrunArtifact -> tryReuseExistingXctestrun and buildRunnerXctestrun, so a failure there counts against this PR. The open question from b6bd6e2 is still unanswered: should a fresh build fail to launch when the manifest writer refuses, and if so, where does prepare ios-runner help say so? Before merge, that question needs an answer and Smoke Tests needs to finish green on the ensureXctestrunArtifact route. |
…fail-closed launch Reading the delta back, `resolveWalkedRoots` collapsed duplicates only against roots kept earlier in lexical order, so a descendant spelled first still walked its subtree twice. Dedupe now runs once over the fully resolved set: sort, then drop equal or nested candidates, which is the same kept set whatever order the caller used. The lexical pre-pass is gone with it, since it could drop a path whose lexical ancestor resolves somewhere else. The review's open question -- may a fresh build fail to launch when the writer refuses the tree -- is answered yes, and `prepare ios-runner` now says so: the refusal is the point, and the hint names the tree to replace.
|
Answering the open questions at 5086a95. Fail-closed is intentional, and now documented. A fresh build that cannot be certified fails with Split: the two pieces are load-bearing, not bundled. The recipe owner matters because the manifest certifies bytes, while the metadata half claims which build produced them. With the script and the writer emitting settings from two owners, a manifest could honestly describe bytes that a different recipe produced, and the identity keys would still agree — the digest proves "these are the files", not "these are the files this recipe builds". Digest memo: declining on correctness, not cost. A same-process Evidence gaps you listed, now filled.
The two edge cases you called unreachable are fixed and pinned: equal canonical roots from different spellings now collapse through |
|
Reviewed at 5086a95; the code is ready for human review. The prior evidence-pending note is resolved: the macOS reuse run satisfies the daemon-reuse ask, since macOS is one of the "macOS or physical-device" options; physical-iOS-device reuse is still unexercised, but that was always the "or" case. Does the new descendant-first test at packages/platform-apple/src/runner/tests/runner-artifact-manifest.test.ts#L243 actually pin the fix? It only uses direct paths with no symlink alias, so it can't distinguish sort-before-resolve from sort-after-resolve, which is the exact ordering the prior review flagged. Adding a symlink inside the derived path to Debug-iphonesimulator whose name sorts after Runner.app, listed after the direct Runner.app path, and asserting reuse_ready, would close that gap. I did not run the tests myself; that reverting the sort fails the new test is traced from the code, not confirmed by a mutation run. The live runs are author-reported in the PR body and comment with no attached artifact, so I can't confirm the macOS run was against 5086a95 rather than 7125529; the author says the tamper probes ran at this head. The digest cost (13-18 ms warm) is also author-measured only. Smoke Tests is still running, and it exercises the changed resolveWalkedRoots on both the writer and reader routes (ensureXctestrunArtifact -> buildRunnerXctestrun/writeRunnerCacheMetadataForArtifacts -> buildRunnerCacheArtifactManifest -> resolveWalkedRoots on write, evaluateExistingXctestrun -> validateRunnerCacheArtifactManifest -> resolveWalkedRoots on reuse), so a failure there counts against this change. All other checks pass. Smoke Tests needs to finish green on 5086a95 before this can merge. Not blocking: the symlink-ordering test gap above can be closed now or left for a follow-up, your call. |
thymikee
added
the
ready-for-human
1 participant
Summary
Hardens how an Apple runner build is authorized for reuse. No CI fast path is enabled; Xcode still verifies every build.
mtime— a signature an in-place binary patch survives. The writer now digests every file under the cache root, plus modes and symlink targets; the reader walks the same tree and refuses the first disagreement..xctestrunor an unreadable subtree is a typed refusal; duplicate and nested roots collapse to one walk whatever the spelling order.restore-keycache kind are gone..pbxprojused to reuse a stale runner; everything in an Xcode package is now build input except user state.xcodebuildechoed. Schema is 3; older caches rebuild once.Certification is fail-closed by design: an uncertifiable tree fails the build naming the path rather than launching bytes no manifest vouches for. Documented under
prepare ios-runner.Validation
check:affected --run, build, lint, typecheck, layering and eager-closure budgets green; 665 apple-runner tests, each fix non-vacuous by reverting it. Daemon on Xcode 26.2: macOS device-kindbuilt_new→reuse_readyover 58 files and 21 framework symlinks. On a fresh iOS tree:digest_mismatchfor a one-byte flip at unchanged size and mtime,symlink_escapes_cachenaming the target. Digest 13–18 ms warm.