Skip to content

fix: finish an interrupted ring pubky pick - #1393

Closed
Jasonvdb wants to merge 6 commits into
masterfrom
fix/1363-interrupted-ring-pick
Closed

Jasonvdb wants to merge 6 commits into
masterfrom
fix/1363-interrupted-ring-pick

Conversation

@Jasonvdb

@Jasonvdb Jasonvdb commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #1363
Refs: #1329, synonymdev/bitkit-ios#774

This PR makes a Pubky Ring pick on Pubky Choice end in one consistent state when the user presses Back mid-pick or activation fails, and re-enables the pubkyauth handler once Pubky Ring is reachable again.

Sign-in runs on ServiceQueue.CORE, whose own SupervisorJob keeps it running after the caller is cancelled. A Back press at any point during sign-in therefore saved and activated the session, and only then cancelled the pick, skipping the signed-in state, the pending profile setup flag and the failure cleanup.

Adoption was left cancellable in #1329 (r4093783423) because an abandoned pick that failed later could delete the reference of a retried pick. Serializing picks removes that race, so a pick can now finish.

#1339 reworked adoption on master while this was open: Back during sign-in rolled the pick back, and the pending flag was still written after the profile and contact load. This PR replaces that with the finish-on-Back behaviour above, and keeps #1339's lock order, its per-identity cache reset, its re-checked source-gone clear and its rule that an automatic restore retry does not replace a pick (paykit-clock-changes.md, Connection loss step 5).

Description

  • Finishes a pick once its first write lands, even if the user leaves Pubky Choice: the Ring reference, sign-in, profile lookup and pending profile setup flag are written under NonCancellable, the identity is published last, and contacts still load, matching iOS where the pick is never cancelled.
  • Serializes picks on the initialization lock, so a pick retried while an abandoned one is still finishing waits for it and opens Profile instead of showing "Authorization Failed", and ignores a second tap during a pick.
  • Lets an automatic restore retry yield to a pick waiting for the lock, so the pick is not replaced, while a cold start still restores the saved identity first and the queued pick opens Profile.
  • Aborts a pick before writing anything when the saved session or Ring reference cannot be read, so an unreadable session is kept rather than discarded.
  • Rolls back only what a failed pick wrote: a session it saved is revoked, or forgotten, or cleared locally, before the Ring reference is deleted, and a session kept from before the pick keeps its reference.
  • Skips the sign-up fallback once sign-in has saved a session, so an activation failure no longer publishes a homeserver record and a second grant.
  • Clears the session credentials before the Ring reference when a session is torn down, so a failed delete cannot leave a session with nothing to sign for.
  • Re-checks the pubkyauth handler when an adopted pubky's Pubky Ring becomes reachable or unreachable, so pubkyauth:// links reach Bitkit again without a restart.
  • Logs a failed pick in the repository so it is still logged after Back, makes the source-gone clear non-cancellable, and redacts only the pubky in the source-loss warning.

Out of Scope

  • PubkyRepo: process death mid-pick; NonCancellable covers cancellation only, the same as iOS. A durable pick-in-progress marker would close it.
  • PubkyRepo: remote residue of a failed pick, such as the receiver marker, the identity republish and a grant whose revoke fails. On the emulator, revoking after an injected handle.initialize() failure returned identity_error ("restore Pubky grant before revocation") and the rollback fell back to forgetting the session locally.
  • ui/screens/profile: a pick that finishes after Back shows no spinner if Pubky Choice is reopened and offers no contact import; the header turns signed in a few seconds after Back.
  • PubkyRepo: createIdentity, sign-out and the Ring source check are not serialized with a pick.
  • bitkit-ios: single-flight pick and a teardown fallback in discardAbandonedSession, in a separate PR.

Design

N/A — no UI changes.

Preview

N/A — no UI changes. Driven on an API 36 emulator with Pubky Ring at 57ab5c5, before the review fixes and the merge with master: Back mid-pick saved the session 3.4 s later and finished with the identity signed in and no toast, a retried pick opened Profile, an injected activation failure left no session or reference, and the auth handler turned off and on again as Pubky Ring was disabled and re-enabled.

QA Notes

Journeys

N/A — not drivable; see Manual Tests.

Manual Tests

  • Paykit on, signed out → Profile → Pubky Choice → tap a Ring pubky, back during the spinner → header turns signed in a few seconds later, no toast; relaunch stays signed in — Pubky Ring not in Capabilities
  • Ring pubky without a profile → tap it, back during the spinner → Create Profile opens; relaunch resumes Create Profile — Pubky Ring not in Capabilities
  • Tap a Ring pubky, back, reopen Pubky Choice and tap it again → Profile, no "Authorization Failed" toast — Pubky Ring not in Capabilities
  • Signed in with a Ring pubky → disable Pubky Ring, reopen Bitkit → MainActivityPubkyAuth disabled; enable Pubky Ring, reopen Bitkit → enabled again without a restart — Pubky Ring not in Capabilities
  • regression: tap a Ring pubky and wait → Profile, Pay Contacts or Create Profile as before — Pubky Ring not in Capabilities

