Skip to content

fix: keep request preparation in its session lifetime - #3143

Closed
thymikee wants to merge 1 commit into
fix/session-ownership-reviewfrom
fix/request-session-health
Closed

thymikee wants to merge 1 commit into
fix/session-ownership-reviewfrom
fix/request-session-health

Conversation

@thymikee

@thymikee thymikee commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

Summary

Locked request preparation captures the session lifetime before observing recording health. It refreshes that lifetime afterward and refuses retirement, instead of re-storing the captured record or adopting a successor. Handler context follows the same lifetime.

Recording health pins its resource across runner observation and reads the handle's latest state afterward. Intervening record rebuilds remain valid; changed handles, fences and retired sessions stay untouched.

Removes the health upsert and late binding lookup. Four files; 180 gross lines. Part of #3116, stacked on #3142.

Validation

Head 47b63ecc10: 45 focused tests passed. Three production mutants failed the intended resource, stale-upsert and context assertions; restored tests passed. Quick checks and parent-scoped Fallow passed.

pnpm check:affected --base fix/session-ownership-review --run passed on this exact head, including 796 related tests and all selected local gates. Independent read-only audit found no actionable findings. CI and live device validation remain pending.

Review in cubic

@github-actions

github-actions Bot commented Oct 3, 2026 •

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
Installed (including dependencies) 4.96 MB 4.96 MB +104 B
Package (unpacked) 4.96 MB 4.96 MB +104 B
Package (download) 1.49 MB 1.49 MB +37 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 27.2 ms 27.0 ms -0.2 ms
CLI --help 83.9 ms 83.9 ms -0.0 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.

No issues found across 4 files

Re-trigger cubic

@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

The change in 47b63ec looks right to me, but the live iOS run for the runner-backed recording route is still missing, and Smoke Tests failed on a step this diff does not touch.

src/daemon/request-recording-health.ts:14 now inspects the handle after runner observation and checks that the resource is still the same object. That sits on the iOS runner-backed recording route (runner AVAssetWriter with showTouches), and the PR body says live validation is still pending. Without a live run, nothing shows that a real recording still adopts its runner id on the first command, still invalidates after a runner restart, and is not skipped because some production path rebuilds the screenRecording object. Please run this on an iOS simulator at this head: record start with the touch overlay (runner backend), then snapshot. The request log must show the recording with runnerSessionId adopted. Then kill the runner process and run snapshot again. It must fail with COMMAND_FAILED "iOS runner session restarted during recording" or "exited during recording". Finish with record stop.

Not blocking: the "locked request preparation keeps its captured lifetime" cases in request-recording-health.test.ts could move to request-execution-scope.test.ts, which already mirrors that source module. Take it or leave it.

I judged the regression from a read of the pre-change code. I did not run the focused tests or the claimed mutants. I also did not check whether any production path rebuilds the screenRecording object with the same handle and fence while a request is being prepared. Such a rebuild would make health observation skip silently, which is conservative, not wrong.

Smoke Tests failed at step 7, the fixture home-screen text wait, with reason wait_readiness_exhausted in the runner-start phase, so the iOS runner never started within 60s. This looks unrelated: the diff only changes behavior when a session holds a runner-backed screen recording, and that scenario has none at that point. The TS2883 DurableCaptureSlotClearResult errors are in files this diff does not touch, so they appear to come from the base stack, and the build still completed. I attributed that from file paths only, not from base CI logs. A rerun of Smoke Tests should clear it.

Before merge, the live iOS run at this head must show the runner id adopted and the invalidation after a runner restart, and Smoke Tests must pass on a rerun.

@thymikee
thymikee added this pull request to stack #3146 October 3, 2026 08:50
@thymikee

thymikee commented Oct 3, 2026 •

Copy link
Copy Markdown
Member Author

I traced the requested live route at 47b63ecc10. An iOS Simulator cannot exercise this predicate: startAppleRecording selects the simulator implementation, whose handle uses simctl recordVideo. refreshRecordingHealth requires runner AVAssetWriter with touches enabled. Reattachment preserves that backend boundary.

Update, 2026-10-03: a wired CoreDevice-backed iPhone 17 Pro is now connected and unlocked. The real Settings open succeeds, but runner build-for-testing fails before recording starts. Xcode reports No Accounts: Add a new account in Accounts settings and Signing certificate is invalid for the existing Apple Development identity, including when its actual team is supplied. The user has been asked to configure working signing. The test session was closed and its isolated daemon stopped with cleanupConfidence: known, no pending provider releases and no warnings.

The runner-exit/restart live control remains outstanding. The simulator smoke rerun passed on 47b63ecc10; that does not establish physical recording health. The physical attempt used runtime source at 26db357542 and did not reach a recording or health probe.

One acceptance detail also needs correction: a successful current physical recording start already requires runnerSessionId and seeds it into the handle. A live test can prove that ID remains coherent and that an exited/restarted runner invalidates recording. Missing-ID adoption is covered by the helper controls; I will not manufacture a missing ID in a normal live recording to claim that route is reached.

@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

Thanks, you are right. On a simulator startAppleRecording always uses simctl recordVideo, so my requested simulator run could not reach recordingRequiresRunnerHealth. I withdraw that request.

The live check for 47b63ec now needs a CoreDevice-backed physical iPhone or iPad: record start with touches, snapshot, a runner restart, then snapshot again, which should fail with the runner-restart error. Until that run happens, or the maintainer accepts the unit controls as enough for this route, the code verdict stays the same and only this evidence is open.

@thymikee
thymikee force-pushed the fix/session-ownership-review branch from 958983e to 1f29268 Compare October 3, 2026 14:41
@thymikee
thymikee force-pushed the fix/request-session-health branch from 47b63ec to 16c4bee Compare October 3, 2026 14:41
@thymikee
thymikee force-pushed the fix/session-ownership-review branch from 1f29268 to cc0643b Compare October 3, 2026 15:16
@thymikee
thymikee force-pushed the fix/request-session-health branch from 16c4bee to 4829ba2 Compare October 3, 2026 15:16
@thymikee
thymikee force-pushed the fix/session-ownership-review branch from cc0643b to f095e6e Compare October 3, 2026 16:41
@thymikee
thymikee force-pushed the fix/request-session-health branch from 4829ba2 to f6a58ef Compare October 3, 2026 16:41
@thymikee
thymikee force-pushed the fix/session-ownership-review branch from f095e6e to 995874e Compare October 3, 2026 16:56
@thymikee
thymikee force-pushed the fix/request-session-health branch 2 times, most recently from 1eec328 to 2507aae Compare October 3, 2026 17:49
@thymikee
thymikee force-pushed the fix/session-ownership-review branch from 995874e to 1a6fe40 Compare October 3, 2026 17:49
@thymikee
thymikee removed this pull request from stack #3146 October 3, 2026 19:38
@thymikee
thymikee force-pushed the fix/request-session-health branch from 2507aae to a679c1e Compare October 3, 2026 19:39
@thymikee
thymikee force-pushed the fix/session-ownership-review branch from 1a6fe40 to 6c3e88b Compare October 3, 2026 19:39
@thymikee
thymikee added this pull request to stack #3187 October 3, 2026 19:45
@thymikee
thymikee removed this pull request from stack #3187 October 3, 2026 20:59
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

Consolidated into #3140 as part of reducing #3116 to seven PRs. The composition preserves the complete pre-consolidation source tree, including tests and later review corrections. This PR is superseded; its review discussion and native evidence remain available. Outstanding findings transfer to the owning keeper in the implementation record.

@thymikee thymikee closed this Oct 3, 2026
@github-actions

github-actions Bot commented Oct 3, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-10-03 21:16 UTC

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant