Skip to content

Commit c04219b

Browse files
mauricecarrier7tclaude
authored
fix(test): full de-flake sweep — parallel-starvation deadline-polls → deterministic joins (#1319)
* test(network/signin): de-flake state-machine deadline-polls by joining the actual work Four tests waited for fire-and-forget async work via a fixed wall-clock deadline (asyncAfter probes / Task.sleep settle windows) and starved under CI parallel oversubscription — the deadline elapses before the awaited work lands and the test times out even though the work DID complete. Replaced each poll with a deterministic JOIN of the real work; assertions unchanged. - TPPSignInBusinessLogicStateMachineTests.testLogIn_racingAuthDocLoad_firesRequestOnceReady: dropped the 0.5s/1.5s asyncAfter probes; wake the instant the mock records the credential request via a new join seam. - UnifiedOPDSServiceStateMachineTests.testFetchLoansFeed_blocksUntilLoaded_thenFetches & OPDSFeedServiceStateMachineTests (same name): dropped the 80ms settle sleep and joined the held fetch Task with `await fetchTask.value`. Mid-flight negatives hold without a settle — the awaitReady gate can't fire HTTP or complete while state is .detailsLoading. - NotificationServiceStateMachineTests.testHoldNotification_blocksUntilLoaded_thenNavigates: dropped the 30ms subscribe-settle sleep; the state stream is a CurrentValueSubject so a late subscriber replays the terminal state — no lost-wakeup race, `await outcomeTask` is the join. Added a test-only `onExecuteRequest` join seam to TPPRequestExecutorMock (fires synchronously as executeRequest records a URL; cleared in reset()). **Scope:** test-only. Zero production LOC changed — all edits under PalaceTests/. The sign-in test is money-path but its production path (logIn/awaitReadyThenRetryLogIn) is untouched; only the mock and the test's wait mechanism changed, and the prod-behavior assertions (request fires, carries /patrons/me) are preserved. **Not done:** did not touch SignInWebSheetIntegrationTests (real WKWebView cold-start, already documented FLAKE-003-OK — env-bound, not a deadline poll). **Deferred:** none. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(catalog): de-flake parallel-starvation deadline polls via work-unit joins CI runs PalaceTests with sim-clone oversubscription; tests that waited on a fire-and-forget async op (Task.detached refresh, load()'s currentLoadTask) via a fixed wall-clock deadline (awaitCondition / fulfillment(timeout:) / Task.sleep poll) starved past the 120s executionTimeAllowance. Fix: join the actual work unit instead of polling a deadline. CatalogCacheKeyAndIsolationTests: - testConcurrentStaleReads_AcrossDistinctURLs_EachURLGetsItsOwnRefresh — join all 3 concurrent .utility refreshes via _awaitAllBackgroundRefreshesForTesting - testConcurrentStaleReads_BackgroundRefreshDoesNotPoisonNeighborKey — join the single refresh via _awaitBackgroundRefreshForTesting - testInMemoryCache_AfterSystemMemoryWarning_... + testMemoryWarning_ClearsBoth — handler evicts synchronously under NotificationCenter.post; assert directly - removed the now-orphaned awaitCondition wall-clock poll helper CatalogDomainTests: dropped two stale "give cache queue time" Task.sleep(100ms) waits — invalidateCache is a synchronous lock mutation (no queue since the SWR refactor). CatalogViewModelTests: 8 sites (load error/nil/offline, forceRefresh, SWR A→B→A waitUntilLoaded) now join currentLoadTask via _awaitLoadForTesting. Reconnect auto-reload left as a bounded observable-effect poll (documented un-joinable: the reload hops through two untracked bare Tasks off the reachability publisher). SideloadedLaneTests: awaitLoaded/awaitFailure join currentLoadTask instead of a 20s $state fulfillment. Production seams (additive, behavior-identical, gated/defaulted — prod path unchanged): - CatalogRepository._trackRefreshTasksForTesting() + _awaitAllBackgroundRefreshesForTesting() (refreshTracking flag defaults off; prod never appends/grows the tracked-task array) - CatalogViewModel._awaitLoadForTesting() (reads the already-retained currentLoadTask handle) No assertions weakened; no XCTSkip, no raised executionTimeAllowance, no disabled parallelism. Both production files pass `swiftc -parse`. **Scope:** test-only de-flake batch (CATALOG). Prod change is 2 files / 66 LOC, all additive underscore-prefixed test seams + doc comments — zero runtime behavior change on any production path (new tracking is opt-in and defaults off; the load seam only reads an existing handle). No money-path (borrow/DRM/sign-in) code touched. Not done: the reconnect-auto-reload connectivity-publisher hop still lacks a retained-handle join seam (deferred to the connectivity owner); its test keeps a bounded observable-effect poll. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(deflake): join fire-and-forget async work instead of polling wall-clock deadlines (ACCOUNTS/PLATFORM batch) De-flakes fixed-deadline polls that starve under CI sim-clone oversubscription (2 clones on a small runner) and blow the 120s executionTimeAllowance. Every change replaces a wall-clock deadline (Task.sleep-then-assert, RunLoop.run(until:) spin, tight fulfillment ceiling) with a deterministic JOIN on the actual work. Test-side conversions: - AppLaunchTracker{,Extended}Tests: join the fire-and-forget catalogLoaded metrics report via new tracker.awaitPendingMetricsReport() seam (was: sleep 100/200ms). - AppHealthViewModelTests: join loadData() via new viewModel.awaitLoadForTesting() seam (was: 5× sleep 200ms); offline-queue status update now joins the ViewModel's main-hopped $offlineQueueStatus sink via fulfillment (was: sleep 200ms). - AccountStateMachineTests.testStateStream_emitsCurrentThenTransitions: subscription barrier on the stream's first (CurrentValueSubject current-value) emission before driving transitions (was: 2× sleep 30ms; genuine intermediate-state ordering hazard). - AppContainerResetTests: join presenter $hasActiveSession @published emission (was: 2s RunLoop.main.run(until:) spin). - AccountDetailViewModelTests: settle() joins $isLoadingAuth reaching false (was: yield×3 + sleep 50ms). - ViewModelComputedPropertyTests: drainMainQueueAsync() for the other-book no-react case (was: sleep 200ms). - ActiveSessionsViewModelTests: widen 3 genuine-join fulfillment ceilings 0.5s->5.0s to match the already-fixed site 185 precedent (join mechanism unchanged; only the failure-bound widened so a real join can't starve under oversubscription). Production seams (test-only join points, behavior-identical in production): - AppLaunchTracker: retain the metrics-report Task in metricsReportTask + expose awaitPendingMetricsReport(). Same task still runs detached; we only keep its handle. - AppHealthViewModel: retain the loadData() Task in loadTask + expose awaitLoadForTesting(). Same task still runs; we only keep its handle. No assertions weakened; no XCTSkip; no disabled parallelism. Left as-is (correctly): callback-fulfilled expectations, condition-driven bounded spins (UserAccountPublisher deinit-cancel loop, AccountsManagerCancellation suspend-point spin), negative-assertion settle windows (AccountsManagerFirstRunDecode 0.5s no-second-decode; bounded, not a 120s risk), and awaitReady() slow-path tests (CurrentValueSubject replay makes resolution ordering-independent). RemoteFeatureFlags 10s sleep is a withTimeout cancellation probe, not a deadline poll. **Scope:** 7 target test dirs (Accounts/Platform/Crawl/ViewModels/Migrations/ ImageLoading/AppInfrastructure). Crawl + Migrations had no deadline-polls. Skip-list files (OfflineQueueService/PositionSync/Accessibility/PerformanceMonitor/ StateMachineWiring/AccountDetailsURL/PoolResponsivenessProbe) untouched. **Not done:** no build/verify run (CI verifies per batch brief); no push/merge. **Deferred:** none. No money/critical-path production behavior changed — the two prod seams only retain an existing Task handle for a test join. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(reader): join fire-and-forget async instead of polling wall-clock deadlines (de-flake) De-flakes parallel-starvation deadline-poll tests in the reader/PDF/bookmark areas. Under CI's 2-sim-clone oversubscription, tests that waited a FIXED wall-clock interval (Task.sleep / debounce-deadline poll) for a fire-and-forget async op could starve past the deadline and hit the 120s executionTimeAllowance. Fix: JOIN the actual work via retained-Task seams / a persist-completion signal instead of racing a deadline. Also fixes one shared-state root (mock defaulting to the production singleton registry). Prod seams added (all behavior-identical; nil/uncalled in production): - TPPLastReadPositionPoster: retain spawned server-post Tasks + awaitPendingWrites() - TPPReaderTOCBusinessLogic: retain the init TOC-load Task + awaitTOCLoad() - TypographyService: onDidPersistForTesting callback fired after each debounced persist Shared-state root fixed: - MockPDFDocumentMetadata now injects an isolated TPPBookRegistryMock per instance instead of letting the parent init default to AppContainer.production().bookRegistry (every mock was touching the shared singleton — cross-test pollution). Behavior the tests assert (bookmark-set membership) is unchanged. Tests converted from deadline-polls to deterministic joins: - TPPLastReadPositionPosterTests (5 Task.sleep -> awaitPendingWrites) - TPPReaderTOCBusinessLogicTests (4 XCTNSPredicate 10s polls -> awaitTOCLoad) - TypographyServiceTests.testSettingsPersistedAfterDebounce (predicate poll -> persist-signal join) No assertions weakened; no XCTSkip/raised allowances/loosened asserts. **Scope:** Reader2/PDF/Bookmark de-flake only. Left genuine event-driven waits untouched (spy-navigator/completion-fulfilled expectations, count-matched concurrency joins, main-actor-drain barriers) — those already join real work. **Not done:** TypographyServiceIntegrationTests.swift is present in the project group but NOT compiled (no PBXSourcesBuildPhase entry; references a TypographyPersistence protocol that does not exist in prod) — its debounce Task.sleep polls are inert (never run in CI), so left as-is. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(audiobooks): de-flake parallel-starvation deadline-poll waits (swarm AUDIOBOOK) CI runs PalaceTests with 2 sim clones on a small runner; tests that wait for a fire-and-forget async op via a FIXED WALL-CLOCK deadline starve under oversubscription and hit the 120s executionTimeAllowance. This converts those waits to deterministic JOINS of the actual work unit. Fixed: - AudiobookBookmarkBusinessLogic{,PositionWrite}Tests: position-write detached Task now joined via a new `_awaitPositionWriteForTesting()` seam (retained Task-handle) instead of `wait(for:timeout:)` on the completion. - NowPlayingCoordinator{,Background}Tests: debounced `Task.sleep` update now joined via a new `_awaitPendingUpdateForTesting()` seam instead of `awaitCondition(timeout:5)` polling MPNowPlayingInfoCenter. - AudiobookSessionPresenterTests: replaced the fixed-50ms `RunLoop.main.run(until:)` publisher-delivery spin with the deterministic FIFO `drainMainQueue()` (16 sites via the shared helper). - AudiobookDataManagerSyncTests: mock POST completions moved off `.global().asyncAfter` onto a serial completion queue + `drainCompletions()` barrier; predicate-poll expectations replaced with `syncQueue.sync {}` + `drainCompletions()` joins. Production seams added are underscore-prefixed, test-only, and behavior- identical (no production call path invokes them; the write Task runs exactly as before, only its handle is now retained). Left as-is (genuine event-joins / un-joinable, noted): AudiobookLoader{,Dispatch, OPDSShapeMatrix}Tests + vendor adapter tests (real AppContainer.production() pipeline / @MainActor-Task hops driven by runloop spin, no clean seam without a DRM-path refactor); AudiobookOpenStateRace / FirstOpenHang (event-firing gate waits); AudioEngineWrapper / LoaderFinalizeBuild (synchronous completions); the debounce-dealloc crash-survival test (no work unit to await by design). **Scope:** test-only de-flake + two underscore-prefixed test-join seams on AudiobookBookmarkBusinessLogic + NowPlayingCoordinator. No behavior change. **Not done:** vendor-adapter (LCP/Bearer/OpenAccess) and AudiobookLoader Task joins — those hit the real fulfillment/DRM pipeline and warrant a swarm, not a lone seam; left with inline reasoning. **Deferred:** none. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(mybooks): de-flake return/auth/fulfillment deadline-poll waits (join, don't poll) Convert wall-clock deadline-polls on fire-and-forget async to deterministic joins across the return / auth / download-fulfillment money-path tests. Under CI oversubscription (2 sim clones, small runner) these polls (awaitConditionAsync / Task.sleep loops / asyncAfter+wait / while-Date loops) starve and hit the 120s executionTimeAllowance. Join patterns used: - Retained-Task join: await inFlightTasksSnapshotForTesting() Task.value (BookReturnService + DownloadAuthRetryHandler both retain their fire-and- forget Tasks; the auto-removal is the tail of each tracked body, so await task.value joins body+cleanup deterministically). - Synchronous retention insert -> direct assert (returnBook / handleAuth insert the tracked Task synchronously before returning; count is deterministically >=1 with no poll). - Prompt-firing expectation: fulfil an XCTestExpectation from the effect edge (error-publisher sink, reauthenticator.onAuthenticate, delegate.onStartDownload) instead of polling a captured-array count behind a fixed asyncAfter delay. - Main-actor barrier flush (await Task { @mainactor }.value) for all-@mainactor chains and absence-assertions, replacing fixed settle sleeps. Prod seam added (test-only behavior, behavior-identical): - DownloadAuthRetryHandler.inFlightTasksSnapshotForTesting() — a @mainactor read-only copy of the existing inFlightTasks set, mirroring the established BookReturnService seam. Mutates nothing; no production path changes. Un-joinable sites left with an inline note + kept as loud-on-timeout waits (no retention handle available without a larger prod change): - BookSignInRedirectHandler SAML-cookies-expired retry (background Task { }). - LCPFulfillmentHandler stored-fulfillment-task (untracked Task in a void method). - BookReturnService deinit observation (ARC last-release, not awaitable). - TokenRefreshInterceptor coordinator paths — barrier-flush (no retention seam on the interceptor yet; flagged as the complete follow-up). No assertions weakened; no XCTSkip; no raised allowances; parallelism untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(mybooks): de-flake download/borrow/registry deadline-poll waits (join, don't poll) Convert fixed-wall-clock deadline polls (awaitCondition / awaitConditionAsync / Task.sleep loops / RunLoop.run(until:)) that waited on fire-and-forget async work into deterministic joins. Under CI oversubscription (2 sim clones) those polls starve and blow the 120s executionTimeAllowance; joining the actual work removes the wall-clock race entirely. Test-side: await the SUT's own async entry points / retained-Task seams, drain-then-assert for synchronous registry+notification effects, and use prompt-firing NotificationCenter expectations (fulfilled by the SUT's own post) in place of RunLoop deadline polls. Publisher joins via a shared awaitPublished/awaitPublishedAsync helper. Prod: add behavior-identical join seams only — fire-and-forget `Task {}` bodies extracted verbatim into `...Async()` siblings whose sync wrapper is `Task { await ...Async() }` (enqueuePendingAsync, startBorrowAsync, limitActiveDownloadsAsync, pauseAllDownloadsAsync, processAsync), plus retained-Task `private(set) var last...Task` handles for tests to await. Same calls, same order, same side effects; no logic, branch, or state-transition change. Mirrors the existing schedulePendingStartsIfPossible→Async split. No assertions weakened, no XCTSkip, no raised allowance, parallelism untouched. **Scope:** de-flake pass over PalaceTests/MyBooks, PalaceTests/Book, PalaceTests/BookStateManagement (return/auth/fulfillment slice committed separately in the preceding commit). Prod seams are money-path (borrow/download) and confirmed test-only-behavior (behavior-identical joins). **Deferred:** a full retention seam for TokenRefreshInterceptor's ~20 fire-and-forget Task sites (too broad for a de-flake pass; two interceptor poll sites left with UNJOINABLE notes). Genuinely un-joinable sites (real debounce timers, ARC-deinit observation, absence-of-effect settles, SAML retry background Tasks with no handle) left in place with inline UNJOINABLE notes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * test(deflake): add #1315's SignOut/MultiLibrary joins the swarm skipped The full-sweep swarm branched off develop (which lacked #1315), so its Network batch skipped TPPSignInBusinessLogicSignOutTests + MultiLibraryTokenIsolationTests believing they were already on develop. They were only in #1315. Bring those two join fixes + the TPPDRMAuthorizingMock._awaitDeauthorizeCalledForTesting seam they need, so the sweep supersedes #1315 completely (BorrowOperationContractTests kept from the mybooks batch, which fixed the whole Borrow slice). **Scope:** test-only (2 test files + 1 test mock); no production change here. * fix(deflake): sync poster join seam (avoid Swift 6 sending self.poster) The reader batch's `awaitPendingWrites() async` seam, called on the @mainactor test's non-Sendable poster, tripped Swift 6 'sending self.poster'. Reshaped to a synchronous `pendingWriteTasksForTesting() -> [Task]` snapshot (a sync call sends nothing; Task is Sendable) and the test awaits the returned handles. Behavior identical; consolidated sweep now builds clean. **Scope:** compile fix for the consolidated de-flake sweep; no behavior change. * revert(deflake): restore DownloadAuthRetryHandler de-flake to develop (broke behavior) The mybooks batch's join conversion of DownloadAuthRetryHandlerTests asserted before the alert-publish/retry-download effect landed → 2 deterministic failures (testAutoBorrowCompletion_whenBorrowFails, testHandle_401_withoutCredentials). This class's async structure was flagged partial/complex; reverting its de-flake (+ the prod seam) to develop's correct version is safer than shipping a broken money-path join. The class may still occasionally flake under parallel load (pre-existing) — that's a separate, non-regressing follow-up. **Scope:** revert 3 files to develop; rest of the sweep unchanged. --------- Co-authored-by: t <t@t.io> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
1 parent 0b60497 commit c04219b

76 files changed

Lines changed: 1629 additions & 1044 deletions

File tree

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

Palace/Audiobooks/NowPlayingCoordinator.swift

Lines changed: 11 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -359,6 +359,17 @@ public final class NowPlayingCoordinator {
359359
func _test_setLastIsPlaying(_ playing: Bool) {
360360
lastIsPlaying = playing
361361
}
362+
363+
/// Test-only join seam. Awaits the in-flight debounced update `Task`
364+
/// (`pendingUpdate`) so a test can deterministically block on the actual
365+
/// debounce work unit instead of polling `MPNowPlayingInfoCenter` against a
366+
/// fixed wall-clock deadline (`awaitCondition(timeout:)`), which starves
367+
/// under CI sim-clone oversubscription. Production behavior is unchanged —
368+
/// nothing calls this outside tests. No-op when no debounced update is
369+
/// pending (the immediate-apply path leaves `pendingUpdate` nil).
370+
func _awaitPendingUpdateForTesting() async {
371+
await pendingUpdate?.value
372+
}
362373
}
363374

364375
/// Nonisolated `Sendable` holder for the foreground-notification observer

Palace/CatalogUI/ViewModels/CatalogViewModel.swift

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,24 @@ final class CatalogViewModel: ObservableObject {
122122
func appendPrefetchTaskForTesting(_ task: Task<Void, Never>) { prefetchTasks.append(task) }
123123
func cancelPrefetchForTesting() { cancelPrefetch() }
124124

125+
/// Test seam — deterministically join the in-flight load. `load()` /
126+
/// `forceRefresh()` / `reload()` spawn `currentLoadTask` (repo fetch →
127+
/// off-actor `mapFeed` → image-cache warming) and return WITHOUT awaiting it,
128+
/// so the `.loaded`/`.error`/`.offline` transition lands asynchronously. A
129+
/// test that asserts the terminal state must otherwise wait on the `$state`
130+
/// publisher against a fixed wall-clock deadline (`fulfillment(timeout:)`),
131+
/// which STARVES under CI sim-clone oversubscription — the `.userInitiated`
132+
/// map hop and cache warming get deferred past the deadline even though the
133+
/// code is correct. Awaiting the retained Task handle blocks EXACTLY until the
134+
/// load finishes, removing the pool/wall-clock dependence entirely.
135+
///
136+
/// Changes NO production behavior: this only reads a handle the view model
137+
/// already retains; `load()` still spawns the task and returns immediately.
138+
/// Returns at once if no load is in flight.
139+
func _awaitLoadForTesting() async {
140+
await currentLoadTask?.value
141+
}
142+
125143
// MARK: - Connectivity
126144

127145
/// Auto-reload the catalog when connectivity returns while we are showing the

Palace/MyBooks/BorrowErrorPresenter.swift

Lines changed: 63 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,32 @@ final class BorrowErrorPresenter: @unchecked Sendable {
122122
/// path. Called from MBDC's borrow flow on every problem-document
123123
/// failure.
124124
func process(error: [String: Any]?, for book: TPPBook) {
125+
// `book` (non-Sendable `TPPBook`) and `error` (`Any`-valued dict) ride
126+
// whole into the `@MainActor @Sendable` Task via read-only carrier
127+
// boxes; the boxed values are only touched on the main actor inside
128+
// `processAsync`.
129+
let bookBox = BorrowBookBox(book)
130+
let errorBox = BorrowErrorDictBox(error)
131+
Task { @MainActor [weak self] in
132+
await self?.processAsync(error: errorBox.error, for: bookBox.book)
133+
}
134+
}
135+
136+
/// Behavior-identical `async` sibling of `process`. Fire-and-forget
137+
/// `process` is `Task { await processAsync(...) }`; callers already inside
138+
/// an `async @MainActor` context (and tests) can `await` it directly to
139+
/// JOIN every branch's side effects — the alert publish, the re-auth
140+
/// dispatch, and the post-reauth `startDownload` retry — instead of
141+
/// polling a wall-clock deadline for a fire-and-forget hop to settle.
142+
///
143+
/// Structurally identical to the previous `process` body: the branch-1/2/4
144+
/// publishes happen on the main actor (previously via `runOnMainAsync`,
145+
/// now directly — we are already on `@MainActor`, same thread, same
146+
/// order), and the invalid-credentials branch keeps the same guard order,
147+
/// the same one-shot latch, the same shared-flag set, and the same
148+
/// fire-and-forget 2s `isRequestingCredentials` reset Task.
149+
@MainActor
150+
func processAsync(error: [String: Any]?, for book: TPPBook) async {
125151
guard let errorType = error?["type"] as? String else {
126152
showGenericBorrowFailedAlert(for: book)
127153
return
@@ -132,49 +158,30 @@ final class BorrowErrorPresenter: @unchecked Sendable {
132158
switch errorType {
133159
case TPPProblemDocument.TypeLoanAlreadyExists:
134160
let alertMessage = DisplayStrings.loanAlreadyExistsAlertMessage
135-
// Snapshot the Sendable identifier so the non-Sendable `book` does
136-
// not cross into the `@MainActor @Sendable` closure.
137-
let bookId = book.identifier
138-
runOnMainAsync { [weak self] in
139-
self?.progressReporter.publishAndAnnounceError(
140-
DownloadErrorInfo(bookId: bookId, title: alertTitle, message: alertMessage, kind: .borrow)
141-
)
142-
}
161+
progressReporter.publishAndAnnounceError(
162+
DownloadErrorInfo(bookId: book.identifier, title: alertTitle, message: alertMessage, kind: .borrow)
163+
)
143164

144165
case TPPProblemDocument.TypeInvalidCredentials:
145-
// `book` (non-Sendable `TPPBook`) and `error` (`[String: Any]?`,
146-
// `Any` is non-Sendable) both need to ride whole into the
147-
// `@MainActor @Sendable` re-auth Task. Thread them through
148-
// read-only carrier boxes; the boxed values are only touched on
149-
// the main actor inside the Task.
150-
let bookBox = BorrowBookBox(book)
151-
let errorBox = BorrowErrorDictBox(error)
152-
Task { @MainActor [weak self] in
153-
guard let self else { return }
154-
let book = bookBox.book
155-
let error = errorBox.error
156-
157-
guard !self.hasAttemptedAuthentication else {
158-
self.showAlert(for: book, with: error, alertTitle: alertTitle)
159-
return
160-
}
161-
162-
guard !self.credentialRequestState.isRequestingCredentials else {
163-
NSLog("Already requesting credentials, skipping re-authentication for: \(book.title)")
164-
return
165-
}
166+
guard !self.hasAttemptedAuthentication else {
167+
self.showAlert(for: book, with: error, alertTitle: alertTitle)
168+
return
169+
}
166170

167-
self.hasAttemptedAuthentication = true
168-
self.credentialRequestState.isRequestingCredentials = true
171+
guard !self.credentialRequestState.isRequestingCredentials else {
172+
NSLog("Already requesting credentials, skipping re-authentication for: \(book.title)")
173+
return
174+
}
169175

170-
Task { @MainActor [weak self] in
171-
try? await Task.sleep(nanoseconds: 2_000_000_000)
172-
self?.credentialRequestState.isRequestingCredentials = false
173-
}
176+
self.hasAttemptedAuthentication = true
177+
self.credentialRequestState.isRequestingCredentials = true
174178

175-
await self.handleInvalidCredentials(for: book)
179+
Task { @MainActor [weak self] in
180+
try? await Task.sleep(nanoseconds: 2_000_000_000)
181+
self?.credentialRequestState.isRequestingCredentials = false
176182
}
177-
return
183+
184+
await self.handleInvalidCredentials(for: book)
178185

179186
default:
180187
showAlert(for: book, with: error, alertTitle: alertTitle)
@@ -184,21 +191,29 @@ final class BorrowErrorPresenter: @unchecked Sendable {
184191
// MARK: - Invalid-credentials → reauth + retry
185192

186193
@MainActor
187-
private func handleInvalidCredentials(for book: TPPBook) {
194+
private func handleInvalidCredentials(for book: TPPBook) async {
188195
let userAccount = userAccountProvider()
189-
reauthenticator.authenticateIfNeeded(userAccount, usingExistingCredentials: false) { [weak self] in
190-
guard let self = self else { return }
191196

192-
Task { @MainActor [weak self] in
193-
self?.credentialRequestState.isRequestingCredentials = false
194-
195-
if self?.userAccountProvider().hasCredentials() == true {
196-
self?.delegate?.startDownload(for: book, withRequest: nil)
197-
} else {
198-
NSLog("Authentication completed but no credentials present, user may have cancelled")
199-
}
197+
// Bridge the completion-handler reauthenticator API to `async` so the
198+
// caller (`processAsync`) can JOIN the retry. The post-reauth body runs
199+
// on the main actor exactly as before — previously it hopped through
200+
// `Task { @MainActor }` from the (possibly-nonisolated) completion; now
201+
// we resume back onto the main actor via the continuation and run it
202+
// inline. Same thread, same order, same observable effects
203+
// (`isRequestingCredentials` reset → conditional `startDownload`).
204+
await withCheckedContinuation { (continuation: CheckedContinuation<Void, Never>) in
205+
reauthenticator.authenticateIfNeeded(userAccount, usingExistingCredentials: false) {
206+
continuation.resume()
200207
}
201208
}
209+
210+
credentialRequestState.isRequestingCredentials = false
211+
212+
if userAccountProvider().hasCredentials() {
213+
delegate?.startDownload(for: book, withRequest: nil)
214+
} else {
215+
NSLog("Authentication completed but no credentials present, user may have cancelled")
216+
}
202217
}
203218

204219
// MARK: - Generic + problem-doc alerts

Palace/MyBooks/DownloadCancellationHandler.swift

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,16 @@ final class DownloadCancellationHandler: @unchecked Sendable {
5555

5656
weak var delegate: DownloadCancellationHandlerDelegate?
5757

58+
/// Handle to the most recently spawned cancel-teardown `Task`. The cancel
59+
/// paths do their coordinator/map teardown inside a fire-and-forget
60+
/// `Task { }` (spawned either from the no-task branch or from the
61+
/// URLSession `cancel` completion). Retaining the handle lets callers —
62+
/// and tests — `await lastCancelTeardownTask?.value` to join that teardown
63+
/// deterministically instead of polling for the resulting state. Behavior
64+
/// is unchanged: the same Task is created and runs exactly as before; only
65+
/// a reference to it is now kept.
66+
private(set) var lastCancelTeardownTask: Task<Void, Never>?
67+
5868
private let stateManager: DownloadStateManager
5969
private let bookRegistry: TPPBookRegistryProvider
6070
#if FEATURE_DRM_CONNECTOR
@@ -94,7 +104,7 @@ final class DownloadCancellationHandler: @unchecked Sendable {
94104
bookRegistry.setState(.downloadNeeded, for: identifier)
95105
delegate?.broadcastUpdate()
96106

97-
Task { [weak self] in
107+
lastCancelTeardownTask = Task { [weak self] in
98108
guard let self else { return }
99109
await self.stateManager.downloadCoordinator.removeCachedDownloadInfo(for: identifier)
100110
await self.stateManager.downloadCoordinator.registerCompletion(identifier: identifier)
@@ -128,7 +138,7 @@ final class DownloadCancellationHandler: @unchecked Sendable {
128138
info.downloadTask.cancel { [weak self] _ in
129139
guard let self else { return }
130140

131-
Task { [weak self] in
141+
self.lastCancelTeardownTask = Task { [weak self] in
132142
guard let self else { return }
133143
// Clear both maps so retry isn't blocked by stale entries.
134144
_ = await self.stateManager.bookIdentifierToDownloadInfo.remove(identifier)

Palace/MyBooks/DownloadQueueOrchestrator.swift

Lines changed: 20 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -91,15 +91,26 @@ final class DownloadQueueOrchestrator: @unchecked Sendable {
9191
bookRegistry.setState(.downloading, for: book.identifier)
9292

9393
Task { [weak self] in
94-
guard let self else { return }
95-
await self.downloadCoordinator.enqueuePending(book)
96-
let queueSize = await self.downloadCoordinator.queueCount
97-
Log.debug(#file, "📋 Enqueued '\(book.title)' for download, queue size: \(queueSize)")
98-
99-
// Notify UI to refresh
100-
runOnMainAsync {
101-
self.notificationCenter.post(name: .TPPMyBooksDownloadCenterDidChange, object: nil)
102-
}
94+
await self?.enqueuePendingAsync(book)
95+
}
96+
}
97+
98+
/// Async body of `enqueuePending`: the actor-hopping portion (coordinator
99+
/// enqueue + DidChange broadcast) that `enqueuePending` fires as a
100+
/// detached `Task`. Exposed so callers already inside an `async` context
101+
/// — and tests — can `await` the enqueue to completion deterministically
102+
/// instead of polling for the resulting queue/notification state. Mirrors
103+
/// the `schedulePendingStartsIfPossible()` → `schedulePendingStartsAsync()`
104+
/// split. The synchronous `.downloading` state broadcast stays in
105+
/// `enqueuePending` (it runs before the Task hop, same as before).
106+
func enqueuePendingAsync(_ book: TPPBook) async {
107+
await self.downloadCoordinator.enqueuePending(book)
108+
let queueSize = await self.downloadCoordinator.queueCount
109+
Log.debug(#file, "📋 Enqueued '\(book.title)' for download, queue size: \(queueSize)")
110+
111+
// Notify UI to refresh
112+
runOnMainAsync {
113+
self.notificationCenter.post(name: .TPPMyBooksDownloadCenterDidChange, object: nil)
103114
}
104115
}
105116

Palace/MyBooks/DownloadStartCoordinator.swift

Lines changed: 49 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -71,9 +71,15 @@ protocol DownloadStartCoordinatorDelegate: AnyObject {
7171
/// `DownloadAuthRetryHandler`) that captures `[weak delegate]`/`[weak self]`
7272
/// and mutates non-Sendable state.
7373
/// INVARIANT — the boxed closure is invoked at most once, inside the single
74-
/// `startBorrow` `Task` (both the success and `catch` terminal paths), never
75-
/// concurrently; its own thread-affinity is the caller's contract (unchanged).
76-
private final class BorrowCompletionBox: @unchecked Sendable {
74+
/// `startBorrow` `Task` (both the success and `catch` terminal paths of
75+
/// `startBorrowAsync`), never concurrently; its own thread-affinity is the
76+
/// caller's contract (unchanged).
77+
///
78+
/// `internal` (not `private`) so the joinable `startBorrowAsync` seam — which
79+
/// takes the box rather than the raw closure to preserve the single boxing at
80+
/// the `Task` boundary — is `await`-able from the test target under
81+
/// `@testable import`. No production behavior depends on the access level.
82+
final class BorrowCompletionBox: @unchecked Sendable {
7783
let call: (() -> Void)?
7884
init(_ call: (() -> Void)?) { self.call = call }
7985
}
@@ -193,27 +199,50 @@ final class DownloadStartCoordinator: @unchecked Sendable {
193199
// `@Sendable`-ing the param, which would ripple to the caller closures.
194200
let borrowCompletionBox = BorrowCompletionBox(borrowCompletion)
195201
Task { [weak self] in
196-
guard let self else { return }
197-
do {
198-
_ = try await self.delegate?.borrowAsync(book, attemptDownload: shouldAttemptDownload)
199-
200-
let newState = self.bookRegistry.state(for: book.identifier)
201-
if newState == .holding {
202-
await self.stateManager.downloadCoordinator.registerCompletion(identifier: book.identifier)
203-
let remainingCount = await self.stateManager.downloadCoordinator.activeCount
204-
Log.info(#file, "📊 Borrow resulted in hold for '\(book.title)', released slot, remaining active: \(remainingCount)")
205-
self.delegate?.schedulePendingStartsIfPossible()
206-
}
207-
208-
borrowCompletionBox.call?()
209-
} catch {
210-
Log.error(#file, "Borrow failed: \(error.localizedDescription)")
202+
await self?.startBorrowAsync(
203+
for: book,
204+
attemptDownload: shouldAttemptDownload,
205+
borrowCompletionBox: borrowCompletionBox
206+
)
207+
}
208+
}
209+
210+
/// Behavior-identical `async` body of `startBorrow`. The fire-and-forget
211+
/// entry point above is `Task { await startBorrowAsync(...) }`; callers
212+
/// already inside an `async` context (and tests) can `await` it directly
213+
/// to JOIN the slot-release + reschedule + completion side effects instead
214+
/// of polling a wall-clock deadline for the detached Task to settle (which
215+
/// starves under CI oversubscription and blows the executionTimeAllowance).
216+
///
217+
/// Takes the `BorrowCompletionBox` (not the raw closure) so the sole
218+
/// caller keeps its single boxing at the `Task` boundary; the ordered
219+
/// side effects — borrowAsync → post-state read → registerCompletion →
220+
/// activeCount → schedulePendingStartsIfPossible → completion — are
221+
/// verbatim what the previous inline Task ran.
222+
func startBorrowAsync(
223+
for book: TPPBook,
224+
attemptDownload shouldAttemptDownload: Bool,
225+
borrowCompletionBox: BorrowCompletionBox
226+
) async {
227+
do {
228+
_ = try await self.delegate?.borrowAsync(book, attemptDownload: shouldAttemptDownload)
229+
230+
let newState = self.bookRegistry.state(for: book.identifier)
231+
if newState == .holding {
211232
await self.stateManager.downloadCoordinator.registerCompletion(identifier: book.identifier)
212233
let remainingCount = await self.stateManager.downloadCoordinator.activeCount
213-
Log.info(#file, "📊 Borrow failed for '\(book.title)', released slot, remaining active: \(remainingCount)")
234+
Log.info(#file, "📊 Borrow resulted in hold for '\(book.title)', released slot, remaining active: \(remainingCount)")
214235
self.delegate?.schedulePendingStartsIfPossible()
215-
borrowCompletionBox.call?()
216236
}
237+
238+
borrowCompletionBox.call?()
239+
} catch {
240+
Log.error(#file, "Borrow failed: \(error.localizedDescription)")
241+
await self.stateManager.downloadCoordinator.registerCompletion(identifier: book.identifier)
242+
let remainingCount = await self.stateManager.downloadCoordinator.activeCount
243+
Log.info(#file, "📊 Borrow failed for '\(book.title)', released slot, remaining active: \(remainingCount)")
244+
self.delegate?.schedulePendingStartsIfPossible()
245+
borrowCompletionBox.call?()
217246
}
218247
}
219248

0 commit comments

Comments
 (0)