Keep every Gmail connection fresh - #137

apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift (1)

3439-3450: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Do not retry canceled loads.

The catch at Line 3441 also handles cancellation, so a canceled AccountView task can issue a second refreshConnections() request and publish a spurious cancellation error. Since completeLoadingConnections() invokes a helper that swallows cancellation, check cancellation before retrying and after completion without updating errorMessage.

Proposed fix

     do {
       try await refreshConnections()
       await completeLoadingConnections()
+      try Task.checkCancellation()
       return true
+    } catch is CancellationError {
+      return false
     } catch {
+      guard !Task.isCancelled else { return false }
       let originalError = error
       do {
+        try Task.checkCancellation()
         try await refreshConnections()
         await completeLoadingConnections()
+        try Task.checkCancellation()
         return true
+      } catch is CancellationError {
+        return false
       } catch {
+        guard !Task.isCancelled else { return false }
         errorMessage = originalError.localizedDescription
         return false
       }
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift` around
lines 3439 - 3450, Update the error handling around refreshConnections in
AccountView so cancellation is detected before issuing the retry and again after
completeLoadingConnections, returning without setting errorMessage in either
case. Preserve the existing retry and original-error reporting behavior for
non-cancellation failures.

apps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swift (2)

1174-1191: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Seed persisted state before constructing the ViewModel.

Line 1175 creates the ViewModel before Line 1177 saves the stale timestamp, so this does not cover removal of state restored at startup. Seed the store first, then construct the ViewModel and apply the first authoritative connection update.

As per coding guidelines, “Define verifiable success criteria and validate each implementation step; add regression tests for bug fixes and tests for validation behavior.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swift`
around lines 1174 - 1191, Update
testMailboxFreshnessClearsLastSuccessRemovedBeforeViewModelStarts so the
successStore.save call seeds the persisted timestamp before
makeMailboxFreshnessFixture constructs the ViewModel. Then obtain the fixture,
apply the initial authoritative update via updateConnections, and retain the
assertion that the removed connection’s state is cleared.

Source: Coding guidelines

---

1322-1335: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Make the gate-contention check deterministic. pollAttempted only proves the task started, not that it reached MailboxConnectionSyncGate.acquire. Replace the Task.yield() loop with a test hook that signals when the poll is actually queued, then assert pollAcquired is still false before release.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swift`
around lines 1322 - 1335, Replace the fixed Task.yield loop in the
gate-contention test with a synchronization hook that fulfills when the poll
reaches or queues at MailboxConnectionSyncGate.acquire. Await that hook before
checking probe.pollAcquired, keeping the assertion false while the push still
holds the gate, then release the gate afterward.

apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift (2)

1981-1989: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Concurrent pin toggles on different messages can misattribute errors.

togglePin (PinViewModel, lines 2374-2396) permits concurrent toggles across different messageIds — only same-message reentry is guarded via updatingMessageIds. The caller at lines 1982-1989 awaits its own toggle, then reads the shared pinViewModel.errorMessage to decide whether to show readerErrorMessage. If message A's toggle succeeds while a concurrently-started toggle for message B fails, A's completion sees B's leftover errorMessage and incorrectly shows a "Message action failed" alert for a successful action.

Have togglePin return the outcome directly instead of relying on shared mutable state read after the fact.

🐛 Proposed fix to thread the toggle result back to the caller

-  func togglePin(_ messageId: StableProviderMessageIdentity) async {
+  `@discardableResult`
+  func togglePin(_ messageId: StableProviderMessageIdentity) async -> String? {
     guard !updatingMessageIds.contains(messageId) else { return }
     ...
     do {
       try await service.setPinned(
         shouldPin,
         messageId: messageId,
         session: session
       )
       completedToggleGenerations[messageId, default: 0] += 1
+      return nil
     } catch is CancellationError {
       setPinnedLocally(wasPinned, messageId: messageId)
+      return nil
     } catch {
       setPinnedLocally(wasPinned, messageId: messageId)
       errorMessage = error.localizedDescription
+      return error.localizedDescription
     }
   }

   togglePin: {
     Task {
-      await togglePin(message.id)
-      if let errorMessage = pinViewModel.errorMessage {
+      if let errorMessage = await pinViewModel.togglePin(message.id) {
         readerErrorMessage = errorMessage
       }
     }
   }

Also applies to: 2062-2065, 2330-2417

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift` around
lines 1981 - 1989, Update PinViewModel.togglePin to return the individual toggle
outcome, and have each caller, including the togglePin closure near the reader
action and the other call sites, use that returned result to set
readerErrorMessage rather than reading shared pinViewModel.errorMessage after
awaiting. Preserve the existing same-message concurrency guard and shared error
state behavior where otherwise required, but ensure concurrent toggles cannot
attribute one message’s failure to another.

---

3267-3286: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Cancellation is swallowed instead of stopping the prefetch loop.

The catch-all at line 3281-3283 also catches CancellationError, so cancelling bodyPrefetchTask (e.g. a newer prefetch superseding it) doesn't stop iteration — the loop continues to the next connection. This is inconsistent with the catch is CancellationError { return ... } pattern used elsewhere in this file (e.g. lines 2961-2963, 2993-2995, 3062-3064).

🔧 Proposed fix to stop the loop on cancellation

     bodyPrefetchTask = Task { [weak self] in
       guard let self else { return }
       for connection in connections {
+        guard !Task.isCancelled else { return }
         do {
           try await bodyPrefetcher.prefetchMessageBodies(
             connection: connection,
             pinnedMessageIds: navigationSnapshot.pinnedMessageIds,
             referenceDate: Date(),
             session: self.session
           )
+        } catch is CancellationError {
+          return
         } catch {
           // Prefetch is best effort and must not block cached mailbox use.
         }
       }
     }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift` around
lines 3267 - 3286, Update the Task loop in startBodyPrefetch to handle
CancellationError separately and return immediately when cancellation occurs,
before the existing best-effort handling for other errors. Preserve the current
behavior of continuing to the next connection for non-cancellation failures.

apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift (4)

275-305: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Clear stale authorization-required status after reauthorization.

updateConnections() preserves the existing status for IDs that remain present. When a connection changes from .required to .authorized, status(for:) returns that stale .authorizationRequired phase until a later sync rewrites it. This is especially visible when the connection cannot synchronize immediately.

Reset the phase to .idle on a required-to-authorized transition while preserving the stored successful timestamp.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift` around
lines 275 - 305, The updateConnections method must reset a connection’s stale
authorizationRequired phase to idle when its authorizationState transitions from
required to authorized, while retaining its stored successful timestamp. Compare
the previous knownConnections state with the incoming connections and update the
corresponding statuses entry without affecting other phases or connections.

---

467-478: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Do not roll back newer freshness timestamps after backfill failure.

If a push or another sync records a newer success while this backfill is running, the cancellation/failure paths restore priorStatus and overwrite that newer lastSuccessfulSyncAt in memory. Because status(for:) prefers statuses over the store, the sidebar can continue showing stale freshness data.

Use the current status when completing the backfill and preserve the newest successful timestamp; only restore the prior phase when this backfill still owns the connection.

Also applies to: 604-610

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift` around
lines 467 - 478, Update the cancellation and failure handling around the
historical backfill flow and its corresponding logic near the second referenced
block to read the current connection status before completing. Preserve the
newest successful timestamp from any push or concurrent sync, and restore
priorStatus only when historicalBackfills[connection.id]?.id still matches
backfillId; otherwise leave the newer status unchanged.

---

393-413: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Guard sync result handling by syncId.

After updateConnections() or cancelAll() removes an in-flight entry, the canceled task can still resume here and write .idle or .failure. If a replacement sync starts first, the old task can overwrite its .syncing or successful state.

Require inFlightSyncs[connection.id]?.id == syncId plus current-session/connection checks before mutating status or persistence. Add a regression test for cancellation followed by a replacement sync.

Suggested direction

+let ownsSync = inFlightSyncs[connection.id]?.id == syncId
+guard ownsSync, isSessionCurrent(session), knownConnections[connection.id] != nil else {
+  throw CancellationError()
+}
 removeSync(connectionId: connection.id, syncId: syncId)

Apply the same ownership check in each catch branch before updating statuses.

As per coding guidelines, add regression tests for bug fixes.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift` around
lines 393 - 413, Guard both cancellation catch branches in the sync flow around
the visible error handling with an ownership check requiring
inFlightSyncs[connection.id]?.id == syncId and valid current-session/connection
state before mutating statuses or persistence; ensure stale tasks perform no
updates after removal or replacement by updateConnections() or cancelAll(). Add
a regression test covering cancellation followed by a replacement sync and
verifying the old task cannot overwrite the replacement state.

Source: Coding guidelines

---

354-356: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win

Defer backfill until after sync completes loadAfterConnectionChange(..., synchronizes: true) currently starts historical backfill before sync(), so the sync path cancels and may restart the same work. Pass startsBackfill: false here and let sync() schedule it afterward.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/unwired-mail/unwired-mail/Features/Account/AccountView.swift` around
lines 354 - 356, Update the loadAfterConnectionChange call in the
synchronizes=true path to pass startsBackfill: false, preventing historical
backfill from starting before sync. Keep sync() responsible for scheduling the
backfill after the in-flight sync completes.

apps/unwired-mail/unwired-mail/Services/Gmail/GmailPushRelayService.swift (1)

1360-1382: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Revalidate the route before recording sync success.

A session/connection can change after syncRecentInbox accepts shouldPersist but before it returns. The guard at Line 1382 prevents watermark advancement, but Lines 1360-1381 already persist and publish a stale successful sync. Guard currentWatchForRoute() before saving successfulSyncAt and add a session-change-after-sync regression test.

Proposed fix

+    guard currentWatchForRoute() != nil else { return false }
     let successfulSyncAt = Date()
     successStore.save(
       successfulSyncAt,

As per coding guidelines, “add regression tests for bug fixes and tests for validation behavior.”

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/unwired-mail/unwired-mail/Services/Gmail/GmailPushRelayService.swift`
around lines 1360 - 1382, The sync-success path currently records and publishes
success before validating that the route/session is still current. In the flow
around syncRecentInbox and currentWatchForRoute(), move the current-route guard
before successfulSyncAt creation, successStore.save, fallback scheduling, and
publishSyncStatus; return false when the route changed. Add a regression test
covering a session/connection change after sync acceptance but before
completion, asserting no stale watermark or success status is persisted.

Source: Coding guidelines

apps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swift (2)

1129-1132: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Assert polling coverage per connection.

A total count equal to fixture.connections.count still passes if one mailbox is synchronized twice and another is skipped. Assert the recorded IDs as a set and retain the count check for duplicate detection.

Proposed assertions

     let receivedDurations = await sleeper.receivedDurations()
-    let syncCallCount = await fixture.service.syncCallCount()
     XCTAssertEqual(receivedDurations, [.seconds(300), .seconds(300)])
-    XCTAssertEqual(syncCallCount, fixture.connections.count)
+    let syncedConnectionIds = await fixture.service.syncedConnectionIds()
+    XCTAssertEqual(
+      Set(syncedConnectionIds),
+      Set(fixture.connections.map(\.id))
+    )
+    XCTAssertEqual(syncedConnectionIds.count, fixture.connections.count)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swift`
around lines 1129 - 1132, Update the test assertions around
sleeper.receivedDurations, syncCallCount, and fixture.service to also retrieve
the recorded synchronization IDs and compare them as a set with the expected
connection IDs. Keep the existing syncCallCount equality assertion so duplicate
synchronizations remain detectable.

---

1373-1408: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Track the cancelled and next waiters separately.

Because both waiters write to the same pollAcquired boolean, the test can pass even if the cancelled waiter acquires after release. Use separate probes—or record acquisition labels—and assert the cancelled probe remains false while the next waiter becomes true.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@apps/unwired-mail/unwired-mailTests/GmailMessageMetadataServiceTests.swift`
around lines 1373 - 1408, Update
testMailboxConnectionSyncGateKeepsLockWhenQueuedTaskIsCancelled to track the
cancelled waiter and next waiter with separate acquisition probes or labels.
Assert the cancelled waiter never records acquisition after cancellation, while
the next waiter remains false before release and true after release.