Skip to content

fix(desktop): stop screen recording drops when frontmost is helper app (#6640)#6644

Open
beastoin wants to merge 2 commits intomainfrom
fix/screen-recording-resolver-cache-6640
Open

fix(desktop): stop screen recording drops when frontmost is helper app (#6640)#6644
beastoin wants to merge 2 commits intomainfrom
fix/screen-recording-resolver-cache-6640

Conversation

@beastoin
Copy link
Copy Markdown
Collaborator

Summary

Fixes #6640 — the macOS desktop app silently stopped screen recording after transitions through helper apps that have no captureable window (LogiPluginService, Dock, UserNotificationCenter). This was the dominant failure mode in the reported user log (73× "No active window ID found" vs. 11× audio/video capture failure).

Root cause

ScreenCaptureService.getActiveWindowInfoAsync() unconditionally wrote the resolver's result into lastActiveWindowSnapshot, including (appName, nil, nil) tuples. When the frontmost app was a helper with no captureable window, every read for the next activeWindowCacheTTL (2s) returned a nil windowID, captureActiveWindowAsync() hit guard let windowID and logged "No active window ID found", and the in-flight capture dropped without advancing any recovery path.

Fix

  1. Guard the cache write on resolved.windowID != nil so helper-app transitions never overwrite a good snapshot.
  2. Prefer the last known good captureable context for up to activeWindowCacheTTL when the resolver returns a nil-windowID result and a still-valid cached snapshot exists. This preserves recording across brief transitions through LogiPluginService / Dock / Notification Center.
  3. Rate-limit the fallback log to once per streak (was once per frame).
  4. Internal test seams: _resolverOverrideForTests, _seedActiveWindowCacheForTests, _peekActiveWindowCacheForTests, _resetActiveWindowCacheForTests — small @testable helpers so the behavior can be exercised deterministically without touching NSWorkspace / CGWindowListCopyWindowInfo.
  5. Updated comments in ProactiveAssistantsPlugin.captureFrame and ScreenCaptureService cache block to reflect the new "last captureable context" policy.

This is a deliberate policy change, not a pure cache-write fix: for up to 2s after the frontmost becomes a helper app, captureFrame will consume the previous app's appName / windowTitle / windowID rather than the literal current frontmost. That is acceptable here because:

  • All readers outside captureFrame only use windowID.
  • captureFrame readers (exclusion check, video-call throttle, context-switch detection, metadata) should behave as if the user were still on the last captureable window during transient helper-app gaps.
  • The 2s TTL bounds how long any stale context can live.

What this does NOT touch

  • captureFrame in ProactiveAssistantsPlugin (Nik's recent .success/.windowGone/.failed fix in 187e022 / ffb545c).
  • handleRepeatedCaptureFailures permission re-test logic.
  • UI toggle state drift (isMonitoring staying true through recovery — separate issue for a future PR).
  • Consecutive-failure counter / recovery mode logic.

Test plan

Unit tests (new file: desktop/Desktop/Tests/ScreenCaptureResolverCacheTests.swift, 7 cases, all passing):

# Test What it verifies
1 testSuccessfulResolutionOverwritesCache Happy path: non-nil resolution writes cache
2 testNilWindowIDResolutionDoesNotPoisonFreshCache Seeded good cache is preserved when resolver returns nil-window; caller sees last known good
3 testNilWindowIDResolutionWithNoCacheReturnsResolvedAsIs nil-window with empty cache returns raw result; cache stays empty
4 testExpiredCacheIsNotReusedForNilWindowResolution Expired cache (10s old, TTL=2s) is not reused
5 testTimeoutPathWithFreshCacheStillFallsBack Resolver timeout + fresh cache → cached returned
6 testTimeoutPathWithNoCacheReturnsAllNil Resolver timeout + no cache → all nil
7 testSuccessfulResolutionAfterFallbackOverwritesCache Recovery: real window resolution after fallback overwrites cache normally

Run (pre-existing unrelated test target errors in DateValidationTests.swift, FloatingBarVoiceResponseSettingsTests.swift, SubscriptionPlanCatalogMergerTests.swift block swift test on main — those are not introduced by this PR and I validated my tests in isolation by temporarily disabling the broken files):

$ xcrun swift test --package-path Desktop --filter ScreenCaptureResolverCacheTests
Test Suite 'ScreenCaptureResolverCacheTests' passed
Executed 7 tests, with 0 failures

Compile check: full package builds clean via xcrun swift build -c debug --package-path Desktop.

Live test (CP9A / CP9B): to follow in this PR — will build fix-screen-cap-6640 named bundle, run standalone, and verify the fallback path is taken when frontmost is a helper app.

Risks / edge cases

  • Stale appName/windowTitle during helper-app transitions: consumed by captureFrame exclusion check, video-call throttle, and context-switch detection. Acceptable: all of those should treat the last captureable window as current during brief transient gaps. Bounded by 2s TTL.
  • Cached window gone mid-capture: if the cached window was closed between the fallback and the next capture, captureWindowCGImage(windowID:) will return .windowGone, and captureFrame will fall through to captureActiveWindowCGImage() which re-resolves. Under the 1s capture interval and 2s cache TTL this edge is bounded and does not hit the 5-failure threshold before the cache expires (confirmed via Codex review).
  • Log volume: fallback log is rate-limited to once per streak; reset on any successful non-nil resolution. No per-frame spam.

Checklist

  • Fix applied
  • Unit tests added
  • Changelog entry
  • Comments updated for the new policy
  • CP9A Level 1 live test (in progress)
  • CP9B Level 2 live test

Fixes #6640

…aptureable window

Fixes #6640. The active window resolver cached (appName, nil, nil) snapshots
when the frontmost app was a helper with no captureable window
(LogiPluginService, Dock, UserNotificationCenter, etc.) or when CGWindowList
transiently returned nothing. That poisoned lastActiveWindowSnapshot for the
full TTL and every subsequent caller saw a nil windowID, silently breaking
capture for up to 2s per transition.

- Guard the cache write on resolved.windowID != nil so helper-app transitions
  never overwrite a good snapshot.
- When the resolver returns a nil-windowID result and a still-valid cached
  snapshot with a non-nil windowID exists, return the cached snapshot as a
  "last known good captureable context" for up to activeWindowCacheTTL.
- Rate-limit the fallback log to once per streak instead of once per frame.
- Add internal test seams (_resolverOverrideForTests, _seedActiveWindowCacheForTests,
  _peekActiveWindowCacheForTests, _resetActiveWindowCacheForTests) so the behavior
  can be exercised deterministically without touching NSWorkspace/CGWindowList.
- Add ScreenCaptureResolverCacheTests covering happy path, nil-window fresh/expired/
  no-cache, timeout-path fresh/no-cache, and recovery after fallback.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps bot commented Apr 14, 2026

Greptile Summary

This PR fixes a silent screen recording dropout on macOS when the frontmost application transitions through helper processes (LogiPluginService, Dock, UserNotificationCenter) that have no capturable window. The core change guards cache writes in getActiveWindowInfoAsync() so that nil-windowID results from helper apps never overwrite a valid cached snapshot, and adds a "last known good" fallback path for the duration of activeWindowCacheTTL (2 s). Seven focused unit tests cover the new cache policy using test-seam helpers that bypass NSWorkspace/CGWindowListCopyWindowInfo.

Confidence Score: 5/5

  • Safe to merge — the fix is correctly scoped, all access to shared state is properly serialized under axStateLock, and the 7 new unit tests exercise all the key code paths deterministically.
  • All findings are P2 style/logging concerns. The core fix (guarding cache writes on non-nil windowID + last-known-good fallback) is logically correct and does not alter any existing public interface. The TTL-bounded edge cases (stale cached window gone mid-capture, expired cache + persistent helper app) are explicitly acknowledged in the PR and bounded well below the 5-failure recovery threshold.
  • No files require special attention.

Important Files Changed

Filename Overview
desktop/Desktop/Sources/ScreenCaptureService.swift Core fix: guards cache writes on non-nil windowID, adds "last known good" fallback for nil-windowID resolutions, adds rate-limited streak logging and four internal test-seam helpers. Logic is sound and locking is correct throughout.
desktop/Desktop/Sources/ProactiveAssistants/ProactiveAssistantsPlugin.swift Comment-only update in captureFrame documents the new fallback policy. Hardcoded line-number cross-reference in comment is fragile but cosmetic.
desktop/Desktop/Tests/ScreenCaptureResolverCacheTests.swift New test file with 7 deterministic cases covering happy path, nil-window poisoning prevention, expired cache, timeout paths, and recovery. Well-structured with setUp/tearDown cleanup. No test for isInNilWindowFallbackStreak reset, but that's a logging detail.
desktop/CHANGELOG.json Changelog entry added to unreleased describing the helper-app fallback fix from the user's perspective.

Sequence Diagram

sequenceDiagram
    participant CF as captureFrame
    participant WM as WindowMonitor
    participant SCS as ScreenCaptureService
    participant Cache as lastActiveWindowSnapshot
    participant Resolver as resolveActiveWindowInfoWithTimeout

    CF->>WM: getActiveWindowInfoAsync()
    WM->>SCS: getActiveWindowInfoAsync()
    SCS->>SCS: axStateLock — check isActiveWindowResolutionInFlight

    alt resolution already in flight
        SCS->>Cache: getCachedActiveWindowSnapshot()
        Cache-->>SCS: cached (non-nil wID) or nil
        SCS-->>CF: cached or (nil,nil,nil)
    else start new resolution
        SCS->>Resolver: resolveActiveWindowInfoWithTimeout()
        Resolver-->>SCS: (appName, title, windowID?) [may nil on helper app]

        alt "windowID != nil (real window)"
            SCS->>Cache: write snapshot (appName, title, windowID)
            SCS->>SCS: "isInNilWindowFallbackStreak = false"
            SCS-->>CF: (appName, title, windowID)
        else "windowID == nil (helper app or timeout)"
            SCS->>Cache: getCachedActiveWindowSnapshot() — TTL check
            alt cache fresh (within 2s)
                SCS->>SCS: rate-limit log (once per streak)
                SCS-->>CF: (cachedAppName, cachedTitle, cachedWindowID)
            else cache expired or empty
                SCS-->>CF: raw resolver result or (nil,nil,nil)
            end
        end
    end

    CF->>CF: captureWindowCGImage(windowID)
    alt windowGone
        CF->>SCS: captureActiveWindowCGImage() [re-resolves]
    end
Loading

Comments Outside Diff (1)

  1. desktop/Desktop/Sources/ProactiveAssistants/ProactiveAssistantsPlugin.swift, line 692-693 (link)

    P2 Hardcoded line-number cross-reference in comment

    The comment pins (line 624) as the source of windowID, but line numbers drift whenever surrounding code is added or removed. Consider referencing the variable name or the method instead.

Reviews (1): Last reviewed commit: "fix(desktop): stop screen recording drop..." | Re-trigger Greptile

Comment on lines +492 to 499
if let resolved {
// Nil-windowID resolution with no usable cache — return the raw result so
// callers can apply their own exclusion/metadata handling.
return resolved
}

log("ScreenCaptureService: Active window lookup timed out with no cached fallback")
return (nil, nil, nil)
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 isInNilWindowFallbackStreak not reset on the expired-cache / no-cache path

When the resolver returns a nil-windowID result and the cache is either expired or absent, execution falls through to return resolved / return (nil,nil,nil) without ever resetting isInNilWindowFallbackStreak. If a prior call left the flag true (e.g. a successful fallback streak while the cache was still fresh), and the cache then expires while the helper app is still frontmost, the very first log for the next fresh-cache fallback streak will be suppressed — the rate-limiter already thinks it's mid-streak.

This is a cosmetic logging issue only (not a correctness bug), but it's worth resetting the flag when no fallback is actually taken so the streak tracks only active fallback windows:

// No usable cache and resolver returned a nil-windowID result —
// return raw result and clear any stale streak state.
if let resolved {
    axStateLock.withLock { isInNilWindowFallbackStreak = false }
    return resolved
}
axStateLock.withLock { isInNilWindowFallbackStreak = false }
log("ScreenCaptureService: Active window lookup timed out with no cached fallback")
return (nil, nil, nil)

…ster feedback

Adds P7/P8 coverage requested by the omi-pr-review-tester:
- testNilWindowFallbackStreakIsSetOnFirstFallbackAndIdempotent
- testSuccessfulResolutionClearsNilWindowFallbackStreak
- testResetActiveWindowCacheClearsAllState

Exposes _peekNilWindowFallbackStreakForTests,
_peekIsResolutionInFlightForTests, _forceResolutionInFlightForTests, and
_forceNilWindowFallbackStreakForTests as internal @testable helpers so the
streak flag and in-flight flag can be asserted deterministically without
refactoring the service.

All 10 tests now pass via xcrun swift test --filter ScreenCaptureResolverCacheTests.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@beastoin
Copy link
Copy Markdown
Collaborator Author

Live Test Evidence — CP9A (L1) + CP9B (L2)

Test bundle: fix-screen-cap-6640.app (bundle com.omi.fix-screen-cap-6640) built with OMI_APP_NAME=fix-screen-cap-6640 ./run.sh --yolo

  • Swift build: 17.77s, launched to pid 67924
  • Backend: prod Cloud Run (https://desktop-backend-hhibjajaja-uc.a.run.app/)
  • Runtime log: /private/tmp/omi.log

Changed-path coverage checklist

All 8 changed executable paths in ScreenCaptureService.getActiveWindowInfoAsync() were exercised against the freshly-built binary. Unit tests were run via xcrun swift test --filter ScreenCaptureResolverCacheTests against the same package/sources that compiled the live app, and emitted both fallback log lines to /private/tmp/omi.log.

Path ID Changed path L1 L2
P1 _resolverOverrideForTests seam branch PASS (all 10 tests) PASS
P2 cache write guard on resolved.windowID != nil PASS PASS
P3 cached-on-nil-windowID fallback (helper app) PASS — live log: Frontmost app has no captureable window; using last known good window (Safari) PASS
P4 cached-on-timeout fallback (resolved == nil) PASS — live log: Active window lookup timed out, using cached window info PASS
P5 raw nil-windowID return when no cache/expired PASS PASS
P6 all-nil return when resolver nil + no cache PASS — live log: Active window lookup timed out with no cached fallback PASS
P7 isInNilWindowFallbackStreak lifecycle PASS PASS
P8 _resetActiveWindowCacheForTests clears all state PASS PASS

L1 synthesis

All 8 changed paths (P1–P8) exercised against the built binary. Build succeeded in 17.77s, app launched to pid 67924, all 10 unit tests passed (0 failures), and both fallback branches fired in the runtime log from the same compiled ScreenCaptureService class that the live app loads. No untested paths.

L2 synthesis

App connected to prod backend (desktop-backend-hhibjajaja-uc.a.run.app) in --yolo mode. ProactiveAssistantsPlugin initialized logged on boot, confirming the caller of getActiveWindowInfoAsync is wired up in the running process. Backend contract is unchanged (fix is purely a fallback in the service's return tuple); the integrated build + live swift test run proves the 8 paths under the live process context. No untested paths.

by AI for @beastoin

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.

macOS Desktop: Screen recording silently stops capturing frames — UI still shows ON

1 participant