Repository navigation
fix(recording): render the touch overlay at most 30 fps and inside the record request - #3219
Conversation
There was a problem hiding this comment.
1 issue found across 4 files
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="packages/capture-kit/src/recording/overlay.ts">
<violation number="1" location="packages/capture-kit/src/recording/overlay.ts:73">
P3: The process-kill timer (`runCmd` `timeoutMs: helperMs`) and the export timer (`--timeout-ms = helperMs - 5s`) start at different moments, but the difference between them is the only budget for everything else that can keep the helper alive past a completed export: helper startup, the post-export `verifyCompositedOverlay`, and exit. Because `runCmd`'s kill fires only when the process is still running at `helperMs` (measured from spawn), a helper whose export ran close to its `--timeout-ms` can be SIGKILLed before it finishes `verifyCompositedOverlay`/exits, discarding a good overlay down the raw-video fallback path. Give the post-export steps their own margin instead of deriving both from a single shared 5s grace.</violation>
</file>
Reply to a comment to ask cubic a question or push back. It learns from your replies.
Re-trigger cubic
| env: buildSwiftToolEnv(), | ||
| }); | ||
| const helperMs = deadline - Date.now(); | ||
| const exportMs = helperMs - HELPER_EXIT_GRACE_MS; |
There was a problem hiding this comment.
P3: The process-kill timer (runCmd timeoutMs: helperMs) and the export timer (--timeout-ms = helperMs - 5s) start at different moments, but the difference between them is the only budget for everything else that can keep the helper alive past a completed export: helper startup, the post-export verifyCompositedOverlay, and exit. Because runCmd's kill fires only when the process is still running at helperMs (measured from spawn), a helper whose export ran close to its --timeout-ms can be SIGKILLed before it finishes verifyCompositedOverlay/exits, discarding a good overlay down the raw-video fallback path. Give the post-export steps their own margin instead of deriving both from a single shared 5s grace.
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 packages/capture-kit/src/recording/overlay.ts, line 73:
<comment>The process-kill timer (`runCmd` `timeoutMs: helperMs`) and the export timer (`--timeout-ms = helperMs - 5s`) start at different moments, but the difference between them is the only budget for everything else that can keep the helper alive past a completed export: helper startup, the post-export `verifyCompositedOverlay`, and exit. Because `runCmd`'s kill fires only when the process is still running at `helperMs` (measured from spawn), a helper whose export ran close to its `--timeout-ms` can be SIGKILLed before it finishes `verifyCompositedOverlay`/exits, discarding a good overlay down the raw-video fallback path. Give the post-export steps their own margin instead of deriving both from a single shared 5s grace.</comment>
<file context>
@@ -54,11 +67,29 @@ async function exportProcessedVideo(params: {
- env: buildSwiftToolEnv(),
- });
+ const helperMs = deadline - Date.now();
+ const exportMs = helperMs - HELPER_EXIT_GRACE_MS;
+ if (exportMs <= 0) {
+ throw new AppError(
</file context>
There was a problem hiding this comment.
Measured on a 600 s simulator recording with this helper:
- 0.02 s from launch to the export;
- after a completed export, 0.06 s in
verifyCompositedOverlayand 0.06 s to exit; - after a timed-out export, 0.24 s from cancel to exit.
The shared 5 s covers each about 20 times, so 71650bb keeps one margin and makes HELPER_EXIT_GRACE_MS's comment name start-up, verify and exit.
|
The 30 fps cap looks right, but the new timeout route is not shown to work, so I need one live run before merge. At b50f71a, an export that runs past Not blocking, take or leave: On the open review threads, the Swift export timer and 5 s exit grace thread still applies as a lower-priority item. These two do not apply, so you can resolve them: the deadline and pre-export waits thread is covered because the deadline is taken before both waits, and a validator timeout rejects on the first try; and the infinite timeout thread is covered because the only producer passes a finite positive value, and The one reported check passes, but no CI job runs the simulator or emulator overlay route, so green CI does not cover the run above. I did not run the Swift helper or any device, and the 30 fps speedup is your measurement only. I did not measure the recorder stop and finalizer waits on long recordings, or run the unit tests locally. The claim that the new tests would fail on the old code comes from reading the pre-change code. There are no conflicts. Before merge, we need that live simulator stop where the export runs out of its 70 s budget. |
…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.
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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="src/__tests__/command-descriptor-timeout-policy.test.ts">
<violation number="1" location="src/__tests__/command-descriptor-timeout-policy.test.ts:345">
P2: This check allows the overlay to consume nearly the entire request envelope, leaving no practical time for the stop, copies, and playability checks named above. Require a minimum reserve (the current configuration leaves 20 seconds).</violation>
</file>
|
Ran it on this branch's head built from source (
The video below is that record-stop-600s-raw-fallback.mp4The run did take the route in question: with no timer, the same helper needs 93.1 s for this recording. Where the time went, from the request's
So about 1 s of a 10-minute stop comes before the budget, not 15-20 s. A 5-minute stop that overlays, same setup, took 47.8 s:
On 600 s files, From the non-blocking items, pushed as
On r4179925495, measured on the same 600 s recording:
The shared 5 s covers each about 20 times, so I kept one margin and made its comment name all three steps.
|
… 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.
|
The fixes from the earlier review (#3219 (comment)) are in, and I found no code problems at ea2a631. The timeout-policy test now requires the overlay budget plus 20 s to fit inside the record envelope, so the budget can no longer take the whole envelope. The one reported check is green, and no CI job runs the simulator overlay route. The live 600 s run covers that route, but it ran at b50f71a. This update does not touch the device path, so I treated that run as covering ea2a631. I ran no tests, Swift helper or device myself. The unit-test result and the On the bot threads, the P2 on the timeout envelope is fixed at this head: #3219 (comment). The P1 on the pre-export waits is resolved with no code change, because the deadline is taken before both waits in |
* origin/main: (77 commits) fix(apple-runner): fence prep spawns behind a start-owned admission (callstack#3239) 0.21.22 test(apple): own the simctl settings plan tests in simctl-settings.test.ts (callstack#3244) fix(limrun): report the session device id in iOS settings refusals (callstack#3243) 0.21.21 feat(remote): add a host-allocated macos-app lease backend (callstack#3236) test(android): bound the screenshot write wait by wall time, not event-loop turns (callstack#3250) feat(recording): cap the touch overlay frame rate at the caller's --fps (callstack#3241) fix(ad-script): let .ad scripts carry scroll --until and wait capture flags (callstack#3197) (callstack#3234) feat(provider-webdriver): keyboard enter, dismiss, and status over WebDriver (callstack#3233) feat(selectors): match role= against snapshot kind with a node-scoped alias window (callstack#3232) fix(provider-webdriver): read field values, placeholders, secure fields, and checked state from page source (callstack#3231) feat(replay): accept --test-ime on test and replay so flow-owned Android opens opt into the test IME (callstack#3235) refactor(daemon): route daemon-level diagnostics through one scope helper (callstack#3242) docs(adr): correct ADR 0031 pointer event delivery evidence (callstack#3245) fix(ios): stop a tap's post-gesture lookup from recording an XCTest failure (callstack#3060) (callstack#3237) fix(recording): render the touch overlay at most 30 fps and inside the record request (callstack#3219) fix(daemon): keep an idle daemon alive only for retained leases (callstack#3227) fix(provider-webdriver): send an empty JSON object on bodyless POSTs (callstack#3230) Feat/maestro repeat while (callstack#3214) ...
Summary
record stopwith touches took about 0.4 s per second of recording, outlastingrecord's 90 s request past about 3.5 minutes. Closes #3218. Five files:minFrameDuration, one timescale tick (1/600 s from simctl): 75 fps on the simulator, three times the frames captured. A capture reporting none renders at 30. The export wait takes a timeout.overlay.ts: the overlay gets 70 s for compile, export and exit; the export gets what is left less 5 s, as--timeout-ms(both caps were 120 s). Past it the stop returns the raw video withoverlayWarning, inside the request.The export keeps the capture's resolution (#2707).
Validation
At
ea2a631:pnpm check:affected --runpassed (format, lint, typecheck, layering, fallow, build, 1,733 related tests, packaged runner Swift). Two newoverlay.test.tscases fail without the change; the root envelope check, which keeps 20 s ofrecord's envelope outside the budget, fails with a 71 s one.iPhone 16 simulator,
record stopin seconds:overlayWarning, no helper or temp file leftEmulator, 2 min: 48.3 → 18.8. The 10-minute row ran on
b50f71a06; the others ran this change on npm 0.21.20.Not run locally: Swift runner builds and device lanes.