Automated Checks

  • added PubkyRepoTest.kt — a pick cancelled during sign-in (through the real ServiceQueue), the profile lookup or the contact load finishes with the identity, pending setup and contacts, and one cancelled before the reference write writes nothing
  • added PubkyRepoTest.kt — a failed pick discards the session it saved before the reference, falls back to forgetting and then clearing it, never signs up after a saved session, and keeps a session from before the pick
  • added PubkyRepoTest.kt — a retried pick waits for an abandoned one and gets already signed in, and the Ring reachability flag follows the source check and a finished pick
  • added PaykitSdkServiceTest.kt — session teardown deletes the credentials before the Ring reference and keeps the reference when that fails
  • added PubkyChoiceViewModelTest.kt — an already signed-in pick opens Profile without a toast, and a second tap during a pick is ignored
  • added PubkyAuthHandlerRegistrarTest.kt — the auth handler follows Pubky Ring becoming reachable or unreachable for the same pubky
  • added PubkyRepoTest.kt — a cold start publishes a restored identity over a queued pick, an automatic restore retry yields to a pick waiting for the lock, and an unreadable session or reference aborts the pick with nothing written
  • updated PubkyRepoTest.kt — a mismatching credential no longer touches the Ring reference
  • updated PubkyRepoTest.kt — fix: recover interrupted paykit sessions #1339's adoption tests now expect a cancelled profile lookup to finish the pick and a wipe to wait for it, and a pick over a signed-in identity to be refused

@greptile-apps

greptile-apps Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 2/5

[Medium risk] Fixes interrupted Pubky Ring identity selection flow.

The PR should not merge until failed-pick rollback and concurrent source checks preserve the correct session and identity.

Findings

  1. P1 Revoked session remains saved ▶
  2. P1 Unreadable session is discarded ▶
  3. P1 Stale check clears new identity ▶

Summary

The PR makes Ring adoption finish after its first write, serializes picks, adds failure rollback, and refreshes the authorization handler when Ring reachability changes.

  • The rollback can leave a saved session without its Ring reference or discard a session that preceded the pick.
  • An in-flight source check can clear a newly adopted identity.

Reviews (1) · Last reviewed commit: "fix: load contacts and log failures of a..."

}

