Conversation
Size Report
Startup median (7 runs, lower is better):
|
|
Reviewed at 2c277cd. The design looks sound, but several claims made for this PR were not checked here, so merge readiness is not settled yet. I could not run Xcode, so the five throwaway-edit gate-failure proofs, the tvOS/visionOS builds, and the 308/309 runner unit result are author claims I did not reproduce. The runner's deployment target is iOS 15.6 and macOS 13, and MainActor.assumeIsolated back-deploys there, but I did not check that its main-thread check passes on the iOS 15/16 runtime or that it avoids a trap there (swift_task_isCurrentExecutor falling back to the main thread); CI and the author only ran current simulators. The recorder signature change (non-escaping bootstrap, @sendable frame) only runs on a physical device, and the iOS physical-device smoke step is still pending and may be skipped when no device is present. The deferred-write route (hasAbandonedMainThreadWork, then DispatchQueue.main.async with assumeIsolated) only runs under watchdog abandonment; the author says a live smoke reached it, but I did not verify that. The Xcode 16 compiler floor is unverified, since the author tested only Xcode 26.2. Is there a smaller design than one @mainactor owner type, the attributes, and a log scan with a phrase allowlist? SWIFT_STRICT_CONCURRENCY=targeted/complete flags every preconcurrency XCUI use in the capture plan, which is out of scope; -Werror groups need Swift 6.1 and break the Xcode 16 floor; SWIFT_TREAT_WARNINGS_AS_ERRORS fails the base on four unrelated warnings. The production net is +149 lines, well under the 700-line threshold, and this looks like the minimal set the issue proved. Should #2882 stay open until item 8 lands, since that covers the source guard for the remaining undiagnosed hole shapes (a non-Sendable escaping closure formed in a @mainactor function, and DispatchSource handlers)? Not blocking: scripts/build-xcuitest-apple.sh should probably clean DerivedData (or leave a marker) when its diagnostics scan fails, so a rerun after a partial build still catches isolation warnings instead of silently exiting 0, and the new sendResponse(result:over:) overload in RunnerTests+Transport.swift could keep the prior [weak self] capture in afterSend instead of capturing self strongly, but both can be taken or left. The only check still running is iOS Smoke Tests (run 36009184154); it has hit "Run gesture pan-duration smoke replay" with nothing failed so far, and the step that builds the runner with the new scan from a cold start already passed, along with the replay steps that exercise the changed main-actor hops through assumeIsolated. macOS, Android, and Linux smoke jobs passed, as did Coverage, which runs the new vitest scan test. I have no information on conflicts. The next thing to happen is for that in-progress iOS Smoke Tests run to finish; no finding here stands in the way of merging once it does. |
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Reviewed at 2c277cd. Main-owned state extraction is useful, but the PR's isolation story has three real holes: one lifecycle hop bypasses the helper and loses its MainActor annotation, the helper is file-private rather than the shared chokepoint the PR body advertises, and the new scan has no positive control. I checked and discarded a few near misses: the Sendable-result comment on runOnMainActor is accurate, the build tee/status plumbing preserves the exit code, and the warm-cache partial scan is CI-unreachable because the cache key covers the runner sources and scan script.
| } | ||
| withoutActuallyEscaping(operation) { escapableOperation in | ||
| let block: @convention(block) () -> Void = escapableOperation | ||
| let block: @MainActor @convention(block) () -> Void = { escapableOperation() } |
There was a problem hiding this comment.
The @mainactor annotation stops at this line. The block variable is typed as a MainActor block, but passing it to performWithOptions below converts it to the nonisolated @convention(block) parameter, and I could not get a Swift 5 or Swift 6 compile to diagnose the loss. So this main hop relies entirely on XCTest calling the block on the main actor, and the new diagnostics scan cannot see the violation class. Either route this through the helper the PR introduces, or use a plain block plus MainActor.assumeIsolated inside it if XCTest's call thread is the intended assumption.
| /// Runs `work` on the main thread the caller is already on. `MainActor.assumeIsolated` returns | ||
| /// only `Sendable` values, so the result leaves through a captured `Result`: a `T: Sendable` bound | ||
| /// on the hop would promise something no gate checks. | ||
| private static func runOnMainActor<T>(_ work: @MainActor () throws -> T) -> Result<T, Error> { |
There was a problem hiding this comment.
private static in an extension is file-scoped, so this is not the one assumeIsolated chokepoint the PR body describes. applyMainOwnedSnapshotState already duplicates the hop directly in RunnerTests+SnapshotCaptureTarget.swift at lines 83 and 89. Make this helper internal, or move it beside RunnerMainOwnedState, and route those calls through it so the captured-result workaround lives in one place.
| * "call to main actor-isolated … [#ActorIsolatedCall]", and "converting function value … loses | ||
| * global actor 'MainActor'". | ||
| */ | ||
| const ISOLATION_PHRASES = ['actor-isolated', 'loses global actor']; |
There was a problem hiding this comment.
A two-phrase substring scan plus zero-matches-means-success is fragile: if Swift rewords an actor-isolation diagnostic, the gate can go green while the underlying warning remains. I also saw #SendableClosureCaptures diagnostics fire for isolation-shaped mistakes without either phrase. Add a canary positive control compiled with the same flags and assert the scan sees its diagnostic, or match stable diagnostic identifiers/tags instead of English prose where possible.
|
Update for 2c277cd: this branch now conflicts with |
2c277cd to
077cfff
Compare
|
Addressed the review at 2c277cd. New head 077cfff.
Not-blocking suggestions: took the DerivedData one — Rebase: onto current Quality pass: found and fixed two more instances of the pattern the review flagged — nine Gate result on 077cfff: |
|
Reviewed at 077cfff. The isolation fix looks correct and the runner gates pass. This follows up on the earlier review at 2c277cd (#2911 (comment)). The The 18 CI checks on 077cfff are green. The change touches the runner build script, the isolation scan, and the main-thread hop behind type, swipe, gesture, and alert, and the iOS and macOS smoke jobs exercise those paths and pass. I could not run Xcode, so I did not reproduce the canary firing on all four marked lines or confirm the scan is clean on iOS and macOS; that rests on the author's claims plus green CI. Not blocking: whether the smoke run actually exercises the trap, and whether the canary flag needs to stay on for every build including the release workflow (scripts/build-xcuitest-apple.sh:136, https://github.com/callstack/agent-device/blob/077cfff/scripts/build-xcuitest-apple.sh#L136) rather than only gate builds, can be taken or left. No blocking finding. This is ready for human review; the unverified Xcode 16 floor and the iOS 15/16 back-deploy question remain open limits for the maintainer to accept. |
thymikee
added
the
ready-for-human
|
The code verdict at 077cfff is unchanged, but this branch now conflicts with main after the latest merges. I removed ready-for-human until it is rebased. Please rebase and resolve the conflict; if the resolution touches only unrelated upstream code, the earlier review still applies. |
thymikee
removed
the
ready-for-human
077cfff to
bf14bb6
Compare
|
Rebased onto main Conflicts. Four runner files conflicted with #2899 (app launch policy axis) and #2929 (appstate through the runner). The PR's main-owned-state rule now covers the code those PRs added:
Runner code outside tests has no new casts or No test dropped. Every test name on main is present at the new head in each conflicted file. The only union names that are gone are the two tests #2899 deleted and the one it renamed. Checks on
|
|
The mainOwned/@mainactor gate now catches off-main access to main-owned capture state at bf14bb6, and I found nothing to block on in the diff. The PR has merge conflicts with main. All 19 checks pass on bf14bb6. The delta touches the runner files the swift-runner build, isolation scan, and runner unit jobs compile and run, and those are green, so no failure needs attribution. I did not build the runner or run the isolation scan or the canary, so the zero-violations claim, the canary firing on 4 lines, and the 324 passing unit tests are the author's numbers, backed only by green CI. I also did not check #2929 appstate or #2899 launch-policy behavior on a device; the judgement that behavior is unchanged rests on reading the diff, where the non-annotation hunks are renames only. The branch is out of date again: current main also touches RunnerTests+CommandDispatch.swift, SnapshotExecution, SynthesizedTextEntry, TextTyping, and the Recording/TextEntryPolicy/TextTyping/Transport unit tests (for example from #2900), so this review does not cover those files. Please rebase onto current main and apply the same mainOwned/@mainactor rule to whatever changes those files pick up, since that is the next step before merge. |
…dState Target identity, accessibility health and the post-snapshot delay flag move into one @mainactor type, so Swift 5 mode rejects a read or write of them from the command queue instead of leaving the boundary to review (#2882). Every main hop's work closure is @mainactor and runs through one MainActor.assumeIsolated helper; timeoutError and onAbandoned are @sendable, so the #2836 routing-probe shape no longer compiles. Functions that only run on main carry @mainactor; the transport, command queue, capture plan, watchdog and the locked completion/abandonment owner stay nonisolated. Escaping closures carry @mainactor or @sendable: transport completions and in-flight waiters, DispatchSource handlers, the phase-timer clock and the unit-test hooks. The recorder's capture is @mainactor and is called only inside the frame tick's main hop; ScreenRecorder.start takes a non-escaping bootstrap and a @sendable frame. Unit tests that touch the state run on the main actor, or step onto it with MainActor.assumeIsolated where they also dispatch work off main.
Swift 5 mode reports several off-main uses of main-actor state only as warnings, for example a RunnerMainOwnedState read inside a DispatchQueue.async closure. build-xcuitest-apple.sh now tees the build-for-testing log, keeps xcodebuild's exit status, and after a successful build fails on any Swift diagnostic line, warning or error, that contains "actor-isolated" or "loses global actor". Other warnings stay ungated. The script says so when it reuses DerivedData, because an incremental build prints diagnostics only for the files it recompiles.
The build-for-testing step now runs scripts/runner-isolation-diagnostics.ts, so a change to the scan misses the cache and is exercised on a cold build, like the other scripts that build runs.
…inActor The quiescence-skip block converted a @mainactor block to XCTest's nonisolated block parameter, which the compiler accepts without a diagnostic. It is now a plain block that enters isolation through runOnMainActor, as do both applyMainOwnedSnapshotState hops, so every main-actor entry checks the thread in one internal helper.
The runner gate builds compile RunnerIsolationCanary.swift with the runner's own flags, and the scan fails unless every canary line carries a concurrency diagnostic, so a reworded diagnostic fails the gate instead of passing it. The scan also matches the ActorIsolatedCall and SendableClosureCaptures groups and Sendable-capture prose. A failed scan drops the intermediates so a rerun recompiles and rescans every file. The npm package does not ship the canary.
…ode to the main-owned state performSynthesizedGesture reads accessibility health through mainOwned and runs on the main actor with its callers; the unit tests that set the health or resolve alerts on main carry @mainactor.
…ecordings through one helper
…uplicate line-count test
bf14bb6 to
806fa93
Compare
|
Rebased onto main Only #2904 overlaps with this PR. #2944, #2949 and #2950 touch CI, Vitest and TS contracts, not runner Swift. All conflicts were in commit 1, and the range-diff shows commits 2–9 unchanged.
No test was dropped: every test name on main is still present. Checks on
Main keeps editing the same runner files, so merging soon would stop the rebase loop. |
This branch has not been deployed
1 participant
Summary
RunnerMainOwnedState(@MainActor) replaces five plainvars onRunnerTestsholding targetidentity, accessibility health and the post-snapshot-delay flag — a boundary #2621, #2781 and #2836
each patched one site of. Every main hop takes a
@MainActorwork closure through oneMainActor.assumeIsolatedhelper;timeoutError/onAbandonedare@Sendable, so the #2836routing-probe shape no longer compiles. About 50 main-only functions carry
@MainActor; captureplan, transport, dispatch and watchdog stay nonisolated.
build-xcuitest-apple.shtees the buildlog and fails
swift-runner-ios/-macoson anyactor-isolatedorloses global actordiagnostic, warnings included.
Part of #2882 — step 1 of 2 (items 1-7). Item 8, the source guard for rules (a)-(g), is a separate
stacked branch, not yet pushed; #2882 stays open until it lands. No runtime, timeout, wire-shape or
TS production change.
54 files touched. Net production lines: +149 (+72 Swift, +77 scan/build script/CI action).
Validation
Tested head
806fa93551, rebased ontomainc31cc49f6e(conflicts with #2899, #2929 and #2904 resolved under the main-owned-state rule). Rerun on this head:check:affected --rungreen;check:xctest-selection334 methods, 0 unreachable;swift-runner-ios/-macos, cleanDerivedData: exit 0, scan clean, the isolation canary fires on all 4 marked lines; runner unit suite on a throwaway iOS simulator 332/332 passed.
Carried forward from the
2c277cd60b42run (not rerun on this head): tvOS/visionOS clean, same warning counts base-vs-head; five throwaway-editproofs for shapes (a)-(e) each fail the real gate command with the expected diagnostic or scan
line; macOS host lane not
run (local signing); live iOS simulator smoke reached the changed route
(
applyMainOwnedSnapshotStateabandonment).Risks: Xcode 16 floor unverified (26.2 only); macOS host lane untested; recorder change needs a
physical device; a pre-existing, unrelated nightly count mismatch (309/306). No adversarial or ship
review is recorded for this branch.