feat(limrun): screen recording through the provider's server-side recorder - #2646
okwasniewski wants to merge 4 commits into
Conversation
Live Limrun iOS evidenceRun on Limrun iOS instance
Frame at 6 s: Full clip as written: limrun-record.mp4. Timeline and commands: timeline.txt, README. The session was closed afterwards and the instance reads |
There was a problem hiding this comment.
🟡 Changes recommended
Two moderate unresolved findings remain regarding production binding coverage and cleanup status handling.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds Limrun provider-side screen recording through the SDK, with shared finalization and runtime capability integration.
Changes:
- Implements recording start/stop, quality mapping, download, and cleanup.
- Wires Limrun device sessions into runtime ownership and updates capability facts.
- Adds tests, fixtures, and changelog documentation.
File summaries
| File | Summary | Final review comments |
|---|---|---|
packages/provider-limrun/src/runtime.ts |
Wires device-session access. | — |
packages/provider-limrun/src/recording-runtime.ts |
Implements recording lifecycle. | Moderate (1): Treat refused output removal as cleanup-pending/error. |
packages/provider-limrun/src/recording-runtime.test.ts |
Tests recording behavior. | — |
packages/provider-limrun/src/facts-runtime.ts |
Updates recording capability facts. | Nit (3): Update or supersede ADR 0024. |
packages/provider-limrun/src/facts-runtime.test.ts |
Verifies capability facts. | — |
packages/provider-limrun/src/app-log-runtime.ts |
Binds recording operations. | Moderate (1): Add binding-level regression coverage for the live Limrun path. |
packages/provider-limrun/src/app-log-runtime.fixtures.ts |
Updates owner wiring fixtures. | — |
CHANGELOG.md |
Documents Limrun recording support. | — |
Review details
Suppressed comments (2)
packages/provider-limrun/src/app-log-runtime.ts:315
- The new unit tests exercise
createLimrunScreenRecordingOperationsdirectly, but no test bindscreateLimrunPlatformRuntimeOwnerand callsbinding.operations.screenRecordingStart/stop. This spread is the production seam that suppliesgetDeviceSession, so the advertised facts can pass while the CLI route is unwired; add a binding-level regression test for the live Limrun path.
...createLimrunScreenRecordingOperations({
host: options.host,
device,
owner,
signal,
getDeviceSession: options.getDeviceSession,
}),
packages/provider-limrun/src/recording-runtime.ts:180
ScreenRecordingOutputHost.removeis non-throwing and returnspresentwhen deletion is refused. Ignoring that result letsforceCleanupreturn{ status: 'cleaned' }while the output still exists, so the durable resource can be marked complete despite failed cleanup; treatpresentas a cleanup-pending error.
await host.screenRecording.outputs.remove(outputPath);
- Files reviewed: 8/8 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| screenRecordingStart: available, | ||
| screenRecordingReattach: available, | ||
| screenRecordingCleanup: available, |
|
Reviewed at 6065c1f. The stop path and the evidence for Android leave this not ready to merge yet.
Neither startLimrunRecording is close to a line-for-line copy of startWebRecording (platform-web/src/recording/runtime.ts#L73-205): same prepare step, memoized docs/adr/0024-screen-recording-provable-signal.md#L319 still lists Limrun as unavailable, and this PR does not update that matrix, so the design record no longer matches shipped behavior for live stop, recovery, and collect. Please add a Limrun row (stop is the provider RPC, recovery is unreattachable, collect is an HTTP download, and after daemon loss the state is cleanup-pending with the instance deleted on release), and mention the Limrun quality mapping alongside Android and Apple in the The refusal test at recording-runtime.test.ts#L115 checks CHANGELOG.md#L5 and facts-runtime.ts#L174 say recording is available on Android, but the Android SDK client uses a different command name, download URL, and recorder than the iOS client that was actually run. Please run it live on Android — All three CI jobs were still running with no logs at review time. Smoke Tests drive local simulators and emulators through the Apple and Android recorders and never load the Limrun runtime, so this diff does not reach that route; Coverage runs the unit suites, which do include the new recording-runtime and facts tests, so it does reach this diff. A few things this review could not confirm: how the server responds to a second stopVideoRecording/stopRecording when nothing is recording (the fix above assumes it errors or returns without a fresh file); the order of forceCleanup versus lease release and instance delete on daemon close, since cleanup always reports transport-failed if release runs first; whether two sessions can hold the same Limrun device and double-start its recorder; and the iOS evidence comes from the author’s run of this branch merged with #2645; it was not re-run here. Splitting the remote stop from the retriable, cancellable download, and getting real Android evidence (or narrowing the facts to iOS), are what stand between this and merge. |
…order The Limrun runtime declared screen recording unavailable although the SDK exposes startRecording and stopRecording on both instance clients. Bind a recording runtime that asks the live session to record, maps the two export presets onto Limrun's 5..10 quality scale, downloads the finished MP4 to the output path on stop, and refuses --fps and --hide-touches. Reattach and cleanup after a daemon restart stay unavailable: the recording lives and dies with the instance.
6065c1f to
6f181c4
Compare
…ter the stop/download split (callstack#2646)
|
Thanks for the review; everything below is at 31a1bf5. Stop split from download. Signal and deadline. Duplication. ADR and help. ADR 0024 §3 has a Limrun row (stop is the provider RPC with the URL memoized; no signal, unreattachable; bounded cancellable HTTP collect; cleanup-pending after daemon loss, instance deleted on release). Tests. Refusals assert Android, live.
iOS, re-run on this branch alone (no #2645 merged in): medium preset → 602x1310, 9.6 s;
Files as written by On the points you could not confirm. A second remote stop is no longer sent, so the server's answer to stopping an idle recorder no longer matters. If release runs before cleanup, cleanup's stop fails and is reported as |
…b and Limrun A recorder behind a provider transport needs the same lifecycle every time: refuse unsupported options before the output is touched, prepare it, start the recorder and stop it again if the request is cancelled, stop exactly once on finish, collect the media when the provider serves it rather than writes it, finalize, and confirm the recorder. capture-kit now owns that in startTransportScreenRecording on its own subpath, so no entry's import-time closure grows; the web runtime keeps only its own admission rules and transport calls.
…bounded, retriable step The first cut asked the SDK to stop and download in one call, so a dropped transfer after a successful server stop left the next record stop re-stopping an idle recorder and never fetching the finished file. The device session now stops without a local path and answers the served URL; the host downloads it with a deadline and the discard signal through a new LimrunHostAdapter.downloadFile port, and the transport helper memoizes the stop so only the download is retried. ADR 0024 gains the Limrun row and --quality help names the Limrun mapping.
…ath for the layering guard
31a1bf5 to
5855046
Compare
|
The Coverage job on 31a1bf5 failed in |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved download cancellation and failed-retry cleanup issues block approval.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
packages/capture-kit/src/screen-recording-transport.ts:132
- Although this aborts
discard, it cannot cancel acollectthat is already in flight:createScreenRecordingLiveHandle.forceCleanupwaits for the existingfinishpromise before invoking this implementation. A stalled Limrun download will therefore wait for the full 120-second deadline instead of being interrupted by cleanup. The discard signal needs to be triggered by the handle coordination before it waits for the active finish.
discard.abort(new Error(`${transport.backend} recording discarded`));
- Files reviewed: 28/28 changed files
- Comments generated: 2
- Review effort level: Lite
| } catch (error) { | ||
| throw downloadFailure(error, options, timeout); | ||
| } |
| if (!response.ok) { | ||
| const body = await response.text().catch(() => ''); | ||
| throw new AppError('COMMAND_FAILED', `Limrun download failed with HTTP ${response.status}`, { | ||
| url: options.url, | ||
| statusCode: response.status, | ||
| body: body.slice(0, RESPONSE_BODY_PREVIEW_CHARS), | ||
| }); | ||
| } | ||
| if (!response.body) { |
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved download safety and recording cleanup issues remain.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (3)
packages/capture-kit/src/screen-recording-transport.ts:134
- This cleanup signal cannot abort an in-flight collection:
ScreenRecordingLiveHandle.forceCleanup()waits forfinish()to settle before invoking this callback, whilefinish()is blocked intransport.collect(..., discard.signal). A session teardown during a slow Limrun download can therefore wait for the full 120-second deadline instead of canceling the transfer; the cleanup path needs a cancellation hook that is triggered before waiting for the finish promise.
forceCleanup: async () => {
discard.abort(new Error(`${transport.backend} recording discarded`));
try {
await stopOnce();
src/sdk/limrun-download-file.ts:56
- When the caller aborts at the same time as the 120-second deadline, this check wins and discards the caller's abort reason, even though the download API promises to preserve caller cancellation. Prefer treating the timeout as authoritative only when
options.signalis not also aborted, as the existing HTTP transport code does.
if (timeout.aborted) {
return new AppError('COMMAND_FAILED', 'Limrun download timed out', {
url: options.url,
src/sdk/limrun-download-file.ts:27
- If a non-2xx response body stalls,
response.text()rejects when the combined timeout/caller signal aborts, but this catch discards that error and the code then reports only the HTTP status. A 120 s timeout or cleanup cancellation can therefore surface as, for example, a 404 instead of the timeout or caller reason; preserve the caught error throughdownloadFailureand only build the HTTP-status error after body consumption succeeds.
const body = await response.text().catch(() => '');
- Files reviewed: 28/28 changed files
- Comments generated: 2
- Review effort level: Lite
| const body = await response.text().catch(() => ''); | ||
| throw new AppError('COMMAND_FAILED', `Limrun download failed with HTTP ${response.status}`, { | ||
| url: options.url, | ||
| statusCode: response.status, | ||
| body: body.slice(0, RESPONSE_BODY_PREVIEW_CHARS), |
| acquired = true; | ||
| signal.throwIfAborted(); | ||
| } catch (error) { | ||
| if (acquired) await stopOnce().catch(() => {}); |
|
Reviewed at 5855046. One cancellation gap remains, and the in-flight cleanup test does not cover it. The discard signal cannot cancel a running download. The test "aborts a collect in flight" at screen-recording-transport.test.ts#L142 uses a A small one: the refusal test at #L196 takes The shared helper looks like the right size, because web and Limrun had the same prepare, rollback, stop and envelope code. Is there a reason to keep the discard controller if it cannot cancel anything? The live run you posted covers the successful stop and download. Retry after a failed download and cleanup during a download have unit coverage only. All 13 checks pass, and there are no conflicts. Next: make the discard signal cancel a running download through the live handle, or remove it and correct the ADR row, and make the in-flight test fail on the current code. |
Summary
The Limrun runtime declared
recordunavailable ("Limrun does not expose an exact-owner screen-recording runtime") although the Limrun SDK exposes a server-side recorder on both instance clients. This binds a screen-recording runtime for Limrun-owned devices.record startasks the live instance to record;--quality mediummaps to Limrun quality 5,highto 8.record stopasks the instance to stop once and memoizes the served URL. The download to the output path is a separate step with a 120 s deadline and a discard signal, done by the host through a newLimrunHostAdapter.downloadFileport. A dropped transfer is retried by the nextrecord stopwithout a second remote stop; cleanup stops and never downloads.startTransportScreenRecordingon its own subpath (@agent-device/capture-kit/screen-recording-transport), so no entry's import-time closure grows; the web recorder rides the same helper with its 12 tests unchanged.--scoperecords the whole simulator or emulator screen;--fpsand--hide-touchesare refused at admission; reattach and cleanup after a daemon restart stay unavailable. ADR 0024 gains the Limrun row and the--qualityhelp names the mapping.Four commits: feature, the shared-transport refactor, the stop/download fix, and a gates commit declaring the new capture-kit subpath for the layering guard.
Validation
record stopwrote playable H.264 MP4s withrecorder: confirmedon both; Android gestures during the clip went through the bundled helper (androidSnapshot.backend: android-helper,helperVersion: 0.21.4).pnpm check:affected --run --base upstream/main: all runnable checks passed on 5855046 with the full local gate set (444 test files, 3241 tests, includingpackage).