Closed Bug 1425152 Opened 2 years ago Closed 2 years ago

Persistent engine failure prevents last-sync timestamp from updating

Categories

(Firefox :: Sync, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 60
Tracking Status
firefox60 --- fixed

People

(Reporter: markh, Assigned: tcsc)

References

Details

Attachments

(1 file)

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: formatResponse@kinto-http-client.js:2377:21 < processResponse@kinto-http-client.js: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: formatResponse@kinto-http-client.js:2377:21 < processResponse@kinto-http-client.js: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
Flags: needinfo?(markh)
There doesn't seem to be any issue WRT the "prolonged error state", but this issue does prevent lastSync from updating.
Flags: needinfo?(markh)
Summary: Persistent engine failure prevents last-sync timestamp from updating and may cause us to enter a "prolonged error" state → Persistent engine failure prevents last-sync timestamp from updating
Priority: -- → P3
Re-nominating this for triage as I think the fix is easy and the potential for user confusion is large.
Priority: P3 → --
Priority: -- → P2
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)
Duplicate of this bug: 1435449
Assignee: nobody → tchiovoloni
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 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)
Attachment #8952572 - Flags: review?(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!
Attachment #8952572 - Flags: review?(markh) → review+
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/017d83ffe0a4
Update lastSync timestamp even if some sync engines failed r=markh
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
Flags: needinfo?(tchiovoloni)
It turns out this was actually broken by bug 1441666, and not this one.
Flags: needinfo?(tchiovoloni)
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/640e1784f2db
Update lastSync timestamp even if some sync engines failed r=markh
https://hg.mozilla.org/mozilla-central/rev/640e1784f2db
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 60
You need to log in before you can comment on or make changes to this bug.