fix(account): fix stuck login and stale active-user state when adding a second account - #6637
Merged
Merged
Conversation
mahibi
force-pushed
the
bugfix/noid/fixStuckLoginProcessForMultipleUsers
branch
from
September 3, 2026 09:54
a2dcab8 to
99446a0
Compare
…tion proceedWithLogin() skipped setting the account active and navigating onward whenever the newly stored account already looked like the active user in the DB. Since storeProfile() always inserts new accounts with current=true and the current-user query picks the row with the highest id, this was true for any 2nd+ account added, leaving the verification screen stuck forever. Why the check existed: It was added in ac5061d (2023) to stop proceedWithLogin() firing more than once for the same account and launching ConversationsListActivity twice. SignalingSettingsWorker loops over every logged-in user and posts one SIGNALING_SETTINGS event per account, and it is triggered both from this verification flow and, separately, from NextcloudTalkApplication.initWorkers() on every app start - so two matching events for the same account were possible while this activity was still alive and subscribed. The guard relied on currentUser still pointing at the previously-active account until setUserAsActive() had genuinely run once; that held back then because the underlying query had no ORDER BY and an unordered scan happened to return the older row first. f4157de (2026), for an unrelated reason (deterministic duplicate-account cleanup), made that query ORDER BY id DESC LIMIT 1, which flipped the tie-break to always favor the newest row and silently invalidated this guard's assumption. Possible follow-ups to restore the intent safely: - Guard against a genuine duplicate SIGNALING_SETTINGS event with a local instance flag in the activity instead of a DB query, so it no longer depends on current-user tie-break timing. - Give ConversationsListActivity a launchMode (singleTask/singleTop) or launch it with FLAG_ACTIVITY_SINGLE_TOP/CLEAR_TOP, since it has none today and any duplicate startActivity() call stacks a second instance. - Scope SignalingSettingsWorker's event posting to the account being verified instead of looping over and emitting for every user, to remove the cross-trigger duplicate risk at the source. - Audit other reads of getActiveUser()/getActiveUserObservable()/ getActiveUserSynchronously() for the same class of bug, since any code written before f4157de may have relied on the old (effectively oldest-row) tie-break behavior. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
mahibi
force-pushed
the
bugfix/noid/fixStuckLoginProcessForMultipleUsers
branch
from
September 3, 2026 09:54
99446a0 to
1b6923f
Compare
mahibi
marked this pull request as draft
September 3, 2026 09:58
Contributor
|
APK file: https://github.com/nextcloud/talk-android/actions/runs/33741453467/artifacts/9889463391 |
storeProfile() inserted a brand-new account with current=true right away, before capabilities/push/signaling verification had even run. Nothing in that verification pipeline needs the row to be current - CapabilitiesWorker, SignalingSettingsWorker and setupPushNotifications() all operate on the explicit internal user id, and push registration iterates every account regardless of its current flag. Marking it current here only created a window where two rows were current=1 at once (the original source of the stuck-login bug), and some consumers that read the raw per-row current flag instead of the resolved active user (e.g. the account switcher/import lists filtering on "!user.current") can misbehave while that window is open. setUserAsActive(), called from proceedWithLogin() once verification succeeds, is the only place in the app that flips a row's current flag via the atomic single-owner update - so let it stay the only one. ELI5: The app keeps a notebook with a checkmark next to whichever account is "currently logged in". Adding a second account put a checkmark next to the new account immediately, before even checking that it worked, and never erased the old account's checkmark - so two names ended up checked at once. A separate, unrelated rule said "if I'm done setting up an account and it's already checked, I must have handled this already - skip it". With two checkmarks, that rule fired immediately on the very first (and only) real completion, so the app skipped the one step that opens your chats and just froze. Fixing this means the new account only gets checked off once it's actually confirmed to work, so there's never a two-checkmark mix-up to trip that rule. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
After switching the active account, ConversationsListActivity/ ConversationsListViewModel and the account-chooser dialog could show two different accounts as "current" at the same time. Both read CurrentUserProviderOld's app-wide singleton cache, which only ever refreshed asynchronously via Room's InvalidationTracker noticing the `current` column changed. AccountVerificationActivity.proceedWithLogin() flips that column and, almost immediately after, starts a new ConversationsListActivity - fast enough to usually win the race against the async cache refresh, so the freshly-launched screen (and its ViewModel, which drives which account's rooms/credentials get loaded, not just the toolbar avatar) captured the stale, previous account. The account-chooser dialog re-reads the same singleton fresh on every recomposition, so by the time it was opened later the async refresh had usually caught up - producing two screens that disagreed. UserManager.setUserAsActive() is the sole place in the app that flips which account is active (see the two preceding commits), so it's the right place to make that change observable immediately: it now pushes the newly-active user into a serialized BehaviorSubject synchronously, in addition to Room's own reactive query still feeding that subject as a backstop for any change to the `current` flag that doesn't go through setUserAsActive(). CurrentUserProviderOld/CurrentUserProvider need no changes themselves, since they already just observe UserManager.currentUserObservable - they benefit automatically. This only works if every consumer shares the same UserManager instance, which it turns out they didn't: provideUserManager() in UserModule had no scope, so Dagger handed out a fresh UserManager (and thus a disconnected subject) per injection site despite the app having a single @singleton component. Scoped it @singleton to match how every dependent class already assumed it behaved. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
CurrentUserProviderImpl is coroutine-based but had to bridge into Flow via userManager.currentUserObservable.asFlow().stateIn(...) on Dispatchers.IO - an unnecessary RxJava round trip and an extra coroutine dispatch hop for a value UserManager already updates synchronously in memory. UserManager now also pushes into a MutableStateFlow at the same point it pushes into the existing RxJava subject (inside setUserAsActive()), so CurrentUserProviderImpl can read it directly, with no scope of its own to manage and no thread hop between the write and the value being observable. CurrentUserProviderOld keeps using the RxJava-based currentUserObservable unchanged, since it's a deprecated, non-coroutine class not worth converting further. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
…ding an account proceedWithLogin() started a new ConversationsListActivity with a plain Intent and no flags. If an instance of it was already resident in the back stack - e.g. the conversation list was open for the current account when the user added a second one from within it - that old instance was left behind rather than replaced, reachable via back navigation and showing stale data (its own account, credentials, avatar) forever, since it's a different Activity instance that never gets recreated. Add FLAG_ACTIVITY_CLEAR_TOP, matching the pattern already used by ChooseAccountDialogCompose's own manual "switch to this account" row. ConversationsListActivity uses the default 'standard' launch mode, so this finishes that stale instance and starts a genuinely fresh one via onCreate() instead of leaving it in place - which, combined with the preceding commit making UserManager's active-user signal update synchronously, means the fresh instance reads the correct, currently active user. Assisted-by: Claude Code:claude-sonnet-5 Signed-off-by: Marcel Hibbe <dev@mhibbe.de>
mahibi
changed the title
mahibi
marked this pull request as ready for review
September 3, 2026 11:52
Contributor
|
APK file: https://github.com/nextcloud/talk-android/actions/runs/33751906629/artifacts/9892592189 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adding a second (or later) account got permanently stuck on the verification screen. Along the way to fixing that, a related but distinct bug surfaced: once login could actually complete, the
conversation list and the account-chooser dialog could briefly disagree about which account was active.
Root causes
account just verified — but storeProfile() already inserted new accounts with current = true immediately on creation, and a recent, unrelated fix (f4157de, made the "active user" query
deterministic for duplicate-account cleanup) changed how ties between two current = 1 rows resolve. Combined, the guard's condition was false on the very first, legitimate completion for any 2nd+
account — not just on a hypothetical duplicate — so it silently skipped the step that sets the account active and opens the conversation list.
flips the active-account flag and, almost immediately after, opens the conversation list — fast enough to usually win that race, so the freshly-shown screen (and which account's rooms/credentials it
loaded) could still reflect the previous account.
back stack if one was already open when a second account was added.
Fix
Room's async invalidation.
stack is replaced instead of left behind.
🏁 Checklist
/backport to stable-xx.x🤖 AI (if applicable)