Skip to content

fix: detect screen recording permission revocation during active monitoring#5820

Merged
kodjima33 merged 1 commit intomainfrom
fix/issue-5792
Mar 19, 2026
Merged

fix: detect screen recording permission revocation during active monitoring#5820
kodjima33 merged 1 commit intomainfrom
fix/issue-5792

Conversation

@kodjima33
Copy link
Copy Markdown
Collaborator

Fixes #5792

Problem: If the user revokes Screen Recording permission via System Settings while monitoring is active, the app silently fails on every capture attempt — losing Rewind data without any user notification.

Fix: Added periodic permission recheck (every 60 seconds) inside the capture loop:

  • Calls ScreenCaptureService.checkPermission() on a 60s interval
  • If permission is revoked, stops monitoring gracefully
  • Sends permissionLost event so UI can show a notification/banner
  • Updates the cached permission state to stay consistent

Scope: Minimal bug fix — no new features or refactoring.

…toring

- Add periodic permission recheck every 60 seconds during capture
- Stop monitoring gracefully when permission is revoked
- Send permissionLost event for UI notification
- Prevents silent data loss when user revokes permission via System Settings
- Fixes #5792
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 19, 2026

Greptile Summary

This PR addresses issue #5792 by adding a 60-second periodic recheck of screen recording permission inside the active capture loop in ProactiveAssistantsPlugin.swift. When revocation is detected, monitoring is stopped gracefully and a permissionLost event is emitted to the UI.

The intent is correct and the scope is minimal, but there is a critical performance bug introduced by the implementation:

  • Main thread block (critical): ScreenCaptureService.checkPermission() internally runs /usr/sbin/screencapture as a subprocess and calls process.waitUntilExit() synchronously. Because captureFrame() is isolated to @MainActor, invoking this every 60 seconds will freeze the main thread (and the UI) for the duration of the subprocess. A non-blocking alternative (e.g. CGPreflightScreenCaptureAccess() alone, or dispatching to a background Task.detached) is needed.
  • Double event emission (warning): sendEvent("permissionLost") is sent before calling stopMonitoring(), which itself unconditionally emits sendEvent("monitoringStopped"). Depending on how the UI handles monitoringStopped, the permission-lost notification may be overwritten.
  • State not reset on stop (minor): lastPermissionCheckTime is not cleared in stopMonitoring(), causing the first recheck interval after a restart to be non-deterministic.

Confidence Score: 2/5

  • Not safe to merge as-is — the periodic check blocks the main thread via a synchronous subprocess every 60 seconds.
  • The fix targets a real bug and the structure is correct, but the implementation introduces a recurring main-thread freeze by calling a synchronous blocking subprocess from @mainactor. This is a regression in responsiveness that will affect all users while monitoring is active.
  • desktop/Desktop/Sources/ProactiveAssistants/ProactiveAssistantsPlugin.swift — specifically the new permission-check block in captureFrame() at lines 588–598, and the related ScreenCaptureService.checkPermission() call at line 590.

Important Files Changed

Filename Overview
desktop/Desktop/Sources/ProactiveAssistants/ProactiveAssistantsPlugin.swift Adds a periodic 60-second permission recheck inside captureFrame(). The approach is sound but has a critical defect: ScreenCaptureService.checkPermission() internally spawns a blocking subprocess (/usr/sbin/screencapture + waitUntilExit()) on the @MainActor, freezing the UI every 60 seconds. Additionally, sendEvent("permissionLost") fires immediately before stopMonitoring(), which unconditionally emits sendEvent("monitoringStopped"), creating a double-event race in the UI.

Sequence Diagram

sequenceDiagram
    participant Timer as captureTimer (every ~1s)
    participant CF as captureFrame() [@MainActor]
    participant SCS as ScreenCaptureService
    participant Proc as /usr/sbin/screencapture subprocess
    participant SM as stopMonitoring()
    participant UI as UI Event Handler

    Timer->>CF: fires (every ~1s)
    CF->>CF: now.timeIntervalSince(lastPermissionCheckTime) >= 60?
    alt Every 60th second
        CF->>SCS: checkPermission() [BLOCKING, main thread]
        SCS->>Proc: process.run() + waitUntilExit()
        Note over CF,Proc: Main thread BLOCKED until subprocess exits
        Proc-->>SCS: exit code + file check
        SCS-->>CF: Bool (granted or not)
        alt Permission revoked
            CF->>UI: sendEvent("permissionLost")
            CF->>SM: stopMonitoring()
            SM->>UI: sendEvent("monitoringStopped")
            Note over UI: Two events arrive in quick succession
            SM-->>CF: returns
            CF-->>CF: return (exit frame)
        end
    else Normal path (no recheck needed)
        CF->>CF: continue normal capture logic
    end
Loading

Last reviewed commit: "fix: detect screen r..."

let now = Date()
if now.timeIntervalSince(lastPermissionCheckTime) >= permissionCheckInterval {
lastPermissionCheckTime = now
let permissionGranted = ScreenCaptureService.checkPermission()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 Main thread blocked by synchronous subprocess every 60 seconds

ScreenCaptureService.checkPermission() internally calls testCapturePermission(), which spawns a /usr/sbin/screencapture subprocess and calls process.waitUntilExit() — a synchronous blocking call. Since captureFrame() is isolated to @MainActor, this will freeze the main thread (and the entire UI) every 60 seconds for however long the screenshot subprocess takes (typically hundreds of milliseconds, potentially longer under load).

This was acceptable in startMonitoring() (a one-shot user-triggered action), but introducing it in the hot path of the 60-second capture loop creates a recurring UI freeze.

Fix: Either perform only the fast, non-blocking CGPreflightScreenCaptureAccess() check for the periodic recheck, or dispatch the permission test to a background queue:

// Option 1 – fast, non-blocking preflight only (good enough for revocation detection)
let permissionGranted = CGPreflightScreenCaptureAccess()

// Option 2 – offload the blocking subprocess to a background thread
let permissionGranted = await Task.detached(priority: .utility) {
    ScreenCaptureService.checkPermission()
}.value

Comment on lines +595 to +596
sendEvent(type: "permissionLost", data: ["permission": "screenRecording"])
stopMonitoring()
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Double event emission on permission revocation

sendEvent(type: "permissionLost", ...) is called at line 595, and then stopMonitoring() is called at line 596 — which unconditionally emits a second sendEvent(type: "monitoringStopped", data: [:]) at its end (line 488 of the file). The UI will therefore receive two back-to-back events: permissionLost immediately followed by monitoringStopped.

If the UI has a generic handler for monitoringStopped that dismisses or resets the banner/notification, it could silently swallow the permissionLost message before the user sees it. There is no synchronisation mechanism between the two events since stopMonitoring() is synchronous and fires its event in the same run-loop iteration.

Consider either:

  • Passing a reason/cause parameter to stopMonitoring() so it can embed it in the monitoringStopped payload, avoiding a separate permissionLost event, or
  • Skipping the separate sendEvent here and relying on a reason: "permissionLost" key inside monitoringStopped.

Comment on lines +44 to +45
private var lastPermissionCheckTime: Date = .distantPast
private let permissionCheckInterval: TimeInterval = 60
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 lastPermissionCheckTime not reset in stopMonitoring()

lastPermissionCheckTime retains its value across monitoring sessions. If monitoring is stopped and restarted within 60 seconds of the last check, the first periodic recheck in the new session will fire sooner than the configured interval (or not at all if the restart happens right at the boundary). More significantly, if monitoring restarts at T+59s after a check at T+0s, the first recheck in the new session will be delayed until T+60s — i.e., only 1 second after restart rather than 60 seconds, which is fine — but conversely if it restarts at T+1s the first check won't happen until T+60s (nearly a full minute into the new session while startMonitoring has already validated permission).

Consider resetting it in stopMonitoring() for consistent behavior:

Suggested change
private var lastPermissionCheckTime: Date = .distantPast
private let permissionCheckInterval: TimeInterval = 60
lastPermissionCheckTime = .distantPast
private var lastPermissionCheckTime: Date = .distantPast

Or, more practically, reset it alongside the other session state in stopMonitoring():

// Inside stopMonitoring(), alongside frameCount = 0 etc.
lastPermissionCheckTime = .distantPast

@kodjima33 kodjima33 merged commit a04d4ef into main Mar 19, 2026
2 checks passed
@kodjima33 kodjima33 deleted the fix/issue-5792 branch March 19, 2026 20:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Desktop: No screen recording permission monitoring — silent degradation

1 participant