1287687 - Syncs happen every 90 seconds when stale device records exist.

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Mark Hammond [:markh] [:mhammond]]

Bug 1264498 (landed in 48) made a change to "stale" device records were not shown to the user. This patch had the side effect of causing a Sync to happen every 90 seconds when such stale devices exist. The reason is that on every Sync we report that we re-applied those stale devices, causing the scheduler to constantly think we had new incoming items and increase the Sync rate accordingly.

The patch here doesn't actually delete the records locally, but instead adds a "stale" flag to them. Functions that expose these device names for the UI now filter based on this flag. The end result is that subsequent Syncs no longer re-apply these records, so Sync settles down to normal.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: Bug 1287687 - prevent the clients engine from scheduling syncs every 90 seconds.

Review commit: https://reviewboard.mozilla.org/r/65140/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65140/

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8772290 [details]
Bug 1287687 - prevent the clients engine from scheduling syncs every 90 seconds.

Edouard and Thom, it would be great if you could both have a good look at this and try and find any holes in it, as I want to uplift this all the way to beta. The general idea is that we just hide stale, duplicate devices from all functions that expose device names outside of Sync.

Comment 3

[Mark Hammond [:markh] [:mhammond]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22963a81833d

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Mark, are you going to ask for an uplift of this fix? Thanks

Comment 5

[Thom Chiovoloni [:tcsc] (ex-moco)]

Comment on attachment 8772290 [details]
Bug 1287687 - prevent the clients engine from scheduling syncs every 90 seconds.

https://reviewboard.mozilla.org/r/65140/#review62240

I don't feel stellar about the fact that we don't know why this bug only became a visible problem in 50 as of around the 5th.  If it were triggered by some change in FxA, you'd expect it to be in 49 (and maybe 48?) too, so it seems likely that something else landed to cause us to have so many stale devices... which could indicate a bug elsewhere.

That said, this is still a valuable fix, and is a straightforward way of approaching the problem. r=me if you're confident that we don't want to ignore stale records coming from the server.

::: services/sync/modules/engines/clients.js:630
(Diff revision 1)
>        record.cleartext = this._remoteClients[id];
> +      if (record.cleartext.stale) {
> +        // It's almost certainly a logic error for us to upload a record we
> +        // consider stale, so make log noise, but still remove the flag.
> +        this._log.warn(`Preparing to upload record ${id} that we consider stale`);
> +        delete record.cleartext.stale;

Hm, I'm not sure I understand why we just remove the flag here. Wouldn't we want to ignore this record entirely?

Also, it seems like we might want to ues `this._log.error` so we dump an error log instead, but I could be wrong.

Comment 6

[Edouard Oger [:eoger]]

Comment on attachment 8772290 [details]
Bug 1287687 - prevent the clients engine from scheduling syncs every 90 seconds.

https://reviewboard.mozilla.org/r/65140/#review62278

Same as Thom, I'm not sure I understand what's the logic behind the change in *clients.js:630* but I trust you.
Otherwise LGTM!

Comment 7

[Mark Hammond [:markh] [:mhammond]]

https://reviewboard.mozilla.org/r/65140/#review62240

> Hm, I'm not sure I understand why we just remove the flag here. Wouldn't we want to ignore this record entirely?
> 
> Also, it seems like we might want to ues `this._log.error` so we dump an error log instead, but I could be wrong.

> Hm, I'm not sure I understand why we just remove the flag here. Wouldn't we want to ignore this record entirely?

Yes - I think this patch will cause the record to be ignored completely, so we should never get here. I was considering throwing, but I felt that slightly more risky for uplift (ie, I probably would have thrown if targetting only 50)

> Also, it seems like we might want to ues this._log.error so we dump an error log instead, but I could be wrong.

SGTM, I'll do that.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/abd1161f6a90
prevent the clients engine from scheduling syncs every 90 seconds. r=tcsc,eoger

Comment 9

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8772290 [details]
Bug 1287687 - prevent the clients engine from scheduling syncs every 90 seconds.

Approval Request Comment
[Feature/regressing bug #]: Bug 1264498
[User impact if declined]: Sync will sometimes run every 90 seconds causing impact for the user and our operations.
[Describe test coverage new/current, TreeHerder]: Landed with new tests
[Risks and why]: Fairly low risk limited to Sync.
[String/UUID change made/needed]: None

Comment 10

[Gerry Chang [:gchang]]

Comment on attachment 8772290 [details]
Bug 1287687 - prevent the clients engine from scheduling syncs every 90 seconds.

This is a new regression and we care about sync. We'll have 48 beta 10 before RC to fix potential regressions. Let's take it in 48 beta 10 and aurora.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/abd1161f6a90

Comment 12

[Carsten Book [:Tomcat]]

does not apply cleanly to aurora

grafting 354913:abd1161f6a90 "Bug 1287687 - prevent the clients engine from scheduling syncs every 90 seconds. r=tcsc,eoger"
merging services/sync/modules/engines/clients.js
merging services/sync/tests/unit/test_clients_engine.js
warning: conflicts while merging services/sync/modules/engines/clients.js! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')

Comment 13

[Mark Hammond [:markh] [:mhammond]]

Attachment: 0001-Bug-1287687-prevent-the-clients-engine-from-scheduli.patch

This version of the patch applies cleanly on Aurora and Beta.

Try runs:
Aurora: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa41d240a1d0
Beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3b85edb67cf8

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/929a78239631

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/512d3ff7151c

Comment 16

[Carsten Book [:Tomcat]]

uplift done, thanks mark!