fix: recover completed Android recording from pending-only manifest#1141
Conversation
When the daemon crashes in the brief window between writing the pending recovery manifest (before screenrecord starts) and upgrading it to a `current` manifest, the screenrecord process can still finish and leave a complete MP4 on the device. record stop previously discarded it as stale because the pending-only recovery path never checked for an on-device file, unlike the `current` path which already recovers a finished recording. Extend the pending-only path to recover the completed file with the same finished-recording warning, and skip the stop signal when the recovered recording has no tracked pid (a pending chunk never records one, and probing an empty pid is unsafe).
isRecord accepted arrays (typeof [] === 'object'), so a stray `[]` recovery manifest was classified as blocked rather than deleted, wedging every subsequent record stop. Reject arrays and null so a non-object manifest is cleaned up like other malformed metadata.
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
|
thymikee
added
the
ready-for-human
|
Review pass: I do not see actionable blockers in the pending-only Android recording recovery follow-up. The patch keeps the normal live-pending path unchanged, recovers the finished pending-only file via the same remote-file existence check used by finished Checks are green for the code/test gates I checked; |
1 participant
Summary
Follow-up to #1135 (Android recording durable-manifest recovery). Fixes one confirmed data-loss gap plus a small parser hardening. The other review items either turned out to be already addressed on
mainor looked like deliberate conservative choices — see Scope notes below.1. Recover a completed recording from a pending-only manifest
record startwrites apendingrecovery manifest before the screenrecord process starts, then upgrades it to acurrentmanifest once frames are flowing. If the daemon crashes in that brief window and the screenrecord process later finishes on its own, it leaves a complete MP4 on the device — butrecord stopdiscarded it asstale, because the pending-only recovery path never checked for an on-device file. Thecurrentpath already recovers a finished recording viaandroidRemoteFileExists; this makes the pending-only path consistent.A pending chunk never records a pid (the manifest is written before the process exists), so the recovered recording carries no pid.
finishCurrentAndroidRecordingChunknow skips the stop signal when there is no tracked pid — probing/signalling an empty pid is unsafe (isAndroidProcessRunning('')can report a false positive via[''].includes('')).2. Treat JSON arrays as invalid recovery manifests
isRecordaccepted arrays (typeof [] === 'object'), so a stray[]manifest was classifiedblocked(which is never cleaned up) rather thandelete, wedging every subsequentrecord stop. Now rejects arrays andnull.Validation
pnpm typecheckpnpm lintpnpm formatpnpm check:fallow --base origin/mainpnpm vitest run --project provider-integration test/integration/provider-scenarios/android-recording.test.ts(21 passed, incl. newrecovers finished pending manifest after process exit)pnpm vitest run --project unit-core src/daemon/handlers/__tests__/record-trace.test.ts(48 passed)Scope notes (other review items, deliberately not included)
main: the scoped hint now says "retry record stop from the original working directory without --session," which is the working recovery path. No change needed.ps -pnon-zero exit →uncertain— treating any pid-probe failure as uncertain is a deliberate, tested choice (a test assertsps -Ais not consulted after aps -pfailure). Left as-is; ifps -pturns out to exit non-zero for dead pids on some OEM builds, that's worth revisiting separately.manifests.length > 1→ ambiguous even when exactly one is owned — narrow (needs two live manifests across/sdcard+/data/local/tmp) and consistent with the existing conservative "cannot safely recover" stance. Left as-is.Happy to fold any of these in if you'd like a different call on them.