Skip to content

fix(capture): honor cancellation during ScreenCaptureKit retry sleeps#286

Closed
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/screencapture-retry-honor-cancel
Closed

fix(capture): honor cancellation during ScreenCaptureKit retry sleeps#286
SebTardif wants to merge 1 commit into
openclaw:mainfrom
SebTardif:fix/screencapture-retry-honor-cancel

Conversation

@SebTardif

Copy link
Copy Markdown
Contributor

What Problem This Solves

After a transient ScreenCaptureKit denial (SCStreamError -3801), three retry paths sleep with try? await Task.sleep before a second attempt:

  1. ScreenCaptureFallbackRunner.run (generic/video-style capture)
  2. ScreenCaptureFallbackRunner.runCapture (image capture)
  3. ScreenRecordingPermissionChecker.hasPermission (SCShareableContent permission probe)

try? swallows CancellationError, so a canceled CLI/MCP capture or permission check can still burn a full second ScreenCaptureKit attempt after the ~350ms delay instead of stopping promptly.

This is the same cancellation-swallow class as merged #270 / #271 / #230 / #267. Closed #279 and #280 each covered a subset; maintainer triage asked for one combined fix covering all three sites (see #279 closeout).

Evidence

Before (main)

try? await Task.sleep(nanoseconds: delay)
// second attempt / probe always runs; cancel is swallowed

After (this branch)

try await Task.sleep(nanoseconds: delay)
try Task.checkCancellation()
// permission path maps CancellationError to not-granted (Bool API)

Red / green on production helpers

Red without production fix (cancel mid 350ms sleep):

$ swift test --package-path Core/PeekabooCore --filter ScreenCaptureFallbackRunnerTests
✘ cancel during transient denial sleep skips second capture attempt
  Expectation failed: (attempts → 2) == 1
✘ cancel during transient denial sleep skips second generic attempt
  Expectation failed: (attempts → 2) == 1

Green with fix:

$ swift test --package-path Core/PeekabooCore --filter ScreenCaptureFallbackRunnerTests
✔ Test run with 6 tests in 1 suite passed after 0.355 seconds

$ swift test --package-path Core/PeekabooAutomationKit --filter ScreenRecordingPermissionCancelTests
Test Case '…testCancelDuringTransientPermissionRetrySkipsSecondProbe' passed (0.055 seconds).
Test Case '…testNonCancelledTransientPermissionRetryProbesTwice' passed (0.355 seconds).
Executed 2 tests, with 0 failures

Non-canceled transient denial still retries once (control case green).

Live terminal pattern proof (after fix)

$ swiftc -parse-as-library -o /tmp/prove-sck-engine scripts/prove-screencapture-engine-cancel.swift
$ /tmp/prove-sck-engine
=== ScreenCaptureKit engine transient-denial cancel proof (combined cancel fix) ===
Test 1: Unfixed (try? sleep) — cancel after 50ms into 350ms delay
  attempts=2 elapsed_ms=51 outcome=retry-failed
Test 2: Fixed (try sleep + checkCancellation) — cancel after 50ms into 350ms delay
  attempts=1 elapsed_ms=50 outcome=cancelled
PASS: unfixed burned a second capture attempt after cancel (2 attempts)
PASS: fixed cancelled during denial sleep without second attempt (1 attempt, 50ms)

$ swiftc -parse-as-library -o /tmp/prove-sck-perm scripts/prove-screencapture-permission-cancel.swift
$ /tmp/prove-sck-perm
=== Screen-recording permission probe cancel proof (combined cancel fix) ===
Test 1: Unfixed — cancel after 50ms into 350ms delay
  probes=2 elapsed_ms=52 granted=false
Test 2: Fixed — cancel after 50ms into 350ms delay
  probes=1 elapsed_ms=50 granted=false
PASS: unfixed ran second SCShareableContent probe after cancel (2 probes)
PASS: fixed stopped at first probe without second SCK call (1 probe, 50ms)

Also: pnpm run build:cli passed on this branch.

Real behavior proof

  • Behavior or issue addressed: Canceling during a ScreenCaptureKit transient-denial retry sleep must not run a second capture attempt (run / runCapture) or a second shareable-content permission probe.

  • Real environment tested: macOS (arm64), Peekaboo worktree on branch fix/screencapture-retry-honor-cancel at tip of this PR, Swift 6.x toolchain.

  • Exact steps or command run after this patch:

    swiftc -parse-as-library -o /tmp/prove-sck-engine scripts/prove-screencapture-engine-cancel.swift
    /tmp/prove-sck-engine
    swiftc -parse-as-library -o /tmp/prove-sck-perm scripts/prove-screencapture-permission-cancel.swift
    /tmp/prove-sck-perm
    swift test --package-path Core/PeekabooCore --filter ScreenCaptureFallbackRunnerTests
    swift test --package-path Core/PeekabooAutomationKit --filter ScreenRecordingPermissionCancelTests
  • Evidence after fix: Terminal transcripts above: unfixed paths run 2 attempts/probes after cancel; fixed paths stop at 1. Production helpers with injected transient -3801 errors match those counts for engine and permission.

  • Observed result after fix: Cancellation during the ~350ms denial sleep aborts before the retry attempt(api) / second SCShareableContent probe. Non-canceled transient denial still retries once.

  • What was not tested: Live ScreenCaptureKit TCC denial from a real display capture under a canceled CLI task on a signed binary (requires TCC denial injection). Delay length matches production ScreenCaptureKitTransientError (350ms). Same live-gate limitation noted in fix(capture): honor cancellation during ScreenCaptureKit retry sleep #279 closeout.

Summary

Related

@steipete

Copy link
Copy Markdown
Collaborator

Triage Wave 5 closeout: closing without merge, not requesting changes.

Thanks @SebTardif for following the #279 closeout and combining the full cancellation family. Head 5a42133f36db0efd4a19b53ddd2f28c53b5a0a9b covers both ScreenCaptureFallbackRunner retry sleeps plus the adjacent ScreenRecordingPermissionChecker retry, with focused cancellation/control coverage, standalone pattern scripts, and one changelog entry. All five hosted macOS CI jobs are green.

The result is still blocked by the same mandatory live boundary documented on #279 and #280. This PR explicitly says a real signed ScreenCaptureKit/TCC denial under a canceled caller was not tested. The injected -3801 tests and mirrored scripts are useful deterministic regression evidence, but they do not authenticate the behavior of the real ScreenCaptureKit call. The earlier maintainer attempts through caller-local and signed GUI/on-demand paths, plus a current-Developer-ID-signed XCTest host, timed out despite granted permissions; app and replayd restarts did not recover that boundary. There is no new signed live proof, independently healthy-host evidence, or owner waiver here.

The re-entry condition is precise: provide new authenticated, properly signed live ScreenCaptureKit/TCC evidence from an independently healthy macOS host showing a real transient denial, cancellation during retry backoff, and no second capture/probe while the non-canceled control still retries once; alternatively, an owner must explicitly waive that live gate. Until then, the standing contributor-policy outcome is close rather than change-request.

No commit is being merged and there is no merge SHA. Closing appreciatively with the implementation and deterministic proof preserved in this PR.

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.

2 participants