fix: harden thread-safety edges from the capture-load reliability work - #27
Merged
Timo972 merged 5 commits intoAug 26, 2026
Conversation
Follow-ups from the final review of #23 (DRO-2713 layers 1+2): - FBVideoStreamManager: hoist the XCUIScreen.mainScreen displayID read out of the @synchronized (self.sessions) region so a wedged XCUI call cannot block the control-marked capture routes on the same monitor. - /status: snapshot UIDevice systemName/systemVersion/userInterfaceIdiom behind a dispatch_once (FBSessionCommands.cachedDeviceInfo) and burn the once-token on the main thread in FBWebServer startServing, next to the existing status-cache pre-warm - the handler reads them from a connection queue while UIDevice is formally main-thread-only. - testAutomationRequestsDoNotNestInsideRunLoopSpin: widen the gap between the two probe requests from 0.1 s to 0.3 s (matching the analogous test) and the probe's spin window from 0.4 s to 1.0 s so the second request still lands well inside the first's spin on a loaded CI runner. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- fb_pixelBudget: reject the exact 2^64 boundary — (double)NSUIntegerMax rounds up to 2^64, so the > comparison accepted a value whose NSUInteger cast overflows (UB). Use >= and pin it with a test. - FBVideoStreamManager: give the aborted-start and session-limit errors distinct codes behind a named enum instead of a shared code 1. - Comment precision (review feedback): the FBWebServer pre-warm note no longer overstates the dispatch_once race guarantee, the hoisted displayID read documents the discarding abort path, and the retimed dispatch test explains what the 0.3 s gap actually buys. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up on the hoist: reading displayID between the bind and the insert left a wedge point where the socket and encoder were already live but the session was not yet visible to stopAllSessions — a stop would return while the listener, encoder, and pendingStarts reservation stayed stranded. Reading it first means a wedged XCUI call strands nothing: no monitor hold, no reservation, no bound-but-unstoppable session. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-up: moving the displayID read ahead of the reservation lock also moved it ahead of the stopGeneration snapshot, so a stop-all completing while a start was stuck in the lookup got absorbed into the start's baseline — both abort checks would pass and a live capture could appear after the stop-all had already returned success. Snapshot the generation under a brief lock before the lookup and reject the start at reservation time if it moved; the insert-time re-check keeps covering the bind window, and a wedged lookup still strands nothing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review follow-ups: - Warm the /status device-info snapshot on the main thread BEFORE the server binds. Warming it after bind (next to the version pre-warms) left a window where an early /status request could win the dispatch_once and run the UIDevice reads on its connection queue — the exact off-main access the snapshot exists to remove. Unlike FBTestmanagerdVersion(), the read is cheap and local, so it cannot delay binding; the version pre-warms stay post-bind for the reasons already documented there. - Fast-fail over-cap starts before the XCUIScreen read. The hoist made a ninth start pay the displayID lookup before the capacity check, so under an XCUI stall an ineligible start could wedge the automation funnel. An advisory check in the early snapshot lock returns the limit error immediately; the authoritative check stays in the reservation lock to count starts that slip in during the lookup. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Timo972
deleted the
timo/dro-2747-wda-harden-thread-safety-edges-from-the-capture-load
branch
August 26, 2026 07:24
Timo972
added a commit
that referenced
this pull request
Aug 26, 2026
…m sync Only FBRouteTests.m conflicted: master's timing tweaks to the spinning probe test (0.3 s gap, 1.0 s spin, expanded comment) are kept, with the comment's dispatch wording updated to the funnel-as-routeQueue architecture this branch introduces. FBSessionCommands' new cachedDeviceInfo pre-warm and the video-stream session hardening merged cleanly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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.
Follow-ups from the final review of #23 (DRO-2713 layers 1+2). All were graded Minor — pre-existing patterns or theoretical windows.
Changes
startSessionWithConfiguration:read[XCUIScreen.mainScreen displayID]while holding@synchronized (self.sessions). If that XCUI call ever wedged, the control-marked capture routes (stop/list/get/keyframe) would block on the same monitor, undermining their wedge-immunity. The read is now hoisted out of the locked region into a local that is assigned under the lock.UIDevice.currentDevicesystemName/systemVersion/userInterfaceIdiomwere read from a connection queue — effectively constant, but formally UIKit-off-main. They are now snapshotted once behind adispatch_once(FBSessionCommands.cachedDeviceInfo), and the once-token is burned on the main thread inFBWebServer startServing, next to the existingFBSDKVersion()/FBTestmanagerdVersion()status-cache pre-warm.testAutomationRequestsDoNotNestInsideRunLoopSpinused a 0.1 s gap between its two probe requests (the analogous existing test uses 0.3 s). The gap is now 0.3 s, and the probe's run-loop spin is widened from 0.4 s to 1.0 s so the second request still lands well inside the first's spin window on a loaded CI runner.Plus the two residual nits tracked on the ticket:
fb_pixelBudget:boundary:(double)NSUIntegerMaxrounds up to exactly 2^64, so the>range check accepted that one value and the subsequentNSUIntegercast overflowed (undefined behavior). Now>=, with a regression test.Verification
UnitTestssuite on iPhone 17 simulator: 175 tests, 0 failures (1 pre-existing skip), rerun after every change.FBWebServerDispatchTests(including the retimed test) pass.WebDriverAgentLib_watchOScompiles (thecachedDeviceInfowatch branch is watch-only code the simulator build does not cover).Closes DRO-2747
🤖 Generated with Claude Code