Skip to content

fix: recover interrupted paykit sessions - #796

Open
ben-kaufman wants to merge 6 commits into
masterfrom
fix/paykit-clock-recovery
Open

ben-kaufman wants to merge 6 commits into
masterfrom
fix/paykit-clock-recovery

Conversation

@ben-kaufman

@ben-kaufman ben-kaufman commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

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

  • Automatically retries saved-session restoration on reconnect and foreground from any screen, coalescing concurrent attempts. Recovery cannot overwrite an active authorization or reinstate a session after teardown.
  • Reads the saved identity without restoring its old grant, and preserves unreadable SDK state so a failed reconnect cannot treat existing contacts as a fresh identity.
  • Holds credential reads, grant import and fallback sign-in under the SDK operation lock. Discards late UI restoration results across identity mutations; wallet sync and currency refresh run independently of profile recovery.
  • Retains the saved session and cached profile after restoration fails so correcting the clock can recover them.
  • Clears cached profile/contact overrides on a confirmed identity change while preserving same-identity recovery and legacy backup caches. The profile button opens reauthorization when restoration has failed.
  • Resumes identity publication after clock rollback and uses elapsed time for incoming-payment presentation retries.
  • Schedules billing reminders at explicit UTC dates, replacing existing relative triggers, so timezone and clock changes do not shift the agreed boundary.

Out of Scope

  • Pubky grant and payment-expiry validation: real expiration and authorization checks remain enforced; no backend clock-skew bypass.
  • Wallet backup/restore protection: tracked separately in fix: preserve paykit payment state #786. The reported missing Bitcoin wallet remains unconfirmed and is not claimed fixed here.

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.

  • regression: with a saved profile/contact and valid local or Ring session, cold-start offline, then reconnect or foreground from the contact/profile screen → cached data stays present and the same identity recovers without signing out or reauthorization; an expired/revoked grant still needs authorization — network fault injection not in Capabilities.
  • regression: start Ring authorization, sign out or reset while recovery is pending → automatic recovery cannot replace the authorization or resurrect deleted credentials — network fault injection not in Capabilities.
  • regression: use fresh test wallets with a saved contact, covering local-secret and Ring sessions → move the device clock a month ahead, attempt recovery, correct it and retry; repeat with a backward change → saved contacts/profile remain available and private payments recover, with reauthorization if the grant expired — isolated device-clock control not in Capabilities.
  • regression: after failed restoration, open the profile button and reauthorize with Ring without signing out → the same identity keeps its cache; a different identity cannot display the old name/avatar or contact labels even when its profile is unavailable — device-clock fault injection not in Capabilities.
  • regression: change only the timezone between America/New_York, Pacific/Kiritimati and Pacific/Pago_Pago, including a daylight-saving boundary → authentication and UTC billing boundaries stay unchanged — OS timezone and date control not in Capabilities.
  • regression: schedule a subscription reminder and defer an unavailable payment request, then change the clock → no payment-due reminder before its billing boundary, and retries recover after time is corrected — device-clock fault injection not in Capabilities.

Automated Checks

  • updated 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.
  • updated 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.
  • updated PaykitSdkClientConfigTests.swift — serialized import/fallback recovery cannot restore credentials after session teardown.
  • updated PubkyIdentityRepublishTests.swift — clock rollback retries publication and then resumes throttling.
  • updated PaykitPaymentRequestServiceTests.swift — timezone/DST changes preserve UTC billing reminders, and wall-clock jumps do not alter elapsed presentation backoff.
  • Ran all four affected suites through XcodeBuildMCP: 196 tests passed. The final lifecycle/test-fixture correction was rebuilt and passed all 75 profile/SDK tests. SwiftFormat and git diff --check passed. Test build used DEBUG E2E_BUILD UNIT_TESTING, E2E_BACKEND=network and E2E_NETWORK=regtest. No host or funded test-device clock was changed.

@greptile-apps

greptile-apps Bot commented Sep 24, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

The PR appears safe to merge based on the reviewed changes.

Summary

The PR preserves Paykit state after failed clock-skew recovery and changes publication retries, payment-request retries, and billing reminders to handle clock changes.

  • Keeps the saved session and cached profile available for a later restoration attempt.
  • Uses elapsed time for presentation backoff and UTC calendar dates for billing reminders.
  • Adds focused tests and a manual clock-change journey.

Reviews (1) · Last reviewed commit: "fix: recover paykit after clock changes"

@ben-kaufman ben-kaufman changed the title fix: recover paykit after clock changes fix: recover interrupted paykit sessions Sep 25, 2026
@jvsena42
jvsena42 self-requested a review September 25, 2026 16:22

@jvsena42 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ContinuousClock deadlines match Task.sleep(for:). Deadlines are pruned on approve, reject and remove, and the presented-request IDs are unchanged.
  • restorePersistedSession holds operationLock across the credential read, import and sign-in. A restore queued behind sign-out or forget finds no credentials, and every identity mutation bumps sessionRevision, so late UI results are discarded.
  • No nested withLock in the restore path. The MainActor hop for clearCachedIdentityMetadata is 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 .restorationFailed rather than deleted.
  • restoreSessionBackupState runs before restoreContactProfileOverrides, so an identity-change clear cannot wipe restored overrides.

guard let attemptID = activeAuthAttemptID else {
throw CancellationError()
}
Self.beginSessionMutation()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 in MainNavView does 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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified in 003030e. Back now goes through cancelAuthentication(). The authState == .authenticating guard keeps it from firing after a completed authorization.

Comment thread Bitkit/AppScene.swift
}
if wallet.walletExists == true {
Task {
async let sessionRecovery: Void = network.isConnected ? pubkyProfile.restoreSessionIfNeeded() : ()

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jvsena42 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

d4dd1ce resolves all three threads.

  • Mutation guard: beginSessionMutation() now runs inside willActivate, after request.complete returns 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 single defer checks mutationBegun, 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, and previousPublicKey read under the lock.
  • Automatic recovery no longer sets sessionRestorationFailed or resets isInitialized. 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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants