fix(deflake): AccountsManager useBeta handler off poster-thread + CrossDeviceSyncE2E joins - #1321
Merged
Merged
Conversation
…ssDeviceSyncE2E joins The two deadline-poll/hang flakes that survived #1306's crasher fix: - AccountsManager.updateAccountSetFromNotification (testUseBetaDidChange 120s hang): .TPPUseBetaDidChange is delivered SYNCHRONOUSLY by NotificationCenter on the poster's thread (main, from a Settings toggle). The handler did an accountSetsLock.sync read (performRead) that blocks until any in-flight background catalog-refresh barrier on that lock drains — under CI full-suite load that barrier can hold for the whole 120s execution allowance, hanging the poster (and, in production, stalling the Settings UI). Dispatch the account-set update off the poster's thread (.userInitiated) — it's inherently async (it may reload catalogs), so this is behavior-correct AND unblocks the poster. - CrossDeviceSyncE2ETests (test_audiobookPositionOnDeviceA ~32s): all 10 wait/fulfillment(timeout:) deadline-polls on fire-and-forget backend annotation completions converted to withCheckedContinuation joins. Bookmark types are NSObject-based (non-Sendable), so each site projects the asserted primitive facts inside the completion (off-isolation) and resumes with a Sendable projection — no non-Sendable object crosses the @mainactor boundary. Verified: 56 tests pass in isolation (51 E2E + 5 AccountsManager). **Scope:** one critical-path AccountsManager behavior fix (settings-notification handler no longer blocks the poster) + E2E test-hermeticity. The async dispatch preserves the account-set update; only the thread it runs on changes. **Not done:** the broader deadline-poll tail beyond these observed-failing tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🏗️ CodeAtlas Ledger Analysis✅ All Checks Passed♿ Accessibility (via AccessLint)
✅ No accessibility issues detected 🧪 Test Coverage (via QAAtlas)
🏛️ Architecture Analysis
🔍 Reachability Analysis
✅ No dead code detected 📊 0 files analyzed | 📦 Download Full Report Powered by CodeAtlas Ledger |
🧪 Unit Test Results📊 View Full Interactive Report ❌ 3 TESTS FAILED7966 tests | 7829 passed | 3 failed | 132 skipped | ⏱️ 10m 9s | 📊 98.3% | 📈 46.1% coverage Tests by Class — 909 classes, 3 with failures
Failed Tests (click to expand)📊 Testing Coverage BreakdownUnit Test Line Coverage (testable surfaces): 46.1% Total coverage incl. UI/lifecycle: 45.1% (17 files excluded from testable denominator — see
🔗 Interactive HTML Report | CI Run Details 📦 Downloadable Artifacts
|
mauricecarrier7
added a commit
that referenced
this pull request
Jul 23, 2026
…starvation (#1328) Prevents reintroduction of recurrence class `parallel-clone-starvation` (docs/regressions/recurrence-classes.md, fixed in #1319/#1321): tests that wait on fire-and-forget async via a fixed wall-clock deadline (fulfillment(of:timeout:), wait(for:timeout:), waitForExpectations(timeout:)) lose the CPU race under the 2 parallel sim clones on the CI macOS runner and fail all 3 -retry-tests-on-failure iterations because the load persists across iterations. Three coordinated changes: 1. docs/Testing/Test_Patterns.md 10.1 — the old "Good" example WAS the anti-pattern (await fulfillment(of:[e], timeout: 5.0)). Rewrote it to explain the starvation failure mode and present the deterministic Task-join seam as the fix, with a before/after and the three real seams in the tree (AccountsManager._awaitAllCrawlTasksForTesting, CatalogRepository._awaitAllBackgroundRefreshesForTesting, TokenRefreshInterceptor._awaitAuthDispatchForTesting). 2. scripts/lint-test-quality.py — new STARVE-001 rule flagging NEW deadline-poll waits, DIFF-SCOPED to ADDED lines only (--diff <path|-> / --changed [BASE]) so it never chokes on the ~675 legacy occurrences. Per-line // STARVE-001-OK escape hatch. Updated FLAKE-001 remediation text to stop recommending the XCTestExpectation deadline pattern and point at the Task-join seam. Existing rules and the full-scan exit behavior are untouched; STARVE-001 never runs in full-scan mode. 3. .github/workflows/tooling-checks.yml — new diff-scoped ubuntu `flake-lint` job runs the gate against the PR's base branch (fetch-depth: 0 + base fetch), failing the PR on a new deadline-poll. No Xcode/sim needed (~seconds). Verification (green-board contract): - scripts/tests/test_lint_test_quality.py (new, 16 tests): violation-path fires on all three deadline shapes; CLEAN-DIFF pass asserted (Task-join seam + pure sync); scoping guards (context lines, comments, allow-list, non-test paths); parser unit tests; existing MISSING-001 full-scan still fires and STARVE-001 never leaks into full-scan; --changed git entry point end-to-end (flag + clean). 339/339 in scripts/tests/ pass. - Dry-run `--changed origin/develop` on this branch: 0 findings (this PR adds no test files). Over a real 40-commit range: 30 true-positive historical adds, 0 false positives, no crash. - tooling-checks.yml parses (2 jobs); linter py_compiles; verify-pr.sh's --per-file invocation is unaffected by the new early-return modes. **Scope:** CI-gating infra only — docs + Python linter + one CI job + its pytest. No production Swift changed; the three cited Task-join seams already exist on develop and are referenced, not modified. **Not done:** the ~675 legacy deadline-poll occurrences are intentionally NOT migrated here (added-line scoping keeps the board green); STARVE-001 is not wired into the pre-commit hook (CI-gate only, by design). Migration of legacy polls is separate follow-up work. Claude-Session: https://claude.ai/code/session_016P51JUco66jhqqYE85XUvZ Co-authored-by: t <t@t.io> Co-authored-by: Claude Opus 4.8 (1M context) <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.
Fixes the two deadline-poll/hang flakes that survived #1306's crasher fix — the remaining tail on develop.
testUseBetaDidChange (120s hang)
.TPPUseBetaDidChangeis delivered synchronously by NotificationCenter on the poster's thread (main, from a Settings toggle).AccountsManager.updateAccountSetFromNotificationdid anaccountSetsLock.syncread that blocks until any in-flight background catalog-refresh barrier on that lock drains — under CI full-suite load that can hold for the whole 120s execution allowance, hanging the poster (and, in production, stalling the Settings UI). Now dispatched off the poster's thread; the account-set update is inherently async (it may reload catalogs), so this is behavior-correct.CrossDeviceSyncE2E (~32s)
All 10
wait/fulfillment(timeout:)deadline-polls on fire-and-forget backend annotation completions →withCheckedContinuationjoins. Bookmark types are NSObject-based (non-Sendable), so each site projects the asserted primitive facts inside the completion and resumes with a Sendable projection.Verified: 56 tests pass in isolation (51 E2E + 5 AccountsManager).
🤖 Generated with Claude Code