fix(apple-runner): trim build scratch from runner cache keys and keep packaging out of the shared cache - #3248
janicduplessis wants to merge 5 commits into
Conversation
|
The code in a790173 looks good and I found nothing that blocks it. Live evidence is still pending: the iOS simulator run (trim, then launch and reuse_ready) comes from the PR body, and I did not re-run it. The macOS and physical-device runner routes were not run by the author, and I did not check whether Not blocking, so take or leave these. On simplicity, the trim fits at the cache seam: it sits beside All 16 checks pass and there are no conflicts. The PR is still a draft, so the author needs to mark it ready for review. After this PR or #3247 merges, the other must rebase over the textual conflicts in |
There was a problem hiding this comment.
Copilot review overview
🟢 Approval recommended
The production changes are coherent and well covered; the remaining test-environment restoration finding is non-blocking.
Review effort: Balanced
Findings: 1
What changed in this PR
Adds post-build Apple runner cache trimming and isolates package-time XCTest builds from the shared cache.
Changes:
- Retains only certified runner products and metadata in keyed caches.
- Builds package XCTest targets in disposable repository-local scratch space.
- Documents cleanup behavior, configuration, and legacy caches.
| File | Description |
|---|---|
website/docs/docs/configuration.md |
Documents the trim environment variable. |
website/docs/docs/commands.md |
Explains trimming and legacy cache cleanup. |
vitest.config.ts |
Adds the packaging test to the fast suite. |
src/__tests__/npm-package-scripts.test.ts |
Updates package-build expectations. |
scripts/build-package-xcuitest.mjs |
Builds XCTest targets in temporary storage. |
scripts/__tests__/build-package-xcuitest.test.ts |
Tests scratch cleanup and failure handling. |
packages/platform-apple/src/runner/runner-cache.ts |
Adds the trim diagnostic reason. |
packages/platform-apple/src/runner/runner-cache-trim.ts |
Implements safe cache-key trimming. |
packages/platform-apple/src/runner/runner-artifact.ts |
Invokes trimming after cache certification. |
packages/platform-apple/src/runner/__tests__/runner-cache-trim.test.ts |
Tests trimming and reuse behavior. |
package.json |
Routes package builds through the isolated script. |
CONTRIBUTING.md |
Documents package-build scratch isolation. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
4 issues 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. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/platform-apple/src/runner/runner-artifact.ts">
<violation number="1" location="packages/platform-apple/src/runner/runner-artifact.ts:395">
P2: This trim is uninterruptible and unbounded while holding the per-key cache lock. The `catch {}` swallows every error, including `createRequestCanceledError`, and `trimRunnerBuildScratch` has no deadline or signal of its own, so a canceled request waits out the full recursive delete (potentially hundreds of MB of scratch) before the phase returns — and the cache process lock is held for the whole abort. The PR treats trim failures as non-fatal, but cancellation should still be honored: check the request signal around the removal loop (and before the lock-hold work) instead of absorbing all errors indiscriminately.</violation>
</file>
<file name="packages/platform-apple/src/runner/__tests__/runner-cache-trim.test.ts">
<violation number="1" location="packages/platform-apple/src/runner/__tests__/runner-cache-trim.test.ts:121">
P2: `elsewhere.app` does not exist, so `realpathSync` fails before the trim checks whether a real product is outside the cache. Create an existing outside product to exercise this safety guard.</violation>
</file>
<file name="packages/platform-apple/src/runner/runner-cache-trim.ts">
<violation number="1" location="packages/platform-apple/src/runner/runner-cache-trim.ts:40">
P1: A keyed `derived` path can itself be a symlink; resolving it here does not stop `trimDirectory` from deleting unkept entries in the symlink target. Refuse to trim when `lstat(derived)` reports a symlink.</violation>
</file>
<file name="website/docs/docs/commands.md">
<violation number="1" location="website/docs/docs/commands.md:283">
P3: The trimmed-size claim "a key goes from about 160-230 MB to about 5 MB" generalizes a range that was never measured: the only validated run in this PR is a single iOS simulator key going 165.7 MB to 5.9 MB, and the PR description explicitly states macOS, tvOS, visionOS, and physical-device runs were not performed. Reword to state the observed single data point, or soften the range, so the docs don't assert unvalidated numbers.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
| function resolveKeptPaths(derived: string, protectedPaths: readonly string[]): Set<string> | null { | ||
| const kept = new Set<string>([RUNNER_CACHE_METADATA_FILE]); | ||
| try { | ||
| const realDerived = fs.realpathSync(derived); |
There was a problem hiding this comment.
P1: A keyed derived path can itself be a symlink; resolving it here does not stop trimDirectory from deleting unkept entries in the symlink target. Refuse to trim when lstat(derived) reports a symlink.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/platform-apple/src/runner/runner-cache-trim.ts, line 40:
<comment>A keyed `derived` path can itself be a symlink; resolving it here does not stop `trimDirectory` from deleting unkept entries in the symlink target. Refuse to trim when `lstat(derived)` reports a symlink.</comment>
<file context>
@@ -0,0 +1,84 @@
+function resolveKeptPaths(derived: string, protectedPaths: readonly string[]): Set<string> | null {
+ const kept = new Set<string>([RUNNER_CACHE_METADATA_FILE]);
+ try {
+ const realDerived = fs.realpathSync(derived);
+ for (const protectedPath of protectedPaths) {
+ const lexical = path.relative(derived, protectedPath);
</file context>
| const removed = await trimRunnerBuildScratch(derived, protectedPaths); | ||
| if (removed.length > 0) | ||
| emitRunnerXctestrunDecision('clean', 'build_scratch_trimmed', { derived }); | ||
| } catch {} |
There was a problem hiding this comment.
P2: This trim is uninterruptible and unbounded while holding the per-key cache lock. The catch {} swallows every error, including createRequestCanceledError, and trimRunnerBuildScratch has no deadline or signal of its own, so a canceled request waits out the full recursive delete (potentially hundreds of MB of scratch) before the phase returns — and the cache process lock is held for the whole abort. The PR treats trim failures as non-fatal, but cancellation should still be honored: check the request signal around the removal loop (and before the lock-hold work) instead of absorbing all errors indiscriminately.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/platform-apple/src/runner/runner-artifact.ts, line 395:
<comment>This trim is uninterruptible and unbounded while holding the per-key cache lock. The `catch {}` swallows every error, including `createRequestCanceledError`, and `trimRunnerBuildScratch` has no deadline or signal of its own, so a canceled request waits out the full recursive delete (potentially hundreds of MB of scratch) before the phase returns — and the cache process lock is held for the whole abort. The PR treats trim failures as non-fatal, but cancellation should still be honored: check the request signal around the removal loop (and before the lock-hold work) instead of absorbing all errors indiscriminately.</comment>
<file context>
@@ -381,6 +382,19 @@ async function buildXctestrunArtifact(params: {
+ const removed = await trimRunnerBuildScratch(derived, protectedPaths);
+ if (removed.length > 0)
+ emitRunnerXctestrunDecision('clean', 'build_scratch_trimmed', { derived });
+ } catch {}
+}
+
</file context>
| const before = tree(derived); | ||
|
|
||
| assert.deepEqual( | ||
| await trimRunnerBuildScratch(derived, [path.join(base, 'elsewhere.app')], {}), |
There was a problem hiding this comment.
P2: elsewhere.app does not exist, so realpathSync fails before the trim checks whether a real product is outside the cache. Create an existing outside product to exercise this safety guard.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/platform-apple/src/runner/__tests__/runner-cache-trim.test.ts, line 121:
<comment>`elsewhere.app` does not exist, so `realpathSync` fails before the trim checks whether a real product is outside the cache. Create an existing outside product to exercise this safety guard.</comment>
<file context>
@@ -0,0 +1,194 @@
+ const before = tree(derived);
+
+ assert.deepEqual(
+ await trimRunnerBuildScratch(derived, [path.join(base, 'elsewhere.app')], {}),
+ [],
+ );
</file context>
| await trimRunnerBuildScratch(derived, [path.join(base, 'elsewhere.app')], {}), | |
| await trimRunnerBuildScratch(derived, [fs.mkdtempSync(path.join(base, 'elsewhere.app-'))], {}), |
| - If health checking exposes a bad restored runner artifact, Agent Device marks that artifact bad and rebuilds once. | ||
| - If a fresh runner launch gets stuck before accepting connections, Agent Device invalidates that runner session and launches it once more without forcing a rebuild. | ||
| - CI may cache `~/.agent-device/apple-runner/derived` when the cache key includes the exact Agent Device package contents and selected Xcode version. | ||
| - After a successful build, Agent Device removes the build scratch from the new cache key (intermediates, precompiled and module caches, logs) and keeps `Build/Products` and the metadata file, which is all reuse and launch read; a key goes from about 160-230 MB to about 5 MB. A key that fails certification is rebuilt from scratch either way, and a runner source or Xcode change mints a new key, so the scratch is never used again. `AGENT_DEVICE_IOS_RUNNER_CACHE_TRIM=0` keeps it. A set `AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH` is never trimmed, because a fixed path rebuilds into the same tree incrementally. |
There was a problem hiding this comment.
P3: The trimmed-size claim "a key goes from about 160-230 MB to about 5 MB" generalizes a range that was never measured: the only validated run in this PR is a single iOS simulator key going 165.7 MB to 5.9 MB, and the PR description explicitly states macOS, tvOS, visionOS, and physical-device runs were not performed. Reword to state the observed single data point, or soften the range, so the docs don't assert unvalidated numbers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At website/docs/docs/commands.md, line 283:
<comment>The trimmed-size claim "a key goes from about 160-230 MB to about 5 MB" generalizes a range that was never measured: the only validated run in this PR is a single iOS simulator key going 165.7 MB to 5.9 MB, and the PR description explicitly states macOS, tvOS, visionOS, and physical-device runs were not performed. Reword to state the observed single data point, or soften the range, so the docs don't assert unvalidated numbers.</comment>
<file context>
@@ -280,6 +280,8 @@ agent-device prepare ios-runner --platform ios --timeout 240000
- If health checking exposes a bad restored runner artifact, Agent Device marks that artifact bad and rebuilds once.
- If a fresh runner launch gets stuck before accepting connections, Agent Device invalidates that runner session and launches it once more without forcing a rebuild.
- CI may cache `~/.agent-device/apple-runner/derived` when the cache key includes the exact Agent Device package contents and selected Xcode version.
+- After a successful build, Agent Device removes the build scratch from the new cache key (intermediates, precompiled and module caches, logs) and keeps `Build/Products` and the metadata file, which is all reuse and launch read; a key goes from about 160-230 MB to about 5 MB. A key that fails certification is rebuilt from scratch either way, and a runner source or Xcode change mints a new key, so the scratch is never used again. `AGENT_DEVICE_IOS_RUNNER_CACHE_TRIM=0` keeps it. A set `AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH` is never trimmed, because a fixed path rebuilds into the same tree incrementally.
+- Agent Device versions from before the Apple runner rename kept their cache in `~/.agent-device/ios-runner`. Current versions never read it, and it is not removed automatically because an older version installed on the same machine may still use it. Delete it with `rm -rf ~/.agent-device/ios-runner` once no old version runs.
- Runner reuse is authorized only by the cache metadata's content manifest: a restored tree whose files no longer match the recorded digests, modes, or symlink targets is discarded and rebuilt. A cache key must stay exact — the runtime never falls back to a broader cache.
</file context>
| - After a successful build, Agent Device removes the build scratch from the new cache key (intermediates, precompiled and module caches, logs) and keeps `Build/Products` and the metadata file, which is all reuse and launch read; a key goes from about 160-230 MB to about 5 MB. A key that fails certification is rebuilt from scratch either way, and a runner source or Xcode change mints a new key, so the scratch is never used again. `AGENT_DEVICE_IOS_RUNNER_CACHE_TRIM=0` keeps it. A set `AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH` is never trimmed, because a fixed path rebuilds into the same tree incrementally. | |
| - After a successful build, Agent Device removes the build scratch from the new cache key (intermediates, precompiled and module caches, logs) and keeps `Build/Products` and the metadata file, which is all reuse and launch read; a reported iOS simulator key went from 165.7 MB to 5.9 MB. A key that fails certification is rebuilt from scratch either way, and a runner source or Xcode change mints a new key, so the scratch is never used again. `AGENT_DEVICE_IOS_RUNNER_CACHE_TRIM=0` keeps it. A set `AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH` is never trimmed, because a fixed path rebuilds into the same tree incrementally. |
…ailures, drop the trim opt-out
|
Pushed e88a58f. Evidence first, then the changes.
|
| // An AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH override is a fixed tree rebuilt incrementally, not a keyed cache. | ||
| if (path.basename(derived) === resolveRunnerCacheKey(expectedCacheMetadata)) { | ||
| await trimRunnerBuildScratchBestEffort(derived, [built, ...builtProductPaths]); | ||
| } |
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/platform-apple/src/runner/runner-artifact.ts">
<violation number="1" location="packages/platform-apple/src/runner/runner-artifact.ts:370">
P2: This basename check also trims an explicit `AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH` override when its final component matches the generated cache key. Skip trimming whenever the override is set; only the default resolver produces a keyed cache path.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
| derived, | ||
| ); | ||
| // An AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH override is a fixed tree rebuilt incrementally, not a keyed cache. | ||
| if (path.basename(derived) === resolveRunnerCacheKey(expectedCacheMetadata)) { |
There was a problem hiding this comment.
P2: This basename check also trims an explicit AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH override when its final component matches the generated cache key. Skip trimming whenever the override is set; only the default resolver produces a keyed cache path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At packages/platform-apple/src/runner/runner-artifact.ts, line 370:
<comment>This basename check also trims an explicit `AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH` override when its final component matches the generated cache key. Skip trimming whenever the override is set; only the default resolver produces a keyed cache path.</comment>
<file context>
@@ -365,7 +366,10 @@ async function buildXctestrunArtifact(params: {
);
- await trimRunnerBuildScratchBestEffort(derived, [built, ...builtProductPaths]);
+ // An AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH override is a fixed tree rebuilt incrementally, not a keyed cache.
+ if (path.basename(derived) === resolveRunnerCacheKey(expectedCacheMetadata)) {
+ await trimRunnerBuildScratchBestEffort(derived, [built, ...builtProductPaths]);
+ }
</file context>
| if (path.basename(derived) === resolveRunnerCacheKey(expectedCacheMetadata)) { | |
| if (!process.env.AGENT_DEVICE_IOS_RUNNER_DERIVED_PATH?.trim()) { |
|
The cache-key trim in e88a58f looks correct to me, and it fixes what I raised on a790173: the opt-out env var and the hook that deleted it are gone. All 17 checks pass and there are no conflicts, but three open inline threads listed below still apply and need a fix before merge. I did not re-run pnpm package:npm, the iOS simulator launch after a trim, or the macOS trim plus reuse_ready run, so those rely on your quoted output. No macOS runner launch from a trimmed key was observed: the host hung, and an untrimmed control key hung the same way. I ran no physical device, tvOS, or visionOS launch after a trim, so a read outside Build/Products after launch on those routes is not ruled out, although the xctestrun ProductPaths are TESTROOT-relative. I also did not run the test suite, so I judged the override test from reading the code. A macOS launch from a trimmed key, on a host with no other runner session, would close the last evidence gap. Not blocking: you can take or leave the small duplication where Of the open inline threads, these still apply: the override-basename trim in runner-artifact.ts (#3248 (comment), also #3248 (comment)), the symlinked derived path (#3248 (comment)), and the outside-product test that never reaches the check (#3248 (comment)). One lower-priority docs thread also still applies. The two env var threads (#3248 (comment) and #3248 (comment)) are fixed at e88a58f, so you can resolve them. The cancellation thread (#3248 (comment)) does not apply: the trim never reads the request signal, and it runs only after a successful build under the same lock. |
Summary
Follow-up to #3246, covering the three items it lists after the eviction PR. This PR is independent of #3247: it touches
buildXctestrunArtifactrather thanensureXctestrunArtifact. Merging the two textually conflicts in two places, both trivial: the decision-reason union inrunner-cache.ts(each adds one member) and the Apple runner setup row inconfiguration.md(each adds one env var). Whichever lands second rebases.1. Trim build scratch from a runner cache key after its build. After the manifest is written, and still under the cache lock, the build removes everything in the key except
Build/Productsand.agent-device-runner-cache.json:Build/Intermediates.noindex,SDKExplicitPrecompiledModules,ModuleCache.noindex,Logs,CompilationCache.noindex,SDKStatCaches.noindex,SourcePackages,info.plist. Reuse and launch read only the products and the metadata. The kept set is derived from the.xctestrunand its product paths, resolved throughrealpath: a product outsideBuild/Productsis kept too, a symlinked product keeps its target's directory, and a product outside the key makes the trim a no-op.AGENT_DEVICE_IOS_RUNNER_DERIVED_PATHis never trimmed, and neither is a directory that is not acache-<hash>key. A fixed override path is the development loop that rebuilds into the same tree, and eviction already skips it.Trade-off: a trimmed key cannot be built into incrementally. Nothing in the daemon does that today. A source or Xcode change mints a new key (a cold build in either case), a key that fails certification is deleted and rebuilt from scratch (
cleanRunnerDerivedArtifacts), and only a key with no metadata is built into, which a trimmed key never is. So the default costs no rebuild time. The only incremental build left ispnpm build:xcuitest:*, which uses its own path and is not trimmed.2. Legacy
~/.agent-device/ios-runner. Documented, not deleted. Current versions never read it, but an older agent-device installed on the same machine still uses that layout, including leases next to it, and an automatic delete would pull products from under it.commands.mdsays torm -rf ~/.agent-device/ios-runneronce no old version runs.3.
pnpm build:package/prepackno longer write to the shared cache root.scripts/build-package-xcuitest.mjs(pnpm package:xcuitest, run bybuild:package) builds ios, macos, tvos and visionos withbuild-xcuitest-apple.shinto<repo>/.tmp/package-xcuitest/<platform>, and removes that directory before the builds (an interrupted run leaves nothing behind) and after them, also when a build fails. The script is not namedbuild:xcuitest:*becausesetup-apple-runner-buildhashes those script names into the CI runner cache key. Nothing reads those products (the package ships the runner source), so the compile check is unchanged. Packaging now always builds from scratch, so the isolation scan covers every file.pnpm build:xcuitest:<platform>is unchanged. npm users are unaffected: scripts are not in the tarball, and the daemon's keyed cache is not touched.Validation
Measured on macOS 27 / Xcode 27.0, an iOS 27.0 simulator created for the run, with
HOMEpointed at a scratch directory so the shared~/.agent-devicewas never touched.Build/ProductsBuild/Intermediates.noindexSDKExplicitPrecompiledModulesModuleCache.noindexprepare ios-runneron a fresh home built the key (about 10 s) and trimmed it; the runner then launched from the trimmed key andopen+snapshot -ireturned the Settings tree through the xctest backend. The trim itself took 66 ms for 165 MB.prepare ios-runnerloggedreuse_readyand no newbuilt_new: no rebuild.prepare ios-runnerbuilt a second key (about 8 s, trimmed to 5.9 MB) and left the first in place.build-xcuitest-apple.shin one directory takes 8.0 s cold and 3.6 s incrementally, which is what an in-place rebuild would save, and the daemon never takes that path.buildPackageXcuitest({ platforms: ['ios'] })built into.tmp/package-xcuitest, removed it, and left the sharedderived/untouched.Checks:
pnpm check:affected --run(the full set, becausepackage.jsonis workflow tooling) passes, includingcheck:fallow --base origin/main, the eager-closure and package-closure tests, and all ofpackages/platform-apple/src/runner.src/__tests__/npm-package-scripts.test.tsnow expectspackage:xcuitestinbuild:package. New tests cover the kept and removed entries, a build under a derived path override that keeps its scratch, a product outside the key, a symlinked product, a realensureXctestrunArtifactbuild (stubbedxcodebuild) followed by a reuse with no second build, and the packaging script's scratch cleanup on success, failure and after an interrupted run.Not run: macOS, tvOS and visionOS runners and a physical device from a trimmed key (the trim walks the same product paths for every platform, and the macOS product repair reads only those paths); the packaging script for macos, tvos and visionos;
pnpm package:npmend to end.