SyncScheduler should obey backoffInterval at all times



Cloud Services
Firefox Sync: Backend
6 years ago
6 years ago


(Reporter: philikon, Assigned: philikon)


({verified-aurora, verified-beta})

verified-aurora, verified-beta
Dependency tree / graph

Firefox Tracking Flags

(firefox7 affected, firefox8 verified, firefox9 verified, firefox10 verified)


(Whiteboard: [verified in services][qa!])


(2 attachments, 1 obsolete attachment)

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.)


6 years ago
Summary: SyncScheduler should obey to backoffInterval at all times → SyncScheduler should obey backoffInterval at all times
Blocks: 692006
Created attachment 564743 [details] [diff] [review]

Make sure we always adhere to backoffInterval in scheduleNextSync() and in the back-from-idle observer that triggers a sync.
Assignee: nobody → philipp
Attachment #564743 - Flags: review?(rnewman)
Comment on attachment 564743 [details] [diff] [review]

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 <= + requestedInterval);
> +  do_check_eq(SyncScheduler.syncTimer.delay, requestedInterval);
> +
> +  // Request a sync at the smallest possible number.


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

Hmm, US or international english?
Attachment #564743 - Flags: review?(rnewman) → review+
Severity: normal → critical
OS: Mac OS X → All
Hardware: x86 → All


6 years ago
status-firefox10: --- → affected
status-firefox7: --- → affected
status-firefox8: --- → affected
status-firefox9: --- → affected
Waiting for additional test fixes for altered logic.
Created attachment 564749 [details] [diff] [review]

Addressed nits and made two failing tests pass.
Attachment #564743 - Attachment is obsolete: true
Comment on attachment 564749 [details] [diff] [review]

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.

Attachment #564749 - Flags: review+
Whiteboard: [fixed in services]
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.
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.
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.
Whiteboard: [fixed in services] → [verified in services]
Comment on attachment 564749 [details] [diff] [review]

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.
Attachment #564749 - Flags: approval-mozilla-aurora?
Created attachment 565075 [details] [diff] [review]
patch for beta and release

Requesting approval for Beta and Release. Please see comment 10 for justification.
Attachment #565075 - Flags: approval-mozilla-release?
Attachment #565075 - Flags: approval-mozilla-beta?
Last Resolved: 6 years ago
status-firefox10: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla10


6 years ago
Attachment #564749 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+


6 years ago
Attachment #565075 - Flags: approval-mozilla-beta? → approval-mozilla-beta+


6 years ago
Attachment #565075 - Flags: approval-mozilla-release? → approval-mozilla-release+
status-firefox9: affected → fixed
status-firefox8: affected → fixed
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.
No longer blocks: 691612
Depends on: 691612


6 years ago
Depends on: 693413
qa- as there does not appear to be much QA can do to verify this fix in Firefox.
Whiteboard: [verified in services] → [verified in services][qa-]
(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.
Thanks for clarifying. Feel free to set qa+ in the future when a qa- can be proven wrong.
Whiteboard: [verified in services][qa-] → [verified in services][qa+]
So, yes. Tracy and I can work with petef to get this verified from the server side.
(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.


6 years ago
Keywords: verified-beta
Whiteboard: [verified in services][qa+] → [verified in services][qa+][qa!:9]
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.
Whiteboard: [verified in services][qa+][qa!:9] → [verified in services][qa+][qa!:8]


6 years ago
Keywords: verified-aurora
Whiteboard: [verified in services][qa+][qa!:8] → [verified in services][qa!:8][qa!:9]
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.
status-firefox8: fixed → verified
status-firefox9: fixed → verified
Whiteboard: [verified in services][qa!:8][qa!:9] → [verified in services][qa+][qa!:8][qa!:9]
yes, it's good on 10
status-firefox10: fixed → verified
Thanks Tracy.
Whiteboard: [verified in services][qa+][qa!:8][qa!:9] → [verified in services][qa!]
You need to log in before you can comment on or make changes to this bug.