fix(record): keep a failed capture's evidence for the retry - #2591
Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Keeping a failed stop's evidence fixes the Android In The iOS and macOS recording paths change behavior here, but the only live run is on Android. A simulator run of a failed and then retried |
|
Update: after #2587 landed, the branch conflicts with main in |
ADR 0024 rule 6, shipped first. `record stop` that failed to produce its export ran compensating forced cleanup anyway. On Android that reaches `cleanupVerifiedAndroidEvidence`, which stops the recorders and removes the remote chunks and the device-side manifest: an `adb pull` that failed once under load left nothing to pull twice, and the manifest the next stop would re-drive from was gone with it. The shared coordinator now asks the kind's own definition what a failed finish may do (`failedFinishPolicy`), and never infers disposal from "finish threw". Screen recording declares `preserve-retry-material`, so a failed collect leaves the record `open`, keeps the live handle with the session, and lets the next stop re-collect from the artifacts still on the device. Forced cleanup stays exactly where rule 6 puts it: session teardown and failed-start rollback. App-log, audio-probe, and perf-capture declare `dispose-on-failed-finish` and keep today's behavior unchanged until their own failed-finish test states what their retry needs.
ADR 0024 rule 6 earns its scope per kind. App-log's compensating cleanup is not a disposal of anything a retry reads: `createAppLogLiveHandleFromFinish` derives it from the same idempotent finish transaction, and the captured log file at the canonical artifact path is never on its delete path. So app-log declares `dispose-on-failed-finish` and this test names why: a stop whose first finish attempt fails is retried by that very cleanup (two finish calls, one error returned to the caller), the stream's bytes stay on disk, and the record terminalizes so a replacement stream may start on the same device.
…d finish ADR 0024 rule 6, per kind. Audio-probe's whole capture is the status file its helper publishes; the kind's cleanup terminates the sampler by exact identity and has no path that removes that file. A helper that died before publishing its terminal checkpoint cannot be revived by keeping it alive, so nothing a retry reads is destroyed by disposing here. Audio-probe therefore declares `dispose-on-failed-finish`, and this test drives the real host capture operations through a failed finish to show what that guarantees: the sampler is terminated exactly once by the compensating cleanup, the last checkpoint stays on disk for `audio probe status` and recovery to read, and the record terminalizes so a replacement probe may start.
ADR 0024 rule 6, per kind. Perf-capture's retry needs the profiler artifact: `stopAndroidNativePerfSession` pulls the device trace and only then removes it, and the refusal it raises on a failed pull tells the caller to retry stop with the same session. The compensating cleanup was the `rm -f` that emptied that promise. Perf-capture declares `preserve-retry-material`, and the live handle stops memoizing a refused finish, mirroring the recording handle: a stop the profiler refused has to be re-driven by the next `perf stop`, which is what re-pulls the trace the record now keeps. The test drives a pull failure through the daemon's own perf record and shows the trace surviving the first stop, the record staying open, and the second stop collecting and releasing it.
Versioned help owned nothing about the failed-stop path, so an operator reading `agent-device help record` after a lost export had no signal that the Android artifact and its native manifest were still on the device, that the session's recording manifest stayed open, or that `record stop` in the owning session is the retry — and that only closing the session disposes them.
…aration The package-boundary re-export had no consumer: every durable kind names the literal where it builds its definition, and the one type-level consumer — the capture-kit fixture that builds a preserving definition — resolves the declaration module directly. The dead-code audit on the changed diff flagged the export.
A finish now says why it was asked for. A `capture` finish tries to produce the export and may keep retry material under a preserving kind's policy; a `disposal` finish is session teardown handing the resource back for good, and disposes what a failed finish left no matter what the kind preserves. Without the distinction, the policy I just added to recording and perf-capture reached teardown and left a closing session unable to give its recorder back — the coordinator skipped the forced cleanup that ADR 0024 rule 6 assigns to teardown, and the two teardown tests that watch for it failed. `finishLiveDurableCapture` and `finishDurableCaptureHandle` require the intent rather than defaulting it, so every caller states which authority it acts for; the three teardown wrappers in `session-teardown.ts` are the only disposal callers.
Three tests watched the coordinator compensate a failed `record stop` by force-cleaning the recording and terminalizing its manifest, and one allowed a replacement `record start` over the top of it. Those were the behavior ADR 0024 rule 6 retired, so they now assert the rule: the finish error stays primary, the live handle and the open manifest survive, phase rests at `completing` rather than `cleanup-pending`, and the next `record stop` re-enters the recorder through the same handle — twice called, zero forced cleanups. The two macOS provider scenarios keep their original point that an unfinalized recording is never published as a completed artifact; only the retained state changed.
Two Apple facts made the retry my rule-6 change promises impossible, and the review caught both. The runner path memoized `stopRunner` with `??=`, so once the runner rejected a stop the same rejection came back to every later `record stop` and the runner was never asked again. Only an in-flight or completed stop is shared now, which is what the live handle already does for a refused finish, so the next stop re-enters `recordStop`. The simulator path threw on a non-zero `simctl recordVideo` exit, which made the settled exit code the permanent answer and threw away a recording the recorder had already written. An exit is an observation about the recorder, not about the export (ADR 0024 2.2), and the finalizer is what answers whether the file is a video: the exit is now disclosed — `exited with code N`, or `was killed by SIGKILL` — and collection proceeds. Simulator and macOS recordings that used to fail here now export; a refusal that really stands is re-driven by the next stop instead of being remembered.
A preserved recording can reach a file no retry will ever turn into a video — the simulator recorder SIGKILLed mid-write leaves zero bytes behind. That failure said only what the file is not, with the generated "retry with --debug" hint, while the manifest stays open and `record start` keeps refusing until the session closes. The error now carries `recording-output-unplayable`, stays retriable for the case a retry does fix (a recorder still finalizing its file), and names session close as the way out.
03cfd29 to
7466adb
Compare
|
Rebased on Runner stop — simctl exit — you're right that a settled exit can't be re-driven, so I stopped treating it as a failure. An exit is an observation about the recorder; the finalizer is what answers whether the file is a video (ADR 0024 2.2). The exit is disclosed ( When nothing can be retried — a recorder killed mid-write leaves zero bytes, and no retry fixes that. That error now carries Simulator run (iPhone 16 Pro, booted):
Killing the recorder left the CoreSimulator host recording lock (simctl exit 16) — a simulator shutdown/boot clears it. Nothing left behind: no
|
|
The Apple retry works now at 7466adb. A refused runner stop is asked again, and a simulator recorder that exited early goes to the finalizer, which still refuses a file that is not a playable video. Two gaps remain in the simulator path. When simctl exits with an error and the file is unplayable, the error no longer carries the exit code, signal or stderr. Before this change the error named the exit code. Now the user gets only the generic The new warning can blame the wrong side. When our own stop escalates from SIGINT to SIGTERM or SIGKILL, Also, |
Two wrong blame assignments in the disclosure I added, both from reading the process result as though the host had not already interpreted it. `describeSimctlRecorderExit` checked `signal` first, and the simulator host normalizes a termination record stop *asked for* to exit code 0 with the signal still attached — so our own SIGINT-to-SIGTERM escalation came out as the recorder being "killed by SIGTERM before record stop". The exit code is the normalized fact, so it answers first now. And dropping the exit from an unreadable recording was a regression: the old error named the exit code, the new one named only the file, and every retry waits again on bytes that cannot change. The simulator finish now puts the exit in the error it raises, carries stderr and the exit code/signal under `processExitError`, keeps the unplayable reason, clears a retriable flag it no longer deserves, and points at closing the session. The re-drive test used a clean exit, which main already re-drove, so it uses a non-zero one.
Three finishes ask for a completion nobody reads: `audio start` settles the probe it replaces, `audio status` settles a sampler that already exited, and `logs clear --restart` settles the stream whose files it deletes a line later. They are disposals — whatever runs next expects the record settled, not left open for a retry — and labelling them `capture` made them a trap for any kind that later decides its retry needs material. Audio-probe and app-log dispose on a failed finish either way, so nothing changes today. The intent's own doc said session teardown was the only disposal caller, which is the claim that just narrowed.
|
All three addressed at Exit details — the simulator finish now wraps the export failure instead of replacing it:
Wrong blame — you read the host right. Test — the re-drive test uses Went back over the whole change while doing this: three finishes that only make room were labelled
|
|
All three points from the last review are fixed at c9ed057: the unplayable error carries the recorder's exit, our own stop no longer gets blamed, and the re-drive test fails on main. Two small issues remain in the new wrap.
The exit details are built by hand. Smoke Tests are still running, and there are no conflicts. |
The wrap I added explained too much. It took every error the export path raised — a telemetry write that failed once, a finalize hiccup — gave it `retriable: false` and the "close this session" hint, and dropped the original `hint` along the way, so a failure a retry would fix told the user to give up. It now fires only on `recording-output-unplayable`, keeps the details it was handed and adds the exit facts to them; anything else the export path raises comes through exactly as it was. The reason the two layers match on is now one exported constant instead of a string each, and the exit details come from `execFailureDetails`, whose parameter accepts the null exit code a process killed by a signal reports — the shape `AppErrorDetails.exitCode` already allows.
|
Both fixed at Only the unreadable file is final — the wrap is now gated on the reason, and keeps what it was handed: const original = asAppError(exportError, 'COMMAND_FAILED');
if (original.details?.reason !== RECORDING_OUTPUT_UNPLAYABLE_REASON) return exportError;
return new AppError(original.code, `${original.message}; ${exit}`, execFailureDetails(result, {
...(original.details ?? {}),
...(result.signal === undefined ? {} : { signal: result.signal }),
retriable: false,
hint: '…Close this session to release the device, then record again.',
}));Anything else the export path raised returns untouched, so a transient telemetry or finalize failure keeps its own code, The reason both layers match on is now one exported
|
|
Reviewed at 2a8e330. The retry-path evidence issue from the c9ed057 review is fixed, and I found no new problems in the delta. One question, not a blocker: The iOS simulator smoke job fails with |
thymikee
added
the
ready-for-human
|
Reviewed at 2a8e330. Both points from the last review are addressed, and the delta raises nothing new. There are no conflicts. Smoke Tests failed in the iOS simulator fixture run because |
|
1 participant
Summary
Implements wave 0 of
docs/adr/0024-screen-recording-provable-signal.mdrule 6: an export failure is no longer a disposal. A durable kind now says what its retry needs withfailedFinishPolicy, and the shared coordinator stops force-cleaning on a failed finish for preserving kinds (screen-recording,perf-capture);app-logandaudio-probesaydispose-on-failed-finishexplicitly.A finish also says why it was asked for.
capturemay preserve retry material;disposal— session teardown, the only such caller — disposes whatever a failed finish left. Recording and perf-capture would otherwise have left a closing session unable to hand its recorder back.createCaptureHandle.finishno longer memoizes a refused finish, so a secondrecord stopre-enters the recorder. 29 files, one command family plus the coordinator it sits on.Validation
Tested at
4e1e17084b.pnpm check:affected --runpassed locally (372 files, 2,560 tests), along withpnpm typecheck,pnpm lintandpnpm check:layering.Live proof on Android emulator
emulator-5554(Pixel 7 CI) with the pull path patched to fail its first three attempts:record start/record stopreturned the pull error and wrote no local file, whileadb -s emulator-5554 shell ls -l /sdcard/agent-device-recording-*showed the 90,338-byte chunk andagent-device-recording-active.jsonintact and the session manifestopen. A secondrecord stopexported those bytes and cleared the device. Patch reverted before the pushed head.Known follow-up, not fixed here: on that retry Android infers "stopped before record stop, likely the 180s limit" from its own earlier stop — ADR step 3 recorder observation owns it.