Closed Bug 1453887 Opened 2 years ago Closed 2 years ago
Sync attempts to sync as the network link goes down
59 bytes, text/x-review-board-request
Thom noticed a spike in sync errors, which appear to be mostly network related. After running a few notebooks  we came up with the following theory: * At  we observe "network:link-status-changed", and if this.offline is false we sync now. * In bug 1420802 we changed the offline getter to ignore the link status. * End result is that we try to sync on notifications of the link going *down*. The fix appears easy - check .offline *and* NetworkLinkService.isLinkUp before syncing (although we should also consider if the |this.shouldSyncWhenLinkComesUp| logic removed by that bug has value.)  https://gist.github.com/mhammond/92f8ff67ecc13e1d4bd7590336f13619  https://searchfox.org/mozilla-central/rev/9f3da81290054c5b8955bb67ff98cae66676f745/services/sync/modules/policies.js#204
This graph is sufficiently convincing that this is the cause of the errors: https://sql.telemetry.mozilla.org/queries/52529/source#139886 I don't really see why we'd want `this.shouldSyncWhenLinkComesUp`. Did that logic ever land anyway?
Comment on attachment 8967841 [details] Bug 1453887 - Avoid syncing as the network link changes to down https://reviewboard.mozilla.org/r/236550/#review242378 Looks good, thanks!
Attachment #8967841 - Flags: review?(eoger) → review+
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/d10dc4dc4162 Avoid syncing as the network link changes to down r=eoger
Needinfoing myself to follow up and check if this does indeed resolve the issue.
Please also request uplift to beta after you have verified things and are confident that nothing has been made worse here. Also, FWIW, I think that fallthrough was intended, but now that .offline doesn't take the link status into account, I think the patch is probably correct. The scheduler is a mess :(
Yeah I'll request uplift after confirming it fixes it. I think the fallthrough only made sense if there was a break after scheduleNextSync, since otherwise we'll sync twice in the cases we call that.
https://sql.telemetry.mozilla.org/queries/52561#140010 indicates that builds after this landed have substantially fewer unknown_host errors. We still expect some, since, well, networks ¯\_(ツ)_/¯.
Comment on attachment 8967841 [details] Bug 1453887 - Avoid syncing as the network link changes to down Approval Request Comment [Feature/Bug causing the regression]: Bug 1420802 [User impact if declined]: Many attempted syncs in cases where sync will certainly fail (we just got a notification that the network went down). [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Yes, as well as via sync error telemetry. [Needs manual test from QE? If yes, steps to reproduce]: No, verified via automated tests, and we have telemetry saying this fixes the issue. (It wouldn't be hard to test manually, but doesn't seem necessary). [List of other uplifts needed for the feature/fix]: N/A [Is the change risky?]: Low risk. [Why is the change risky/not risky?]: This part of the sync scheduler is completely optional for sync working correctly (it's an opportunistic sync when the network comes online), and we'll be syncing strictly less often this way anyway. [String changes made/needed]: N/A
Attachment #8967841 - Flags: approval-mozilla-beta?
Comment on attachment 8967841 [details] Bug 1453887 - Avoid syncing as the network link changes to down avoid useless sync on link down, approved for 60.0b14
Attachment #8967841 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
2 years ago
You need to log in before you can comment on or make changes to this bug.