We very cleverly track a numeric change count for every entry in Places. We decrement that count when changes have been uploaded. Any change during a sync, when we're not "tracking", results in a non-zero change counter at the end of a sync. We can narrow down the duration of potential inconsistency -- e.g., Bug 1313967 -- by spotting this situation and doing another sync. Naturally we should make sure we don't spin our wheels forever, but one or two follow-up bookmark syncs should be very cheap and potentially very useful.

That will also impact the validator - we run a validation directly after a sync, but we are almost guaranteed validation issues of those items exist after the sync.

Thom, I borrowed the `needsResync` concept from your `canSafelyValidate` implementation in bug 1317223; hope you don't mind. (It might even make sense to have both; I'm not sure). Instead of changing the signature of `pullChanges`, I added a separate read-only query that returns a COUNT, but I can replace that with `preventUpdate` if you'd prefer.

Comment on attachment 8836228 [details] Bug 1335891 - Skip validation and trigger another sync if tracked items have changed during the last sync. https://reviewboard.mozilla.org/r/111696/#review112974 ::: services/sync/modules/engines/bookmarks.js:538 (Diff revision 1) > } finally { > try { > this._deletePending(); > } finally { > // Reorder children. > + this._tracker.ignoreAll = true; Oops, I meant to do this in its own patch.

Comment on attachment 8836230 [details] Bug 1335891 - Partially revert bug 1299338; it's safe to ignore observer notifications with the new tracker. https://reviewboard.mozilla.org/r/111698/#review112992 It seems strange for this to be part of this bug, but I don't have an issue with the code.

Comment on attachment 8836228 [details] Bug 1335891 - Skip validation and trigger another sync if tracked items have changed during the last sync. https://reviewboard.mozilla.org/r/111696/#review112994 This mostly looks fine but I'm going to flag markh for review since I'd feel better if he took a look at it. ::: services/sync/modules/service.js:1101 (Diff revision 2) > /** > * Sync up engines with the server. > */ > _lockedSync: function _lockedSync(engineNamesToSync) { > return this._lock("service.js: sync", > this._notify("sync", "", function onNotify() { I'm unsure if things (e.g. telemetry) will be upset by getting the same engines syncing multiple times per, err, sync. seems possibel that it would be problematic late in the pipeline, e.g. will the redash queries be broken/confused? But I don't know for sure... Not opening an issue since this concern is pretty vague. ::: services/sync/modules/service.js:1123 (Diff revision 2) > - // wait() throws if the first argument is truthy, which is exactly what > + // wait() throws if the first argument is truthy, which is exactly what > - // we want. > + // we want. > - cb.wait(); > + engineNamesToResync = cb.wait(); > > - histogram = Services.telemetry.getHistogramById("WEAVE_COMPLETE_SUCCESS_COUNT"); > + histogram = Services.telemetry.getHistogramById("WEAVE_COMPLETE_SUCCESS_COUNT"); > - histogram.add(1); > + histogram.add(1); Do we want to increment this counter multiple times?

Comment on attachment 8836228 [details] Bug 1335891 - Skip validation and trigger another sync if tracked items have changed during the last sync. Sorry to add to your queue mark :p, changing the semantics here just concerns me a little, but maybe not that much.

Comment on attachment 8836228 [details] Bug 1335891 - Skip validation and trigger another sync if tracked items have changed during the last sync. https://reviewboard.mozilla.org/r/111696/#review112994 > I'm unsure if things (e.g. telemetry) will be upset by getting the same engines syncing multiple times per, err, sync. seems possibel that it would be problematic late in the pipeline, e.g. will the redash queries be broken/confused? > > But I don't know for sure... Not opening an issue since this concern is pretty vague. That's a good point. Mark, do you know? > Do we want to increment this counter multiple times? I think we do, since it's technically a separate sync. But you caught a bug where I didn't move `WEAVE_START_COUNT` into the loop body, so we recorded multiple `WEAVE_COMPLETE_SUCCESS_COUNT` per start.

Comment on attachment 8836228 [details] Bug 1335891 - Skip validation and trigger another sync if tracked items have changed during the last sync. https://reviewboard.mozilla.org/r/111696/#review113208 IIUC, the existing "global score" mechanism should be able to do this in a simpler way - ie, schedule a new sync at a shorter interval when it completes with the global score beyond the threshold. This means we need to be careful to ensure that sync itself doesn't bump its own global score (as we've seen in the past, and may still have for a couple of engines), but we are going to need to deal with that anyway once we extend this to other engines (which there's no reason not to do). That also has the benefit of treating it as a different sync, side-stepping the "multi-engine-syncs-per-ping" issue discussed above. If we do go that route we probably still want something like the limit on how often we will immediately sync, but I think that should be reasonably simple to implement. Is there a reason I'm missing why the approach above isn't as good as this?

(In reply to Mark Hammond [:markh] from comment #13) > IIUC, the existing "global score" mechanism should be able to do this in a > simpler way Thinking more about this, I'm fairly sure I stated an alternative fact ;) To be clear, the existing mechanism *can't* do it, but I think it can be taught. My assumptions: * bookmarks is the only engine that can differentiate changes made by sync vs changes made externally. * every other engine *should* ignore changes made while syncing, via stop/startTracking. We should ensure this is as granular as possible and that all engines do the right thing (ie, never actually increment their score while syncing - I think at least 1 gets this wrong) * the globalScore is reset to zero at the start of the sync. So, in theory: * bookmarks can continue to increment the change counter during syncs. * all other engines must be sure to only increment the change counter while they are "tracking" - they may miss some, but cest la vie. * the scheduler changes to handle the global score being past its limit at the end of a sync and reschedule a new one, with a counter to avoid this happening too often. To be safe in the face of this going wrong, I'd be inclined to say that the *first* time this gets hit we ignore the clue for the rest of the browser session (and maybe emit a telemetry event?) 3rd party engines are also a bit of a concern, but we could probably handle this if we care enough.

(In reply to Mark Hammond [:markh] from comment #13) > Is there a reason I'm missing why the approach above isn't as good as this? No reason, except I didn't think of that approach at all. :-) That makes sense, and generalizes well to the other engines. Sidestepping the duplicate engines per sync is a nice advantage. I am worried about the possibility of us scheduling extra syncs, since none of the other engines have granular source tracking, and `ignoreAll` is a (potentially incorrectly applied, as you note in comment 14) kludge. But that's an issue we'll have to address, anyway.

Comment on attachment 8836228 [details] Bug 1335891 - Skip validation and trigger another sync if tracked items have changed during the last sync. Kit agreed using the scheduler probably makes more sense.

Bug 1292693 is a similar one for the clients collection - depending on the implementation, this bug may also solve that.

Comment on attachment 8844171 [details] Bug 1335891 - Ensure and test that Sync changes don't bump the score. https://reviewboard.mozilla.org/r/117700/#review119464 ::: services/sync/modules/policies.js:361 (Diff revision 1) > } > > this._log.trace("Global score updated: " + this.globalScore); > + }, > + > + calculateScore: function calculateScore() { let's make this a "new style" function delc while we are here.

Comment on attachment 8844171 [details] Bug 1335891 - Ensure and test that Sync changes don't bump the score. https://reviewboard.mozilla.org/r/117700/#review119476 doh! I meant to say... Thanks - that looks like approach I had in mind. However, the forms and prefs engines look like they will always trigger a second sync whenever anything is applied - forms.js should only increment the score if addChangedID returns true, and prefs.js might need to reach into engine.ignoreAll to check if the score should be incremented or not. We should also test the other engines - on a new profile (but an existing account), set the log prefs such that the "trace" entries are seen (ie, services.sync.log.logger.service.main=Trace), sign in to sync, then examine the logs for any evidence of the score being incremented during a sync, just to make sure that all engines are not incrementing the score as it applied incoming records. Sadly, just disconnecting and reconnecting an existing profile will not trigger this, as, eg, the forms engine will consider all incoming records, but then not apply them as they are identical, and hence won't trigger the "increment during sync" behaviour. More generally, I'm a little concerned how often we hit this - eg, if a 3rd party engine triggers this regularly it would be a bad outcome, and I'm concerned we don't have any visibility into this happening. What do you think about a histogram that records that counter just before you reset it to zero? While we would expect a few cases where it's > 1, it should be rare we see it > 2, and seeing any significant number with 5 implies something bad is happening. I also think we should have a test for this logic in test_syncscheduler.

Comment on attachment 8844171 [details] Bug 1335891 - Ensure and test that Sync changes don't bump the score. Still working on the tests; this isn't ready for review yet.

Comment on attachment 8844171 [details] Bug 1335891 - Ensure and test that Sync changes don't bump the score. https://reviewboard.mozilla.org/r/117700/#review128406 I know you removed the review flag, but thought I should do a drive-by anyway :) Are you having specific problems with the tests, or is it just a matter of filling out the new test below? ::: services/sync/modules/engines/prefs.js:252 (Diff revision 2) > Services.prefs.removeObserver("", this); > }, > > observe(subject, topic, data) { > Tracker.prototype.observe.call(this, subject, topic, data); > + if (this.ignoreAll) { I'm not sure we should skip the profile-before-change notification if we happen to be syncing when we get it (which seems unlikely but possible?) ::: services/sync/modules/stages/enginesync.js:88 (Diff revision 2) > > // Convert the response to an object and read out the modified times > + let modifiedTimes = info.obj; > for (let engine of [this.service.clientsEngine].concat(engineManager.getAll())) { > - engine.lastModified = info.obj[engine.name] || 0; > + if (engine.name in modifiedTimes) { > + engine.lastModified = modifiedTimes[engine.name]; IIUC, this is a semantic change - engines not in modified times previously got 0, but here they are unchanged.

(In reply to Mark Hammond [:markh] from comment #25) > I know you removed the review flag, but thought I should do a drive-by > anyway :) Are you having specific problems with the tests, or is it just a > matter of filling out the new test below? I'm still puzzled why the new forms test is racy. I've found it's easiest to trigger if I pipe the xpcshell output to Sublime, or use `_` instead of `dump` for logging, but it's still intermittent. If I use `dump`, or pipe to Sublime: * After the first sync, the score is 0 (wrong; we definitely added the item during the sync), and no record on the server (right). * After the second sync, the score is 0 (right; syncing should've reset it), and the record is on the server. I think that's right: we added the record during the fetch, and we only ignore changes when we go to apply the downloaded records. If I run the test directly: * After the first sync, the score is 10 (right), and no record on the server (right). * After the second sync, the score is 0 (right), but still no record on the server (wrong. We bumped the score, and the tracker saw the change. Why didn't we upload it?) I wonder if triggering an immediate sync on next tick has something to do with this. > ::: services/sync/modules/stages/enginesync.js:88 > (Diff revision 2) > > > > // Convert the response to an object and read out the modified times > > + let modifiedTimes = info.obj; > > for (let engine of [this.service.clientsEngine].concat(engineManager.getAll())) { > > - engine.lastModified = info.obj[engine.name] || 0; > > + if (engine.name in modifiedTimes) { > > + engine.lastModified = modifiedTimes[engine.name]; > > IIUC, this is a semantic change - engines not in modified times previously > got 0, but here they are unchanged. It is a semantic change, but I'm not clear on the implications. Without this, though, none of the other engines after the first test will sync. I think it's because of https://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/services/sync/modules/engines.js#1235: before the first sync, `lastModified` is undefined for all engines. After the first sync, we set `lastModified = 0` for *all* engines, so `lastModified = 0` and `lastSync = 0` for all subsequent tests. (Since we're syncing those engines for the first time, too). Maybe the right approach is to sync via `Service.sync(["engineName"])`, instead of using the `sync_engine_and_validate_telem` helper. I haven't tried that yet.

Comment on attachment 8844171 [details] Bug 1335891 - Ensure and test that Sync changes don't bump the score. https://reviewboard.mozilla.org/r/117700/#review141388

Comment on attachment 8853202 [details] Bug 1335891 - Trigger another sync if tracked items have changed during the last sync. https://reviewboard.mozilla.org/r/125282/#review141392 Awesome, thanks Kit

Pushed by kcambridge@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d2c8f7dbb889 Trigger another sync if tracked items have changed during the last sync. r=markh https://hg.mozilla.org/integration/autoland/rev/662fd2e22b6b Ensure and test that Sync changes don't bump the score. r=markh

https://hg.mozilla.org/mozilla-central/rev/d2c8f7dbb889 https://hg.mozilla.org/mozilla-central/rev/662fd2e22b6b