691663 - SyncScheduler should obey backoffInterval at all times

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

These SyncScheduler methods don't always or at all obey backoffInterval:

* scheduleAtInterval: it does some weird thing with the sync error count and SyncScheduler.
* scheduleNextSync: when you pass a number in for 'interval', backoffInterval gets ignored.
* 'back' observer: it calls sync directly (via Utils.nextTick), it should go through scheduleNextSync, provided that one is fixed to obey backoffInterval

(Note that all but the last behaviour was carried over when SyncScheduler was broken out of Service.)

Comment 1

[Philipp von Weitershausen [:philikon]]

Attachment: v1

Make sure we always adhere to backoffInterval in scheduleNextSync() and in the back-from-idle observer that triggers a sync.

Comment 2

[Richard Newman [:rnewman]]

Comment on attachment 564743 [details] [diff] [review]
v1

Review of attachment 564743 [details] [diff] [review]:
-----------------------------------------------------------------

Hooray for pair programming!

::: services/sync/tests/unit/test_syncscheduler.js
@@ +204,5 @@
>    run_next_test();
>  });
>  
> +add_test(function test_scheduleNextSync_noBackoff() {
> +  _("scheduleNextSync() will use the current syncInterval if no interval is provided and ignore scheduling requests for larger intervals that the current one.");

_("If no interval is provided, scheduleNextSync() will use the current syncInterval. " +
  "Scheduling requests for intervals larger than the current one will be ignored.");

@@ +243,5 @@
> +  SyncScheduler.scheduleNextSync(requestedInterval);
> +  do_check_true(SyncScheduler.nextSync <= Date.now() + requestedInterval);
> +  do_check_eq(SyncScheduler.syncTimer.delay, requestedInterval);
> +
> +  // Request a sync at the smallest possible number.

interval.

@@ +254,5 @@
> +  run_next_test();
> +});
> +
> +add_test(function test_scheduleNextSync_backoff() {
> + _("scheduleNextSync() will honour backoff in all scheduling requests.");

Hmm, US or international english?

Comment 3

[Richard Newman [:rnewman]]

Waiting for additional test fixes for altered logic.

Comment 4

[Philipp von Weitershausen [:philikon]]

Attachment: v2

Addressed nits and made two failing tests pass.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 564749 [details] [diff] [review]
v2

Review of attachment 564749 [details] [diff] [review]:
-----------------------------------------------------------------

::: services/sync/modules/policies.js
@@ +349,5 @@
>    /**
>     * Set a timer for the next sync
>     */
>    scheduleNextSync: function scheduleNextSync(interval) {
> +    // If no interval was specific, use the current sync interval.

s/specific/specified.

Comment 6

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/services/services-central/rev/2fc5fa5a744d

Comment 7

[Philipp von Weitershausen [:philikon]]

STRs for QA:

All score and and back-from-idle triggered syncs should now obey backoff, too. They previously would ignore backoff and start a sync immediately.

* Score triggered syncs happen when bookmarks, passwords, and prefs are modified.
* Back-from-idle triggered syncs occur when the machine was idle for the duration of 'services.sync.scheduler.idleTime' (default is 300 seconds) and the user comes back to the machine.

Comment 8

[Philipp von Weitershausen [:philikon]]

Note that score and back-from-idle triggered syncs only happen when there are at least 2 devices connected to the account. Single device users will not see this.

Comment 9

[Tracy Walker [:tracy]]

score triggers and back-from-idle was ignored during back-off timer.

restart sync happened as expected, because the nextSync timer is not persisted across sessions.

Comment 10

[Philipp von Weitershausen [:philikon]]

Comment on attachment 564749 [details] [diff] [review]
v2

Requesting approval for Aurora. This patch will help us further regulate load on the Sync infrastructure as it makes more sync triggers aware of the backoff setting. Patch is already verified by Services QA.

Comment 11

[Philipp von Weitershausen [:philikon]]

Attachment: patch for beta and release

Requesting approval for Beta and Release. Please see comment 10 for justification.

Comment 12

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/2fc5fa5a744d

Comment 13

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ac11059899af

Comment 14

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/releases/mozilla-beta/rev/8f6cce824102

Comment 15

[Philipp von Weitershausen [:philikon]]

Thanks for the approvals, Christian and Asa. This bug depends on bug 691612 (was incorrectly marked as a blocker, sorry), so it won't make sense to land this without landing that first.

Comment 16

[u279076]

qa- as there does not appear to be much QA can do to verify this fix in Firefox.

Comment 17

[Philipp von Weitershausen [:philikon]]

(In reply to Anthony Hughes, Mozilla QA (irc: ashughes) from comment #16)
> qa- as there does not appear to be much QA can do to verify this fix in
> Firefox.

That's not true. Services Ops can easily configure the staging servers to send backoff notices. Perhaps the folks from Services QA can chime in here and help out with verifying.

This fix is absolutely vital for the health of our Sync server infrastructure. We need to make sure it works in the released versions of Firefox.

Comment 18

[u279076]

Thanks for clarifying. Feel free to set qa+ in the future when a qa- can be proven wrong.

Comment 19

[James Bonacci [:jbonacci]]

So, yes. Tracy and I can work with petef to get this verified from the server side.

Comment 20

[Philipp von Weitershausen [:philikon]]

(In reply to James Bonacci [:jbonacci] from comment #19)
> So, yes. Tracy and I can work with petef to get this verified from the
> server side.

There is no server side to this in terms of code. petef can help you set the backoff header on stage, but the code that needs verifying is all client side. Tracy verified it for the s-c hand-off already, but we need to ensure it's fixed in Aurora and Beta as well.

Comment 21

[Tracy Walker [:tracy]]

this was verified in beta, when beta was 8.  Shortly, I'll be verifying on current beta, which is 9.  status-firefoxX: "verified" will be very useful.

Comment 22

[u279076]

Tracy or James, can one of you confirm if this is fixed in Firefox 10? I see it's been verified on Firefox 8 and 9 already.

Comment 23

[Tracy Walker [:tracy]]

yes, it's good on 10

Comment 24

[u279076]

Thanks Tracy.