fix: clean up an interrupted ring pubky pick

[ovitrif]

Fixes #1363
Refs: #1329

This PR fixes an interrupted or failed Pubky Ring pick leaving a hidden signed-in session behind.

Description

• Rolls back a Ring pick that is cancelled before it completes, such as by pressing back on Pubky Choice, so the user ends signed out with no saved session and no Ring reference, instead of being signed in as the abandoned pubky after a relaunch.
• Ends a pick whose sign-in saved a session and then failed signed out with no session and no Ring reference, instead of falling back to sign-up, which would save the Ring key as a local key without its Ring reference.
• Runs Ring picks one at a time, so a second pick starts only after the first has finished or rolled back, and a rollback can no longer undo or be blocked by another pick.
• Leaves a Ring reference that a wipe or sign-out already replaced untouched during the rollback.

Out of Scope

• PubkyAuthHandlerRegistrar: the pubkyauth:// alias disabled while Ring was unavailable at launch is only recomputed when the identity changes, raised in bug: an interrupted ring pubky pick leaves a half-saved session #1363.
• PubkyRepo: the source-loss warning redacts the whole Ring reference, raised in bug: an interrupted ring pubky pick leaves a half-saved session #1363.
• PaykitSdkSessionProvider: clearSessionAccess deletes the Ring reference before the session credentials, so a failed session delete leaves a session without its reference, raised in fix: clean up an interrupted ring pubky pick #1386 (comment).

Design

N/A — no UI changes.

Preview

N/A — no user-visible changes.

QA Notes

Journeys

N/A — not drivable; see Manual Tests.

Manual Tests

• No profile → Pubky Choice → tap a Ring pubky → back before it finishes → header shows signed out → relaunch → still signed out, no Ring pubky restored — Pubky Ring app not in Capabilities
• regression: No profile → Pubky Choice → tap a Ring pubky and let it finish → signed in as that pubky, Create Profile shown when it has no profile — Pubky Ring app not in Capabilities

Automated Checks

• added PubkyRepoTest.kt — a pick cancelled after sign-in ends signed out with no session and no Ring reference, also when revoking the session fails
• added PubkyRepoTest.kt — a sign-in that saves a session and then fails ends with no session and no Ring reference, without signing up
• added PubkyRepoTest.kt — a newer pick of the same pubky starts only after a cancelled older pick rolled back, and keeps its session and Ring reference
• added PubkyRepoTest.kt — cancelling a pick leaves no session or Ring reference when a pick of another pubky fails at the credential lookup or at sign-in
• updated PubkyRepoTest.kt — replaces the test that kept the identity when profile loading was cancelled
• ran Local verification: just compile, just test, just lint

[github-actions]

Regtest APK

Built from effae80 (run).

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

[jvsena42]

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:

• No adoption happens without a user tap.
• The rollback's reference-match guard runs under initializeMutex with NonCancellable.
• Back during sign-in or profile load discards the new session and clears only the reference.
• There is no manifest or registrar change, and shipped local identities are unaffected.

[Jasonvdb]

I've closed #1393 in favour of this one. Rolling back on Back is simpler on Android than finishing the pick off-screen, which needed extra serialization there and led to the problem jvsena42 found on it.

Two things from that work that might help here:

