1425152 - Persistent engine failure prevents last-sync timestamp from updating

Status: RESOLVED FIXED

(Firefox :: Sync, enhancement, P2)

Description

[Mark Hammond [:markh]]

Via bug 1425019, if an engine continuously fails, the last-sync timestamp doesn't update and we may enter a "prolonged error" state, which may restrict how often we sync.

While that bug is extension storage, I believe that's likely to be true for any constant engine failure (eg, "Services.logins is undefined" password failures, etc). I believe we should avoid those states in this case.

From the logs in that bug:
1513198015293	Sync.Engine.Extension-Storage	ERROR	Syncing {73a6fe31-595d-460b-a920-fcc0f8843232}: request failed: Error: HTTP 507; Error: HTTP 507 Insufficient Storage: Resource access is forbidden for this user (Maximum bytes per object exceeded (16507 > 16384 Bytes.) (resource://services-common/kinto-http-client.js:2354:21) JS Stack trace: [e-mail]:2377:21 < [e-mail]:2352:14
1513198015293	Sync.Engine.Extension-Storage	WARN	Syncing failed: Error: HTTP 507; Error: HTTP 507 Insufficient Storage: Resource access is forbidden for this user (Maximum bytes per object exceeded (16507 > 16384 Bytes.) (resource://services-common/kinto-http-client.js:2354:21) JS Stack trace: [e-mail]:2377:21 < [e-mail]:2352:14
1513198015294	Sync.Status	DEBUG	Status for engine extension-storage: error.engine.reason.unknown_fail
1513198015294	Sync.Status	DEBUG	Status.service: success.status_ok => error.sync.failed_partial
...1513198015297	Sync.ErrorHandler	ERROR	Some engines did not sync correctly.
1513198015297	Sync.Status	DEBUG	Status.sync: success.sync => error.sync.prolonged_failure
1513198015297	Sync.Status	DEBUG	Status.service: error.sync.failed_partial => error.sync.failed
1513198015297	Sync.Telemetry	TRACE	observed weave:service:sync:finish

Comment 1

[Mark Hammond [:markh]]

There doesn't seem to be any issue WRT the "prolonged error state", but this issue does prevent lastSync from updating.

Comment 2

[Mark Hammond [:markh]]

Re-nominating this for triage as I think the fix is easy and the potential for user confusion is large.

Comment 3

[Mark Hammond [:markh]]

A subtlety here will be that we don't want "total failure" to update the timestamp - so I think we'll want update on error only if the status is SYNC_FAILED_PARTIAL (or something like that)

Comment 5

[Thom Chiovoloni [:tcsc] (please ni?)]

Attachment: Bug 1425152 - Update lastSync timestamp even if some sync engines failed

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

Comment 6

[Thom Chiovoloni [:tcsc] (please ni?)]

Comment on attachment 8952572 [details]
Bug 1425152 - Update lastSync timestamp even if some sync engines failed

https://reviewboard.mozilla.org/r/221828/#review227696

::: services/sync/tests/unit/test_errorhandler_2.js
(Diff revision 1)
>    Svc.Obs.remove("weave:ui:login:error", onUIUpdate);
>    await clean();
>    await promiseStopServer(server);
>  });
>  
> -add_task(async function test_sync_prolonged_server_maintenance_error() {

This test reports SERVER_MAINTENANCE and not PROLONGED_SYNC_FAILURE now, which seems like an improvement.

Comment 7

[Mark Hammond [:markh]]

Comment on attachment 8952572 [details]
Bug 1425152 - Update lastSync timestamp even if some sync engines failed

https://reviewboard.mozilla.org/r/221828/#review228074

LGTM, although I think we need to ensure we have test coverage for lastSync not updating on "massive" failure (which might already exist) and also for the new functionality (ie, that a single engine failure does cause it to bump)

Comment 8

[Thom Chiovoloni [:tcsc] (please ni?)]

Comment on attachment 8952572 [details]
Bug 1425152 - Update lastSync timestamp even if some sync engines failed

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/221828/diff/1-2/

Comment 9

[Mark Hammond [:markh]]

Comment on attachment 8952572 [details]
Bug 1425152 - Update lastSync timestamp even if some sync engines failed

https://reviewboard.mozilla.org/r/221828/#review228504

Thanks!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/017d83ffe0a4
Update lastSync timestamp even if some sync engines failed r=markh

Comment 11

[Dorel Luca [:dluca]]

Backed out changeset 017d83ffe0a4 (bug 1425152) for xpcshell failure on browser/extensions/formautofill/test/unit/test_sync.js on a CLOSED TREE

Log of the failure:
https://treeherder.mozilla.org/logviewer.html#?job_id=165329929&repo=autoland&lineNumber=2373

Backout:
https://hg.mozilla.org/integration/autoland/rev/1f32df80a818f1bc5e03ecbec5d1425c4412a59c

Push that got backout: 
https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=017d83ffe0a47a22d342ab61224ca09977a308a5

Comment 12

[Thom Chiovoloni [:tcsc] (please ni?)]

It turns out this was actually broken by bug 1441666, and not this one.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/640e1784f2db
Update lastSync timestamp even if some sync engines failed r=markh

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/640e1784f2db