Closed Bug 691663 Opened 13 years ago Closed 13 years ago

SyncScheduler should obey backoffInterval at all times


(Firefox :: Sync, defect)

Not set



Tracking Status
firefox7 --- affected
firefox8 --- verified
firefox9 --- verified
firefox10 --- verified


(Reporter: philikon, Assigned: philikon)



(Keywords: verified-aurora, verified-beta, Whiteboard: [verified in services][qa!])


(2 files, 1 obsolete file)

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.)
Summary: SyncScheduler should obey to backoffInterval at all times → SyncScheduler should obey backoffInterval at all times
Blocks: 692006
Attached patch v1 (obsolete) — Splinter 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
Waiting for additional test fixes for altered logic.
Attached patch v2Splinter 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+
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?
Requesting approval for Beta and Release. Please see comment 10 for justification.
Attachment #565075 - Flags: approval-mozilla-release?
Attachment #565075 - Flags: approval-mozilla-beta?
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
Attachment #564749 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #565075 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #565075 - Flags: approval-mozilla-release? → approval-mozilla-release+
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
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.
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]
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.
Whiteboard: [verified in services][qa!:8][qa!:9] → [verified in services][qa+][qa!:8][qa!:9]
yes, it's good on 10
Thanks Tracy.
Whiteboard: [verified in services][qa+][qa!:8][qa!:9] → [verified in services][qa!]
Component: Firefox Sync: Backend → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.