refactor(apple): give snapshot and fold one native-build owner - #3018
Conversation
Move the shared native-build cache, toolchain identity, and build host
out of snapshot-source into a domain-neutral packages/platform-apple/src
/native-build module group, so the fold helper no longer imports the
snapshot bridge's deadlines, errors, host construction, or host types
to compile a binary that needs none of them.
- native-build/{cache,deadline,host,toolchain-identity,errors}.ts carry
the one cache implementation, one lock implementation, and one
toolchain-identity read every runtime clang build in this package
shares (#2796, #2712); NativeBuildHost exposes only file access,
command execution, locking, and process identity, with no bridge
socket start/connect and no target-process inspection.
- NativeBuildError is domain-neutral (cancelled/timeout/unsupported);
no shared build module imports SnapshotSourceError or emits
bridge-specific error fields. snapshot-source/errors.ts and
deadline.ts map it onto SnapshotSourceError at the boundary, and
foldable/fold-helper-cache.ts maps it onto its own
fold-helper-build-failed AppError, preserving both public error
shapes byte for byte (see the moved and updated test suites).
- fold-helper-cache.ts drops its whole-snapshot-host construction
(building a full SnapshotSourceHost and overriding `run`) for
createNativeBuildHost(runAppleToolCommand), the narrow host the
compile actually needs.
- snapshot-source/host.ts drops its own copy of the lock-acquisition
implementation in favor of the shared one.
- cache-identity.ts keeps only the bridge's simulatorRuntime addition;
the toolchain retry/probe/cancellation logic it used to own moved to
native-build/toolchain-identity.ts with its test coverage.
Net production lines grow (+132 across the touched files, per
`git diff --numstat -M`) because achieving actual decoupling needs a
second, narrow error/deadline type and a translation boundary, not
because anything was merely relocated: the whole-snapshot-host
construction, the duplicate lock implementation, and the fold helper's
dependency on the bridge's deadline/error/host types are all deleted,
and NativeBuildHost's type now statically forbids a future build/cache
change from reaching back into bridge-only capabilities.
Live-validated on this Darwin host with the production code: a cache
miss then a cache-key-stable cache hit for both the snapshot bridge
(779ms build -> 127ms reuse) and the fold helper (2.9s build -> 2.2s
reuse, dominated by the unchanged per-call toolchain probe) via
ensureSnapshotBridgeBinary/ensureFoldHelperBinary directly. No Swift
changes.
![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 14 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/platform-apple/src/foldable/fold-helper-cache.ts">
<violation number="1" location="packages/platform-apple/src/foldable/fold-helper-cache.ts:141">
P2: Abort during fold-helper preparation now returns `NativeBuildError` unchanged, so callers lose `reason: 'request_canceled'` and `isRequestCanceledError` no longer recognizes the cancellation. Map native-build cancellation to the request-canceled error shape, including the deadline construction path.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
| if (!(error instanceof SnapshotSourceError) || error.failureKind === 'cancelled') return error; | ||
| const { bridgeFailure: _kind, bridgeFailureCode: cause, ...details } = error.details ?? {}; | ||
| return foldHelperBuildFailed({ ...details, cause }, error); | ||
| if (!(error instanceof NativeBuildError) || error.buildFailureKind === 'cancelled') return error; |
There was a problem hiding this comment.
P2: Abort during fold-helper preparation now returns NativeBuildError unchanged, so callers lose reason: 'request_canceled' and isRequestCanceledError no longer recognizes the cancellation. Map native-build cancellation to the request-canceled error shape, including the deadline construction path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/platform-apple/src/foldable/fold-helper-cache.ts, line 141:
<comment>Abort during fold-helper preparation now returns `NativeBuildError` unchanged, so callers lose `reason: 'request_canceled'` and `isRequestCanceledError` no longer recognizes the cancellation. Map native-build cancellation to the request-canceled error shape, including the deadline construction path.</comment>
<file context>
@@ -137,13 +133,13 @@ async function compileFoldHelper(
- if (!(error instanceof SnapshotSourceError) || error.failureKind === 'cancelled') return error;
- const { bridgeFailure: _kind, bridgeFailureCode: cause, ...details } = error.details ?? {};
- return foldHelperBuildFailed({ ...details, cause }, error);
+ if (!(error instanceof NativeBuildError) || error.buildFailureKind === 'cancelled') return error;
+ return foldHelperBuildFailed({ ...error.buildDetails, cause: error.buildFailureCode }, error);
}
</file context>
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed at 185da6d. The split changes how a cancelled fold is reported, so cancellation is no longer detected. On main, a fold-helper cancellation was a Could a smaller design do this? Not blocking: the test at cache-identity.test.ts:44 says "rejected before any probe runs" but asserts CI: Smoke Tests is still queued. The iOS run builds the snapshot bridge, so it exercises this change. |
Abort during fold-helper or toolchain-probe preparation lost reason: 'request_canceled' once native-build/cache errors stopped mapping onto SnapshotSourceError, so isRequestCanceledError no longer recognized a canceled fold or lock wait. NativeBuildError now stamps the same reason SnapshotSourceError already does; a lock wait with no abort signal now maps an expired deadline to a typed timeout instead of the raw lock error; and a mid-exec abort during a toolchain probe now maps through the module's cancelled contract instead of leaking the exec layer's raw cancellation. Also: fixes the cache-identity and toolchain-identity test title/ comment mismatches cubic flagged, relocates the shared exec-timeout test fixture out of snapshot-source so native-build's own tests stop depending on it, folds SnapshotSourceHost's file/exec/lock members into an intersection with NativeBuildHost instead of restating them, and extracts the toolchain-probe failure classification into its own function to keep runToolchainProbe under the complexity gate.
|
Fixed at baa48fa. Cancellation reason: Smaller design: I did the safe part of this — Not blocking, fixed anyway: the cache-identity test title said the blank-runtime rejection happens "before any probe runs" while the assertion (and the code) says the opposite — renamed it. The toolchain-identity retry-timeout comment claimed the retry was "charged the remainder" when the numbers show it got the full ceiling again — reworded to point at the case that actually shows the remainder. Moved the shared exec-timeout test fixture out of
CI: still finishing on the new commit; the earlier run was all green except one Smoke Tests job that was still queued when it was last checked, unrelated to this change. |
There was a problem hiding this comment.
2 issues found across 9 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. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/platform-apple/src/foldable/fold-helper-cache.test.ts">
<violation number="1" location="packages/platform-apple/src/foldable/fold-helper-cache.test.ts:203">
P3: The 50 ms sleep is the only mechanism aiming the abort at the lock-wait branch, and the test still passes when it misses: if the abort lands before the waiter reaches `acquireLock`, `remainingNativeBuildMs` or the aborted-signal checks in `createNativeBuildDeadline`/`acquireNativeBuildLock` reject with `NativeBuildError('cancelled', 'abort-signal')`, so `isRequestCanceledError` holds without exercising the lock-wait path this test is named for. That silently weakens the regression coverage the fix (lock-wait abort keeping `reason: 'request_canceled'`) needs. Signal from the waiter's lock acquisition instead of sleeping: wrap `host.acquireLock` in the waiter's host so it resolves a `lockWaitStarted` promise, then `await lockWaitStarted` before `controller.abort()`. The outcome is not flaky either way: every pre-lock abort path also yields a cancelled error, so the assertion itself is deterministic.</violation>
<violation number="2" location="packages/platform-apple/src/foldable/fold-helper-cache.test.ts:211">
P3: `releaseClang()` and `await holder` only run when the cancellation assertion passes. If the waiter rejects with a non-cancelled error, `assert.rejects` throws, the gate is never released, and the `holder` build stays pending forever while the `finally` already removes the cache root. Release the gate in a `finally` so the gated build always settles on every exit path.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
| return true; | ||
| }); | ||
|
|
||
| releaseClang(); |
There was a problem hiding this comment.
P3: releaseClang() and await holder only run when the cancellation assertion passes. If the waiter rejects with a non-cancelled error, assert.rejects throws, the gate is never released, and the holder build stays pending forever while the finally already removes the cache root. Release the gate in a finally so the gated build always settles on every exit path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/platform-apple/src/foldable/fold-helper-cache.test.ts, line 211:
<comment>`releaseClang()` and `await holder` only run when the cancellation assertion passes. If the waiter rejects with a non-cancelled error, `assert.rejects` throws, the gate is never released, and the `holder` build stays pending forever while the `finally` already removes the cache root. Release the gate in a `finally` so the gated build always settles on every exit path.</comment>
<file context>
@@ -156,6 +157,64 @@ test('a compile exec killed at its budget reports the fold-helper build, not the
+ return true;
+ });
+
+ releaseClang();
+ await holder;
+ } finally {
</file context>
| signal: controller.signal, | ||
| }); | ||
| // Give the waiter time to reach the lock's poll loop before aborting it. | ||
| await new Promise((resolve) => setTimeout(resolve, 50)); |
There was a problem hiding this comment.
P3: The 50 ms sleep is the only mechanism aiming the abort at the lock-wait branch, and the test still passes when it misses: if the abort lands before the waiter reaches acquireLock, remainingNativeBuildMs or the aborted-signal checks in createNativeBuildDeadline/acquireNativeBuildLock reject with NativeBuildError('cancelled', 'abort-signal'), so isRequestCanceledError holds without exercising the lock-wait path this test is named for. That silently weakens the regression coverage the fix (lock-wait abort keeping reason: 'request_canceled') needs. Signal from the waiter's lock acquisition instead of sleeping: wrap host.acquireLock in the waiter's host so it resolves a lockWaitStarted promise, then await lockWaitStarted before controller.abort(). The outcome is not flaky either way: every pre-lock abort path also yields a cancelled error, so the assertion itself is deterministic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/platform-apple/src/foldable/fold-helper-cache.test.ts, line 203:
<comment>The 50 ms sleep is the only mechanism aiming the abort at the lock-wait branch, and the test still passes when it misses: if the abort lands before the waiter reaches `acquireLock`, `remainingNativeBuildMs` or the aborted-signal checks in `createNativeBuildDeadline`/`acquireNativeBuildLock` reject with `NativeBuildError('cancelled', 'abort-signal')`, so `isRequestCanceledError` holds without exercising the lock-wait path this test is named for. That silently weakens the regression coverage the fix (lock-wait abort keeping `reason: 'request_canceled'`) needs. Signal from the waiter's lock acquisition instead of sleeping: wrap `host.acquireLock` in the waiter's host so it resolves a `lockWaitStarted` promise, then `await lockWaitStarted` before `controller.abort()`. The outcome is not flaky either way: every pre-lock abort path also yields a cancelled error, so the assertion itself is deterministic.</comment>
<file context>
@@ -156,6 +157,64 @@ test('a compile exec killed at its budget reports the fold-helper build, not the
+ signal: controller.signal,
+ });
+ // Give the waiter time to reach the lock's poll loop before aborting it.
+ await new Promise((resolve) => setTimeout(resolve, 50));
+ controller.abort();
+
</file context>
|
Reviewed at baa48fa. There are no conflicts, the evidence gap from the earlier review is closed, and this is ready for human review. Not blocking: in host.ts a no-signal lock-wait timeout now maps through CI: Smoke Tests is still queued. It exercises the snapshot bridge through the native-build files this PR changes, so it needs to pass on baa48fa. |
thymikee
added
the
ready-for-human
|
1 participant
Summary
Moves the TS native-build cache, lock, toolchain-identity, and build-host code out of
snapshot-sourceinto a domain-neutralpackages/platform-apple/src/native-buildmodule group, sofold-helper-cache.tsno longer imports the snapshot bridge's deadlines, errors, host construction, or host types to compile a binary that needs none of them.native-build/{cache,deadline,host,toolchain-identity,errors}.tscarry the one cache, one lock, and one toolchain-identity probe every runtime clang build in this package shares.NativeBuildHostexposes only file access, command execution, locking, and process identity — no bridge socket start/connect, no target-process inspection.NativeBuildErroris domain-neutral (cancelled/timeout/unsupported);snapshot-source/errors.tsanddeadline.tsmap it ontoSnapshotSourceErrorat the boundary, andfoldable/fold-helper-cache.tsmaps it onto its ownfold-helper-build-failedAppError, preserving both public error shapes byte for byte.fold-helper-cache.tsdrops constructing a fullSnapshotSourceHost(and overridingrun) forcreateNativeBuildHost(runAppleToolCommand), the narrow host the compile actually needs.snapshot-source/host.tsdrops its own copy of lock acquisition for the shared one;cache-identity.tskeeps only the bridge'ssimulatorRuntimeaddition, with retry/probe/cancellation logic moved tonative-build/toolchain-identity.tsalong with its tests.Closes #2970. 14 files touched.
Validation
Tested commit:
baa48fa8ba5f10cff86a2aa69483bf5c399d3f0f.pnpm check:affected --run: 277 test files / 1823 tests passed, including the Darwin conformance suites (native-runtime.test.ts, fold-helper's-Werrorcompile gate) that do genuinexcrun/clangcompiles against the real toolchain.Live device validation (production code path, no simulator needed — compilation targets the
iphonesimulatorSDK, not a booted device):ensureSnapshotBridgeBinary: cache MISS build 1773ms, then cache HIT reuse 95ms, identical output path both times.ensureFoldHelperBinary: cache MISS build 297ms, then cache HIT reuse 102ms, identical output path both times.ensureFoldHelperBinarycall rejects withisRequestCanceledError(error) === true(this is the fix in this commit: a fold or toolchain-probe cancellation now keepsreason: 'request_canceled'the way it did before the native-build split).No simulator was created: the change doesn't alter when the native artifact is built or reused (cache-key inputs, hash algorithm, manifest schema, and lock semantics are preserved unchanged — only module ownership moved), confirmed by inspection and by the unit + real-compile tests above producing byte-identical cache behavior on both paths.
No unresolved risk.