feat(record): export a recording from a copy of what its recorder wrote - #2631
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed at e2ad807, stacked on #2613. Exporting from a collected copy is the right idea, but a retried stop can lose data, and the design is larger than it needs to be. The finalization checkpoint drops fields. The stop clock is not journaled. Could an abandoned stop leave full-size files behind? Is playability now checked up to three times on the same bytes? The chunk-naming rule now has two owners: Resume is the headline behavior, but At about 787 net production lines, could the host interface be two steps instead of four: Smoke Tests fails while waiting for the fixture home screen in the iOS input scenario. That scenario runs no A live resume run is still needed on an iOS simulator and an Android emulator: make the first |
…exists ADR 0024 2.3 makes a stop resumable because a manifest that names durable artifacts lets it be. A stop that dies between signalling the recorder and writing the export loses every fact it established, so the next attempt re-signals a recorder that may have exited and re-runs a finalizer that already overlaid the video. `finish` now receives the manifest the stop is allowed to write into while it holds its fence, and the recording's sequence names the three steps it commits between: signal the recorder, collect a copy past the playability sniff, and finalize that copy into the export instead of the recorder's own path. Each step records its checkpoint before the next begins, so a retry re-signals only a recorder nobody confirmed, collects once, and never overlays a finished export twice. A checkpoint the next attempt cannot vouch for comes back absent rather than half-trusted: resuming from a claim about an artifact that was never written skips the step that would have noticed it.
…bpath The stop sequence is only needed by a backend that drives its stop through it, so it stays out of the capture-kit façade that every backend evaluates at import time.
…orts ADR 0024 2.3 has a stop collect its recorder's artifact and write the export from that copy, which means moving bytes the platform packages do not own and cannot reach: R13 keeps `node:fs` out of them. The output host is the seam that already prepares an output path, so it gains the four steps the question names — pull the recorder's file to a collected path, write the export from that copy, retire the recorder's file while answering whether it worked, and discard the copy once the export stands. Only the host can see a path, so only the host can say `retired` rather than `retirable`. The finalizer now exposes its playability rule on its own, so a collected copy is examined with the same patience and the same verdict the export gets; a container sniff alone would let a file no player can read become the served video. A finalization also carries the chunk files and captured length an export is served as, because those facts are decided with the export and a replay that reuses a committed finalization still has to serve them.
`simctl recordVideo` wrote the file the caller named, so a stop that failed partway left the caller's path holding bytes the next attempt had to reason about, and the only honest answer about that path was `retirable` — the recorder's file and the served video were one file. The recorder is now told a `.native` sibling, and the stop copies it to a `.collected` sibling, checks playability there, and writes the caller's path once from that copy before retiring the recorder's file and reporting whether the removal went through. A stop that dies before the export exists leaves it absent with the copy in place, and a replay resumes from the copy instead of signalling a recorder that already exited. A recording whose file cannot be read now names the recorder exit that explains it whether that surfaces during collection or during finalization, and the exit travels in the stop's own journal, so the second attempt still says it.
…erve copies `adb screenrecord` writes on the device and `record stop` pulled each chunk straight onto the paths the caller named, so a stop that died between the pull and the overlay left the caller's video holding pulled-but-unfinalized bytes, and a retry re-signalled a recorder that had already exited to earn them again. The chunks now land on `.collected` siblings, the first is checked with the shared playability rule there, and the caller's paths are written from copies once the export is what the finalizer overlays. Because both sets follow one naming rule from their own base, an attempt that never ran the pull can still name the collected files a previous attempt left, which is what makes the stop resumable. The 180s platform limit is disclosed by the stop step that learned it and journalled there, so a stop that has to be driven again keeps the disclosure. `completion.ts` no longer completes anything once the shared builder owns the answer, so the live snapshot it also built moved to `live-snapshot.ts` and a failed cleanup's outcome joined the cleanup that returns it.
Two backends adopted the collected copy in the same change, and each fixture grew the same host file model to fail a stop that names a file it never wrote. The model is the answer to one question — where would the output host be holding these bytes — so it lives behind one fixture subpath now, next to the MP4 fixture the Android suite already reads.
…and the facts Turning a collected set into an export answers three questions — put the bytes where the caller looks, ask the finalizer to overlay them, and say what the export turned out to be — and holding all three in one body made each of them hard to read against the others.
The scenario asserted the pull targeted the caller's own path, which is exactly the claim ADR 0024 2.3 retired. It now names the collected sibling and checks the real host afterwards: the caller's video is there, and the copy the stop made is not.
e2ad807 to
17e7a67
Compare
|
Restacked on #2613 after its probe fix; new head Answering the size report directly: +6.3 kB unpacked is over the 3 kB note, and 707 net production lines is over the size guideline, so say the word and I'll cut Android ( |
…ayability once A retried `record stop` replays the finalization its first attempt journaled. The checkpoint codec kept only four of the six fields, so a chunked Android recording whose completion marker or chunk disposal failed came back with only `outPath`: part-002 onward and `capturedDurationMs` were gone. The codec is now keyed over every `ScreenRecordingFinalization` field, so a field added to that type does not compile until it round-trips, and a finalization whose chunks or length cannot be read is redone rather than served short. The stop clock is journaled with the recorder signal. A stop resumed from its collected copy used to read `Date.now()` again and size Android's idle-tail warning by the gap between attempts. Playability is checked once, on the export. The collected copy of a recorder that already exited is the same bytes, and Android's pull already retries until each chunk plays, so the extra validator spawn only added latency to every iOS and Android stop. `validatePlayable` is gone from the finalizer. The output host has two verbs instead of four: `copy` and `remove`. Collect and export were the same copy, and retire and discard were the same removal; HarmonyOS and web fixtures now use the shared file store instead of throwing stubs. Android chunk paths have one naming rule, `chunkPathAt`, used by the pull, the served set and the completion-marker check, and `pullChunks` loses its unused client-path branch. A test drives a failing stop and a second stop through the real fence and lease and asserts one signal, one collect and the served result. The CHANGELOG says an abandoned stop keeps its video in the `.native`/`.collected` siblings, which are then the only copy.
|
Answered the review at e2ad807 in Checkpoint round-trip. Stop clock. The sequence reads the clock before the signal and journals Leftover siblings. Kept, and the CHANGELOG now says so. After an abandoned stop the Playability. Checked once, on the export. Host surface. Why not rename the collected copy into the export. The overlay is not idempotent. If the copy is renamed and the stop dies after the overlay but before the finalization checkpoint, the retry cannot tell a raw export from an overlaid one and would overlay twice or serve a raw file. The copy is what makes that step safe to redo. Chunk naming. One rule, Resume through the fence. Size. Net production lines go from about 696 to 625 against #2613. Live runs on a build whose source differs from
A failure after the finalization checkpoint (the completion marker write) cannot be provoked on demand without fault injection, so that route is covered by the Android unit test above, not by a device run. Gate at |
…ion is journaled, and never serve a refused export ADR 0024 2.3 records the collected checkpoint only after the copy passes the container sniff. The simulator copy now gets that sniff (`finalize.sniff`: ftyp + moov, no validator spawn); the export still gets the full playability verdict once. The collected copy is removed by a `discard` step the sequence runs after the finalization is journaled, and again on a replay. Removing it inside `finalize` left a crash window in which the checkpoint named a copy that no longer existed, so every later stop failed. When the export copy or the finalizer fails, the caller's paths are removed before the error returns. A killed simctl used to leave an unplayable file at `--out` next to a failed stop.
|
Follow-up in The caller's path never holds a refused export. In The collected checkpoint follows the sniff again. ADR 0024 2.3 records The copy is kept until its finalization is journaled. Removing it inside Live, on
Gate at |
thymikee
added
the
ready-for-human
|
Reviewed at e41d66e, the head that merged, as a follow-up to the review at e2ad807. The earlier points are fixed: the finalization checkpoint round-trips An abandoned stop can still lose its video. After an abandoned iOS stop, the video exists only in Do the refused-export tests guard the retry route? The iOS test in runtime.test.ts#L287 and the Android one in On size, about 749 net production lines are over the guideline. Most of it is the Android move, and the copy, complete and remove-on-refusal blocks in All checks passed at this head. The live resume runs in the PR body cover the copy-failure route; a finalizer refusal of a real unplayable export has unit coverage only. Next: a small follow-up that makes a new recording refuse, or document replacing, an abandoned recording's kept files. |
1 participant
Stacked on #2613. Implements the write path of ADR 0024 2.3 for the first two backends.
Summary
A stop now collects what its recorder wrote and produces the export from that copy, so the caller's path is never a file a recorder is still writing, and a stop that dies partway is driven again from the copy instead of re-signalling a recorder that already exited.
ScreenRecordingOutputHostgains the four local steps a copy needs:collectFromRecorder,writeExportFromCollected,retireRecorderFile,discardCollectedFile. Only the host can see a path, soretireRecorderFileis what answersretiredrather thanretirable.ScreenRecordingFinalizer.validatePlayableexposes the playability rule for a collected copy.simctlrecords to<name>.native.mp4; the stop copies to.collected.mp4, checks playability there, writes the caller's path once, then retires the recorder's file..collectedsiblings and copied out; chunk and--client-output-pathnaming is unchanged. The 180s-limit disclosure now rides the stop's journal, so a retried stop keeps it.Validation
pnpm check:affected --runandpnpm check:toolingpass at17e7a67961(restacked on #2613c69438a5fb, which now also fixes the Android presence probe this branch's dispositions read).Live, on the current head:
record start/record stopon iPhone 17 simulator andemulator-5554each produced a playable MP4 withrecorder: confirmed,nativePathDisposition: retired, and no.native/.collectedsibling left on host or device; a stop whose daemon was killed after the export answeredretiredfrom the device, not from the marker it had frozen.Mutations each fail a named test: recorder writing the export, disposition hardcoded, collect skipping playability, marker published after disposal.