test(deflake): deterministic joins for AccountsManager/Catalog/TokenRefresh async tests

Parallel-CI-clone starvation de-flake (continuation of #1319). Under CI's 2 sim
clones, tests that poll a fire-and-forget async op on a fixed wall-clock deadline
lose the CPU race and fail all 3 retries. Convert each to a deterministic JOIN
that awaits the ACTUAL async work — no clock, cannot starve.

- AccountsManager.swift: add an XCTest-gated first-run-task registry +
  `_awaitAllCrawlTasksForTesting()` join seam (mirrors the existing
  `_trackCrawlTask`/`fetchFromNetworkCountForTesting` conventions). Joins ONLY the
  first-run task (bundled decode → synchronous fetchFromNetwork kickoff), not the
  downstream live network crawl. Lock snapshot split into a synchronous helper
  (NSLock.lock/unlock are banned in async contexts under Swift 6).
- AccountsManagerFirstRunDecodeTests: 3 tests → async + await the seam (was
  RunLoop/wait(for:timeout:5)).
- CatalogRepositoryStaleWhileRevalidateTests: concurrent-reads test now joins
  BOTH background refreshes via _awaitAllBackgroundRefreshesForTesting() (was
  racily joining only the last-scheduled handle).
- TokenRefreshAndRetryQueueTests: every fire-and-forget refresh/retry wait → a
  semaphore signalled at the exact event, bridged to async off the cooperative
  pool via withCheckedContinuation + DispatchQueue.global (Task.detached's
  closure is itself async, so sem.wait() there is a Swift 6 error).

**Scope:** test-hermeticity + one XCTest-gated production seam on AccountsManager
(RELEASE byte-identical — the tracking arrays only populate under XCTest, the
join seam is the only consumer and production never calls it; verified
_trackCrawlTask is also XCTest-gated so the call-site swap is a no-op in RELEASE).
**Not done:** TokenRefreshInterceptorAuthCoordinatorTests (confirmed same
timing/join class, fix in a follow-up commit this session).
**Deferred:** the broader 135-file deadline-poll surface — only the observed-failing
tests are converted; more may surface per CI run and get the same treatment.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Author: t

Palace/Accounts/Library/AccountsManager.swift

@@ -421,6 +421,18 @@ private struct CrawlerHandoffBox: @unchecked Sendable {
         ProcessInfo.processInfo.environment["XCTestConfigurationFilePath"] != nil
     private let _trackedCrawlTasksLock = NSLock()
     private var _trackedCrawlTasks: [Task<Void, Never>] = []
+    /// The subset of tracked tasks that are `loadCatalogs` FIRST-RUN tasks (the
+    /// detached bundled-decode → synchronous `fetchFromNetwork` kickoff). Guarded
+    /// by `_trackedCrawlTasksLock`. Populated only under XCTest (same env gate as
+    /// `_trackedCrawlTasks`). The deterministic-join seam
+    /// `_awaitAllCrawlTasksForTesting()` awaits ONLY these — NOT the downstream
+    /// live network crawl the first-run task spawns via `fetchFromNetwork` (that
+    /// hits the real registry and would reintroduce wall-clock nondeterminism).
+    /// By the time a first-run task's `.value` resolves, its synchronous
+    /// `fetchFromNetwork` call has already returned (bumping
+    /// `_fetchFromNetworkCount`), so joining the first-run task alone is a
+    /// deterministic barrier for the decode + fetch-fired observations.
+    private var _trackedFirstRunTasks: [Task<Void, Never>] = []
 
     /// Resolves the build-time bundled registry snapshot resource. Production
     /// binds `Bundle.main`; tests inject a stub (`BundleResourceResolving`) to
@@ -453,6 +465,63 @@ private struct CrawlerHandoffBox: @unchecked Sendable {
         _trackedCrawlTasksLock.unlock()
     }
 
+    /// Register a `loadCatalogs` FIRST-RUN task. Appends to BOTH the general
+    /// crawl registry (so `cancelBackgroundWork()` cancels it, identical to
+    /// `_trackCrawlTask`) AND the first-run subset the deterministic-join seam
+    /// awaits. No-op outside an XCTest process — production never populates
+    /// either list, so RELEASE behavior is byte-identical to a bare
+    /// `_trackCrawlTask` (which is itself a no-op in RELEASE).
+    private func _trackFirstRunTask(_ task: Task<Void, Never>) {
+        guard Self._isRunningUnderXCTest else { return }
+        _trackedCrawlTasksLock.lock()
+        _trackedCrawlTasks.append(task)
+        _trackedFirstRunTasks.append(task)
+        _trackedCrawlTasksLock.unlock()
+    }
+
+    /// Test-only deterministic JOIN seam. Snapshots the currently-tracked
+    /// FIRST-RUN tasks under the lock and `await`s each to completion, so a test
+    /// can join the ACTUAL off-main first-run work (bundled decode → synchronous
+    /// `fetchFromNetwork` kickoff) spawned by `loadCatalogs` instead of polling a
+    /// wall-clock deadline. Under CI's parallel sim clones a fixed-deadline poll
+    /// starves (the detached `.utility` task loses the CPU race) and fails on all
+    /// 3 retries; awaiting the real handle removes the clock entirely.
+    ///
+    /// Awaits ALL tracked first-run tasks (not just the most-recent), so a
+    /// dedupe-break that erroneously spawns a SECOND first-run task is also
+    /// joined — `testSecondConcurrentLoad` can then assert the decode ran exactly
+    /// once after every first-run task has finished. It deliberately does NOT
+    /// await the downstream live network crawl that `fetchFromNetwork` spawns
+    /// (which hits the real registry and would reintroduce nondeterministic
+    /// latency); the first-run task's synchronous `fetchFromNetwork` call has
+    /// already returned (bumping `_fetchFromNetworkCount`) by the time its
+    /// `.value` resolves, so joining the first-run task alone suffices.
+    ///
+    /// `nonisolated` + a synchronous lock-guarded snapshot: the manager is only
+    /// touched synchronously here (the `Task<Void, Never>` handles are Sendable),
+    /// so awaiting this from an `@MainActor` test never "sends" the non-Sendable
+    /// manager across an isolation boundary. Behavior-identical to a no-op in
+    /// RELEASE — the first-run-tasks array only ever populates under XCTest (the
+    /// `_trackFirstRunTask` env gate), and production never calls this method. It
+    /// only READS/awaits existing handles; it spawns no work and mutates no
+    /// production state.
+    /// Synchronous lock-guarded snapshot of the tracked first-run tasks. Split
+    /// out of the `async` join seam because `NSLock.lock()`/`.unlock()` are
+    /// unavailable from an async context (Swift 6 forbids them even when the
+    /// critical section never spans a suspension point). The `await` happens in
+    /// the caller, strictly after the lock is released here.
+    nonisolated private func _snapshotFirstRunTasksForTesting() -> [Task<Void, Never>] {
+        _trackedCrawlTasksLock.lock()
+        defer { _trackedCrawlTasksLock.unlock() }
+        return _trackedFirstRunTasks
+    }
+
+    nonisolated func _awaitAllCrawlTasksForTesting() async {
+        for task in _snapshotFirstRunTasksForTesting() {
+            _ = await task.value
+        }
+    }
+
     /// Initializer is `internal` rather than `private` so `AppContainer` can
     /// construct the single live instance directly. Outside of `AppContainer`
     /// (and tests that need an isolated instance), do not call this directly
@@ -1225,7 +1294,7 @@ private struct CrawlerHandoffBox: @unchecked Sendable {
             Log.debug(#file, "Loading catalogs from network for hash \(hash)…")
             self.fetchFromNetwork(targetUrl: targetUrl, hash: hash)
         }
-        _trackCrawlTask(firstRunTask)
+        _trackFirstRunTask(firstRunTask)
     }
 
     /// Fetches catalog data using the first-page fast path:
@@ -2073,6 +2142,10 @@ extension AccountsManager {
         _trackedCrawlTasksLock.lock()
         let crawlTasks = _trackedCrawlTasks
         _trackedCrawlTasks.removeAll()
+        // Clear the first-run subset too — its handles are a subset of
+        // `_trackedCrawlTasks` (cancelled above), so dropping them here keeps the
+        // deterministic-join seam from re-awaiting an already-cancelled task.
+        _trackedFirstRunTasks.removeAll()
         _trackedCrawlTasksLock.unlock()
         crawlTasks.forEach { $0.cancel() }
         networkExecutor.cancelNonEssentialTasks()

PalaceTests/Accounts/AccountsManagerFirstRunDecodeTests.swift

@@ -129,34 +129,15 @@ final class AccountsManagerFirstRunDecodeTests: PalaceWiringTestCase {
         return defaults
     }
 
-    /// Spin the run loop until `condition` is true or the deadline passes.
-    /// Used to observe the off-main first-run task's synchronous counters from
-    /// the main test thread without a fixed sleep.
-    @discardableResult
-    private func waitUntil(timeout: TimeInterval = 5,
-                           _ condition: () -> Bool) -> Bool {
-        let deadline = Date().addingTimeInterval(timeout)
-        while !condition() && Date() < deadline {
-            RunLoop.current.run(until: Date().addingTimeInterval(0.02))
-        }
-        return condition()
-    }
-
     // MARK: - 1. Dedupe fires BEFORE the bundled decode
 
     /// A second load arriving while the first is still in flight must
     /// short-circuit on the consolidated dedupe guard — which sits ABOVE the
     /// bundled branch — so the ~2.4 MB bundled snapshot is decoded exactly
     /// once, not once per caller.
-    func testSecondConcurrentLoad_shortCircuitsBeforeBundledDecode() throws {
+    func testSecondConcurrentLoad_shortCircuitsBeforeBundledDecode() async throws {
         let snapshotURL = try writeStubSnapshot()
-        let decodeRan = expectation(description: "bundled snapshot resolved")
-        decodeRan.expectedFulfillmentCount = 1
-        // A broken dedupe (decode runs per caller) over-fulfills → test fails.
-        decodeRan.assertForOverFulfill = true
-        let resolver = CountingSnapshotResolver(snapshotURL: snapshotURL) {
-            decodeRan.fulfill()
-        }
+        let resolver = CountingSnapshotResolver(snapshotURL: snapshotURL)
 
         let defaults = isolatedDefaults()
         let manager = makeFreshAccountsManager(defaults: defaults) {
@@ -172,10 +153,12 @@ final class AccountsManagerFirstRunDecodeTests: PalaceWiringTestCase {
         // second first-run task, no second decode.
         manager.loadCatalogs(completion: nil)
 
-        wait(for: [decodeRan], timeout: 5)
-        // Settle briefly so an (erroneous) second decode would have a chance to
-        // bump the counter before we assert.
-        _ = waitUntil(timeout: 0.5) { false }
+        // Deterministic JOIN: await the ACTUAL first-run task(s) to completion
+        // rather than racing a wall-clock deadline (which starves under CI's
+        // parallel sim clones). This awaits ALL tracked crawl tasks, so a
+        // dedupe-break that spawned a SECOND first-run task would also be joined
+        // here — after which callCount==1 proves the decode ran exactly once.
+        await manager._awaitAllCrawlTasksForTesting()
         XCTAssertEqual(resolver.callCount, 1,
                        "Bundled snapshot must be decoded exactly once; the second concurrent load must dedupe before the decode")
 
@@ -190,7 +173,7 @@ final class AccountsManagerFirstRunDecodeTests: PalaceWiringTestCase {
     /// own registration and `return` before the fetch — leaving this counter
     /// at 0 and stranding the picker on stale bundled data. This test fails
     /// loudly on that regression.
-    func testFirstRun_networkFetchStillFiresOnce_afterBundledDecode() throws {
+    func testFirstRun_networkFetchStillFiresOnce_afterBundledDecode() async throws {
         let snapshotURL = try writeStubSnapshot()
         let resolver = CountingSnapshotResolver(snapshotURL: snapshotURL)
 
@@ -204,12 +187,14 @@ final class AccountsManagerFirstRunDecodeTests: PalaceWiringTestCase {
 
         manager.loadCatalogs(completion: nil)
 
-        // The fetch is dispatched from the off-main first-run task AFTER the
-        // bundled decode. A swallowed fetch keeps this at 0 → the wait fails.
-        let fired = waitUntil { manager.fetchFromNetworkCountForTesting >= 1 }
-        XCTAssertTrue(fired, "Network fetch must fire after the bundled decode — it appears to have been swallowed by the dedupe")
+        // Deterministic JOIN: the fetch is dispatched from the off-main first-run
+        // task AFTER the bundled decode. Awaiting the actual task (instead of
+        // polling a wall-clock deadline that starves under parallel CI clones)
+        // guarantees the fetch has fired by the time the await returns; a
+        // swallowed fetch leaves the counter at 0 and fails the assert below.
+        await manager._awaitAllCrawlTasksForTesting()
         XCTAssertEqual(manager.fetchFromNetworkCountForTesting, 1,
-                       "Network fetch must fire exactly once")
+                       "Network fetch must fire exactly once after the bundled decode — it appears to have been swallowed by the dedupe")
         XCTAssertEqual(resolver.callCount, 1,
                        "The bundled snapshot must have been decoded once before the network fetch (fetch fires AFTER it, not instead of it)")
 
@@ -222,14 +207,9 @@ final class AccountsManagerFirstRunDecodeTests: PalaceWiringTestCase {
     /// must be hopped off-main — otherwise the ~2.4 MB decode blocks the main
     /// thread on cold first launch (the original bug via
     /// `presentFirstRunFlowIfNeeded`).
-    func testFirstRun_bundledDecode_runsOffMainThread() throws {
+    func testFirstRun_bundledDecode_runsOffMainThread() async throws {
         let snapshotURL = try writeStubSnapshot()
-        let decodeRan = expectation(description: "bundled snapshot resolved")
-        let resolver = CountingSnapshotResolver(snapshotURL: snapshotURL) {
-            decodeRan.fulfill()
-        }
-        decodeRan.expectedFulfillmentCount = 1
-        decodeRan.assertForOverFulfill = false
+        let resolver = CountingSnapshotResolver(snapshotURL: snapshotURL)
 
         let defaults = isolatedDefaults()
         let manager = makeFreshAccountsManager(defaults: defaults) {
@@ -239,7 +219,11 @@ final class AccountsManagerFirstRunDecodeTests: PalaceWiringTestCase {
         XCTAssertTrue(Thread.isMainThread, "test drives loadCatalogs from the main thread")
         manager.loadCatalogs(completion: nil)
 
-        wait(for: [decodeRan], timeout: 5)
+        // Deterministic JOIN: await the off-main first-run task to completion
+        // instead of racing a 5s wall-clock deadline that starves under parallel
+        // CI clones. Once it returns, the decode has run and recorded the thread
+        // it ran on.
+        await manager._awaitAllCrawlTasksForTesting()
         XCTAssertEqual(resolver.lastCallOnMainThread, false,
                        "The bundled snapshot decode must not run on the main thread")
 

PalaceTests/CatalogDomain/CatalogRepositoryStaleWhileRevalidateTests.swift

@@ -91,10 +91,13 @@ final class CatalogRepositoryStaleWhileRevalidateTests: XCTestCase {
     }
 
     // NOTE: the former private `awaitCondition` poll helper was removed — every
-    // background-refresh wait now joins the repository's refresh Task directly
-    // via `_awaitBackgroundRefreshForTesting()`, which is deterministic under
-    // pool oversubscription (a fixed-deadline poll of a `.utility` detached
-    // task's side effect was the parallel-clone flake, CI run 29805821296).
+    // background-refresh wait now joins the repository's refresh Task(s) directly
+    // via `_awaitBackgroundRefreshForTesting()` (single-refresh tests) or
+    // `_awaitAllBackgroundRefreshesForTesting()` (the concurrent test, which
+    // schedules two refreshes and arms `_trackRefreshTasksForTesting()` first).
+    // Both are deterministic under pool oversubscription (a fixed-deadline poll
+    // of a `.utility` detached task's side effect was the parallel-clone flake,
+    // CI run 29805821296).
 
     // MARK: - Fresh cache window (< 10 min)
 
@@ -369,6 +372,16 @@ final class CatalogRepositoryStaleWhileRevalidateTests: XCTestCase {
         testNow.value = testNow.value.addingTimeInterval(1800) // 30 minutes
         api.stubbedFeeds[testURL] = CatalogAPIMock.makeMockFeed(title: "Refreshed-concurrent")
 
+        // Arm multi-refresh tracking BEFORE firing the concurrent reads so the
+        // repository retains EVERY scheduled refresh handle, not just the
+        // most-recently-scheduled one. Both stale reads each schedule their own
+        // `.utility` detached refresh; the single-handle
+        // `_awaitBackgroundRefreshForTesting()` only joins the last one, which
+        // would let this test observe the cache mid-flight if the un-joined
+        // refresh is the one still pending. Tracking is opt-in and defaults off,
+        // so this is a no-op in production.
+        sut._trackRefreshTasksForTesting()
+
         // Sendable local so the async-let children capture it instead of reading
         // @MainActor `self.testURL` (which would send self). `sut` is already local.
         let testURL = testURL
@@ -381,16 +394,16 @@ final class CatalogRepositoryStaleWhileRevalidateTests: XCTestCase {
         XCTAssertEqual(resultB?.title, "Original-concurrent",
                        "Concurrent stale read B must return cached value")
 
-        // Join the background refresh deterministically. Both stale reads
+        // Join BOTH background refreshes deterministically. Each stale read
         // scheduled a `.utility` detached refresh SYNCHRONOUSLY before
-        // returning; the repository retains the last-scheduled one. Awaiting
-        // it blocks until that refresh's cache write lands — regardless of how
-        // starved the cooperative pool is under parallel-clone oversubscription
-        // (the old two-poll approach timed out at :401/:408 exactly there in CI
-        // run 29805821296). Both refreshes fetch + write the same
-        // "Refreshed-concurrent" feed, so the last one landing proves the
-        // contract.
-        await sut._awaitBackgroundRefreshForTesting()
+        // returning; awaiting ALL tracked handles blocks until every refresh's
+        // cache write lands — regardless of how starved the cooperative pool is
+        // under parallel-clone oversubscription (the old two-poll approach timed
+        // out at :401/:408 exactly there in CI run 29805821296). Using the "all"
+        // variant (not the single-handle join) makes the fetch-count and
+        // cache-refresh assertions below independent of which of the two
+        // concurrent refreshes finishes last.
+        await sut._awaitAllBackgroundRefreshesForTesting()
 
         // The refresh fired (mutation kill: a no-op'd background branch never
         // bumps the count past 1). Deterministic now that the Task has joined.