Skip to content

fix: drop a queued contact save when the pubky sign-in ends - #1417

Open
Jasonvdb wants to merge 4 commits into
masterfrom
fix/contact-save-sign-in-change
Open

Jasonvdb wants to merge 4 commits into
masterfrom
fix/contact-save-sign-in-change

Conversation

@Jasonvdb

@Jasonvdb Jasonvdb commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

This PR drops a contact tag change or edit that is still waiting to save when the Pubky sign-in it was made in ends, so it can no longer save through the next identity or write back what sign-out cleared.

It follows #1399. QA found this race on iOS #854 (thread), and the QA review of #1399 judged that Android was not affected. Android does have it: the new ContactSaveSessionChangeTest fails 6 of 6 on b46f007, the head #1399 merged with.

Description

  • Fixes queued tag changes saving through the wrong identity. A tag change on a contact's screen waits for that contact's profile lookup. If the user signed out before the lookup finished, each queued tag then saved through whichever identity was signed in by the time it ran:
    • When that identity had saved a contact with the same key, the tags landed on its contact and were stored as its local override.
    • When it had not, each tag showed a "Contact no longer exists" error toast.
    • When the user stayed signed out, each tag showed an error toast.
    • When the same identity signed straight back in, the save re-created the local override that sign-out had cleared.
  • Fixes an Edit Contact save that is still running at sign-out. It used to write its override into the next identity's store, rename that identity's row and show "Contact saved".
  • Adds PubkySignIn, which holds the signed-in public key and a sign-in generation. It holds no secret.
    • clearAuthenticatedState advances the generation before anything else, so sign-out and a wipe end the sign-in.
    • Every sign-in starts a new one before it publishes its key, as on iOS. That covers adopting a Ring identity, creating an identity, signup approval and backup restore. So switching A → B → A, or re-adopting the identity already signed in, also ends the earlier sign-in.
    • Restoring the stored session at start-up, and refreshing the session of the identity already signed in, keep the current sign-in. A refresh therefore doesn't drop an edit that is still saving.
  • The contact screen takes the sign-in when a tag is tapped, and Edit Contact takes it when Save is tapped. PubkyRepo.updateContact now requires it and checks it:
    • before the save;
    • inside the SDK save, which gets expectedIdentity and checks it under the same lock as the write;
    • as the store applies the override;
    • before the contact row is updated under contactsLock.
  • A tag change also checks the sign-in after the contact load and again after the profile lookup.
  • A change whose sign-in has ended saves nothing, writes no override and shows no toast. It fails with the new PubkyContactError.SignInChanged and is logged at info level.
  • Removes "On Android" from the 300 ms refresh batching and the ten-minute lookup freshness in the pubky-profile journey README and in contacts-list-loading.xml, since iOS feat: one-time stale channel monitor recovery #854 does both too. The 10 s import-preview deadline is still Android only.

Out of Scope

  • EditContactViewModel.kt: edits typed while signed in as one identity can still be saved by a Save tap made after another identity has signed in. That Save binds to the new sign-in. It is hard to reach: sign-out pops back to the contact screen, and adopting a Ring identity pops everything above Home. It would need the sign-in taken when the form loads, or the form closed when the sign-in changes.
  • PubkyRepo.kt: addContact and removeContact do not carry a sign-in. When addContact gets no profile from its caller, it looks the contact up before saving, so a sign-out during that lookup leaves the same window. Nobody has reported it, and it is left for a separate change.
  • PubkyRepo.kt: adopting a Ring identity while another is signed in keeps the previous identity's contact overrides, because clearProfileIfIdentityChanged leaves pubkyStore alone. The contacts load also applies them without checking ownerPublicKey. This predates this PR, and it is hard to reach the same way: the choice screen redirects once you are signed in.

Design

N/A — no UI changes.

Preview

N/A

QA Notes

Journeys

N/A — not drivable; see Manual Tests.

Manual Tests

  • Sign in to a Pubky profile with 60+ saved contacts, relaunch, open Contacts and at once open a contact whose row still shows only its label. Add two tags, then sign out before they save → no error toast. Sign back in → the contact has neither tag — holding a profile lookup open across a sign-out is not in Capabilities
  • Repeat, but adopt a second identity that has saved the same contact instead of signing back in → the second identity's contact has neither tag — same reason

