Skip to content

fix(ios): end a bridge creation that outlives the snapshot source close - #4131

Open
thymikee wants to merge 5 commits into
mainfrom
fix/sm7-snapshot-bridge-close
Open

thymikee wants to merge 5 commits into
mainfrom
fix/sm7-snapshot-bridge-close

Conversation

@thymikee

@thymikee thymikee commented Nov 1, 2026 •

Copy link
Copy Markdown
Member

Summary

SnapshotBridgeManager.close() now adopts in-flight request turns and bridge creation, interrupts refused work, and waits for cleanup. A creation cannot publish a helper after close has collected its sessions.

The helper exit wait starts one diagnostic observer, then races raw status against the request's remaining delay, interrupted by close. Status precedence, typed cancellation, and deadline bounds are preserved. Late status still records the crash after close, timeout, or cancellation. Close's dispose map is simplified. Two files; no scope expansion or shared helpers. Tracked as SM-7.

Validation

Head 893e41ac957f01306fb8a7f7d34308404ec3dac2; reviewed base c62f0c8d62bcfd4f4720d8edf61bcca5aecbf8ff.

Cancellation regression: base loses the late diagnostic (1 failed, 1 passed, 24 skipped); head lifecycle file 26 passed, including the exact bridge-request-deadline assertion.

pnpm format and build pass. pnpm check:affected --run --head 893e41ac957f01306fb8a7f7d34308404ec3dac2 is base-blocked: six stages pass; related tests report 301 passed, 1 failed. The provider output guard's fake record exceeds 20s. Identical focused commands on unchanged base and head each produce 2 passed, 1 failed. Receipt .tmp/validation/run-TnEc9k/receipt.json: exactCommit=true, checkoutChanged=false. Unit and remaining integration stages were not run. CI owns coverage/device lanes. Draft pending the recording-guard blocker.

Live at this head: local iOS 26.2 simulator CAE86E90-9D1D-42A4-B020-179F0FE092DE; private state directory; Settings open, snapshot -i returns 18 nodes through the bridge, close and graceful daemon stop succeed; no owned bridge remains. Failure interleavings are covered by unit tests.

A capture can pass the manager's closed checks and then pause inside the host while it makes its
bridge session. Close marked the source closed and stopped only the sessions it had already published,
so the paused creation resumed, started a helper, published it, and nobody stopped it again. Close now
adopts the turn and the creation underway: it ends them and waits for each to stop the process and
connection it started, and refuses queued requests instead of leaving them behind a bridge being
stopped.
@vercel

vercel Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
agent-device-website Ready Ready Preview Nov 1, 2026 9:22pm UTC

Request Review

@github-actions

github-actions Bot commented Nov 1, 2026 •

Copy link
Copy Markdown
Contributor

Size Report

Metric Base Current Diff
Installed (including dependencies) 5.67 MB 5.64 MB -29.7 kB
Package (unpacked) 5.67 MB 5.64 MB -29.7 kB
Package (download) 1.72 MB 1.72 MB -7.1 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 40.1 ms 40.3 ms +0.3 ms
CLI --help 87.9 ms 85.8 ms -2.2 ms

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

@thymikee thymikee left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: needs-changes.

Reviewed exactly ff8c2285c808135b592e77f53ae3ce28d31786b8..ae7374fff96fdb4a4964b0853213513e25531e38 against the coordinator's close-owns-creation requirement.

  • [P2] Make the process-exit await interruptible by close — packages/platform-apple/src/snapshot-source/lifecycle.ts:263-266 (and the settlement wait at :188-194). Concrete interleaving: a connect attempt fails, the helper exits, and the retry observes isAlive() === false while its wait promise is still pending. This is a real distinction in the production host: host.ts:86-87 reads child exit/signal status for liveness, whereas runCmdBackground settles its promise on child close (packages/host-kit/src/internal/exec.ts:587-593). The creation now awaits bridgeProcessExited(), which awaits that promise without a deadline or close signal. If close() arrives here, aborting the creation does nothing to this await, and close now waits for both adopted settled promises. Inherited pipes can keep close pending indefinitely; the command runner itself documents this case at exec.ts:865-868, but the bridge's abort is not connected to that runner. Even when the exit wait eventually resolves, the resumed creation throws process-crash / bridge-exited, bypassing the new source-closed refusal. Make this exit-status wait observe the creation's interruption, let the creation clean up without requiring an indefinitely pending exit-status promise, and preserve unsupported / source-closed when close wins. Add a regression that pauses process.wait after liveness becomes false, closes the manager, and asserts cleanup and the typed refusal without releasing that pause.

