Repository navigation
fix(record): start the touch overlay budget when record stop starts finalizing - #3697
Conversation
|
Size Report
Startup median (7 runs, lower is better):
|
![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 6 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
|
The code in 2d09bbb looks right to me: the touch overlay now gets its budget from when record stop starts finalizing, so earlier waits no longer leave it with nothing. I did not run the tests or the live cold-start comparison here, and I took the helper --timeout-ms values from the PR body. Regression coverage is judged from reading the pre-change code. Not blocking: the finalization deadline in Please keep in mind that the budget does not bound the waits before the overlay (a cold 30 s validator compile plus up to 12 polls), only what the overlay gets after them. The EAS cloud exit 1 was not reproduced, so it is unproven that this change alone removes it. The Cubic P2 thread on the unbounded post-export wait still applies: #3697 (comment). That wait existed before this PR and the 20 s reserve in Smoke Tests failed at "open regular visible-depth fixture" with a |
…inalizing The overlay budget started when the overlay did, after the playability checks that run first. On a cold host those compile their validator (measured 5.8 s, 21.6 s at background QoS; its own budget is 30 s), so the overlay could still spend its full 70 s on top and push record stop past the 90 s request envelope: the client exits 1 with "Daemon request timed out" while the daemon finishes with an overlayWarning that the next record stop replays. The finalizer now starts the budget before those checks and hands it to the overlay, so their time comes out of the overlay's share instead of being added to it. The overlay no longer repeats the stable-file and playability waits the finalizer has just run on the same file.
The playability wait after the export polled on its own schedule, so a slow check could still run past the overlay budget and rename the composited file afterwards. It now stops at the deadline, and an expired budget keeps the recorded video.
2d09bbb to
3cc6003
Compare
|
Addressed at 3cc6003 (rebased on main after #3613):
|
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="website/docs/docs/commands.md">
<violation number="1" location="website/docs/docs/commands.md:959">
P3: The finalizer runs another `isPlayableVideo` check after the overlay returns, outside the deadline, so this wording overstates what the fixed budget covers. Say that it covers the pre-overlay checks and export verification, and disclose that the final check runs afterward.</violation>
</file>
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
| - iOS recording works on simulators and CoreDevice-backed physical devices. Physical devices capture runner screenshots, not a native video stream: no audio, dropped frames are expected, and the default is 15 FPS. `--fps <n>` (1-120) caps the rate; a busy app or long command yields fewer frames. | ||
| - `--quality medium|high` (default `medium`) sets export quality on Android and Limrun; Apple export always keeps the captured resolution. Numeric `5`-`7` map to `medium` and `8`-`10` to `high`. | ||
| - With touch visualization on, recordings also produce a gesture telemetry sidecar. Burned-in touch overlays export only on macOS hosts; elsewhere `record stop` returns the raw video and telemetry with `overlayWarning`. The overlay renders at most 30 FPS (or `--fps` if lower) within a fixed budget; an export past the budget returns the raw video with `overlayWarning`. | ||
| - With touch visualization on, recordings also produce a gesture telemetry sidecar. Burned-in touch overlays export only on macOS hosts; elsewhere `record stop` returns the raw video and telemetry with `overlayWarning`. The overlay renders at most 30 FPS (or `--fps` if lower) within a fixed budget, which also covers the video checks that run before and after it; an export past the budget returns the raw video with `overlayWarning`. On a host that has not run a recording before, part of that budget goes to one-time tool setup, so the first `record stop` of a long recording is more likely to return the raw video. |
There was a problem hiding this comment.
P3: The finalizer runs another isPlayableVideo check after the overlay returns, outside the deadline, so this wording overstates what the fixed budget covers. Say that it covers the pre-overlay checks and export verification, and disclose that the final check runs afterward.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At website/docs/docs/commands.md, line 959:
<comment>The finalizer runs another `isPlayableVideo` check after the overlay returns, outside the deadline, so this wording overstates what the fixed budget covers. Say that it covers the pre-overlay checks and export verification, and disclose that the final check runs afterward.</comment>
<file context>
@@ -956,7 +956,7 @@ agent-device record stop # Stop active recording
- iOS recording works on simulators and CoreDevice-backed physical devices. Physical devices capture runner screenshots, not a native video stream: no audio, dropped frames are expected, and the default is 15 FPS. `--fps <n>` (1-120) caps the rate; a busy app or long command yields fewer frames.
- `--quality medium|high` (default `medium`) sets export quality on Android and Limrun; Apple export always keeps the captured resolution. Numeric `5`-`7` map to `medium` and `8`-`10` to `high`.
-- With touch visualization on, recordings also produce a gesture telemetry sidecar. Burned-in touch overlays export only on macOS hosts; elsewhere `record stop` returns the raw video and telemetry with `overlayWarning`. The overlay renders at most 30 FPS (or `--fps` if lower) within a fixed budget, which also covers the video checks that run before it; an export past the budget returns the raw video with `overlayWarning`. On a host that has not run a recording before, part of that budget goes to one-time tool setup, so the first `record stop` of a long recording is more likely to return the raw video.
+- With touch visualization on, recordings also produce a gesture telemetry sidecar. Burned-in touch overlays export only on macOS hosts; elsewhere `record stop` returns the raw video and telemetry with `overlayWarning`. The overlay renders at most 30 FPS (or `--fps` if lower) within a fixed budget, which also covers the video checks that run before and after it; an export past the budget returns the raw video with `overlayWarning`. On a host that has not run a recording before, part of that budget goes to one-time tool setup, so the first `record stop` of a long recording is more likely to return the raw video.
- Android uses `adb shell screenrecord` (180s limit per clip). Longer recordings are split into MP4 chunks while the daemon stays alive; after a daemon restart, `record stop` recovers only chunks listed in the device manifest. A clip can end before `record stop` when the screen stopped changing; `durationMs` is wall-clock time, and `capturedDurationMs` plus a warning report how much the video covers.
- Limrun sessions always record the whole screen and refuse `--fps` and `--hide-touches` with `INVALID_ARGS`. A failed download is retried by the next `record stop`; nothing survives a daemon restart.
</file context>
| - With touch visualization on, recordings also produce a gesture telemetry sidecar. Burned-in touch overlays export only on macOS hosts; elsewhere `record stop` returns the raw video and telemetry with `overlayWarning`. The overlay renders at most 30 FPS (or `--fps` if lower) within a fixed budget, which also covers the video checks that run before and after it; an export past the budget returns the raw video with `overlayWarning`. On a host that has not run a recording before, part of that budget goes to one-time tool setup, so the first `record stop` of a long recording is more likely to return the raw video. | |
| - With touch visualization on, recordings also produce a gesture telemetry sidecar. Burned-in touch overlays export only on macOS hosts; elsewhere `record stop` returns the raw video and telemetry with `overlayWarning`. The overlay renders at most 30 FPS (or `--fps` if lower) within a fixed budget, which also covers the video checks before the overlay and its export verification; the final video check runs afterward. An export past the budget returns the raw video with `overlayWarning`. On a host that has not run a recording before, part of that budget goes to one-time tool setup, so the first `record stop` of a long recording is more likely to return the raw video. |
|
The PR is ready at 3cc6003. The expired-budget refusal in the overlay (overlay.ts:107-114) and the polling stop in video.ts:134 fix what I raised on 2d09bbb. Not blocking, and you can take or leave these: https://github.com/callstack/agent-device/blob/3cc6003/packages/capture-kit/src/recording/overlay.ts#L108 can throw away a composite that became playable on the last poll, so the wait could report whether the file played and refuse only when it did not. The finalizer's playable-video check at https://github.com/callstack/agent-device/blob/3cc6003/src/platform-runtime-screen-recording-finalizer-host.ts#L56 runs after the overlay and outside the deadline (the validator can take up to 10 s). Moving the unplayable-output refusal into exportProcessedVideo, where the deadline already lives, would settle that. Rewording commands.md:959 to say "before it and the overlay's own output check" would also settle it. The cubic-dev-ai P3 thread on the finalizer check still stands (#3697 (comment)). The P2 thread on the overlay deadline is fixed at this head, so you can resolve it (#3697 (comment)). I did not run the new tests; I judged them by reading the diff. The live run is from 2d09bbb, so the expired-budget branch is covered only by unit tests, and the EAS exit 1 over 90 s was not reproduced. A single playable-video check inside the wait can still run up to 10 s past the deadline. The 20 s slack under the 90 s envelope likely covers that, but I did not measure it. Coverage is still running, and it runs the capture-kit recording and finalizer-host unit tests this PR touches, so a red result would need a look. The earlier Smoke failure was a simctl openurl timeout, which does not touch the record-stop finalization route. Before merge, Coverage needs to finish green. |
thymikee
added
the
ready-for-human
1 participant
Summary
Found while driving an EAS cloud simulator session. A 62 s recording's touch overlay ran out of its 70 s budget, the first
record stopexited 1, and a secondrecord stopreturned the same file with the overlay warning.On the local CLI path, an overlay timeout already exits 0 and shows
Overlay warning:. The second stop returning the file is the replay from ADR 0024 and #2534. The likely cause of exit 1 is that the stop ran past the 90 s request window. The overlay budget started only when the overlay itself started, so the playability checks before it were not counted. On a cold host those checks first compile a validator (5.8 s measured here, 21.6 s at background QoS, with a 30 s budget of its own), and 70 s on top of that can overrun the window.The finalizer now starts the budget before those checks, so their time comes out of the overlay's share. The overlay also stops repeating the waits the finalizer just ran. 6 files.
Validation
Tested commit:
3cc60037e6(rebased on main after #3613).pnpm check:affected --runpasses (1303 related tests). All new tests fail on base: the overlay test gets70000where45000is expected; with the second commit reverted, the post-export check runs past an expired budget and renames the composited file, and the wait polls its full schedule. Live run below is from2d09bbbbf9; the second commit only changes what happens after the budget runs out.Live run on an iPhone 17 simulator with a cold
AGENT_DEVICE_SWIFT_CACHE_DIR, a short recording with touches, thenrecord stoptwice. The helper's--timeout-mswas read frompsduring the first stop.2a2f3f29062d09bbbbf9--timeout-msOn base, a 5-minute recording overran the overlay budget and
record stopstill exited 0 with the warning (72 s).Risk: I could not push a local stop past 90 s, so the EAS exit 1 was not reproduced. The first stop's stderr there should show
Daemon request timed out.