Automated Checks

  • added ContactSaveSessionChangeTest.kt — runs the real PubkyRepo under the real view models, with a fake SDK that saves only for the signed-in identity. Tags queued before a sign-out save nothing when the next identity has the same contact, when it lacks it, and when the same identity signs back in. Tags waiting on a lookup that sign-out stops save nothing and show no toast. A tag save or an Edit Contact save that a session change overtakes leaves the next identity alone
  • added ContactDetailViewModelSignInTest.kt — a tag change whose sign-in ends while the contact loads does not look it up or save; one whose sign-in ends during the lookup saves nothing; a save that fails after its sign-in ended shows no toast
  • updated PubkyRepoTest.kt:
    • An edit from an ended sign-in saves nothing, also once the same identity signs back in.
    • An edit saves through the SDK for its own identity only.
    • An edit saves nothing after A → B → A, or after its Ring identity is re-adopted while signed in.
    • A sign-in taken while sign-out resets the store ends once the identity is restored.
    • An edit started before its identity's session is refreshed still saves.
  • updated ContactDetailViewModelTest.kt — stubs the sign-in for the new updateContact parameter
  • updated EditContactViewModelTest.kt — stubs the sign-in for the new updateContact parameter
  • ran testDevDebugUnitTest — 3215 tests, 0 failures.
    • With the main sources put back to b46f007, ContactSaveSessionChangeTest fails 6 of 6.
    • The other new tests call the new API, so each of the 8 guards was removed in turn, and every removal failed at least one test.
    • On ee573ac, before the sign-in fix, the three new sign-in tests fail.

A tag change on a contact's screen waits for that contact's profile lookup, and an edit's save can still be running when the user signs out. Either then saved through whichever identity was signed in by the time it ran, which may have saved a contact with the same key, and wrote back the local override that sign-out had cleared, or it showed an error toast after the sign-out.

Each change now carries the sign-in it was made in. A tag change checks it before and after the lookup, and the repository checks it before the save, passes its identity to the SDK, which checks it under the save's lock, and checks it again as the override and the contact row are written. A change whose sign-in has ended saves nothing, writes no override and shows no toast, also when the same identity signs straight back in.
iOS now applies contact profile refresh results at most every 300 ms and skips lookups for profiles resolved in the last ten minutes, as Android does, so the pubky-profile journeys no longer call either Android only. The contacts list loading journey's description matches the iOS one again.
@greptile-apps

greptile-apps Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

RetriggerConfidence Score: 4/5

[Medium risk] Contact save operations now check the sign-in session.

The PR should not merge until switching away from and back to a Ring identity permanently invalidates contact edits captured before the switch.

Findings

  1. P1 Old sign-in becomes current again ▶

Summary

The PR binds contact edits and tag changes to a captured Pubky sign-in, checks that sign-in around lookups and persistence, and adds race tests and documentation. Ring adoption still permits an old sign-in token to become valid again after an A → B → A switch.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Capture token for A] --> B[Adopt B]
  B --> C[Adopt A again]
  C --> D[Old token matches key and unchanged generation]
  D --> E[Delayed edit can pass save guards]
Loading

Reviews (1) · Last reviewed commit: "docs: describe contacts refresh batching..."

Comment on lines +669 to +670
fun isCurrent(signIn: PubkySignIn): Boolean =
signInGeneration.get() == signIn.generation && _publicKey.value == signIn.publicKey

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 Old sign-in becomes current again

If a user adopts Ring identity A, switches to B, then adopts A again, a contact edit started during the first A sign-in can still save. Ring adoption changes the public key without advancing the sign-in generation, so the old token matches both checks when A returns. The delayed edit can then write into A’s later sign-in instead of being dropped.

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 ddb21e6. You were right: adoptRingIdentity published the new key without starting a new sign-in. A sign-in taken under A was current again after A → B → A, and re-adopting A while A was signed in kept it too.

  • Every sign-in starts a new one. Ring adoption, identity creation, signup approval and backup restore now call startSignIn, which advances the generation before publishing the key. iOS feat: one-time stale channel monitor recovery #854 already renews its session revision on each of these.
  • A refresh keeps the current sign-in. Restoring the stored session at start-up and refreshSessionIfPossible use continueSignIn. They keep the current sign-in when the key is unchanged, so a session refresh doesn't silently drop an edit that is still saving. When the key changes, including from signed out, they start a new one.
  • A second gap is closed too. clearAuthenticatedState advances the generation, then suspends before it clears the key. A sign-in taken in that window could become current again after the same identity was restored.

Tests added to PubkyRepoTest:

  • After A → B → A, and after re-adopting A while signed in, the old edit fails with SignInChanged, saves nothing through the SDK and writes no override. Both failed on ee573ac with expected:<SignInChanged> but was:<null>.
  • A sign-in taken while sign-out resets the store ends once the identity is restored. This also failed on ee573ac.
  • An edit that is saving while its identity's session is refreshed still saves. This one passes on both. It fails if the refresh path starts a new sign-in.

Full unit suite: 3215 tests, 0 failures. detekt is unchanged.

@github-actions

github-actions Bot commented Oct 3, 2026 •

Copy link
Copy Markdown
Contributor

Regtest APK

Built from ddb21e6 (run).

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

Adopting a Ring identity published its key without starting a new sign-in, so a sign-in taken under A was current again after adopting B and then A, and a contact edit still waiting from A's first sign-in saved into its later one; re-adopting A while A was signed in kept the earlier sign-in as well. Adoption, identity creation, signup and backup restore now each start a new sign-in before they publish the key, as on iOS, which also ends a sign-in taken while a sign-out was clearing the store. Refreshing or restoring the session of the identity already signed in keeps its sign-in, so an edit under way for that identity still saves; it starts a new one only when the key changes.

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.

1 participant