Short design pass: the ownership can be simpler. Every creation is already awaited by its request turn, so tracking both as independent OwnedWork records duplicates cancellation and settlement. The creating entry contains no resource or completion handle and is otherwise only skipped by close. One owned turn spanning creation/cleanup, with a live-session map, appears sufficient. This is nonblocking design feedback.

Other checks: the directory/connect/queued-request regression tests distinguish the base by inspection, but do not exercise the exit-status await above. I found no activeWork removal leak on paths that settle, and the connect/retry split otherwise preserves the previous retry and timeout policy. Close during an already dispatched exchange can also still surface transport-failure/process-crash because its transport sees only the caller signal; that behavior predates this range, so I have not listed it as a separate regression. No code edits, commits, pushes, unit-test runs, or CI wait.

@apex-sentinel-staging

apex-sentinel-staging Bot commented Nov 1, 2026 •

Copy link
Copy Markdown

Apex Sentinel

Verdict: approve

Since the last review: 2 new · 0 still open · 4 no longer raised

2 finding(s): 2 nit. Verdict: approve. reached max rounds (1).

🔵 Nit (2)
Where Finding
packages/platform-apple/src/snapshot-source/lifecycle.ts:304 🆕 bridgeExitOrClose always reads remainingSnapshotSourceMs, so a caller abort is reclassified as bridge-exit-deadline
packages/platform-apple/src/snapshot-source/lifecycle.ts:310 🆕 bridgeExitOrClose distinguishes race winners by answer !== undefined instead of the sentinel it already defines

Reply under an inline comment with @apex-sentinel-staging note ... or @apex-sentinel-staging pivot ... to re-check that finding.
Comment @apex-sentinel-staging review on this conversation to run a full review.

@apex-sentinel-staging apex-sentinel-staging Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apex Sentinel: 3 new findings on ae7374f. The verdict and the full list are in the summary comment.

The connect loop asked the helper why it had gone away before it checked whether a close owned the
work. The host settles that exit status when the child's own pipes close, so a descendant holding an
inherited pipe open can keep the read pending indefinitely, and a close that landed there was left
waiting on a question nothing could answer. The creation also reported the crash rather than the close
that already owned it.

The close now answers that read: the refused creation stops what it started and names the close, while
the status read nobody waits on still records the exit if it ever settles.
The connect loop raced the close against its own typed conversion of the helper's exit status, and that
conversion takes a turn of its own. A close landing in that turn overwrote an answer the helper had
already given: the capture reported the close's refusal even though the manager had been told the helper
crashed.

The race now subscribes to the raw status, so the close wins only while it is still pending. A status
read that fails counts as an answer too: the manager knows the helper is not coming back.
@thymikee

thymikee commented Nov 1, 2026

Copy link
Copy Markdown
Member Author

Accepted the round-2 [P2] masking finding. Fixed in 349d69c268; one commit, one push, on top of the reviewed 4f3d27e816. No refactor beyond the arbitration, per the final-round instruction.

Change. bridgeExitOrClose now races the close against the raw process.wait and only converts the winner to the typed error afterwards, so a status that answered first cannot lose to a close landing inside that conversion turn. A status read that rejects counts as answered — the manager knows the helper is not coming back — and bridgeProcessExited still reads the winner and owns both settle paths.

Regressions, written first (both via the existing helperGoneAtConnect seam, which leaves the manager inside a pending status read):

  • an exit status that answers before the close still reports the helper crash — resolves the status, then closes in the same synchronous turn.

  • a status read that fails before the close still reports the helper crash — same interleaving with the status promise rejecting, per your "including a rejected status promise".

  • reviewed head 4f3d27e816: both fail with failureCode: "source-closed" instead of process-crash / bridge-exited.

  • new head 349d69c268: both pass; the round-2 hang test stays green (22 passed in the file, 145 passed in the group). Mutation check: making a rejected status count as unanswered fails the second test.

Gate. pnpm check:affected --run --head 349d69c268 → all runnable checks passed, receipt .tmp/validation/run-OTn6gB/receipt.json. format, lint, typecheck, fallow audit clean; the complexity finding from round 1 did not return.

Live. Not rerun: this diff changes only which typed error the connect loop throws after the helper is already gone — no device-facing dispatch, capture or lifecycle route changed. Round-1/2 live evidence on the T3 simulator (bridge route served, no bridge survives close) still stands.

@thymikee

thymikee commented Nov 1, 2026

