fix: don't ack device.screenrecord until the broadcast is confirmed live - #339
Open
gmegidish wants to merge 2 commits into
Open
fix: don't ack device.screenrecord until the broadcast is confirmed live#339gmegidish wants to merge 2 commits into
gmegidish wants to merge 2 commits into
Conversation
handleScreenRecord fired the DeviceKit/ReplayKit startup in a goroutine and returned status:"recording" immediately, racing any device command sent right after against the still-in-progress (and sometimes failing) broadcast picker click on real iOS devices.
📝 WalkthroughWalkthroughScreen recording startup now reports readiness or failure through ChangesScreen recording readiness
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant handleScreenRecord
participant ScreenRecordCommand
participant iOSCapture
Client->>handleScreenRecord: request screen recording
handleScreenRecord->>ScreenRecordCommand: start with Ready channel
ScreenRecordCommand->>iOSCapture: start capture with OnReady
iOSCapture-->>ScreenRecordCommand: capture is live
ScreenRecordCommand-->>handleScreenRecord: readiness result
handleScreenRecord-->>Client: success or startup error
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/server.go`:
- Around line 1159-1161: In the screen-recording startup timeout branch, call
recorder.stop() before recorder.clear() so the active session is stopped before
its reference is removed. Preserve the existing timeout error return and apply
this ordering within the timeout case handling the screenRecordReadyTimeout
event.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 76a8d952-1005-4442-bf4d-56ab2d13f865
📒 Files selected for processing (5)
commands/screenrecord.godevices/common.godevices/ios.goserver/recording.goserver/server.go
Comment on lines
+1159
to
+1161
| case <-time.After(screenRecordReadyTimeout): | ||
| recorder.clear() | ||
| return nil, fmt.Errorf("timed out waiting for recording to start") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Stop the recording before clearing the session.
recorder.clear() only removes the session reference. It does not close session.StopChan.
If startup exceeds 60 seconds, the command goroutine can continue. A real iOS user can then confirm the broadcast after this RPC returns a timeout. The capture can start without an active session, and a later request can start another recording.
Close the session through recorder.stop() before clearing it.
Proposed fix
case <-time.After(screenRecordReadyTimeout):
+ _, _ = recorder.stop()
recorder.clear()
return nil, fmt.Errorf("timed out waiting for recording to start")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case <-time.After(screenRecordReadyTimeout): | |
| recorder.clear() | |
| return nil, fmt.Errorf("timed out waiting for recording to start") | |
| case <-time.After(screenRecordReadyTimeout): | |
| _, _ = recorder.stop() | |
| recorder.clear() | |
| return nil, fmt.Errorf("timed out waiting for recording to start") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/server.go` around lines 1159 - 1161, In the screen-recording startup
timeout branch, call recorder.stop() before recorder.clear() so the active
session is stopped before its reference is removed. Preserve the existing
timeout error return and apply this ordering within the timeout case handling
the screenRecordReadyTimeout event.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
device.screenrecordwas ackingstatus: "recording"the instant its handler goroutine was scheduled, not once the recording was actually confirmed live. On real iOS devices, "live" requires DeviceKit to launch, DeviceKitH264 to start, and the ReplayKit "Start Broadcast" system sheet to be clicked (devices/ios.go'sclickStartBroadcastButton) — all of which happens asynchronously, after the ack was already sent.Any device command (
device.dump.ui,device.io.tap, etc.) issued right afterdevice.screenrecordreturns could therefore race the still-in-progress broadcast picker. If it collided with DeviceKit's own click attempt, the picker got stuck on "Press to Start Broadcasting" and the failure only surfaced ~13s later, whendevice.screenrecord.stopwas called — by which point the caller had already been driving the app blind against a system sheet instead of the real UI.Reproduced and root-caused via a real device run cross-referencing the mobilewright driver log with mobilefleet-client's server logs:
Handling device.screenrecord→Screen recording started1.7ms later, acked before DeviceKit had even presented the picker.device.io.taplanded right as the picker appeared.dump.uishowed"Press to Start Broadcasting"stuck for ~8s.screenrecord.stop, called 13s after start, is where the failure first surfaced:failed to click Start Broadcast button: timeout waiting for BroadcastUploadExtension button to appear.Fix
Thread a "ready" signal from the point DeviceKit is actually confirmed running up to the RPC handler:
devices.ScreenCaptureConfig.OnReady— fired indevices/ios.goright after DeviceKit is confirmed running (reused or freshly started with the broadcast picker clicked), before connecting to the H.264 stream.commands.ScreenRecordRequest.Ready/signalReady()— plumbs that (or an early error) up throughScreenRecordCommand. Android/simulator/remote devices signal ready immediately since they have no equivalent async on-device UI step; every early-error return now also signals the error.server.RecordingSession.Ready— a buffered channel carrying the signal into the RPC layer.handleScreenRecordnowselects onReady/Done/ a 60s timeout instead of acking unconditionally:device.screenrecorditself, instead of silently surfacing later atstoptime