Skip to content
This repository has been archived by the owner on Nov 1, 2022. It is now read-only.

Connecting to sync immediately syncs twice #7335

Closed
mhammond opened this issue Jun 11, 2020 · 2 comments · Fixed by #7856
Closed

Connecting to sync immediately syncs twice #7335

mhammond opened this issue Jun 11, 2020 · 2 comments · Fixed by #7856
Assignees
Labels
<firefox-accounts> Component: FxA <sync> Component: sync-logins

Comments

@mhammond
Copy link
Contributor

mhammond commented Jun 11, 2020

I'm still diagnosing this, but thought I'd get it out there for comments.

STR:

  • Start Fenix without an fxa user
  • Sign in to FxA
  • Observe via logcat that 2 syncs immediately (but sequentially) happen

I see syncNow:161, WorkManagerSyncDispatcher called twice in quick succession - the first time, reason says FirstSync (which seems reasonable). However, the second time has reason as Startup - even though the app has been started long enough to complete the sign in flow.

It smells as though that second sync should not be happening in this case, but I'm yet to work out exactly why it does.

(And unsurprisingly given that "startup" sync, it doesn't reproduce when disconnecting then reconnecting - once it has reproduced once, you need to uninstall and start fresh to reproduce again)

cc @grigoryk

┆Issue is synchronized with this Jira Task

@mhammond
Copy link
Contributor Author

When tracking down that "declined" issue, I think there's another latent bug here. I believe that the persisted state for syncing was fetched at the time the sync call was queued, not when it was executed. So if there's one sync running and one sync queued, and the first sync changes (say) the declined engines, it seems likely that second sync will execute with the old state, not new state as updated by that first sync.

(This is basically a note-to-self and should possibly be a different issue - I'll try and ping @grigoryk tomorrow)

@grigoryk
Copy link
Contributor

Filed #7874 for the second issue.

I figured out the double-sync issue while working on #7856, will land a fix there. Rough summary is that periodic sync would get scheduled when everything is initialized, and in practice the first one would run pretty much "right away". This coincided with a manual request to "sync now" from the account manager during startup. There's debouncing logic in place but it didn't work in this particular case.

@data-sync-user data-sync-user changed the title Connecting to sync immediately syncs twice FNX3-22764 ⁃ Connecting to sync immediately syncs twice Aug 5, 2020
@data-sync-user data-sync-user changed the title FNX3-22764 ⁃ Connecting to sync immediately syncs twice FNX2-18318 ⁃ Connecting to sync immediately syncs twice Aug 5, 2020
@st3fan st3fan changed the title FNX2-18318 ⁃ Connecting to sync immediately syncs twice Connecting to sync immediately syncs twice Aug 5, 2020
@psymoon psymoon moved this from ⏳ Sprint Backlog to 🏃‍♀️ In Progress in A-C: Android Components Sprint Planning Aug 10, 2020
@pocmo pocmo removed this from 🏃‍♀️ In Progress in A-C: Android Components Sprint Planning Aug 31, 2020
bors bot pushed a commit that referenced this issue Sep 22, 2020
7856: FxA Account Manager refactor r=csadilek a=grigoryk

### State machine changes:
- "profile" states have been removed, `profile` is now handled outside of the state machine
- states and events have been split into two subtypes: external and internal. External states are the `AccountState`s (authenticated, not authenticated, auth problem, etc) and internal states are `InternalState` (beginning auth, completing auth, recovering from auth problems, etc)
- this split should hopefully make navigating the state machine more intuitive
- retry logic and error handling was added for all internal transitions which require networking (Closes #7536)

### account manager changes:
- removed ability to "set sync config" from the public API. we weren't using it (and have no plans to use this), so the added complexity wasn't benefiting anything.

### async/suspend API changes:
account manager, account and device constellation APIs are now `suspend`, which makes both internal code and consuming of these APIs nicer and more flexible. Actual semantics of what's going on didn't change.

### sync manager changes:
introduced a delay to the periodic sync scheduling, so that we can properly manage what goes on during startup (Closes #7335)  

Tests are half-way being updating, so this is still a WIP.

cc @csadilek @rfk @eoger @jonalmeida 



Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>
@mergify mergify bot closed this as completed in #7856 Sep 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
<firefox-accounts> Component: FxA <sync> Component: sync-logins
Projects
No open projects
Development

Successfully merging a pull request may close this issue.

2 participants