fix: recover interrupted paykit sessions - #796
ben-kaufman wants to merge 6 commits into
Conversation
|
jvsena42
left a comment
There was a problem hiding this comment.
One medium finding, gated behind the Paykit UI flag, so it affects opted-in users on released builds and does not block. Two low ones as well. All inline. The repeated-toast finding also applies to synonymdev/bitkit-android#1339.
Checked and clean:
- Billing reminders are notifications only; the tap handler stores a target and never pays. Re-adding with the same identifier replaces the old one, stale identifiers are removed, and the UTC calendar triggers stay in the future.
- Presentation retry: the
ContinuousClockdeadlines matchTask.sleep(for:). Deadlines are pruned on approve, reject and remove, and the presented-request IDs are unchanged. restorePersistedSessionholdsoperationLockacross the credential read, import and sign-in. A restore queued behind sign-out or forget finds no credentials, and every identity mutation bumpssessionRevision, so late UI results are discarded.- No nested
withLockin the restore path. The MainActor hop forclearCachedIdentityMetadatais async, so no deadlock. - Reset wallet: the count blocks new restores, forget runs under the lock, then the blob and keychain are wiped. The static counters stay balanced across instances.
- A Debug build over a TestFlight build (client-id mismatch) still falls back to local-seed re-sign-in. A Ring-only identity is kept as
.restorationFailedrather than deleted. restoreSessionBackupStateruns beforerestoreContactProfileOverrides, so an identity-change clear cannot wipe restored overrides.
| guard let attemptID = activeAuthAttemptID else { | ||
| throw CancellationError() | ||
| } | ||
| Self.beginSessionMutation() |
There was a problem hiding this comment.
A cancelled Ring authorization stops recovery and makes Retry a silent no-op until the relay poll ends.
beginSessionMutation() runs before completeAuth(), the relay long-poll. On Cancel (in Bitkit, or pubky-auth/cancel from Ring), cancelAuthentication() clears activeAuthAttemptID and PaykitSdkService.cancelAuth() clears activeAuthRequest. Neither aborts request.complete(...). The .task is cancelled, but the uniffi async call never forwards cancellation to Rust. pubky's HttpRelayInboxChannel::poll loops on long-poll timeouts until it gets a token or hits 3 consecutive hard failures. Until then sessionMutationCount > 0, and:
restoreSessionIfNeeded()returns at :213 (foreground :948, reconnect :1376)initialize()returns at :193/:196, so the Retry button inMainNavViewdoes nothing and the error stays.
Steps: restore fails (offline or wrong clock) → Profile → Ring → come back without approving → Cancel → fix the network or clock → foreground. Nothing recovers for the rest of the process. The auto-recovery this PR adds is exactly what stops working. Manual re-auth still works.
Both the counter and these guards are new here. Taking the mutation before the poll adds no protection: during an active attempt, activeAuthAttemptID already blocks both entry points, and startAuthentication() bumps sessionRevision.
Fix: begin the mutation after completeAuth() returns and the activeAuthAttemptID == attemptID check passes, as Android does (it takes initializeMutex after waitForAuthApproval). A mutationBegun flag with the existing defer keeps the catch paths balanced.
There was a problem hiding this comment.
Moved the session mutation guard to the SDK activation boundary. A canceled relay poll no longer blocks saved-session recovery, while activation and cleanup of any late session stay protected until the matching session is discarded.
There was a problem hiding this comment.
Fixed for Cancel. Low severity: Back has the same shape. PubkyRingAuthView and PubkyChoiceView have no onDisappear, so Back only cancels the .task. The uniffi poll keeps running, and activeAuthAttemptID and .authenticating stay set. So initialize() returns at :205 (Retry does nothing) and restoreSessionIfNeeded returns at :225 until the poll ends. A new Ring attempt is the only escape. .onDisappear { if isWaitingForRing { Task { await pubkyProfile.cancelAuthentication() } } } in both views routes it through the path you just fixed, and makes the service guard reject a late approval.
There was a problem hiding this comment.
Verified in 003030e. Back now goes through cancelAuthentication(). The authState == .authenticating guard keeps it from firing after a completed authorization.
| } | ||
| if wallet.walletExists == true { | ||
| Task { | ||
| async let sessionRecovery: Void = network.isConnected ? pubkyProfile.restoreSessionIfNeeded() : () |
There was a problem hiding this comment.
Low severity. While restoration keeps failing, "Profile Disconnected" now fires on every foreground and every reconnect. The saved session is now kept on failure, so hasStoredIdentity() stays true. Each retry sets sessionRestorationFailed from false back to true (:228 → :262), and the onChange at :470 toasts each time. Steps: a Ring identity whose grant was revoked, or a wrong clock, where "session has expired" is also misleading. On reconnect it stacks with "Connection restored". Before this PR it fired once per launch.
Fix: announce only from startup or manual retries, e.g. an announceFailure: Bool that is false from restoreSessionIfNeeded.
There was a problem hiding this comment.
Automatic foreground and reconnect retries no longer set sessionRestorationFailed, so they do not repeat the Profile Disconnected notification. Startup and manual retries still report restoration failures.
| initializeSession: @escaping @Sendable () async throws -> SessionInitializationResult | ||
| ) async { | ||
| let revision = Self.sessionRevision | ||
| isInitialized = false |
There was a problem hiding this comment.
Low severity. Automatic retries now go through here, so each foreground or reconnect after a failed restore sets isInitialized = false for the length of the import and re-sign-in round trips. During that time, MainNavView (:537, :617) swaps PubkyChoiceView for the loading view and drops its @State. Header (:126-127) swallows the profile tap, and contact deep links are held. On a flaky link this repeats on every event.
Fix: skip the reset for runs started by restoreSessionIfNeeded, or use a separate recovering flag.
There was a problem hiding this comment.
Automatic recovery now keeps the current initialization and error state while the retry runs, so the existing UI stays usable. A successful retry clears any stale startup error.
jvsena42
left a comment
There was a problem hiding this comment.
d4dd1ce resolves all three threads.
- Mutation guard:
beginSessionMutation()now runs insidewillActivate, afterrequest.completereturns and the service request guard passes. The count is 0 while an orphaned poll runs, so Cancel → fix the clock → foreground or Retry now recovers. Every begin has a matching end: the singledeferchecksmutationBegun, and all throw paths after the begin unwind through it. A late approval after Cancel throws at the service guard before activation and is never persisted. A restore and a Ring completion converge on the same identity in every ordering I traced:sessionRevision,operationLock, andpreviousPublicKeyread under the lock. - Automatic recovery no longer sets
sessionRestorationFailedor resetsisInitialized. Startup and Retry still report once. - The new tests cover the poll-in-flight, activation-in-flight and discard orderings against the real
PaykitSdkService.completeAuth.
One low-severity refinement on the Back path is in the :768 thread.
Pre-existing, not blocking: a Ring approval that arrives after Cancel leaves a live homeserver grant that nothing revokes. The service guard drops the result without signing out, and PubkySessionAccess exposes no revoke. Android activates and then revokes in that case (revokeCompletedAuthSessionIfNeeded).
A failed Paykit session restore after connection loss or a device clock change could make an existing profile appear missing and discard contacts. This PR preserves saved identity data, retries recovery when connectivity returns or the app resumes, and corrects retry and billing-reminder timing.
Related issue: Android #1344.
Counterpart: Android PR.
Related: #786 also changes the persisted-identity lookup as part of backup protection; that overlapping hunk needs reconciling when both PRs merge.
Description
Out of Scope
Design
N/A — no UI changes.
Preview
No new iOS outage or clock-change recording. Simulator regression tests cover recovery and identity replacement; Android offline-start/reconnect recordings verify the reported flow on that platform.
QA Notes
Journeys
N/A — not drivable; see Manual Tests.
Manual Tests
Live grant-session clock-change E2E has not been rerun. iOS recovery was verified with injected failures on the test simulator; a live iOS outage/reconnect run remains pending. The full procedure is in paykit-clock-changes.md.
Automated Checks
PaykitSdkClientConfigTests.swift— unreadable identity state preserves the saved SDK state and session; activation tests cover same/different owners, normalized keys, legacy backup caches, and unavailable profile data after Ring auth or startup restoration.PubkyProfileManagerTests.swift— failed restoration preserves cached profile data, retries after the error is consumed, waits for startup, coalesces connectivity events, skips missing/unreadable/authorizing identities, and ignores late results after Ring or backup replacement.PaykitSdkClientConfigTests.swift— serialized import/fallback recovery cannot restore credentials after session teardown.PubkyIdentityRepublishTests.swift— clock rollback retries publication and then resumes throttling.PaykitPaymentRequestServiceTests.swift— timezone/DST changes preserve UTC billing reminders, and wall-clock jumps do not alter elapsed presentation backoff.git diff --checkpassed. Test build usedDEBUG E2E_BUILD UNIT_TESTING,E2E_BACKEND=networkandE2E_NETWORK=regtest. No host or funded test-device clock was changed.