Skip to content

fix: recover interrupted paykit sessions - #1339

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

ben-kaufman wants to merge 7 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.

Fixes #1344.

Counterpart: iOS PR.

Related: #1334 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.
  • Retains cached profile/contact overrides after restoration fails and limits failed Ring-auth cleanup to a newly installed session, preserving the previous or unreadable session.
  • Keeps Pubky signup disabled while an existing identity is saved, including when its credentials are temporarily unreadable.
  • Clears cached profile/contact overrides on a confirmed identity change while preserving same-identity recovery and legacy backup caches.
  • Resumes identity publication after a backward clock correction instead of waiting for an old future timestamp.
  • Rechecks the billing date when a subscription notification worker runs, retrying if the period is still in the future.

Out of Scope

  • Pubky grant and payment-expiry validation: real expiration and authorization checks remain enforced; no backend clock-skew bypass.
  • Notification delivery precision: WorkManager remains best effort and can delay reminders through OS scheduling or retry backoff.
  • Wallet backup/restore protection: tracked separately in fix: preserve paykit payment state #1334. The reported missing Bitcoin wallet remains unconfirmed and is not claimed fixed here.

Design

N/A — no UI changes.

Preview

Android offline-start/reconnect recordings were captured locally: the original build lost the profile name and did not recover; the fixed build retained the name and contact and restored the same identity about 1.7 seconds after reconnecting. Live clock-change recording remains pending.

QA Notes

Journeys

N/A — not drivable; see Manual Tests.

Manual Tests

Live grant-session clock-change E2E has not been rerun. Android offline cold-start/reconnect and contact preservation were verified on a disposable regtest emulator. 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 PaykitSdkServiceTest.kt — identity lookup failures stop activation without deleting saved state or credentials; activation tests cover same/different owners, normalized keys, legacy backup caches, and cache-reset failure.
  • updated PubkyAuthHandlerRegistrarTest.kt and AppViewModelSendFlowTest.kt — saved/unreadable identities do not advertise signup; reconnect and foreground trigger recovery.
  • updated PubkyRepoTest.kt — automatic retry after failure, unreadable credentials, queued wipe, active Ring authorization, and cancelled completion racing retry; restoration preserves profile data for retry; failed Ring auth cleans up only a session installed by that attempt, including unreadable ownership and cancellation coverage; identity switches discard stale in-memory profile/contact data and reset the contact-load marker.
  • updated PubkyIdentityRepublishTest.kt — clock rollback retries publication and then resumes throttling.
  • updated PaykitSubscriptionNotificationSchedulerTest.kt — a worker running before the billing boundary defers its reminder.
  • updated PaykitSubscriptionTest.kt — timezone and DST changes preserve UTC billing boundaries.
  • ran local Gradle compile, full unit tests and Detekt with a command-scoped init script excluding stale Maven Local artifacts — all 2,911 tests passed across 192 suites, with no failures or skips. Detekt has no findings in changed files and 15 existing findings in untouched files. Final cleanup also passed all 110 PubkyRepo tests, compile and APK build. No host or funded test-device clock was changed.

@github-actions

github-actions Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Regtest APK

Built from 0d32249 (run).

Download bitkit-dev-debug universal APK (expires in 30 days).

@greptile-apps

greptile-apps Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

RetriggerConfidence Score: 4/5

The PR should not merge until preserved Pubky cache data is isolated when recovery proceeds with a different identity.

Findings

  1. P1 Cached identity data crosses accounts ▶

Summary

The PR preserves Paykit session and cached Pubky data across failed recovery, retries identity publication after clock rollback, and prevents subscription reminders from posting before their billing boundary.

  • Adds recovery, timing, and timezone tests plus a manual clock-change journey.
  • The preserved Pubky cache needs an identity boundary before a different account can authenticate.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Identity A restoration fails] --> B[Global Pubky cache retained]
  B --> C[Identity B authenticates through Ring]
  C --> D[B contacts loaded]
  B --> D
  D --> E[A contact overrides may affect B]
