MergeConflictError shouldn't be considered a sync failure
Categories
(Firefox :: Sync, enhancement)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox90 | --- | fixed |
People
(Reporter: markh, Assigned: skhamis)
Details
Attachments
(1 file)
Periodically, about:sync notifies me of a sync error. If it's not kinto, it's a MergeConflictError. This is expected, so ideally we wouldn't treat it as an error.
FWIW, the logs imply that the "livemarks" addon was updating bookmarks and that's what triggered the sync in the first place - it's actually surprising I don't see MergeConflictErrors more often when that happens!
(On the other hand, it's rare, so simply ignoring this every time it happens might be a future foot-gun. I'm not sure what the best way to approach this is yet, or even if we should do anything at all...)
1587079302444 Sync.ErrorHandler DEBUG bookmarks failed: MergeConflictError: The database is busy(resource://gre/modules/SyncedBookmarksMirror.jsm:1424:5) JS Stack trace: MergeConflictError@SyncedBookmarksMirror.jsm:1424:5
handleError@SyncedBookmarksMirror.jsm:766:22
1587079302444 Sync.SyncScheduler TRACE Handling weave:engine:sync:error
1587079302444 Sync.Telemetry TRACE observed weave:engine:sync:error bookmarks
...
1587079303715 Sync.Service INFO No change to declined engines. Not reuploading meta/global.
1587079303715 Sync.Service TRACE Event: weave:service:sync:finish
1587079303715 Sync.ErrorHandler TRACE Handling weave:service:sync:finish
1587079303715 Sync.ErrorHandler TRACE Status.service is error.sync.failed_partial
1587079303715 Sync.ErrorHandler ERROR Some engines did not sync correctly.
1587079303715 Sync.SyncScheduler TRACE Handling weave:service:sync:finish
|
||
Comment 1•5 years ago
|
||
I agree, we can log these at WARN or INFO at a minimum—they aren't really actionable, but, as you point out, seeing many MergeConflictErrors might indicate something else is going on, and we don't want to ignore that. I guess we'll want to special-case them to still fail the sync, but do it quietly.
Should we also record them in telemetry? The dashboard currently filters out syncs that fail with this because they're so noisy, but it could be useful to have a graph of the percentage of syncs that see this, so that we know it's not spiking.
|
Reporter | |
Comment 2•4 years ago
|
||
Telemetry might be nice, but let's not do that as part of this ticket - we've got this far without it.
It turns out there's already some special handling for this exception at here and all we need to do is log.warn about it and not rethrow it (ie, just returning normally from that function is hopefully going to be fine)
Testing is going to be a challenge because this is going to be tricky to reproduce. I suspect we might want to do something hacky like mock out one of the async functions in PlacesSyncUtils which is in the callstack for this error, and instead of resolving immediately we should create a new bookmark and then resolve the mocked promise. I think this will trigger the exception.
|
Assignee | |
Comment 3•4 years ago
|
||
|
||
Updated•4 years ago
|
|
||
Comment 6•4 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/0fe59d3044ab
https://hg.mozilla.org/mozilla-central/rev/f4e48f7e8bf8
Description
•