Skip to content

fix: make Android record stop survive static screens, slow moov finalization, and dead-pid recovery - #1651

Merged
thymikee merged 2 commits into
mainfrom
claude/inspiring-gagarin-2224ab
Aug 6, 2026
Merged

fix: make Android record stop survive static screens, slow moov finalization, and dead-pid recovery#1651
thymikee merged 2 commits into
mainfrom
claude/inspiring-gagarin-2224ab

Conversation

@thymikee

@thymikee thymikee commented Aug 6, 2026

Copy link
Copy Markdown
Member

Symptom

record stop on Android fails with failed to copy recording chunk 1: … pulled file is not a playable MP4 (reproduced 2/2 on a loaded Pixel_7_CI emulator), and separately, once a daemon dies mid-recording and screenrecord exits, every retry fails forever with active Android recording manifest could not be verified.

Root causes (all live-verified on Pixel_7_CI, 2026-08-06)

1. The video validator rejected valid recordings (the dominant failure). An 8s recording of a fully static screen legitimately contains a single frame — screenrecord only encodes on screen updates — and AVFoundation reports its duration as 0. The Swift validator's CMTimeGetSeconds(duration) > 0 check rejected these complete, ffprobe-valid files every time, so no retry budget could help. Whether a run passed depended on whether anything (even the status-bar clock minute tick) changed during the window — which made it look like a load-dependent race.

2. Finalization is invisible to size-based gates. The remote file layout is [ftyp][free→moov][mdat]: screenrecord pre-reserves moov space and patches it in place on stop, so the file size never changes when the recording becomes playable. Early pulls (the genuine 1–3s race under load) have a free placeholder where the moov belongs. The duration > 0 check was also accidentally load-bearing here: AVFoundation reports a moov-first file with a truncated mdat as playable, so relaxing duration alone would have lost truncation detection.

3. Recovery misread the pid-gone signature. toybox ps -o pid=,args= -p <missing-pid> exits 1 with completely empty output (verified: -p 99999 → exit 1 empty; -p 1 → exit 0 with init line). The recovery liveness probe treated every non-zero exit as "uncertain adb failure", so a finished recording behind a live-status manifest — finalized MP4 and manifest sitting on /sdcard — was unrecoverable forever.

Fixes

  • src/utils/video.ts: drop the duration > 0 requirement (single-frame recordings are valid) and make the ftyp+moov container sniff a mandatory precondition in isPlayableVideo — it is the finalization oracle for in-place-patched captures and restores truncation detection honestly.
  • src/daemon/handlers/record-trace-android-copy.ts: replace the single 750ms retry with escalating re-pull delays (750/1500/3000ms — outlasts the observed finalization window with margin); drop the local waitForStableFile call (a pull is complete when adb returns; this also removes ~500ms of dead wait from every Android stop). No remote stat polling: size cannot signal an in-place moov patch.
  • src/daemon/handlers/record-trace-android-recovery.ts: a probe failure with empty stdout+stderr corroborates via the full process list (ps -A, reusing the pending-path helper): healthy listing with the pid absent → recover as finished and pull; listing failure → stay conservatively uncertain. The corroboration is a safety requirement, not politeness — a stale verdict deletes the manifest, so transport health must be proven first. Genuine transport failures keep their conservative path: they leave stderr, and exec-layer timeouts throw before this branch runs.

Tests

Two new provider scenarios, both verified to fail on the previous implementation with the live-observed errors:

  • retries the pull until in-place moov finalization lands — same-size freemoov content flip landing only after the second pull (inside the observed window, past the old 2-attempt budget).
  • recovers finished recording after dead-pid probe — live manifest, ps -p exit-1-empty, responsive device, finalized remote file → stop recovers, pulls once, cleans the manifest.

The existing guard tests are untouched and still pin the conservative behavior: the stale-manifest flow never enters the new branch (its probe exits 0) and still never calls ps -A; the uncertain-probe fixture uses non-empty stderr and stays uncertain.

Live validation

  • 3/3 open → record start → 8s idle → record stop cycles under 5-core CPU load pass, including one producing a duration-0 file (ffprobe duration N/A) — the class that failed 100% before.
  • Recreated the recovery dead-end exactly (SIGKILL the daemon mid-recording, kill -2 screenrecord manually): the next record stop recovers the manifest on the first try and pulls a playable 1080x2400 MP4 with the expected "no longer running / may be truncated" warning.
  • Recompiled validator accepts duration-0 output and still rejects moov-less files.

Full Android recording scenario suite 23/23, recording unit suites green, lint/typecheck/layering/format green.

Follow-up (not in this PR)

The 3232-byte "moov atom not found" manual pull from the original report is real but only reachable when the process-exit gate is bypassed — through the pipeline, waitForAndroidStopExit runs first; the escalating retries cover the residual window where ps lies under load.

…ization, and dead-pid recovery

Three live-reproduced defects on a loaded Pixel_7_CI emulator shared the
"pulled file is not a playable MP4" / "manifest could not be verified"
symptom family:

- The Swift video validator required duration > 0, permanently rejecting
  valid single-frame recordings of fully static screens (AVFoundation
  reports their duration as 0). Whether a run passed depended on whether
  anything — even the status-bar clock — changed during the window.
- screenrecord finalizes by patching a front-reserved moov in place, so
  the remote file size never changes; the copy path now re-pulls with
  escalating delays (750/1500/3000ms) and detects finalization from the
  pulled bytes via a mandatory ftyp+moov container sniff, which also
  preserves the truncation detection the duration check provided by
  accident. The local waitForStableFile call is gone: a pull is complete
  when adb returns.
- toybox `ps -p <missing-pid>` exits 1 with empty output — the normal
  pid-gone signature — but the recovery liveness probe read every
  non-zero exit as an uncertain adb failure, making a finished recording
  behind a live-status manifest unrecoverable forever. Empty-output
  failures now corroborate via the full process list: healthy listing
  with the pid absent recovers the finished recording; listing failure
  stays conservatively uncertain (a stale verdict deletes the manifest,
  so transport health is proven first). Transport failures keep stderr
  and exec-layer timeouts throw, which is what makes the empty-output
  signature safe to trust.

Provider-scenario coverage: in-place same-size finalization landing past
the first retry, and dead-pid recovery on a responsive device — both
fail on the previous implementation with the live-observed errors.
@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.99 MB 1.99 MB +291 B
JS gzip 635.8 kB 635.9 kB +64 B
npm tarball 769.2 kB 769.3 kB +72 B
npm unpacked 2.69 MB 2.69 MB +291 B

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 28.1 ms 27.3 ms -0.7 ms
CLI --help 66.3 ms 67.9 ms +1.6 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/internal/daemon.js +291 B +64 B

@thymikee

thymikee commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

Reviewed exact head 8a510647. The recording fix itself is sound: the real stop path now re-pulls through the observed in-place moov finalization window, requires a finalized container before AVFoundation acceptance, and conservatively corroborates the empty ps -p dead-PID signature with a healthy full process list. Both provider scenarios are production-shaped, revert-sensitive, and backed by strong exact-head Pixel_7_CI evidence. CI is fully green.

  • P2 — this adds behavior/tests to files already beyond the repository’s extraction tripwires. record-trace-android-recovery.ts grows to 763 lines, and test/integration/provider-scenarios/android-recording.test.ts grows from 1,606 to 1,764 lines with another 158-line scenario block. AGENTS.md explicitly requires extraction before adding behavior past 500 LOC and does not exempt tests. Split the finalization/copy scenarios beside the copy module and the dead-PID recovery scenario beside a focused recovery/liveness module; extract the corresponding recovery probe concept so source/test topology remains 1:1.

No functional blocker or conflict found, but no ready-for-human label until the file-shape rule is satisfied.

…s per file-shape rule

record-trace-android-recovery.ts (763 lines) and android-recording.test.ts
(1,764 lines) were both past the 500-LOC extraction tripwire. The
screenrecord liveness/probe concept this PR modified now lives in
record-trace-android-liveness.ts, and the new scenarios moved to test
files mirroring the source modules they cover
(record-trace-android-copy.test.ts, record-trace-android-liveness.test.ts)
with shared scenario plumbing in the android-recording-fixtures.ts
sibling. android-recording.test.ts shrinks to 1,476 lines — below its
pre-PR size.
@thymikee

thymikee commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

Addressed the P2 file-shape finding in 853bd5d:

  • Source: the screenrecord liveness/probe concept (the code this PR modified) is extracted to src/daemon/handlers/record-trace-android-liveness.ts (131 lines: checkRecoverableAndroidScreenrecord, dead-pid corroboration, findLiveAndroidScreenrecordByPath, androidRemoteFileExists). record-trace-android-recovery.ts drops 763 → 661 and now holds only manifest IO + resolution/ownership.
  • Tests (1:1 topology): the finalization scenario moved to record-trace-android-copy.test.ts and the dead-pid scenario to record-trace-android-liveness.test.ts, mirroring the modules they cover. Shared scenario plumbing (manifest builder, scripted adb result, env wrapper, stop helper) moved to the sibling android-recording-fixtures.ts per the shared-fixtures rule. android-recording.test.ts shrinks 1,764 → 1,476 — below its pre-PR 1,606.

All 23 Android recording scenarios pass across the three files; lint/typecheck/layering (1,027 tracked files)/fallow both gates green.

🤖 Addressed by Claude Code

@thymikee

thymikee commented Aug 6, 2026

Copy link
Copy Markdown
Member Author

Re-reviewed exact head 853bd5d4. The P2 file-shape finding is resolved: the changed liveness/probe behavior now lives in the focused 131-line record-trace-android-liveness.ts; the finalization and dead-PID regression scenarios mirror the copy/liveness modules; shared scenario plumbing is centralized in the sibling fixtures module; and the legacy aggregate test is smaller than its pre-PR size. The extraction preserves the production route and the tests remain production-shaped and revert-sensitive. No new code findings or conflicts. All completed checks are green; Android/iOS smoke, size, and native fingerprint jobs are still in progress. Code review is clean and this is ready for human merge review.

@thymikee thymikee added the ready-for-human Valid work that needs human implementation, judgment, or maintainer merge label Aug 6, 2026
@thymikee
thymikee merged commit 8030fc1 into main Aug 6, 2026
25 of 28 checks passed
@thymikee
thymikee deleted the claude/inspiring-gagarin-2224ab branch August 6, 2026 15:38
@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-08-06 15:59 UTC

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