Loading

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

}
is InitResult.RestorationFailed -> {
clearAuthenticatedState()
clearAuthenticatedState(clearCachedProfile = false)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Cached identity data crosses accounts If identity A’s saved session fails to restore and the user then authorizes identity B through Ring, this branch keeps A’s cached data. The cache is not separated by identity, and B’s authentication does not clear it. As a result, A’s saved contact overrides can change how B’s contacts appear, and A’s name or image can appear while B’s profile is unavailable. Keep the cache for a retry with the same identity, but clear or separate it when the identity changes.

Knowledge Base Used: Pubky identity and profile

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.

Fixed in 9890462. Activation now clears the cached profile and contact overrides when the saved SDK owner changes. Same-identity recovery keeps them. A missing SDK owner also preserves restored legacy backup overrides.

Ring completion clears stale in-memory profile/contacts and resets the contact-load marker before loading the new identity. Regression coverage includes same/different owners, normalized keys, legacy backups, and cache-reset failure. All 2,903 Android tests passed.

Applied the matching cache fix and corrected the profile recovery route on iOS #796 too.

@ben-kaufman ben-kaufman changed the title fix: recover paykit after clock changes fix: recover interrupted paykit sessions Sep 25, 2026
@ovitrif ovitrif removed this from the 2.6.0 milestone Sep 25, 2026
@jvsena42
jvsena42 self-requested a review September 25, 2026 16:20

@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. There is also a low one. Both are inline. The toast finding also applies to synonymdev/bitkit-ios#796 (AppScene toasts on every sessionRestorationFailed change).

Checked and clean:

  • Billing reminders only post a notification; the tap path never pays. A retry only happens when nothing was posted, so no duplicate reminders. A worker that returns retry is still an upcoming period, so synchronize() keeps its work name rather than cancelling it.
  • Sign-out and wipe take initializeMutex, so a queued retry then sees no identity. After a failed sign-out, _publicKey stays set and retry does nothing.
  • Ring cancel: before approval, cancelling ends the attempt without holding the lock. After approval, the new session is revoked and the keychain cleared, so retry finds nothing. During Authenticating, retry is guarded.
  • Recovery cannot overwrite a live authorization: _authState != Idle, and completeAuthentication holds the mutex from approval through activation.
  • No nested initializeMutex in deleteProfile, discardAbandonedSession, approveSignupAuth or restoreSessionBackupState.
  • No secrets logged; new logs use redacted().
  • The republish clock-rollback check matches iOS.
  • Signup alias stays disabled on an unreadable keychain (getOrDefault(true)).

}
is InitResult.RestorationFailed -> {
clearAuthenticatedState(clearCachedProfile = false)
_sessionRestorationFailed.update { true }

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 persistent restore failure now shows the "session expired" error toast on every resume and reconnect.

restoreSessionIfNeeded guards only on _publicKey/_authState. After a failed restore, clearAuthenticatedState resets the state to Idle, so every onAppResumed() (while CONNECTED) and every reconnect runs initializeSession() again. It clears the flag at :211 and sets it back to true here. The AppViewModel collector (:536-544) toasts profile__session_expired and clears the flag, with no dedupe.

Steps: Ring-authorized identity whose grant was revoked or expired in Ring. importSession fails with no stored secret key, so the result is RestorationFailed ("Skipped re-sign-in recovery, keeping saved session"). The PR body expects this state. From then on, every return to Bitkit shows an error toast, including returning from a permission or biometric dialog, and each one costs a homeserver round trip. The same happens for a local-secret identity while the homeserver is unreachable on a validated network. On master the flag was set once per process.

Airplane mode alone does not trigger it, because onAppResumed checks CONNECTED.

Fix: give initializeSession a notifyFailure: Boolean = true parameter and pass false from restoreSessionIfNeeded, so the startup attempt still reports and retries stay silent. A minimum interval between automatic retries is optional.

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.

Changed automatic retries to keep restoration failures silent. Startup still reports the expired session once, while resume and reconnect can retry without triggering the toast again.

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.

The flag-set sites are gated now, but the clear at :222 is not. At cold start, the first silent retry can wipe the startup report before it is shown.

MainActivity.onResume() → onAppResumed() runs right away, because isOnline starts as CONNECTED. restoreSessionIfNeeded() waits in awaitInitialization(). Startup initializeSession() hits RestorationFailed and sets the flag to true at :247. When the lock is released, the retry runs initializeSession(notifyFailure = false): ensureServiceInitialized() does nothing, and :222 sets the flag back to false before importSession fails silently. The collector runs on Main and StateFlow conflates, so if Main is still busy with the first frames when this happens, it never sees true. The user then gets no "session expired" for the whole process. On master, and before this commit, the flag stayed true until it was consumed.

Fix: if (notifyFailure) _sessionRestorationFailed.update { false } at :222, and clear it in the Restored branch so a successful silent retry cannot leave a stale true. In failed restoration preserves profile data and credentials for retry, dropping the clearSessionRestorationFailed() before the first restoreSessionIfNeeded() and asserting the flag is still true covers it.

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.

Changed silent retries to preserve an unconsumed startup restoration failure. A successful retry now clears the stale flag, while sign-out, wipe, and backup restoration still clear it normally.

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 0d32249. Silent retries no longer clear an unconsumed startup flag, both at :222 and in clearAuthenticatedState, and Restored clears it.

initializationReady.complete(Unit)

if (result is InitResult.Restored) {
loadProfile()

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. wipeLocalState() now takes initializeMutex, and restoreSessionIfNeeded holds it through loadProfile()/loadContacts(), one profile resolution per contact with no timeout here. A wallet reset started just after connectivity returns waits behind all of that, and WipeWalletUseCase's step has no timeout. The ordering is intentional (the wipe-vs-resurrection test), only the duration matters. Running the two loads after the lock is released keeps that guarantee: both already drop stale results via _publicKey.value != pk, and a wipe under the lock nulls _publicKey.

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 restored profile and contact loading outside initializeMutex. Credential reads, session import, and authenticated-state installation stay serialized, while wipe and sign-out no longer wait for profile or contact network calls.

This branch has not been deployed

No deployments
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.

[Bug]: Pubky profile falls back to "Your Name" after Shop Paykit setup

3 participants