private suspend fun discardRingSession() {
val revocationError = runSuspendCatching { pubkyService.signOut() }.exceptionOrNull() ?: return

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Revoked session remains saved

If a pick saves a session but later fails, a successful signOut() returns here without deleting PAYKIT_SESSION. The service revokes the session but does not remove that saved value, while rollback deletes its Ring reference. The failed pick therefore leaves a saved session with no Ring source to provide its secret key.

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.

Not an issue: a successful signOut() does not leave PAYKIT_SESSION behind. In paykit-rs rc56, sign_out revokes the grant and then calls clear_local_identity_state(). That calls the provider's clearSessionAccess() in PaykitSdkService.kt, which deletes PAYKIT_SESSION, PUBKY_SECRET_KEY and SHARED_PUBKY_SOURCE. If that local clear fails, sign_out returns an error and the rollback falls through to forgetSessionAccess(), then to clearSessionAccess() plus an explicit PAYKIT_SESSION delete.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You’re right. With paykit-rs rc56, a successful sign_out() calls clear_local_identity_state(), and PaykitSdkSessionProvider.clearSessionAccess() removes PAYKIT_SESSION along with the other local credentials and Ring reference. If that cleanup fails, sign_out() returns an error and the rollback proceeds through forgetSessionAccess() and the explicit local cleanup. The saved-session scenario from my comment is therefore covered; this finding is invalid.

Tip: You can customize Greptile's behavior for this repo with .greptile/rules.md and .greptile/config.json.


private suspend fun hasSessionChangedSince(previousSession: Result<String?>): Boolean {
val currentSession = runSuspendCatching { keychain.loadString(Keychain.Key.PAYKIT_SESSION.name) }
if (previousSession.isFailure || currentSession.isFailure) return true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Unreadable session is discarded

If an existing session could not be read during initialization, a Ring pick can proceed with no public key in memory. If that pick then fails before saving a new session, this read failure is still treated as proof that the session changed. Rollback can revoke or forget the user's pre-existing session and delete its Ring reference.

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 26cab51: the pick now reads the saved session and the Ring reference before it writes anything, and aborts if either read fails. So an unreadable session is kept, not discarded. adoptRingIdentity should abort the pick when the saved session cannot be read covers it.

Comment on lines 317 to 321
val adoptedPubky = reference.removePrefix(SharedPubkyContract.RING_SOURCE_PREFIX)
Logger.warn("Adopted ring identity '${redacted(adoptedPubky)}' is gone, clearing session", context = TAG)
runSuspendCatching { pubkyService.clearSessionAccess() }
.onFailure { Logger.warn("Failed to clear adopted session access", it, context = TAG) }
clearLocalState()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1 Stale check clears new identity

A foreground source check can read the old Ring reference, then wait for the Ring response while a new pick commits. If the response says the old pubky is gone, this code clears the session and identity without checking whether the reference changed. The newly adopted identity can be cleared by the earlier check.

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 by #1339, merged in 26cab51: the source-gone clear now runs under initializeMutex and re-reads SHARED_PUBKY_SOURCE first, returning if it no longer matches. A pick that commits in between keeps its identity.

…d-ring-pick

# Conflicts:
#	app/src/main/java/to/bitkit/repositories/PubkyRepo.kt
#	app/src/main/java/to/bitkit/services/PubkyAuthHandlerRegistrar.kt
#	app/src/test/java/to/bitkit/repositories/PubkyRepoTest.kt
#	app/src/test/java/to/bitkit/services/PubkyAuthHandlerRegistrarTest.kt
@github-actions

Copy link
Copy Markdown
Contributor

Regtest APK

Built from 26cab51 (run).

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

@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 inline. Ring adoption is unreleased; Paykit is on by default on master.

#1386 and #1393 both fix #1363 with opposite designs: #1386 rolls a pick back on Back, #1393 finishes it. Only one should merge. They conflict in PubkyRepo.kt and PubkyRepoTest.kt, but PaykitSdkService.kt auto-merges, so resolving PubkyRepo toward #1393 still brings in #1386's activateOrClearLocked. In #1393's flow, that cleanup deletes PAYKIT_SESSION before signInRingIdentity checks hasSessionChangedSince. A first pick then falls through to the sign-up fallback #1393 removed, and with the Ring reference already deleted, persistSessionAccess saves the Ring secret as a local key (the mechanism in greptile r4139882837). Close whichever one is not chosen.

Checked and clean:

  • The manifest change is a comment only, and MainActivityPubkyAuth is unchanged.
  • adoptRingIdentity's only caller is a user tap, and no relaunch or deep link completes a pick.
  • A cancelled lock wait decrements waitingRingPicks.
  • Failed-pick rollback deletes only a reference it wrote, and skips the sign-up fallback once a session was saved.
  • activateBootstrapResult resets pubkyStore/SDK state on an owner change.
  • Shipped local identities are untouched.

}
.onFailure {
Logger.error("Failed to adopt ring identity", it, context = TAG)
if (it is PubkyAlreadySignedInError) {

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.

Picking a different pubky while an abandoned pick finishes signs in as the abandoned one.

After Back, pick A keeps running under NonCancellable, and _publicKey is published only at the end of commitRingIdentity (PubkyRepo.kt:441), so isAuthenticated stays false for the rest of the pick. During that window, Drawer → Profile still routes to Pubky Choice. The new view model has adoptingPubky == null, and the Ring list is tappable.

Tapping pubky B queues on withRingPickLock. When A commits, B throws PubkyAlreadySignedInError (PubkyRepo.kt:381), and this branch opens Profile without a toast. The user picked B and is signed in as A. On master the same sequence ends as B, because Back rolls A back before B takes the lock.

Fix: only a retry of the same pubky needs the silent path. Compare the requested pubky with pubkyRepo.publicKey.value (PubkyPublicKeyFormat.matches) and show the auth error when they differ, or throw PubkyAlreadySignedInError from adoptRingIdentity only when _publicKey matches pubky. Add a PubkyChoiceViewModelTest case with a different signed-in pubky.

@Jasonvdb

Copy link
Copy Markdown
Contributor Author

Closing in favour of #1386, which fixes #1363 by rolling the pick back on Back.

The parts of this PR that don't depend on that choice will come in a follow-up once #1386 lands: the pubkyauth:// alias recompute when Pubky Ring comes back, the source-loss log redaction, deleting the session before the Ring reference on teardown, no sign-up once sign-in has saved a session, and keeping a kept session's reference when a later pick fails.

@Jasonvdb Jasonvdb closed this Sep 30, 2026
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: an interrupted ring pubky pick leaves a half-saved session

2 participants