• Activation cleanup in PaykitSdkService (jvsena42's inline comment and Greptile r4139882837): signIn and signUp also serve relaunch re-sign-in, restoreSessionIfNeeded, refreshSessionIfPossible, restoreSessionBackupState and createLocalIdentitySession, so clearing there can sign out a Ring user after one failed SDK start. In fix: finish an interrupted ring pubky pick #1393 the rollback stayed in the pick instead: snapshot PAYKIT_SESSION before sign-in, and if the pick fails with a different session saved, discard that session and then the reference. The same snapshot also skips the sign-up fallback once sign-in has saved a session, which closes the Greptile path where the Ring secret ends up saved as a local key.
• Teardown order: PaykitSdkSessionProvider.clearSessionAccess deletes SHARED_PUBKY_SOURCE before the session credentials, so a failed session delete leaves the same session-without-a-key state bug: an interrupted ring pubky pick leaves a half-saved session #1363 describes. Deleting the credentials first keeps the reference until the session is gone.

One question: should iOS roll back too, so both platforms match? It finishes the pick today only because adoption runs in an unstructured Task.

Once this lands I'll open a follow-up with the #1363 items it leaves out, the pubkyauth:// alias recompute and the source-loss log redaction, plus the teardown order and keeping a kept session's reference when a later pick fails, unless you'd rather fold any of them in here.

[coreyphillips]

Two independent reviews, nothing blocking a merge.

worth doing, does not block

• Activation cleanup drops the Ring reference before the sign-up fallback runs (app/src/main/java/to/bitkit/repositories/PubkyRepo.kt:414). When activation fails for an adopted Ring pubky, activateOrClearLocked now deletes SHARED_PUBKY_SOURCE inside PaykitSdkService.signIn. The exception then reaches signInOrSignUpAdoptedIdentity (PubkyRepo.kt:414). That code treats any signIn failure as a trigger for the sign-up fallback when hasIdentityRecord returns false. At that point sessionProvider.adoptedPubky() returns null, so persistSessionAccess (PaykitSdkService.kt:956) takes the local-identity branch and writes the Ring secret to PUBKY_SECRET_KEY. If sign-up and activation then succeed, the pick completes as a local identity carrying a copy of the Ring key and no Ring reference. If activation fails again, onlyAdoptedRingIdentity no longer matches, so the session and the local key both stay. In either state rollBackInterruptedAdoption bails out on its reference check, so a later cancel leaves the session in place. Getting here needs homeserver sign-in to succeed, initialize() to throw, and republishIdentity to report no record, so it is unlikely. I confirmed it by reading the code, not by running it. One fix is to rethrow activation failures instead of falling back to sign-up. Another is to skip the fallback once SHARED_PUBKY_SOURCE no longer matches the reference.
• Canceled pick can revoke a newer pick of the same pubky (app/src/main/java/to/bitkit/repositories/PubkyRepo.kt:392). A canceled pick can revoke a newer successful pick of the same Ring pubky. adoptRingIdentity releases initializeMutex during profile loading, so another pick can install a session before the canceled call reacquires the mutex. Both attempts have the same reference, so the older call passes this check, calls discardAbandonedSession(), and clears the newer identity.

nits

• Rollback overwrites the cleanup-pending flag set by discardAbandonedSession (app/src/main/java/to/bitkit/repositories/PubkyRepo.kt:394). If both signOut() and forgetSessionAccess() fail, discardAbandonedSession calls clearLocalState(publicPaykitCleanupPending = true). rollBackInterruptedAdoption then calls clearLocalState() straight after, which sets publicPaykitCleanupPending back to false. It only matters on the double-failure path, and a fresh pick has probably published no endpoints. Still, the second call quietly undoes the first. Passing the flag through, or skipping the second clearLocalState when the fallback already ran, would keep the two consistent.
• Private rollback helper has prohibited KDoc (app/src/main/java/to/bitkit/repositories/PubkyRepo.kt:388). This KDoc documents a private function, contrary to the repository rule that private functions must not have code comments.

[ovitrif]

Pushed dab5dbc, which answers the review feedback so far:

• The activation cleanup no longer lives in PaykitSdkService: signIn and signUp are back to what master has, so session restore and refresh cannot sign out an adopted Ring user after one failed SDK start. The cleanup stays inside the pick in PubkyRepo.
• A pick whose sign-in saved a session and then failed now rethrows instead of falling back to sign-up. The session is discarded while the Ring reference is still set, then the reference is cleared.
• The rollback of a cancelled pick skips when a newer pick started, so it no longer discards a newer pick of the same pubky.
• The rollback keeps the cleanup-pending flag that discardAbandonedSession sets, and the KDoc on the private rollback helper is gone.

I updated the description to match. Local verification: just compile, just test, just lint.

[ovitrif]

In #1393 the rollback stayed in the pick instead: snapshot PAYKIT_SESSION before sign-in, and if the pick fails with a different session saved, discard that session and then the reference. The same snapshot also skips the sign-up fallback once sign-in has saved a session

@Jasonvdb thanks, I took that approach in dab5dbc: PaykitSdkService is back to master, and the pick skips the sign-up fallback once sign-in saved a session, discards that session and then clears the reference.

Once this lands I'll open a follow-up with the #1363 items it leaves out, the pubkyauth:// alias recompute and the source-loss log redaction, plus the teardown order and keeping a kept session's reference when a later pick fails, unless you'd rather fold any of them in here.

A follow-up works for those. The teardown order in clearSessionAccess is existing behaviour, so I listed it under Out of Scope in this PR's description next to the two #1363 items.

[ovitrif]

Activation cleanup drops the Ring reference before the sign-up fallback runs

@coreyphillips thanks. All four are addressed in dab5dbc:

• The pick rethrows instead of falling back to sign-up once sign-in saved a session, and the cleanup no longer runs in PaykitSdkService.
• The rollback skips when a newer pick started, so a cancelled pick no longer revokes a newer pick of the same pubky.
• The rollback skips its own clearLocalState() when discardAbandonedSession already cleared with the cleanup-pending flag.
• The KDoc on the private rollback helper is removed.

[pwltr]

Requesting changes for the MEDIUM-priority rollback finding inline.

[ovitrif]

Pushed e48e277, which answers @pwltr's review: a pick now counts as newer only once it has verified its Ring credential and is about to save its reference, so a pick that fails before that no longer stops an earlier pick's rollback. I added a test for that case and listed it in the description. Local verification: just compile, just test, just lint.

[pwltr]

Follow-up on the rollback ownership issue: the credential-lookup case is fixed, but a later sign-in failure still leaves the earlier Ring session behind after Back.

[ovitrif]

Pushed effae80, which answers @pwltr's follow-up review. Instead of tracking which pick is newer, Ring picks now run one at a time: adoptRingIdentity holds an adoption lock for the whole pick, including profile loading and the cancellation rollback, so a second pick starts only after the first has finished or rolled back. The generation counter from dab5dbc and e48e277 is gone.

This changes one behaviour: a second pick started while another is still loading waits for it instead of running alongside it.

I updated the description and the overlapping-pick tests, and added a test for a later pick that fails at sign-in. Local verification: just compile, just test, just lint.

[jvsena42]

Re-checked dab5dbc, e48e277 and effae80. No findings; the activation-cleanup MEDIUM is resolved. With #1393 closed, the competing-design note no longer applies.

Checked and clean:

• adoptionMutex serializes picks, and an abandoned pick rolls back under NonCancellable before the next one starts, so picking B after Back cannot end up signed in as A.
• A pick rolls back only itself, only while !completed, and re-checks the reference under initializeMutex.
• Lock order is always adoptionMutex → initializeMutex, so there is no deadlock path.
• The sign-up fallback no longer runs after sign-in saved a session, so the Ring secret cannot be stored locally.
• Every cancel point discards the new session and clears the reference.

[ovitrif]

Re-checked dab5dbc, e48e277 and effae80. No findings; the activation-cleanup MEDIUM is resolved.

@jvsena42 thanks for re-checking. Your earlier changes-requested review on 1cf5d73 still blocks the merge, so if nothing is left on your side, could you approve effae80?