fix(ios): drop -Werror from runtime clang builds and cache the fold helper build - #2858
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
|
Reviewed at c4a71a5. The code looks sound, but I can't confirm this fixes the issue without a live run, so I'm holding off on approval until that evidence is in. CI is green on all 20 checks, and the ios.yml path filter now covers apple/fold-helper and foldable, so this PR's own changes exercised both -Werror gates. Live I didn't run the darwin -Werror gates locally and I'm relying on CI for that. I also didn't exercise the live fold route myself; only the fake-provider integration test covers it in this review. And I didn't check that apple/fold-helper/Fold.m ships in the npm package so host.projectRoot() resolves it in an installed build, though the old code used the same findProjectRoot, so that risk predates this PR. Gross diff is 1,123 lines, over the 1,000-line budget, though net production is 235. Dropping -Werror alone is two argv lines plus two unit assertions, and the cache extraction is the reuse the issue itself needs, since fold has to share the bridge's cache. Is there a smaller shape here, like landing the native-build-cache extraction as its own pure-refactor commit first, then a second commit for the fold-on-cache change plus the -Werror drop? Not blocking: you could also tighten timeout-policy.test.ts to assert the 220_000 envelope from the shipped timeout constants instead of literals (or just drop it since command-descriptor-timeout-policy.test.ts already pins fold at 240_000), rename or repoint native-build-cache.ts so its lock description isn't fixed to 'iOS Simulator snapshot bridge cache' when fold uses it, and fold the compile argv (not just source and toolchain) into the cache key in fold-helper-cache.ts so an argv-only change can't serve a stale binary — none of these need to hold up this PR. The next thing standing between this and merge is the live foldable-simulator run showing the cache hit on the second |
c4a71a5 to
7421ec8
Compare
|
Reviewed at 7421ec8. The conflict from the c4a71a5 review is resolved, and I found no code defect in the delta. One validation gap holds this. Only the fake-provider integration test shows that Not blocking: the new sum-of-literals test in Smoke Tests was still queued, so there is no CI result to attribute yet. |
|
Rebased onto main CI on On the requested live run, stating the gap explicitly: no foldable device type is available on this host, so the two-call Not run by me: the live |
7421ec8 to
b46ed59
Compare
|
Addressed the review at c4a71a5 (the branch was since rebased to 7421ec8). New head b46ed59. 1. Live Duo run: not possible on this host. Only Xcode 26.2 (build 17C52) is installed. 2. History split as you suggested, on
Gross diff was 1,482 lines and is now 1,597. The growth is the notes below. The pure extraction can now be reviewed on its own. 3. Non-blocking notes, all taken:
4. Gate on b46ed59: |
Move the snapshot bridge's content+toolchain-keyed build cache (lock, atomic publish, manifest matching, source fingerprinting, and the budgeted xcrun exec) into native-build-cache.ts as generic, reusable primitives. The bridge's manifest shape, cache key derivation, and compile argv (including -Werror) are unchanged; this is a pure move.
…elper Runtime clang builds no longer use -Werror. A new SDK warning in the AX snapshot bridge or the fold helper can no longer break snapshot or fold on a user's machine. A darwin-only compile of each helper's exact production argv with -Werror appended keeps the warning gate in CI instead. The fold helper no longer recompiles Fold.m into a temp dir on every call: it now shares the bridge's content- and toolchain-keyed build cache (native-build-cache.ts) and is built once per source hash and Xcode toolchain, cached under ~/.agent-device/fold-helper. Both cache keys now also fold in the compile argv, so a compiler-flag or framework-list change that touches neither the source nor the toolchain cannot serve a stale binary. The cache's process-lock description is caller-supplied, since the bridge and the fold helper now share one lock implementation over two different resources. FOLD_REQUEST_TIMEOUT_MS rises from 210s to 240s to cover the new preparation budget; its comment lists each step's budget, mirrored in command-descriptor-timeout-policy.test.ts's pinned envelope. Help and docs describe the cache.
…I step ios.yml's clean-installed preparation step now also runs the fold helper's -Werror conformance test and watches apple/fold-helper and packages/platform-apple/src/foldable for changes, matching the existing bridge gate.
b46ed59 to
40e4fb2
Compare
Live
|
| Call | Daemon | clang exec | simctl spawn binary |
Result |
|---|---|---|---|---|
1. fold closed --debug |
fresh | yes, 892 ms | ~/.agent-device/fold-helper/4542d8be74c0854cef35046ba417e61c/fold-helper |
hinge 0°, LCD 466x678 |
2. fold open --debug |
same | none | same path | hinge 180°, LCD-1 669x951 |
3. fold half-open |
same, after clean:daemon and a new open |
none | same path | hinge 130°, LCD-1 |
4. fold closed --debug |
same, after clean:daemon and a new open |
none | same path | hinge 0°, LCD |
- Cache entry: written once.
fold-helperandmanifest.jsonboth have mtime 11:25:50 and were not touched by calls 2–4. - Manifest: the
compileArgvhas no-Werror,toolchain.xcodeisXcode 27.1\nBuild version 27A9269, and abinarySha256is recorded. - Exec sequence on a cache hit (from the session request log, call 2):
xcrun devicectl device info displays 105 ms
xcodebuild -version 82 ms
sw_vers -productVersion / -buildVersion 30 ms
uname -m 6 ms
xcrun simctl spawn <udid> ~/.agent-device/fold-helper/4542d8be…/fold-helper open 351 ms
xcrun devicectl device motion hinge-angle 5088 ms
xcrun devicectl device info displays 153 ms
Wall time was 7.3 s cold and 5.9 s warm. Most of that is the pre-existing ~5 s CoreDevice hinge read. A cache hit still runs the toolchain probe, about 120 ms warm. That is by design, since it is what notices a DEVELOPER_DIR switch.
This closes the live-evidence gap from the earlier reviews. I closed the session afterwards, and device status shows no claims.
Simplification review
There are no blocking correctness issues. I checked the lock, the atomic publish, the temp dir cleanup on failure, and the sha256 recheck on a cache hit. One small bug and several simplifications follow. Together the simplifications would bring the PR back under the 1,000-line budget.
-
Error wrapping drops typed details (bug).
asFoldHelperCacheError(fold-helper-cache.ts:189) replaces everySnapshotSourceError's details with a fixed{reason, hint, cause}. That loses:- the specific
native-build-stalledhint and itstimeoutMs; - the
toolchain-probe-failedstderrandcommand; - the
native-source-missingfilePath.
Its doc comment also says "every cache failure", but a lock timeout (
AppError) and fs errors pass through unwrapped. Suggested fix: one factory used by both throw sites, which also removes the duplicated message and hint:function foldHelperBuildFailed(details: Record<string, unknown>, cause?: unknown) { return new AppError('COMMAND_FAILED', 'Unable to build the simulator fold helper', { hint: FOLD_HELPER_HINT, ...details, reason: 'fold-helper-build-failed' }, cause); }
- the specific
-
Manifest-field matching repeats the cache key (about 150 lines).
- Why it's redundant: the entry directory is named by
cacheKey, andcacheKeyalready hashes every field in bothMANIFEST_FIELDSlists. So oncemanifest.cacheKey === cacheKey, comparing each field again only rebuilds the key. - Change: have
ensureNativeBuildCacheEntrytake the key inputs, derive the key itself, write{...keyInputs, cacheKey, binarySha256}, and count a hit only whencacheKeymatches and abinarySha256is present. - What this deletes:
nativeBuildManifestFieldsMatch,describesReusableEntry, and themanifest/manifestMatchesparams;- both
MANIFEST_FIELDSlists,snapshotBridgeCacheKeyandfoldHelperCacheKey; - the "argv change changes the key" tests, which only prove that sha256 of different JSON differs.
- Behavior: identical short of a 128-bit hash collision.
- Why it's redundant: the entry directory is named by
-
simulator-hid.test.tstests the cache a fourth time through exec fakes (about 90 lines). Mock./fold-helper-cache.tsinstead, the same waypose.test.tsmockssimulator-hid. That also removes the test-onlyoptions.cacheRootparameter onsendSimulatorFoldPose. Move the "clang exit 1 →fold-helper-build-failed" case intofold-helper-cache.test.ts; today it only covers the stall. -
Assert that the argv has no
-Werrordirectly (about 30 lines). For example,assert.ok(!buildSnapshotBridgeCompileArgv({...}).includes('-Werror'))replaces the exec-recording host incache.test.ts. -
In the darwin gates, append
-Werrorinstead of splicing it in after-Wextra(about 12 lines).[...argv, '-Werror']does the same job, since clang doesn't care where the flag sits. -
Drop
execNativeBuildClang'sdeadlineReasonparam. Every step around the compile already uses'native-build-deadline', so fold's separate'fold-helper-build-deadline'is inconsistent. You could also derivelockDescriptionfromlabel.
Worth keeping as is:
- the placeholder-argv key: it's what catches a change to
SNAPSHOT_BRIDGE_COMPILE_FILENAMES, whichsourceHashdoesn't cover; - the
HostToolchainIdentitysplit; - the
runoverride increateFoldHelperCacheHost; - the timeout plumbing;
- the location of
native-build-cache.ts: moving it would mean renaming theSnapshotSource*host, deadline and error types too, which is follow-up material.
Once (1) is fixed, this is fine to merge. (2) to (6) are optional, but they would roughly halve the insertions.
|
Reviewed at 40e4fb2. The -Werror drop and the fold helper build cache look right, and the shared native build cache keeps the snapshot bridge lock, verify and publish steps unchanged. Your iPhone Duo run on this head answers the earlier evidence question: the first fold builds the helper and later folds reuse it without clang. Not blocking: Smoke Tests, Repo Guards and Coverage were still running. They exercise the changed snapshot-source and foldable code, so a failure there is likely related to this diff. |
thymikee
added
the
ready-for-human
…fold build error details ensureNativeBuildCacheEntry now takes the build's key inputs, derives the cache key itself, and returns it. A hit only needs the manifest's cacheKey and binary hash to match, because the key already hashes every field the per-caller MANIFEST_FIELDS lists compared again. That removes nativeBuildManifestFieldsMatch, both field lists, and the snapshotBridgeCacheKey/foldHelperCacheKey wrappers. Keys are byte-identical to before, so existing cache entries still hit. A fold helper cache failure now keeps the underlying failure's hint and typed details (the native-build-stalled hint and timeoutMs, toolchain-probe stderr, a missing source's filePath) instead of replacing them with a fixed hint. Tests: simulator-hid.test.ts mocks the fold helper cache instead of retesting it through exec fakes, which drops sendSimulatorFoldPose's test-only cacheRoot option. The -Werror absence checks assert the argv directly, and the darwin gates append -Werror instead of splicing it in. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
Applied the review above in
The diff is now 896+/354− (1,250 gross), down from about 1,600. Checks on
|
|
Reviewed at cf4d073. The delta looks good: a fold helper cache failure now keeps the underlying hint and typed details (such as Not blocking: a fold helper deadline now reports the cause Smoke Tests was still running at review time. This PR changes the iOS snapshot bridge build cache, which iOS smoke can reach on a cold cache, so a failure there needs a look at its log before we call it unrelated. |
|
Looked at the Smoke Tests failure on cf4d073 (job 107578273158), as the review asked. It does not trace to the bridge build cache.
I reran the failed job once. The non-blocking note: I'm keeping |
FOLD_REQUEST_TIMEOUT_MS was a literal whose basis lived only in a comment, and that comment kept drifting from the route: it first cited one 30s helper build and four 20s hinge reads, missing two 5s display-inventory reads and the HID-dispatch guard/grace, then #2858 hand-summed the route again for its own preparation-phase change and landed a fresh hand-summed figure with the same failure mode. Add a ledger test that drives fold through the public client and daemon against a fake Apple tool provider recording each call's timeoutMs, including the fold-helper cache's toolchain probes and build. A calibration run learns the route's hinge-settle-attempt count from an oscillating angle that never settles; a measured run uses the same oscillation but lands the last allowed read on target, so it succeeds after making exactly as many reads. Summing every call's timeout (minus 1ms, plus any kill grace) gives the route's worst case that still succeeds, asserted against the resolved command envelope with the usual 30s daemon-result margin. The test imports no platform-apple step figure other than MAX_FOLD_DURATION_MS, so it keeps proving the bound however the route's steps change next. Widen FOLD_REQUEST_TIMEOUT_MS to 255_000 (the smallest 5s multiple at least ledger + margin) and point both the constant's comment and the descriptor-timeout-policy pinning test at the ledger test instead of a hand-summed figure.
FOLD_REQUEST_TIMEOUT_MS was a literal whose basis lived only in a comment, and that comment kept drifting from the route: it first cited one 30s helper build and four 20s hinge reads, missing two 5s display-inventory reads and the HID-dispatch guard/grace, then #2858 hand-summed the route again for its own preparation-phase change and landed a fresh hand-summed figure with the same failure mode. Add a ledger test that drives fold through the public client and daemon against a fake Apple tool provider recording each call's timeoutMs, including the fold-helper cache's toolchain probes and build. A calibration run learns the route's hinge-settle-attempt count from an oscillating angle that never settles; a measured run uses the same oscillation but lands the last allowed read on target, so it succeeds after making exactly as many reads. Summing every call's timeout (minus 1ms, plus any kill grace) gives the route's worst case that still succeeds, asserted against the resolved command envelope with the usual 30s daemon-result margin. The test imports no platform-apple step figure other than MAX_FOLD_DURATION_MS, so it keeps proving the bound however the route's steps change next. Widen FOLD_REQUEST_TIMEOUT_MS to 255_000 (the smallest 5s multiple at least ledger + margin) and point both the constant's comment and the descriptor-timeout-policy pinning test at the ledger test instead of a hand-summed figure.
…2916) * fix(ios): derive the fold request envelope from a worst-case ledger test FOLD_REQUEST_TIMEOUT_MS was a literal whose basis lived only in a comment, and that comment kept drifting from the route: it first cited one 30s helper build and four 20s hinge reads, missing two 5s display-inventory reads and the HID-dispatch guard/grace, then #2858 hand-summed the route again for its own preparation-phase change and landed a fresh hand-summed figure with the same failure mode. Add a ledger test that drives fold through the public client and daemon against a fake Apple tool provider recording each call's timeoutMs, including the fold-helper cache's toolchain probes and build. A calibration run learns the route's hinge-settle-attempt count from an oscillating angle that never settles; a measured run uses the same oscillation but lands the last allowed read on target, so it succeeds after making exactly as many reads. Summing every call's timeout (minus 1ms, plus any kill grace) gives the route's worst case that still succeeds, asserted against the resolved command envelope with the usual 30s daemon-result margin. The test imports no platform-apple step figure other than MAX_FOLD_DURATION_MS, so it keeps proving the bound however the route's steps change next. Widen FOLD_REQUEST_TIMEOUT_MS to 255_000 (the smallest 5s multiple at least ledger + margin) and point both the constant's comment and the descriptor-timeout-policy pinning test at the ledger test instead of a hand-summed figure. * fix(ios): keep a daemon-result margin over prepare's default runner budget readPrepareIosRunnerTimeoutMs's fallback and the client envelope both resolved to PREPARE_REQUEST_TIMEOUT_MS (240_000), so with no --timeout the daemon-side runner budget and the client envelope were identical: a slow cold runner build ended in a client-side timeout and daemon reset instead of the daemon's own typed runner_phase_budget result. Every other bounded command keeps a 30s margin between its daemon-side budget and its envelope; prepare's explicit-timeout case already widened past its own base by that same margin (widenToUserBudget), only the default case had none. Split the daemon-side runner budget out as PREPARE_STARTUP_BUDGET_MS (240_000, unchanged) and derive PREPARE_REQUEST_TIMEOUT_MS from it plus the margin (270_000). The handler falls back to the startup budget, not the now-wider request timeout. Add a handler-level test that calls handlePrepareCommand directly with a fake runner binding recording the timeoutMs it receives, then checks that value plus the margin against the same request's resolved client envelope for the no-flag, --timeout above default, and --timeout below default cases - the rule the fix proves, not just the constants it compiles to. * fix(ios): fail the fold ledger test on an unbounded provider call The fold worst-case ledger fake defaulted an unset timeoutMs to 0, so a provider call reaching it with no bound cost 0 virtual ms instead of exposing the true unbounded worst case. Its runCommand handler also skipped `open -a Simulator` before recording, dropping any such call from the ledger with no error. Build the ledger on the shared recording provider and assert every call carries a finite, positive timeoutMs, so an unbounded call fails the test instead of passing silently. * test(ios): give the fold ledger test a cold-import budget The ledger test runs two cold folds through a fresh daemon each. After a platform-apple source change the first import is untransformed, and the test ran past vitest's 5 s default, which hid the ledger printout behind a timeout. Match the provider-scenario precedent of an explicit budget.
1 participant
Summary
Runtime clang builds no longer use
-Werror. A new SDK warning in the AX snapshot bridge or the fold helper can no longer breaksnapshotorfoldon users' machines. Warnings still fail CI: a darwin-only test compiles each helper's exact production argv with-Werror..github/workflows/ios.ymlnow runs both tests, and its path filter coversapple/fold-helperandpackages/platform-apple/src/foldable.The fold helper no longer recompiles
Fold.minto a temp dir on every call. It is built once per source hash and Xcode toolchain, and cached under~/.agent-device/fold-helper. The bridge and fold now share one cache implementation (native-build-cache.ts).FOLD_REQUEST_TIMEOUT_MSrises from 210 s to 240 s to cover the new preparation budget; its comment lists each step's budget. Help and docs describe the cache.Closes #2796. 19 files, 896 insertions and 354 deletions (1,250 gross, over the 1,000-line budget). Most of it is extracting the shared cache from the bridge. The history is split into extract, fix, CI gate, and a simplification commit.
Validation
b4ccf4b00e:pnpm check:affected --rungreen (all 59 checks, including both-Werrorcompile tests);check:production-exportsgreen.161df5d409: build, typecheck and 157 targeted tests green.9fac266b59(CHANGELOG-only conflict; the PR's own diff is unchanged fromc4a71a5a20apart from hunk offsets). Head7421ec8322:pnpm install --frozen-lockfile,pnpm build,pnpm format(no changes),pnpm lint,pnpm typecheckandpnpm check:xctest-selectiongreen; 157 targetedunit-coretests oversnapshot-source,foldableand both timeout-policy tests green, plus theprovider-integrationios-foldscenario.~/.agent-device/fold-helper: the first call builtfold-helper/4542d8be…/fold-helper. Later calls, including calls after daemon restarts, made no clang exec and ransimctl spawnon the cached binary. Hinge poses 0°, 180° and 130° were verified. Headcf4d07353produced the same cache key and still hit the cache.cf4d07353:pnpm check:affected --rungreen; typecheck, lint and format green. 157 targeted tests green, including both darwin-Werrorgates and theios-foldprovider scenario. As a mutation check, an unused variable added toFold.mfailed the gate.