fix(record): keep the daemon alive when a record request times out - #3199
Conversation
A record stop export routinely outlasts the 90s client envelope on long recordings. The default reset-daemon policy SIGKILLed the daemon mid-export, leaving the session's screen-recording manifest open with no owner. A record-only session holds no device claim, so nothing reconciled it, and every later record start on the device refused with cleanup-unconfirmed until that exact session ran record stop. record now preserves the daemon on timeout: the export finishes, a retried record stop serves it, and the next record start is admitted. The local timeout hint now names that retry, as the remote one already did.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
Reviewed f2f48a7... correction: the reviewed commit is f2f48a8. The code looks good, and I found nothing that blocks merge. CI is green, with one check reported and passing, and there are no conflicts. Not blocking: the repeat-safe The open inline thread from Cubic on the "still exporting" hint wording still applies (#3199 (comment)). A stop that was still queued is dropped at lock entry, so no export ran. The retry instruction is right, and only the wording overstates. The PR body claims live simulator validation, but I could not see a transcript, so I can't confirm it. I did not trace physical iOS or macOS runner-backed recordings. |
A record stop that times out while still queued for the device lock is dropped at lock entry, so no export ran. The timeout hint now says the daemon may still be exporting and keeps the retry instruction. The docs no longer call the in-progress export typical only for a remote daemon, since a local daemon now survives the timeout too.
The client's timeout cleanup pkills every Apple runner xcodebuild on the host. On a physical iOS device or macOS the runner is the recorder, so a timed-out record stop could kill the export the preserved daemon was still finishing. record now skips that sweep. The daemon still cancels its own runner work for a timed-out request through the request signal, and the exclusion is keyed on the command, never on the declared platform.
|
Two commits on top of f2f48a8:
Live run on ba61c63 below. I didn't do a runner-backed stop: simulators always use simctl and I didn't have a cabled device. The simulator run shows the cleanup no longer firing (0 terminated, runner pid unchanged) and the retried stop returning the file. Not changed: other commands' timeouts still sweep every runner on the host. Happy to open an issue for that. Live run transcriptLive run:
|
|
This PR is ready. The cubic-dev-ai P2 thread on the retry hint (daemon-client-timeout.ts:141-148) is fixed at ba61c63, so you can resolve it: #3199 (comment). The earlier findings from f2f48a8 are addressed, and the one check reported on this head passes. Not blocking, and you can take or leave these: a timed-out local The runner-backed recording path (physical iOS over CoreDevice, and macOS) was not run live, and the PR says so. Coverage is the unit and route tests plus a simulator run in the transcript. I did not run the tests locally or reproduce the transcript. No conflicts. Before merge, sequence this against #3193 or rebase onto it. |
thymikee
added
the
ready-for-human
Rebase resolution against main@294dc7d (#3199), which rewrote the same hint formatter this PR restructures. Both changes are semantic and both survive here: - #3199 made `record` a preserve-daemon policy and generalized the keep-exporting retry to LOCAL timeouts, because a preserved daemon may still be exporting (and a stop queued for the device lock is dropped before any export starts, hence "may"). This PR's shape decides "preserved" from the probe verdict + declared policy instead of the old sweep/reset booleans, so the retry branch keys on `!resetDaemon` and `action === 'stop'` BEFORE the remote split, with the remote wording merely adding "remote ". The old remote-only "is still exporting" claim becomes the shared "may still be exporting" one at #3199's second commit. - The reset branch stays this PR's: a daemon the probe proved unresponsive was SIGKILLed, is no longer exporting, and gets the probe-verdict wording with no keep-exporting promise — the exact case #3199's note about a mid-export reset predates, where the probe now makes the reset rarer. Route coverage gains a local `record stop` row: the positional rides the real transport context into the hint, the preserve policy must skip the probe entirely (connections: 1), and no sweep call may fire — #3199's runner-survival requirement, which this PR satisfies by removing the sweep for every command instead of excluding `record` by name.
Rebase resolution against main@294dc7d (#3199), which rewrote the same hint formatter this PR restructures. Both changes are semantic and both survive here: - #3199 made `record` a preserve-daemon policy and generalized the keep-exporting retry to LOCAL timeouts, because a preserved daemon may still be exporting (and a stop queued for the device lock is dropped before any export starts, hence "may"). This PR's shape decides "preserved" from the probe verdict + declared policy instead of the old sweep/reset booleans, so the retry branch keys on `!resetDaemon` and `action === 'stop'` BEFORE the remote split, with the remote wording merely adding "remote ". The old remote-only "is still exporting" claim becomes the shared "may still be exporting" one at #3199's second commit. - The reset branch stays this PR's: a daemon the probe proved unresponsive was SIGKILLed, is no longer exporting, and gets the probe-verdict wording with no keep-exporting promise — the exact case #3199's note about a mid-export reset predates, where the probe now makes the reset rarer. Route coverage gains a local `record stop` row: the positional rides the real transport context into the hint, the preserve policy must skip the probe entirely (connections: 1), and no sweep call may fire — #3199's runner-survival requirement, which this PR satisfies by removing the sweep for every command instead of excluding `record` by name.
…ts budget against record's envelope - `exportProcessedVideo` uses `Deadline.fromTimeoutMs(...).remainingMs()` instead of a hand-rolled `Date.now() + budgetMs`. - The budget's comment no longer says the client resets the daemon: since callstack#3199 `record` keeps it on timeout, and the caller gets "Daemon request timed out" while the daemon finishes the stop. - `HELPER_EXIT_GRACE_MS` names what it covers: start-up, then verifying the output or cancelling the export, and exiting. - The check that the budget fits `record stop`'s envelope moves from a 90_000 literal in `overlay.test.ts` to the root timeout-policy test, against `record`'s resolved envelope.
…e record request (#3219) * fix(recording): render the touch overlay at most 30 fps and inside the record request * refactor(recording): take the overlay deadline from host-kit; check its budget against record's envelope - `exportProcessedVideo` uses `Deadline.fromTimeoutMs(...).remainingMs()` instead of a hand-rolled `Date.now() + budgetMs`. - The budget's comment no longer says the client resets the daemon: since #3199 `record` keeps it on timeout, and the caller gets "Daemon request timed out" while the daemon finishes the stop. - `HELPER_EXIT_GRACE_MS` names what it covers: start-up, then verifying the output or cancelling the export, and exiting. - The check that the budget fits `record stop`'s envelope moves from a 90_000 literal in `overlay.test.ts` to the root timeout-policy test, against `record`'s resolved envelope. * test(recording): the overlay budget leaves record stop a 20 s reserve inside its envelope The check let the budget take all but a millisecond of the envelope; the recorder stop, the copies and the playability checks around the overlay need room beyond it.
2 participants
Summary
On iOS simulators,
record stopon a long recording can outlast the 90s client timeout (we saw 40–122s).recordused the defaultreset-daemontimeout policy, so the client killed the daemon mid-export. That left the session's screen-recording manifest in a non-terminal state. A record-only session holds no device claim, so nothing reconciles it, and every laterrecord starton that device fails withcleanup-unconfirmed("has not reached a confirmed terminal state") until someone runsrecord stopon that exact session.In practice this breaks retries in tester-army/e2e: attempt 0 fails, its video stop times out, and attempt 1 can't start recording.
recordnow usespreserve-daemon, likesnapshotandpress: the export finishes, a retriedrecord stopreturns it (#2534), and the next start is admitted. The local timeout hint now says to retryrecord stop, as the remote hint already did. 5 files.Validation
f2f48a8a4:pnpm check:affected --runpasses. New route test: a localrecord stoptimeout never signals the daemon (fails without the fix).record startfails withcleanup-unconfirmed. This branch: the daemon survives, the retried stop returns the video, and the nextrecord startsucceeds.recordrequest no longer resets the daemon; it's cancelled like other preserve-daemon commands.