Copy link
Copy Markdown
Member Author

Coordinator check of round 3 (4f3d27e816..349d69c268, lifecycle.ts only), per the round cap instead of a third full review:

  • The race now subscribes to the raw process.wait, so a close that lands while the status is being converted can no longer overwrite an exit the helper already reported. A rejected status counts as an answer, which matches the round-2 finding.
  • bridgeProcessExited awaits process.wait again. That is the same settled promise, so the second read returns the same outcome and the diagnostic is not emitted twice (diagnosedProcesses).
  • When the close wins, the losing wait keeps the rejection handler that Promise.race attached, so no rejection goes unhandled. The caller (throw await, :266) throws the same typed error whether the function returns it or throws it.

No blocking findings. Round-1 and round-2 findings are resolved.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

Tip: Review your code locally with the cubic CLI to iterate faster.

View guided diff | Turn on auto-fix | Re-trigger cubic

@apex-sentinel-staging apex-sentinel-staging Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apex Sentinel: 4 new findings on 349d69c. The verdict and the full list are in the summary comment.

@thymikee

thymikee commented Nov 1, 2026

Copy link
Copy Markdown
Member Author

Accepted the [P2] process-exit-await finding. Fixed in 4f3d27e816; one commit, one push, on top of the reviewed ae7374fff9.

Change. connectUntilReady now asks bridgeExitOrClose(session, stop) instead of awaiting bridgeProcessExited outright. The close answers that read: the refused creation still stops what it started and fails unsupported / source-closed, and the status read nobody waits on still records the exit if it ever settles. The refusal resolves rather than rejects, so a close arriving after the status did cannot leave an unowned rejection.

Regression (a close while the helper exit status stays unread ends the capture instead of waiting for it). The fixture drops the helper at the connect attempt without ever reporting its status, the test closes the manager while the status read is pending, and never releases it. FakeProcess.wait now counts reads, so the seam proves the manager is inside that read.

  • head under review ae7374fff9: AssertionError: close waited for an exit status that never arrived / Received "pending" — the hang, and once it did settle the capture reported process-crash: bridge-exited.
  • new head 4f3d27e816: close settles, the request fails source-closed, no signal is sent to an already-dead helper, and no exit diagnostic is emitted.
  • Mutation checks: awaiting the refusal before the exit read, and dropping the adopted-work wait, each fail this test.

pnpm exec vitest run --project unit-core packages/platform-apple/src/snapshot-source → 143 passed. pnpm check:affected --run --head 4f3d27e816 → all runnable checks passed, receipt .tmp/validation/run-iFsXXa/receipt.json. Live: this build served 28 nodes from the bridge route on the simulator, and its bridge processes were gone after the source closed.

Not changed, with reason. The cubic items: the assertTargetStillCurrent gap is the documented pre-existing rule for a capture already dispatched to the bridge — its bridge stops with the rest and close waits for it, and rescuing it would need the transport to see the close signal, which predates this range. The ps probes already run under the request deadline and its timeoutMs, so the interrupt adds a second way to end a wait that is already bounded. The design note to collapse OwnedWork to one turn is plausible but nonblocking; I kept it out of this fix so the close ownership change stays reviewable on its own.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

@thymikee thymikee left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: needs-changes.

Reviewed ae7374fff96fdb4a4964b0853213513e25531e38..4f3d27e8168622ca485d0110e7f69e12f0a40b00 and the affected lifecycle interactions. The round-1 shutdown hang is resolved: close interrupts the pending status read, creation disposes its session, and adopted work can settle without releasing process.wait.

  • [P2] Preserve an exit status that settles before close — packages/platform-apple/src/snapshot-source/lifecycle.ts:317. bridgeProcessExited() awaits process.wait before fulfilling the promise raced here, whereas abort fulfills refusal synchronously. If the manager is reading a dead helper's pending status, process.wait resolves with exit code 1, and manager.close() follows in the same synchronous turn, the refusal wins before that async conversion runs. The request reports unsupported / source-closed even though the exit status arrived first; the diagnostic still records the crash. I reproduced this with the checked-in helper bodies and close's adopted-work path: resolving the status immediately before close returns source-closed; letting the exit branch finish before close returns process-crash / bridge-exited. Arbitrate using the underlying status promise versus close before converting the winning exit into its typed error, including a rejected status promise. Add the resolve-status-then-close interleaving as a regression.

Cleanup checks: after either branch settles, finally removes the abort listener. With an exit and no subsequent close, the unused refusal has no remaining listener root. A late status rejection after close is consumed by bridgeProcessExited; the race owns both outcomes. Resolving instead of rejecting the refusal does not itself fix rejection ownership, because Promise.race already installs handlers on both inputs.

The two declined original cubic items remain nonblocking for this scope. The final identity validation follows an already dispatched exchange, which the existing close contract allows to settle; it was not introduced by this delta. Both identity probes pass the remaining request budget as timeoutMs through readSnapshotTargetProcessStartTime to the command runner, so their close latency is already bounded by that budget. The declined ownership simplification remains design feedback.

Validation: read-only source review and an isolated Node promise-order/listener probe of the checked-in helper bodies. No snapshot-source unit-suite run or CI wait. Reviewed source remains clean.

@thymikee

thymikee commented Nov 1, 2026

Copy link
Copy Markdown
Member Author

I found no problem in the code at 4f3d27e that must change before merge. The close-during-creation path now ends the bridge creation, and the regression tests cover it. The author's live simulator run is their own report, and I did not run the unit suite or the base-failure checks. Smoke Tests and Coverage are still running at this commit, so wait for both to pass. No conflicts.

Not blocking, and you can take or leave these. First, a socket assigned one microtask after the last stop check can survive a close: a reconnect at https://github.com/callstack/agent-device/blob/4f3d27e/packages/platform-apple/src/snapshot-source/lifecycle.ts#L357 or https://github.com/callstack/agent-device/blob/4f3d27e/packages/platform-apple/src/snapshot-source/lifecycle.ts#L208 hits the refusal at https://github.com/callstack/agent-device/blob/4f3d27e/packages/platform-apple/src/snapshot-source/lifecycle.ts#L360, and handleRequestFailure does not destroy the socket for unsupported. The rule is that a refusal for a close destroys any socket the refused work assigned. Destroying session.socket where :360 refuses, or in handleRequestFailure, covers both sites. This window is tiny and I reasoned it from the code without a test. Second, the exit status that settles before close in bridgeExitOrClose can report source-closed instead of process-crash, on a source that is closing anyway.

