Repository navigation
Conversation
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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Size Report
Startup median (7 runs, lower is better):
|
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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 observesisAlive() === falsewhile itswaitpromise is still pending. This is a real distinction in the production host:host.ts:86-87reads child exit/signal status for liveness, whereasrunCmdBackgroundsettles its promise on childclose(packages/host-kit/src/internal/exec.ts:587-593). The creation now awaitsbridgeProcessExited(), which awaits that promise without a deadline or close signal. Ifclose()arrives here, aborting the creation does nothing to this await, and close now waits for both adoptedsettledpromises. Inherited pipes can keepclosepending indefinitely; the command runner itself documents this case atexec.ts:865-868, but the bridge's abort is not connected to that runner. Even when the exit wait eventually resolves, the resumed creation throwsprocess-crash / bridge-exited, bypassing the newsource-closedrefusal. Make this exit-status wait observe the creation's interruption, let the creation clean up without requiring an indefinitely pending exit-status promise, and preserveunsupported / source-closedwhen close wins. Add a regression that pausesprocess.waitafter 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 SentinelVerdict: 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)
Reply under an inline comment with |
There was a problem hiding this comment.
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.
|
Accepted the round-2 [P2] masking finding. Fixed in Change. Regressions, written first (both via the existing
Gate. 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. |
|
Coordinator check of round 3 (
No blocking findings. Round-1 and round-2 findings are resolved. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Apex Sentinel: 4 new findings on 349d69c. The verdict and the full list are in the summary comment.
|
Accepted the [P2] process-exit-await finding. Fixed in Change. Regression (
Not changed, with reason. The |
There was a problem hiding this comment.
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
left a comment
There was a problem hiding this comment.
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()awaitsprocess.waitbefore fulfilling the promise raced here, whereas abort fulfillsrefusalsynchronously. If the manager is reading a dead helper's pending status,process.waitresolves with exit code 1, andmanager.close()follows in the same synchronous turn, the refusal wins before that async conversion runs. The request reportsunsupported / source-closedeven 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 returnssource-closed; letting the exit branch finish before close returnsprocess-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.
|
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 Could one owned unit per request turn replace the creation On the open threads, the P2 thread on the unbounded exit wait still applies #4131 (comment). Its cheap fix is a deadline leg in |
|
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 Could the exit read have one owner and one race? Start 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 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. |
|
CI diagnosis — Failing step. Run 41291475550, Why it is unrelated. The timeout fires inside the video-completion validator: Baseline comparison. Main Action. |
… 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.
|
Round-4 dispositions — 5 open threads, 2 real defects fixed in
Evidence. Both new tests were written first and fail their reviewed head: Gate. Smoke Tests. The rerun of run 41291475550 passed ( |
There was a problem hiding this comment.
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
|
Coordinator check of
No blocking findings. The Smoke Tests rerun passed on the previous head, which confirms the flake. |
There was a problem hiding this comment.
Apex Sentinel: 4 new findings on c62f0c8. The verdict and the full list are in the summary comment.
|
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 Could the race be simpler? Start 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. |
|
Addressed the maintainer review in 893e41a, pushed once on top of
Regression evidence. Validation blocker, not a pass. I reproduced the same guard failure without competing tests on an unchanged archive of base Live evidence at 893e41a. Host |
|
Coordinator: I accept the |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Apex Sentinel: 1 new finding on 893e41a. The verdict and the full list are in the summary comment.
|
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: 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. |
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 basec62f0c8d62bcfd4f4720d8edf61bcca5aecbf8ff.Cancellation regression: base loses the late diagnostic (
1 failed, 1 passed, 24 skipped); head lifecycle file26 passed, including the exactbridge-request-deadlineassertion.pnpm formatand build pass.pnpm check:affected --run --head 893e41ac957f01306fb8a7f7d34308404ec3dac2is base-blocked: six stages pass; related tests report301 passed, 1 failed. The provider output guard's fakerecordexceeds 20s. Identical focused commands on unchanged base and head each produce2 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 -ireturns 18 nodes through the bridge, close and graceful daemon stop succeed; no owned bridge remains. Failure interleavings are covered by unit tests.