Repository navigation
Conversation
Fence equality proves ownership, not admission. Two callers that captured the same live resource present the same fence, so serialization alone let a queued cleanup or a late progress write drive a record another attempt had already committed back to `open`, and a cleanup-pending outcome could leave it open with its export already served. The fence owner now revalidates the record's lifecycle while it holds the fence. A completed record is terminal: a run that finds one is answered through `onCompleted` before it receives a lease, so it cannot repeat native or destructive work, and a transition that would reopen a committed record is refused with the typed reason `resource-already-completed`. Recovery's copy of `markCompleting` is gone; the one transition rule lives in `durable-capture/fence.ts`.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Size Report
Startup median (7 runs, lower is better):
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 6 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Apex SentinelVerdict: Since the last review: 6 new · 0 still open · 3 no longer raised 6 finding(s): 3 minor, 3 nit. Verdict: approve. reached max rounds (1). 🟡 Minor (3)
🔵 Nit (3)
Reply under an inline comment with |
![apex-sentinel-staging[bot]](https://avatars.githubusercontent.com/in/5196457?s=60&v=4){kind=small}
There was a problem hiding this comment.
Apex Sentinel: 7 new findings on be20b30. The verdict and the full list are in the summary comment.
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Needs changes. Reviewed exactly merge-base f7879cf2d266ee1dd476d0342b2819bdeceb647b → head be20b30cee2477b7810ad6ee2cf05f94de744f94. Two parts of the coordinator's required terminal invariant remain unenforced, and the blanket finish refusal introduces a teardown regression.
-
P2 — Revoke old leases or revalidate the persisted lifecycle on every write.
packages/capture-kit/src/durable-capture/fence.ts:73-81. The new guard checks only this invocation's capturedcurrent. A run can retain its lease/progress callback and finish while still open; a subsequent run commits the record; invoking the old transition during that subsequent run still sees its own cachedopenand overwrites the completed manifest. A source-level runtime probe reproducedcompleted → open, with no typed refusal, while the later committing run held the fence. This is the stale-lease case required by the brief; the new progress test only checks a lease against its own completion. Revoke the lease when its run exits and validate the current persisted fence/lifecycle before publication, so escaped progress and completion writes cannot overwrite another run's committed outcome. Add coverage for two different leases, rather than only two transitions on one lease. -
P2 — Put descriptor cleanup inside terminal admission.
packages/capture-kit/src/durable-capture/recovery.ts:195-203, alsopackages/capture-kit/src/durable-capture/finish-recovered.ts:105-113. Both unreattachable branches executeauthority.control.cleanup(envelope)before entering the fence. If recovery selects an open record and waits for reattachment, another finish can commit it before recovery resumes withtransport-not-reattachableorowner-unavailable. Recovery then still invokes native/destructive descriptor cleanup against the completed resource; the newonCompletedonly suppresses the subsequent manifest write. A probe reproducedcleanupCalls: 1with lifecycle alreadycompleted. Move the cleanup call and its transition together into the fence'srun; the completed branch must settle/replay before cleanup is invoked. Cover both recovery entrypoints with a completion during authority acquisition. -
P2 — Replay the committed result for a queued recording finish.
packages/capture-kit/src/durable-capture/transitions.ts:73-78. Two finishes that captured the same live handle previously both succeeded:createScreenRecordingLiveHandlememoizes its completed finish. Now the second receives non-retriableresource-already-completed, despite an available committed export. This has a production teardown path:daemon-runtime.ts:792-847proceeds to session teardown after the request-drain deadline even if an export is still in flight;session-teardown.ts:195can queue its disposal finish behind that export. When the export commits, teardown now fails, reportssession_cleanup_incomplete, skipsretainLeasedSessionRecordingimmediately after the await, and prevents the successful-teardown claim release. A probe using the real recording handle reproduced first-finish success, second-finish failure, and exactly one native finish. Supply a kind-owned replay of the stored completion under the existing guards, including to disposal callers; do not call the native finish again. The typed refusal is appropriate when no valid completed outcome can be served, but successful completion must not itself become an unreleased-resource error.
Review coverage: git grep found seven production fence calls, all in these shared capture-kit modules; each supplies the required callback. The shared paths serve screen recording, audio probe, app log, and perf capture. Network capture does not call this fence or participate in the four-kind durable-session resource set. The duplicated markCompleting is collapsed. Ordinary sequential record stop replay still routes through resolveScreenRecordingStopRecovery, so I am not claiming that every second stop fails. Records already completed at startup remain ignored by candidate selection, and a new adoption uses a new fence generation; those paths were not changed. No export/recorder equivalence is needed for any finding above.
Validation: install/build succeeded; ran the durable-capture suite once: 39 passed. The adversarial probes above loaded the reviewed source directly and used scratch manifests, without driving devices or modifying tracked source. I did not rerun the base suite under the single-run limit: the PR reports 3 failed / 36 passed on base, and those counts remain author-reported. The added cases cover queued admission and a lease's own late progress, but not the different-lease and descriptor-cleanup cases above. CI was not awaited.
Live evidence: the PR body says iOS Simulator 26.1 and contains no device id/UDID. It therefore does not establish use of the assigned mad2026-apex, iOS 26.2. The reported stop and close exercise the shared ordinary recording route; the body explicitly says the exact terminal-admission window was not reached. That evidence does not cover stale leases, descriptor recovery after completion, or shutdown after the drain deadline.
|
I found no new problems in be20b30 beyond the open threads below. The two cleanup-ordering threads must be fixed before merge: finish-recovered.ts and recovery.ts. Native cleanup runs before the fence checks terminal admission there, so the change does not fully keep native work away from a completed record. Please run the cleanup inside the fence run, the way recovery.ts already does for forced cleanup. Not blocking, take or leave: a duplicate or retried Of the other open threads, the two above are the ones that still apply, plus one nit about the duplicated cleanup block and comment (thread). One more (thread) is unverified: I did not check whether disposing the recovery control also releases the reattached handle for each kind. These do not apply and can be resolved: the already-missing cleanup comment is never surfaced to users (thread); no doc lists capture-kit fence reasons (thread); the completed branch cannot resolve to undefined (thread); the helper is meant for module-internal use (thread); and the export replaces the deleted private copy that recovery.ts imports (thread). Smoke Tests and Coverage are still running, and nothing has failed yet. Because this code sits under every record, perf and logs stop, a Smoke failure on a capture command would be relevant here. The Apex Sentinel check shows action required from its change-request review, not from a code check. I did not run the unit tests; the base-fails and head-passes result is the author's report. After the cleanup moves inside the fence, please wait for Smoke Tests and Coverage to finish. |
There was a problem hiding this comment.
All reported issues were addressed across 18 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
Apex Sentinel: 7 new findings on 44800d1. The verdict and the full list are in the summary comment.
44800d1 to
eaeb2d2
Compare
There was a problem hiding this comment.
Apex Sentinel: 6 new findings on eaeb2d2. The verdict and the full list are in the summary comment.
Round 2 dispositions — review 5727843200Head Finding 1 — stale lease. Accepted, fixed. Finding 2 — descriptor cleanup outside admission. Accepted, fixed. Destructive cleanup now runs inside the fence's Finding 3 — queued finish refused despite a committed export. Accepted, fixed. Evidence for the round-1 body gap. Accepted, corrected. Round 1 stated iOS 26.1 with no device id. The assigned device was used and is now named: Validation. The 8 new cases fail at CI follow-up on this head. Diff size. 18 files, +926/−84 against merge-base |
…llers Revalidate the persisted lifecycle at every transition, not only at lease issue, and revoke a lease when the run it was issued to ends, so an escaped progress or completion write cannot publish onto a record another run committed. Move descriptor cleanup inside the fence that decides terminal admission, at both recovery entry points, so destructive native cleanup cannot run for a record that committed while recovery authority was being acquired. Answer a finish that arrives after the commit with the completion the record holds, through a kind-owned optional reader, instead of refusing it. Session teardown treats the remaining refusal as settled rather than as an unreleased resource, so a second record stop stays what it was: a stop.
eaeb2d2 to
83d7b4d
Compare
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Approve — no new blocking findings. Reviewed be20b30cee2477b7810ad6ee2cf05f94de744f94 → 83d7b4d7b143ce683086f5334240a704a33c48ea directly, plus the affected callers against merge-base f7879cf2d266ee1dd476d0342b2819bdeceb647b.
All three round-1 findings are resolved:
- Stale leases:
durable-capture/fence.ts:66-88revalidates the persisted fence/lifecycle before each publication and revokes the lease infinally. Both different-lease cases now protect the committed record; an escaped lease also cannot publish while a later run is still open. - Descriptor cleanup: both recovery entrypoints now use
cleanupDurableCaptureDescriptor(transitions.ts:243-260). The destructive callback executes inside admittedrun; a record committed during authority acquisition bypasses it. The new cases cover both entrypoints. - Queued finish / teardown:
transitions.ts:78-81replays a kind-owned completion. Recording reuses the existing reader, app-log validates its three stored keys, andsession-teardown.ts:155-164settles the specific terminal refusal for kinds with no replay. Other cleanup errors still propagate.
Requested interaction checks:
- Second audio/perf stop: sequential behavior is unchanged from merge-base: audio returns the inactive result (
session-audio.ts:181-200); perf already returnedINVALID_ARGSwhen its live slot was empty (session-perf-runtime.ts:290-291). Both commands hold the session execution lock through their handlers (request-execution-scope.ts:295-314,495,request-execution-locks.ts:31-59), so ordinary concurrent requests cannot both capture the old slot. A second direct/shared finish that already captured that slot does change from the memoized handle result on base to a typed refusal. The production overlap identified in round 1 is shutdown after the drain deadline, and its disposal caller now settles that refusal. I found no newly failing user stop route. Persisting full audio/perf results is unnecessary for this fix; a disposal no-op belongs at the caller that needs no result, as implemented. - Metadata / upgrade: no stored shape or envelope version changes. Recording's existing response object and app-log's existing flat completion keys remain owned by their capture kind; durable-capture only invokes the optional reader. Reads validate the JSON envelope and completion fields. A legacy/cleanup-only completed record without completion metadata returns
undefined, then the shared finish givesresource-already-completed; teardown settles it. The sequential recording recovery route instead returnsnonefor that record, as before. None of these paths dereferences missing metadata or fabricates an export. - Revalidation cost / locking: each transition adds one synchronous verified file open/read/close, JSON parse, validation and freeze before the existing synchronous publication. There is no additional fence acquisition or await in
transition, and neither completion reader re-enters the fence. Recording normally writes a handful of stop checkpoints, not a per-frame stream. This adds I/O, but I found no deadlock or concrete performance regression; I did not benchmark stop latency. - ADR 0024: terminal admission prevents overwriting a committed export; it does not change
stopObservationinto proof of recorder termination. Existing recording checkpoints, native-path disposition and thepreserve-retry-materialfailure policy remain intact. Failed capture finishes stay open and can retry. The terminal live-handle cleanup still delegates to the kind's handle; descriptor cleanup after a competing commit is suppressed. Comments equating every commit with confirmed cleanup are too broad for ADR 0024, but I found no new executable export/recorder conflation in this revision. The ADR's remaining completed-but-unsettled recovery rollout is not introduced by this PR. - Size / smaller design: against merge-base, production is +312/-80 and tests/fixtures +614/-4; round 2 alone is production +247/-86, tests/fixtures +443/-10. The scope is justified by the three boundary fixes. One optional reader and one fenced descriptor-cleanup helper avoid a second completion cache, a changed manifest format, or repeated native finish work. The latest Size report shows +2.4 kB unpacked / +639 B download, and Bundle Size passed on this exact head. I do not see a materially smaller fix that preserves the same boundaries; the explanatory comments could be shortened separately.
Validation: pnpm install --frozen-lockfile && pnpm build succeeded. Ran the permitted durable-capture and capture-admission suites together exactly once: 18 files / 82 tests passed. No daemon-wide tests or device runs were performed, CI was not awaited, and the tracked worktree remains clean. This is a scoped code-review approval, not an assertion that all merge checks or native lanes have passed.
The Coverage lane scored the new app-log completion reader CRAP 110 at 0% coverage: nothing exercised it. These cases drive it through the resource it belongs to, so a queued teardown finish is answered from the record the first stop committed, and a record whose journal cannot name its whole completion keeps the typed refusal instead of inventing a log path.
There was a problem hiding this comment.
Apex Sentinel: 7 new findings on daf7555. The verdict and the full list are in the summary comment.
CI fix — new head
|
A finish that reached a committed screen-recording record was answered with the stored stop response whatever had since happened to the clip, so a deleted export came back as a successful stop naming a path nothing can download. The surviving-file check now belongs to the shared completion reader, the one owner both the stop-recovery route and the terminal replay answer from; a gone export keeps the typed already-completed refusal that teardown settles.
There was a problem hiding this comment.
Apex Sentinel: 6 new findings on ce5d76e. The verdict and the full list are in the summary comment.
Defect fix — new head
|
|
Coordinator check of the post-approval commits (
No blocking findings. The round-2 approval (5728514760) still holds. |
|
I reviewed ce5d76e. The earlier findings are fixed. The three open threads named below still apply and need an answer before merge. A completed durable record is now terminal, and recovery and the in-flight close replay the committed result without cleanup. All 19 checks pass at this commit. The earlier Smoke failure was on the Android snapshot route, which this diff does not touch, and it passed on rerun. There are no conflicts. Not blocking, and you can take or leave these: the released-lease guard at fence.ts:76 is never reached, because both escaped-lease tests commit the record first and stop at the completed check on line 73. One test could escape a lease from run A, start run B on an open record, call I did not run unit tests or mutations; the fail-at-base results come from reading the code. The live device run is your report on 83d7b4d, iOS Simulator only, and it does not show whether the in-flight close reached the On the open inline threads: these three still apply: #4139 (comment), #4139 (comment), #4139 (comment) (replay assertion, revocation guard test, teardown assertion). Five lower-priority nits still apply (the hand-copied |
Review follow-up — head
|
There was a problem hiding this comment.
Apex Sentinel: 3 new findings on 5221af1. The verdict and the full list are in the summary comment.
|
I found no new problems at 5221af1, and the findings from the earlier review (#4139 (comment)) are now fixed. CI is green: 19 checks, 0 failing, and the change since the last review is test-only. There are no conflicts. I did not run the focused tests or the three mutations, so my fail-without-fix judgments come from reading the code. The live device evidence is still your earlier run on 83d7b4d (iOS Simulator only). It does not show whether an in-flight close reached the Three of the open inline threads no longer apply, so please resolve them: the recording After you answer those three threads, this can merge on green CI. |
thymikee
added
the
ready-for-human
Captured-handle follow-up — head
|
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
Fixed Cubic P1 r4437703614 at Fail-first regressions reproduced the defect. Exact-head |
There was a problem hiding this comment.
Apex Sentinel: 5 new findings on 2d27d07. The verdict and the full list are in the summary comment.
|
The fixes from the earlier review at 5221af1 are in at 2d27d07: a completed record now disposes the captured handle before replay or refusal, and the binding stays when cleanup is pending. I found no correctness defect, but I did not run any tests. The checks pass (19 checks, none failing), and there are no conflicts. What is still missing is live evidence for the failure path. The iOS start, stop, second stop and close run is reported by the author, and that run cannot induce a terminal disposal failure. I also could not find a production caller that commits a record through a second handle while the daemon holds the live binding, so I am not sure the teardown case in the thread below can happen. Can you show the route that reaches it, or explain why the narrow trigger needs no handling? With intent 'disposal' in transitions.ts, a replay with a failed disposal resolves the teardown step, and the retained binding retires with the session. Not blocking, and you can take or leave these: onCompleted decodes the cleanup outcome from a thrown error through a hand-kept reason table and a cast, although the handle's forceCleanup() already returns the typed outcome. The same memoize-until-confirmed block also appears in four live handles (starting at app-log-live-handle.ts). The screen recording dispose test accepts any rejection instead of checking the typed reason. Could this be simpler? In onCompleted, call forceCleanup() and treat a rejection as cleanup-unconfirmed, which removes the reason table and the cast. One capture-kit helper that takes the clean thunk and the confirm assertion could build all four handles. The finish intent could then decide retention: 'capture' keeps the binding and replays, and 'disposal' reports the pending cleanup as the teardown step's typed failure. The helper can sit beside isConfirmedCleanup. Open threads: the teardown retirement and four retry copies threads still stand, and one lower-priority thread (the finishPromise nit) still applies. These do not apply and can be resolved: #4139 (comment) (the unreplayable phase name is accurate, and the existsSync move predates this change), #4139 (comment) (the swallowed error is thrown only when no serveable export exists), #4139 (comment) and #4139 (comment) (both fixed at 2d27d07), and #4139 (comment) (the rename is done and has no stale callers). Before merge, please answer the design question above. |
thymikee
removed
the
ready-for-human
|
Addressed the maintainer design review at
Fail-first command on pnpm exec vitest run --project unit-core packages/capture-kit/src/durable-capture/transitions-terminal.test.ts packages/capture-kit/src/provider-app-log-poller.test.ts packages/platform-apple/src/logs/start.test.tsAt this head, the eager-closure plus regression run passed 4 files / 912 tests. The first gate at Parent-relative: 14 files; production +81/-134; tests +72/-22; docs +0/-0. Whole PR: 34 files; production +439/-159; tests/fixtures +1219/-11; docs +0/-0. Cold self-review completed; no independent reviewer was started in this implementation round. Record routing is unchanged, so no new live run was needed under the coordinator instruction. Historical simulator evidence remains attributed to |
Summary
Keep completed durable capture records terminal, fence destructive cleanup, and replay only validated kind-owned outcomes.
Terminal finish now consumes typed
forceCleanup()results. Capture intent retains pending cleanup and preserves replay; disposal intent reports a typed teardown failure. One shared helper besideisConfirmedCleanup, exposed through capture-kit, owns cleanup retries for all four handles. App-log retries skip successful release steps.34 files against merge-base: production +439/-159; tests/fixtures +1219/-11. This round: ten production files +81/-134; four test files +72/-22. No record routing change.
Validation
Base
2d27d0755e; tested head8c436df63fce0a77dc3ee96773387e8fcdea1da5. The same three-file regression command: 4 failed / 26 passed on base plus tests; 30 passed at head. Eager-import plus regression suite: 912 passed. An initial eager-import failure was fixed by co-location.AGENT_DEVICE_VITEST_MAX_WORKERS=1 pnpm check:affected --run --head 8c436df63fce0a77dc3ee96773387e8fcdea1da5passed. Receipt.tmp/validation/run-XkPtW7/receipt.json:exactCommit=true,checkoutChanged=false. 1,212 related / 16,784 unit passed; one skipped.No new live run was required because record routing is unchanged. Historical iOS serial smoke remains attributed to
2d27d0755e, with artifact retention enabled; it did not exercise disposal failure.Cold self-review completed; no independent review was started this round. Coverage, Integration Tests and native CI lanes remain GitHub-authoritative. Review dispositions, route audit and validation evidence.