Could one owned unit per request turn replace the creation OwnedWork and the BridgeEntry union (https://github.com/callstack/agent-device/blob/4f3d27e/packages/platform-apple/src/snapshot-source/lifecycle.ts#L220)? The turn is already in activeWork, close() aborts both interrupts together, and the creation always settles before the turn does. Passing turn.interrupt.signal through ensureSession, connectUntilReady and exchange, and publishing into a plain Map<string, BridgeSession> only after the last refusal check, would keep the same guarantees with fewer parts. An earlier review made this suggestion and it was declined as not blocking. I still think it is the better-owned shape.

On the open threads, the P2 thread on the unbounded exit wait still applies #4131 (comment). Its cheap fix is a deadline leg in bridgeExitOrClose, and the wait already existed before this PR. Three lower-priority threads also still apply: please fix the wall-clock assert in the test, the connectOnce pass-through, and the inaccurate comment about the rejection nobody owns. These three do not apply, so please resolve them: #4131 (comment) (both identity probes already use the request budget, and the probe starts no bridge resource); #4131 (comment) (the envelope arrived before the close, the close already disposed the session, and dispatched captures are documented as not rescued); #4131 (comment) (abort() dispatches synchronously, so a client abort that comes first still rejects as cancelled).

@thymikee

thymikee commented Nov 1, 2026

Copy link
Copy Markdown
Member Author

Thanks for the update. At 349d69c, a close that lands while a bridge connect is in flight now ends the creation with a typed refusal and destroys the socket that connect returned, so the problem from the earlier review is fixed. I found no new problems that must change before merge.

Not blocking, and you can take or leave it: in bridgeExitOrClose, exitStatusOrClose sits next to a hand-built refusal promise and abort-listener code, while waitForSnapshotSourceDelay(deadline, ms, code, stop) already resolves on stop and rejects a client abort as cancelled. waitForSimulatorTurn and awaitConnectRetry already race work against it.

Could the exit read have one owner and one race? Start bridgeProcessExited first, since it never rejects and records the diagnostic whenever the status settles. Then race process.wait against waitForSnapshotSourceDelay(deadline, remainingMs, 'bridge-connect-deadline', stop). If the delay side wins, throw closedSourceRefusal() when stop is aborted, otherwise the connect timeout. If the wait wins, return the exit read. That removes exitStatusOrClose, the refusal promise and the listener. It also keeps the "an answer before the close wins" rule, bounds the read by the request deadline, and makes the diagnostic comment true again.

Two open threads still apply and have the same fix: #4131 (comment) (exit diagnostic skipped) and #4131 (comment) (read ignores deadline). Three lower-priority threads also still apply. These threads do not apply and can be resolved: #4131 (comment) (identity probes already run under the request deadline), #4131 (comment) (a dispatched capture is not rescued, as documented), #4131 (comment) (a client abort that comes first reports cancelled), #4131 (comment) (the destroyed socket is the one that refused work), #4131 (comment) (the close is the first answer in the retry wait), #4131 (comment) (outcome does not depend on argument order) and #4131 (comment) (the sentence was deleted in this head).

The failing Smoke Tests run looks unrelated to this diff. The iOS phase failed at smoke:capture-close when record stop timed out after 30000ms while compiling the video validator with xcrun swiftc. That route does not touch packages/platform-apple/src/snapshot-source, and the macOS phase passed. The log does not show whether the same timeout happens on main, so this rests on the absence of route overlap. Because the scenario stopped before its close step, this run did not exercise the session close route or a snapshot-bridge close.

Before merge, please rerun Smoke Tests to get that coverage. I did not run the unit suite, the mutation checks or the affected-check gate. I confirmed that the new regressions fail at 4f3d27e by reading that code, not by running it. The live simulator evidence is your report from an earlier head, and I accept it because this delta only changes which typed error a refused connect throws after the helper is gone.

@thymikee

thymikee commented Nov 1, 2026

Copy link
Copy Markdown
Member Author

CI diagnosis — Smoke Tests iOS failure on 349d69c268 is an environment flake, not this PR. Rerunning the failed job once.

Failing step. Run 41291475550, IOS_OUTCOME: failure. The one real error is scenario smoke:capture-close, step stop screen recording: agent-device record stop … --json returned COMMAND_FAILED — xcrun timed out after 30000ms compiling /private/var/folders/…/agent-device-swift-cache/sources/video-validator-bd93ed6ddc83c518.swift (timeoutMs: 30000, dispatched: "no", empty stdout/stderr). The later iOS simulator E2E coverage is incomplete (missingCommands: close, record) is the cascade of that same scenario. The run also showed one unrelated replay retry (wait "pan duration >=400ms", attempt 1) — a generally loaded runner.

Why it is unrelated. The timeout fires inside the video-completion validator: isPlayableVideo → compileSwiftSourceText (packages/capture-kit/src/recording/video.ts:105, 30 s compile budget). This PR's diff is only packages/platform-apple/src/snapshot-source/lifecycle.ts and its test — no capture-kit, exec, or recording code. The failure is pre-dispatch (dispatched: "no"): the command never reached the device, and no snapshot-bridge code runs on the record stop path. This PR's failure modes are typed SnapshotSourceErrors (source-closed, process-crash), never xcrun timeouts.

Baseline comparison. Main f7879cf2 (run 41280572207) passed smoke:capture-close in 33.8 s; its log contains no video-validator compile lines at all — the runner's swift cache was warm, so the compile never had to prove itself against the 30 s budget. Zero xcrun timed out lines in the three other recent Apple runs (41184680094, 41207911645, 41193644623). This is a cold-compile-under-load timeout in pre-existing code.

Action. gh run rerun 41291475550 --failed. No code change: nothing here is reproducible or fixable in lifecycle.ts.

… its late diagnosis

The close race owned the helper exit-status question alone, so a capture whose helper died without ever
reporting its status had nothing to end the wait: it ran past its own budget and answered only once a
close arrived. The wait now races the request's remaining budget too, and a spent budget or a close that
stops waiting leaves the read behind as an observer, so a status that answers late is still diagnosed.

The connect loop keeps its inline connect; the queued-capture refusal is now proven by its order rather
than a clock bound.
@thymikee

thymikee commented Nov 1, 2026

Copy link
Copy Markdown
Member Author

Round-4 dispositions — 5 open threads, 2 real defects fixed in c62f0c8d62 (one commit, one push, on top of the reviewed 349d69c268). Thread replies carry the per-thread detail; each thread is resolved.

Thread Disposition
discussion_r4434897700 Accepted, fixed. The exit-status wait raced only the close, so a capture whose helper died without reporting its status had nothing to end the wait and ran past its own budget until a close arrived. The race now also answers the request's remaining budget (waitForSnapshotSourceDelay(..., 'bridge-exit-deadline', stop), typed timeout / bridge-exit-deadline; a client abort inside it keeps cancelled). Test a capture whose exit status stays unread ends at its own deadline without a close fails on 349d69c268 by hanging to the vitest timeout.
discussion_r4435004286 Accepted, fixed. When the close or the budget wins while the status is pending, the read is left behind as a detached bridgeProcessExited observer, so a status that answers late still records exit code and bounded log. a close that ends the exit wait still records the crash once the helper status answers fails on 349d69c268 (the status read left behind by the close never diagnosed the exit). Idempotent via the existing diagnosedProcesses set.
discussion_r4435023496 No code change, bound pinned by test. The dispatched-capture turn the close adopts is bounded by the round-trip's own timer, armed for the request budget before the write and cleared only in finish (transport.ts:37-42, 91-100), so a pending process.wait there cannot hold close past that budget; waiting less would return while a capture still runs against a bridge close has stopped, which is the orphan class the adoption exists to prevent. New a close during a dispatched capture waits only for the capture deadline the helper died on drives that exact sequence (helper gone with no status mid-response, 150 ms budget) and asserts close settles inside the grace window with the capture typed timeout.
discussion_r4434793834 Accepted, test-only. The wall-clock assert is replaced by causal ordering: assert.rejects(queued, …) is awaited before paused.release(), so a refusal that waited for the creation would hang rather than pass, plus fixture.connects.length === 1.
discussion_r4434793840 Accepted. host.connect is inlined back into connectUntilReady; the connect-timeout cap reads next to the remainingMs it derives from. The complexity finding that motivated the split did not return.

Evidence. Both new tests were written first and fail their reviewed head: 349d69c268 hangs one (5 s vitest timeout) and loses the diagnostic in the other. File 25 passed, group 148 passed. Base ff8c2285c8 with this test set: 6 failed | 1 passed | 18 skipped under -t "a close". Mutation checks: dropping the budget from the race brings the hang back; dropping the observer loses the late diagnostic.

Gate. pnpm check:affected --run --head c62f0c8d62 → all runnable checks passed, receipt .tmp/validation/run-ZFv0e9/receipt.json. format, lint, typecheck, fallow clean.

Smoke Tests. The rerun of run 41291475550 passed (IOS_OUTCOME: success), confirming the earlier flake call: the failing step was a cold xcrun swiftc of the unrelated video validator under runner load, not this PR.

@cubic-dev-ai cubic-dev-ai Bot left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 2 files (changes from recent commits).

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

@thymikee

thymikee commented Nov 1, 2026

Copy link
Copy Markdown
Member Author

Coordinator check of 349d69c268..c62f0c8d62 (the thread round):

  • The exit-status wait now also races the request's remaining budget. It cannot reject: the budget leg catches its own rejection, a rejected status is mapped to 'status', and caller cancellation surfaces as the typed cancelled error. The finally aborts the budget timer on every path.
  • Close and budget both leave bridgeProcessExited running as an observer, so a late status is still diagnosed. A second read is safe because diagnosedProcesses deduplicates the diagnostic.
  • Inlining connectOnce changes no behavior. The wall-clock assertion is replaced by a causal one.

No blocking findings. The Smoke Tests rerun passed on the previous head, which confirms the flake.

@apex-sentinel-staging apex-sentinel-staging Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apex Sentinel: 4 new findings on c62f0c8. The verdict and the full list are in the summary comment.

@thymikee

thymikee commented Nov 1, 2026

Copy link
Copy Markdown
Member Author

The earlier snapshot-source findings from 349d69c are fixed at c62f0c8: the exit read now races the request budget, and observers no longer leak on close or timeout. The 19 checks pass at this head, and the earlier Smoke Tests failure (record stop xcrun swiftc timeout in the capture-kit video validator) did not touch snapshot-source and passed on rerun. There are no conflicts.

Not blocking, and fine to take or leave: bridgeExitOrClose still has no single owner for the exit read. Every exit-wait outcome that is not the status itself should leave exactly one observer, and one race should decide the winner. Today each branch chooses on its own, and the cancel branch omits the observer, so a late exit diagnostic is lost after a cancel. Starting bridgeProcessExited once before the race closes that by construction. Also, lifecycle.test.ts:655 matches any timeout; asserting failureCode as bridge-request-deadline is a cheap tightening, since the bound is already proven at lines 647-651.

Could the race be simpler? Start bridgeProcessExited once, then race the raw process.wait against waitForSnapshotSourceDelay(deadline, remainingMs, 'bridge-exit-deadline', AbortSignal.any([stop, stopWaiting.signal])). If the status wins, return the observer. If the delay wins, throw closedSourceRefusal() when stop.aborted, otherwise the bridge-exit-deadline timeout. A cancel rejects through the delay, and an already-settled status still wins by iteration order. This drops about 25 lines, the refusal promise, the abort listener, the 'cancelled' in check and both observer copies. The previous review proposed this shape, and the update did not adopt it, so is there a reason to keep the current one?

Open threads: the timeout assertion thread (P2) and the cancel observer thread (P2) still apply, plus one lower-priority thread, r4435436225, which is cosmetic. These do not apply and can be resolved: r4435436217 (a published session is always owned, and the waits are bounded), r4435436222 (a status that answers before the close still wins, and tests pin it), r4435436234 (the budget leg cannot classify a close as cancelled), r4434897700 and r4435004286 (fixed at this head, apart from the cancel path above), r4435023496, r4434793834, r4434793840, r4434777859, r4434777862, r4434793828, r4435023501, r4435023503, r4435023504 and r4434897706 (unchanged or benign).

I did not run the unit tests, the mutation checks or check:affected. The claim that the new tests fail on 349d69c rests on reading that head. The live simulator run in the PR body does not name its head. I accept it because the update only changes the exit-status wait after the helper has died, which a healthy live snapshot does not reach.

@thymikee
thymikee marked this pull request as draft November 1, 2026 21:22
@thymikee

thymikee commented Nov 1, 2026

Copy link
Copy Markdown
Member Author

Addressed the maintainer review in 893e41a, pushed once on top of c62f0c8d62bcfd4f4720d8edf61bcca5aecbf8ff.

  1. r4435422571 — fixed, using the requested simpler race. bridgeProcessExited starts once before the raw process.wait / waitForSnapshotSourceDelay(deadline, remainingMs, 'bridge-exit-deadline', AbortSignal.any([stop, stopWaiting.signal])) race. A status win returns that observer; a delay win checks close before timeout. Typed delay errors propagate, so cancellation remains cancelled / abort-signal. A failed raw status read still returns the observer's crash result. The refusal promise, explicit abort listener, cancellation-object check, and branch-specific observer copies are gone.
  2. r4435422567 — fixed. The dispatched-capture test now asserts both failureKind === 'timeout' and failureCode === 'bridge-request-deadline'.
  3. r4435436225 — fixed. The dispose map directly returns this.dispose(session, true).
  4. All 18 listed threads answered and resolved. The three accepted threads were reopened before their corrective replies. The other 15 have one-line dispositions matching the maintainer review: r4435436217, r4435436222, r4435436234, r4434897700, r4435004286, r4435023496, r4434793834, r4434793840, r4434777859, r4434777862, r4434793828, r4435023501, r4435023503, r4435023504, r4434897706.

Regression evidence. pnpm exec vitest run --project unit-core packages/platform-apple/src/snapshot-source/lifecycle.test.ts -t 'ends the exit wait still diagnoses a late exit' on production code at c62f0c8d62 with the new test gives 1 failed, 1 passed, 24 skipped: the status read left behind by the cancel never diagnosed the exit. On head, the full lifecycle file gives 26 passed. Cancellation retains its exact typed reason, then the late status emits one diagnostic containing exit code 1 and the fixture log. Existing status-before-close and failed-status-before-close tests pass. Parent-relative change: production +17/-37, tests +49/-47, docs 0; the test file remains below 1,000 lines. Cold self-review found no additional issue; no independent reviewer was started by this implementation child.

Validation blocker, not a pass. pnpm format and build pass. pnpm check:affected --run --head 893e41ac957f01306fb8a7f7d34308404ec3dac2 passes format, lint, typecheck, layering, fallow and build, then fails vitest-related: 301 passed, 1 failed. apple-platform-output-guard.test.ts:385, iOS simulator case, reports Command "record" did not return within 20s in the fake harness; it also exceeds the slow-test budget. Receipt .tmp/validation/run-TnEc9k/receipt.json has exactCommit=true, checkoutChanged=false. Unit and the later provider-integration stage were not run; coverage and device lanes remain CI obligations.

I reproduced the same guard failure without competing tests on an unchanged archive of base c62f0c8d62, installed and built separately inside this worktree's ignored .tmp directory, then on head with the identical command: pnpm exec vitest run --project provider-integration test/integration/provider-scenarios/apple-platform-output-guard.test.ts. Both give 2 passed, 1 failed, at the same test, line, timeout and assertion. A live process observation during the head run showed the recording video-validator Swift compiler running; that alone does not prove it is the entire cause. No recording code or timeout was changed. Logs: .tmp/pr4131-base-provider-guard.log and .tmp/pr4131-head-provider-guard.log. PR is draft and base-blocked; @thymikee, the coordinator needs to accept or route this pre-existing recording-guard blocker before restoring readiness.

Live evidence at 893e41a. Host local, iOS 26.2 simulator CAE86E90-9D1D-42A4-B020-179F0FE092DE (issue-2080), session t3-0702db65505853c36d4e6870, private state /private/tmp/pr4131-live-OgjxKd. Used device_open and its bound command for open com.apple.Preferences, snapshot -i, and close, with a private config pointing to this worktree's rebuilt HTTP daemon. Snapshot returned 18 nodes. The owned bridge's socket was /tmp/agent-device-ax-3553d5e3aacf-38483-c4209855e7a6/snapshot.sock; runner log has appState commands and zero command=snapshot entries. Session close and graceful daemon shutdown succeeded, and the owned bridge launcher/helper were absent afterwards. Evidence: .tmp/pr4131-live-evidence.json. The original e2e-apex open was refused by the active sm11-a claim; that session was not reused or closed. Healthy snapshots do not reach the helper-died exit race; unit tests prove those failure interleavings.

@thymikee
thymikee marked this pull request as ready for review November 1, 2026 21:24
@thymikee

thymikee commented Nov 1, 2026

Copy link
Copy Markdown
Member Author

Coordinator: I accept the apple-platform-output-guard.test.ts:385 failure as pre-existing. It reproduces on the unchanged base c62f0c8d62 with the same command, at the same line and timeout. It is the same cold Swift video-validator compile timeout that failed Smoke Tests on this PR earlier and on #4134. This diff touches only snapshot-source, so the failure is not caused by it. I am marking the PR ready for review again. The flake needs its own fix, outside this PR.

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 2 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/snapshot-source/lifecycle.ts">

<violation number="1" location="packages/platform-apple/src/snapshot-source/lifecycle.ts:268">
P2: `close()` aborts `stop`, but this in-flight `host.connect()` receives only `deadline.signal`, so it cannot observe manager shutdown. Because `close()` awaits the active work's `settled` promise, shutdown waits for the full connect attempt instead of interrupting it; pass a signal combining `deadline.signal` and `stop`. 

(Based on your team's feedback about propagating cancellation into underlying lifecycle work.) [bac439e4-e895-44e2-bb93-df8829d4d177]</violation>
</file>

Reply with feedback, questions, or to request a fix.

View guided diff | Turn on auto-fix | Re-trigger cubic

@apex-sentinel-staging apex-sentinel-staging Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apex Sentinel: 1 new finding on 893e41a. The verdict and the full list are in the summary comment.

@thymikee

thymikee commented Nov 1, 2026

Copy link
Copy Markdown
Member Author

I found no remaining problems in the change at 893e41a. The earlier findings from #4131 (comment) are fixed: the cancel path now keeps the late exit diagnostic, and the test pins the timeout failure kind and code.

I read the diff but did not run the lifecycle unit tests, mutation checks or check:affected. The 26-pass result and the claim that the cancel case fails on c62f0c8 come from your report and my read of the old cancel branch. I did not reproduce the live simulator run in the PR body. It cannot reach the dead-helper exit wait that this change touches, so the unit tests cover that path.

The required evidence is in place and I know of no conflicts. Checks are still pending: two Smoke Tests runs and Coverage are in progress, and three Smoke Tests runs show as cancelled. The cancelled runs look superseded by newer run ids. The diff only touches the snapshot-source lifecycle module and its test, and iOS smoke lanes that take a snapshot through the bridge run that module, so a smoke failure would need a look. The earlier Smoke failure (capture-kit video validator swiftc timeout) and the check:affected provider output-guard timeout are outside this module and also reproduce on base.

Of the inline threads, the three at #4131 (comment), #4131 (comment) and #4131 (comment) are fixed at this commit and can be resolved. The thread at #4131 (comment) does not apply: SnapshotSourceProcess.wait is Promise<ExecResult>, so answer !== undefined already covers both status outcomes, and tagging the raw wait would add the extra turn that the docblock keeps out of the race.

Before merge, wait for the in-progress Smoke Tests and Coverage runs to finish green, and rerun the cancelled Smoke Tests runs if they do not re-trigger.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Nov 1, 2026

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-human Valid work that needs human implementation, judgment, or maintainer merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant