Skip to content

fix: keep delayed touch frames within their recording lifetime - #3174

Closed
thymikee wants to merge 1 commit into
chore/session-write-scanner-reviewfrom
fix/capture-reference-frame-ownership
Closed

thymikee wants to merge 1 commit into
chore/session-write-scanner-reviewfrom
fix/capture-reference-frame-ownership

Conversation

@thymikee

@thymikee thymikee commented Oct 3, 2026 •

Copy link
Copy Markdown
Member

Summary

A delayed touch-frame probe publishes only while its captured session lifetime still owns the same recording resource. One private function owns that check for Android size and snapshot probes; rebuilding the current session record remains valid.

Finish the probe review from #3170 for #3116. Two files changed; based on #3172. The parent fixes declaration generation, so this follow-up needs no additional public type.

Validation

Reconciled head: f71996e1978aa6970fefeb991d8298f6aebac616.

  • Exact-head pnpm check:affected --base 14c30ab142d4e977ab6467f3fc84a070f59e0294 --run passes: static/build checks and 971 tests across 172 files.
  • git range-diff confirms an unchanged patch. Retained regression proof: four stale-probe controls fail before the fix; eight owning controls and 20 binding/caller controls pass afterward. Ownership mutations are rejected.
  • Android visual proof, at 6eb2b7cdce: touch-enabled recording, point navigation, a 1080×2400 frame and confirmed MP4 export. Helper/test bytes remain identical. Scratch resources were cleaned up.
  • Prior CI, at 09bd6bb6d2, passes core checks and all four platform smoke lanes. Checks on the reconciled head are pending. The physical runner-health check remains blocked by signing.

@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

Live Android verification at 6eb2b7cdce81b7541108fc36d36ad6f8ae3e5de6:

  • Fresh private Android 36 / Pixel 7 emulator. snapshot -i used android-helper, version 0.21.18, through the persistent-session transport.
  • Started app-scoped adb screenrecord with touches enabled. Its initial live handle has no touch reference frame. The first point click therefore reaches this PR's awaited Android size probe.
  • Clicked the observed Network & internet row at (428,714). Public response returned targetKind: point, referenceWidth: 1080, referenceHeight: 2400; the subsequent snapshot showed Internet, SIMs and Airplane mode.
  • Gesture telemetry independently recorded the tap at tMs: 61512 with the same coordinates and reference frame. The attached five-second excerpt (59–64s of the original recording) visibly shows the tap overlay and navigation; the screenshot shows the resulting page.
  • record stop reported recorder: confirmed, nativePathDisposition: retired. The original H.264 MP4 is 1080×2400, 141.66s; host wall duration was 178.968s. The excerpt is shortened for review.
  • Closed the original session, stopped its isolated daemon with known cleanup/no warnings or pending releases, then shut down and deleted only the test AVD. adb devices is empty.

This proves current-recorder publication through the real route. Retirement and replacement races are controlled by the colocated tests, including republishing the same recorder into a new lifetime. Physical iPhone runner-health verification is a separate pending check.

3116-frame-live-point-proof.mp4

alt=Network and internet after recorded point interaction

@thymikee
thymikee added this pull request to stack #3175 October 3, 2026 16:07

@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

@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 +12 B
Package (unpacked) 4.96 MB 4.96 MB +12 B
Package (download) 1.49 MB 1.49 MB +3 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.7 ms 26.9 ms +0.2 ms
CLI --help 81.6 ms 83.0 ms +1.5 ms

@thymikee
thymikee removed this pull request from stack #3175 October 3, 2026 16:15
@thymikee
thymikee force-pushed the fix/capture-reference-frame-ownership branch from 6eb2b7c to 1b08b4f Compare October 3, 2026 16:19
@thymikee
thymikee force-pushed the chore/session-write-scanner-review branch from 26db357 to bc132d7 Compare October 3, 2026 16:19
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

Typecheck fails at 1b08b4f, so this PR cannot merge yet. The head dropped the definition.ts hunk that existed at 6eb2b7c. DurableCaptureSlotClearResult is no longer exported, but index.ts and session-capture-binding.ts still import it. Repo Guards (freerange), Typecheck & Package and Coverage all fail with TS2305 at that line. The base does not define the type either, so the PR introduces the error. definition.ts still types clear() with the inline union, so the declaration fix the PR describes is not at this head. Please restore export type DurableCaptureSlotClearResult = 'cleared' | 'retired' | 'resource-changed'; before DurableCaptureSessionBinding, and have clear() return it. Then re-run typecheck and pnpm check:affected at the final head, and refresh the PR body, which still names 6eb2b7c.

The live Android run was captured at 6eb2b7c, and I could not re-verify it. Once typecheck is green, one line saying the production source is unchanged since that run is enough. The Smoke Tests and Bundle Size jobs were cancelled or still running after the restack, so they say nothing yet. I did not run tests or tsc myself. The CI attribution comes from the logs and the head tree.

Not blocking, and you can take or leave it: when the recording resource is replaced mid-probe, rememberFrame returns undefined and the point response drops referenceFrame for that touch. That looks intended, so you could state it in the test name.

@thymikee
thymikee force-pushed the chore/session-write-scanner-review branch from bc132d7 to 2ce928f Compare October 3, 2026 17:22
@thymikee
thymikee force-pushed the fix/capture-reference-frame-ownership branch from 1b08b4f to 09bd6bb Compare October 3, 2026 17:22
@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

Fixed at 09bd6bb6d21c0443f675117f85c05c82cfd33a9a. The rebase left a stale type export. The parent already uses the inline clear-result union, so I removed the redundant export and annotation; this PR now changes only the probe helper and its tests.

Exact-head pnpm check:affected --base 2ce928f5e6dc8a89d928889a97582f3ad55c2057 --run passes, including typecheck, Fallow, build and 971 tests across 172 files. There are no TS2305/TS2883 diagnostics. The focused owning/caller/binding run passes 28 controls. The PR body now names this head and these results; new-head CI is still pending.

The production probe helper and its tests are byte-identical to the Android live run at 6eb2b7cdce, verified by a file-scoped diff. The live proof retains that original SHA.

The replaced-recording controls expect undefined and verify that neither the captured nor replacement handle receives a frame. That refusal is intentional: a completed probe has no authority to publish a frame for a different recording.

@thymikee

thymikee commented Oct 3, 2026

Copy link
Copy Markdown
Member Author

Consolidated into #3144 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:14 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