fix: recover interrupted paykit sessions - #1339
ben-kaufman wants to merge 7 commits into
Conversation
Regtest APKDownload bitkit-dev-debug universal APK (expires in 30 days). |
|
| } | ||
| is InitResult.RestorationFailed -> { | ||
| clearAuthenticatedState() | ||
| clearAuthenticatedState(clearCachedProfile = false) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
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. 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
retryis still an upcoming period, sosynchronize()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,_publicKeystays 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, andcompleteAuthenticationholds the mutex from approval through activation. - No nested
initializeMutexindeleteProfile,discardAbandonedSession,approveSignupAuthorrestoreSessionBackupState. - 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 } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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
Out of Scope
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.
Automated Checks
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.PubkyAuthHandlerRegistrarTest.ktandAppViewModelSendFlowTest.kt— saved/unreadable identities do not advertise signup; reconnect and foreground trigger recovery.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.PubkyIdentityRepublishTest.kt— clock rollback retries publication and then resumes throttling.PaykitSubscriptionNotificationSchedulerTest.kt— a worker running before the billing boundary defers its reminder.PaykitSubscriptionTest.kt— timezone and DST changes preserve UTC billing boundaries.