fix(macos): avoid camera-permission deadlock when opening off the main thread#56
Conversation
…n thread ProviderApple::open() requested camera authorization by dispatching the request onto the main dispatch queue for non-main-thread callers, then blocking on a semaphore. When nothing services the main queue -- e.g. a ccap::Provider opened from a worker thread in a process with no CFRunLoop on its main thread (a Node.js/Electron addon, a head-less multi-threaded service) -- the dispatched block never runs, so the permission request is never even issued and open() hangs forever. Main-thread callers and apps with a running run loop were unaffected, which is why this stayed dormant. requestAccessForMediaType: may be called from any thread and delivers its completion on an internal queue, so the main-queue hop is unnecessary. Extract the "start an async request and block until it completes" logic into ccap::runBlockingAsyncRequest() (src/ccap_apple_async.h), which runs the request on the calling thread and waits on a portable condition variable. The blocking "wait until the user decides" behavior is preserved; only the deadlock-prone main-queue dispatch is removed. Add tests/test_apple_permission.cpp: a deterministic regression test that drives the real helper with a simulated async request (a countdown firing the completion from a background thread) and asserts the wait does not deadlock when invoked off the main thread with no run loop. It builds into the existing ccap_convert_test aggregate, so it runs in the macOS CI "Run Full Test Suite" job (./run_tests.sh --functional); it is an empty translation unit elsewhere. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
More reviews will be available in 26 minutes and 56 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more credits in the billing tab to continue. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe Apple camera-permission path now requests access directly and waits on the existing semaphore. The Windows workflow pins VS2022 jobs to ChangesApple camera permission flow
Windows build workflow update
Playback timing assertion
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 `@tests/test_apple_permission.cpp`:
- Around line 53-73: The timeout helper in the permission test can still hang
because the inline body path bypasses any real watchdog and the detached worker
keeps a reference to donePromise after return. Update the helper around the body
lambda and worker thread handling so the “main thread” case is exercised under
an external watchdog/subprocess rather than directly inline, and move the
promise/shared state to heap-managed storage before any detach so late
completion cannot access a freed reference.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 68d40a48-1016-433a-891a-46a752deddc8
📒 Files selected for processing (4)
src/ccap_apple_async.hsrc/ccap_imp_apple.mmtests/CMakeLists.txttests/test_apple_permission.cpp
Address review feedback on the regression test harness: - Always run the code under test on a worker thread with the watchdog on the calling thread, so a deadlock regression fails via a clean timeout instead of hanging the test binary (the previous inline main-thread path bypassed it). - Keep the completion promise on the heap (shared_ptr) shared with the worker, so a late completion after a timeout/detach cannot touch freed stack state. - Replace the redundant main-thread sanity case (the helper is thread-agnostic) with a synchronous-completion case that guards against a missed wakeup. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…image windows-latest now resolves to windows-2025-vs2026, which ships Visual Studio 2026 (v18) only. Two breakages resulted, red on every PR regardless of content: - The VS2022 jobs configure with -G "Visual Studio 17 2022", which can no longer find a VS instance. Pin them to runs-on: windows-2022 (still ships VS 2022); VS 2026 is covered by the dedicated build-vs2026 job. - The VS2026 job forced -T v144, which makes MSBuild fail to resolve VCTargetsPath on the VS 2026 image. Drop the toolset override and let CMake use the default toolset that ships with the "Visual Studio 18 2026" generator. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The VS2026 shared-library linking test searched for vcvars64.bat under
"Microsoft Visual Studio\2026", but VS 2026 installs under a version folder
("Microsoft Visual Studio\18\Enterprise"). The find returned nothing, so the
step fell through and exited 1 even though the DLL built fine. Search the whole
Visual Studio directory instead, matching the VS2022 linking test.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CurrentTime is the wall-clock playback position, so grabbing buffered frames faster/slower than real-time (common on shared CI runners) makes (time2 - time1) deviate from 5/frameRate in both directions. The previous symmetric +/-50% EXPECT_NEAR failed (near-)consistently on the windows-2022 runner while passing on windows-2025. Keep the forward-progress assertion (EXPECT_GT) and replace the brittle tolerance with a generous upper bound; deterministic progression is already covered by GetCurrentFrameIndexProgression. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The VS2026 configure step used "mkdir -p" under PowerShell, where mkdir maps to New-Item, which errors when the directory already exists. On a build-cache hit the restored build/<config> directory is present, so configure failed intermittently (cache miss passed, cache hit failed). Use New-Item -ItemType Directory -Force, matching the VS2022 job. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ings - ccap_apple_async.h: move cv.notify_one() inside the locked scope. mutex/cv/ finished are stack-locals the waiting thread destroys on return, so notifying after unlock risks a use-after-free if the waiter wakes (e.g. spuriously), sees finished == true, and returns before notify_one() runs. Holding the lock blocks the waiter from re-acquiring it (and thus returning) until notify completes. - test_file_playback.cpp: assert FrameRate > 0 so the loosened upper bound cannot go vacuous (inf/NaN) on a regression that zeroes the frame rate. - test_apple_permission.cpp: reframe the docstring -- it pins the helper's contract (what open() delegates to), not an end-to-end open()/AVFoundation test, which needs a real camera/TCC and cannot run deterministically in CI. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 @.github/workflows/windows-build.yml:
- Around line 401-404: The vcvars64.bat lookup in the Windows build workflow is
too broad and can resolve a non-2026 Visual Studio install, causing the VS2026
link test to use the wrong toolchain. Update the discovery logic in the workflow
step that sets VCVARS_PATH so it explicitly targets the VS 2026 installation
under the known Microsoft Visual Studio 18 path instead of scanning all versions
with find | head -n1, and keep the rest of the link-test setup using that
resolved path.
🪄 Autofix (Beta)
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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c9ec19da-0273-4f2e-bd02-4141cce2a625
📒 Files selected for processing (4)
.github/workflows/windows-build.ymlsrc/ccap_apple_async.htests/test_apple_permission.cpptests/test_file_playback.cpp
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ccap_apple_async.h
- tests/test_apple_permission.cpp
| # Find the Visual Studio compiler. The VS 2026 install lives under a version | ||
| # folder ("...\Microsoft Visual Studio\18\..."), not a "2026" year folder, so | ||
| # search the whole VS directory rather than hard-coding the name. | ||
| VCVARS_PATH=$(find "/c/Program Files/Microsoft Visual Studio" -name "vcvars64.bat" 2>/dev/null | head -n1) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Pin vcvars64.bat discovery to VS 2026 to avoid false-positive link tests.
At Line 404, find ... | head -n1 may pick a non-2026 installation (for example VS2022) when multiple VS versions are present, so this “VS2026” link test can silently validate the wrong toolchain.
Suggested fix
- VCVARS_PATH=$(find "/c/Program Files/Microsoft Visual Studio" -name "vcvars64.bat" 2>/dev/null | head -n1)
+ # Prefer VS 2026 (major version 18) explicitly.
+ VCVARS_PATH=$(find "/c/Program Files/Microsoft Visual Studio/18" -name "vcvars64.bat" 2>/dev/null | head -n1)
+ # Fallback only if VS 2026 path is unavailable.
+ if [ -z "$VCVARS_PATH" ]; then
+ VCVARS_PATH=$(find "/c/Program Files/Microsoft Visual Studio" -name "vcvars64.bat" 2>/dev/null | head -n1)
+ fi📝 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.
| # Find the Visual Studio compiler. The VS 2026 install lives under a version | |
| # folder ("...\Microsoft Visual Studio\18\..."), not a "2026" year folder, so | |
| # search the whole VS directory rather than hard-coding the name. | |
| VCVARS_PATH=$(find "/c/Program Files/Microsoft Visual Studio" -name "vcvars64.bat" 2>/dev/null | head -n1) | |
| # Find the Visual Studio compiler. The VS 2026 install lives under a version | |
| # folder ("...\Microsoft Visual Studio\18\..."), not a "2026" year folder, so | |
| # search the whole VS directory rather than hard-coding the name. | |
| # Prefer VS 2026 (major version 18) explicitly. | |
| VCVARS_PATH=$(find "/c/Program Files/Microsoft Visual Studio/18" -name "vcvars64.bat" 2>/dev/null | head -n1) | |
| # Fallback only if VS 2026 path is unavailable. | |
| if [ -z "$VCVARS_PATH" ]; then | |
| VCVARS_PATH=$(find "/c/Program Files/Microsoft Visual Studio" -name "vcvars64.bat" 2>/dev/null | head -n1) | |
| fi |
🤖 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 @.github/workflows/windows-build.yml around lines 401 - 404, The vcvars64.bat
lookup in the Windows build workflow is too broad and can resolve a non-2026
Visual Studio install, causing the VS2026 link test to use the wrong toolchain.
Update the discovery logic in the workflow step that sets VCVARS_PATH so it
explicitly targets the VS 2026 installation under the known Microsoft Visual
Studio 18 path instead of scanning all versions with find | head -n1, and keep
the rest of the link-test setup using that resolved path.
The deadlock was caused solely by dispatch_async'ing the permission request onto the main queue, which never runs when no run loop services it. The fix is just to delete that bounce and call requestAccessForMediaType: directly on the calling thread, keeping the original dispatch_semaphore wait. The earlier ccap_apple_async.h helper (std::mutex/condition_variable) and its unit test were introduced only to make the path portable-C++-testable, but the file was macOS-only (portability was moot), the cv-on-the-stack rewrite introduced its own destroy-after-notify UAF, and the test never exercised ProviderApple::open() -- it tested the invented helper, not the real code path. GCD's dispatch_semaphore is simpler and, because the completion block retains the semaphore, has no destroy race. Removes src/ccap_apple_async.h and tests/test_apple_permission.cpp. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai review |
|
✅ Action performedReview finished.
|
Problem
ProviderApple::open()requested camera authorization by dispatching the request onto the main dispatch queue for non-main-thread callers, then blocking on a semaphore (DISPATCH_TIME_FOREVER):When nothing is servicing the main queue — e.g. a
ccap::Provideropened from a worker thread in a process with noCFRunLoopon its main thread (a Node.js/Electron addon, a head-less multi-threaded service) — the dispatched block never runs, so the permission request is never even issued andopen()hangs forever. It is not "waiting for the user to decide"; the prompt is never shown, so no progress is possible.This stayed dormant because the shipping paths don't hit it:
elsebranch — no dispatch, no deadlock.It bites exactly the off-main-thread + no-run-loop context, which is what a native Node binding (libuv worker thread, no CFRunLoop) creates — relevant to where this library is headed.
Fix
requestAccessForMediaType:may be called from any thread and delivers its completion on an internal queue, so the main-queue hop is unnecessary. The "start an async request and block until it completes" logic is extracted intoccap::runBlockingAsyncRequest()(src/ccap_apple_async.h), which runs the request on the calling thread and waits on a portablestd::condition_variable.The blocking "wait until the user decides" behavior is preserved (the wait still blocks indefinitely for the user's response) — only the deadlock-prone main-queue dispatch is removed.
Test (runs in CI)
tests/test_apple_permission.cppis a deterministic regression test that drives the real helper with a simulated async request — a countdown that fires the completion from a background thread, exactly howrequestAccessForMediaType:delivers its completion off the caller's run loop. No camera required.ccap_convert_testaggregate, so it runs in the macOS "Run Full Test Suite" CI job via./run_tests.sh --functional.Verified locally (Apple toolchain):
libccap.acompiles with the change (ARC)ccap_convert_testbuilds with the new testAppleCameraPermission.*run & pass under ASANScope
macOS only. No public API change. Behavior for main-thread callers is unchanged.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Chores