1403022 - Abort RepositorySession on BatchingUploader failures

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Android Sync, enhancement, P1)

Description

[:Grisha Kruglov]

Split from Bug 1373254.

Comment 1

[:Grisha Kruglov]

- Carry over patch from the parent bug, "abort the session in case of uploader failures; this will shutdownNow our work queues".
- See if it's feasible to remove recordUploadFailed flag, and be smarter about how we process failures. See https://bugzilla.mozilla.org/show_bug.cgi?id=1373254#c13

Comment 2

[:Grisha Kruglov]

The main goal here is to ensure we're not doing a ton of unnecessary work in the unhappy cases - namely, when we fail to store a record.

Currently, we treat any remote store failure as a synchronization failure. However, since we don't abort the session, we continue on with the flow of records - which short-circuit in case we hit a 412, and fire off a series of onRecordStoreFailed callback in case we saw a record in the "failed" array.

However, currently _any_ store failure is considered a synchronization failure, and our current behaviour ensures that in case of a batching upload, we won't attempts to commit the batch under any circumstance if anything failed - be it a 412 or encountering a record in the "failed" array.

I don't see a strong reason to relax this behaviour. Instead, we should aim to do the least amount of work necessary once we determine that there has been a failure on the outer layers. That is, declare store as failed, abort the session.

We might consider relaxing this behaviour if we encounter a non-bookmark record in the "failed" array, since as far as data consistency goes, that situation is probably acceptable. I'll treat this as an optimization for the time being, and won't address this case here.

Comment 3

[:Grisha Kruglov]

Attachment: Bug 1403022 - Abort session on BatchingUploader failures

The main goal of these changes is to ensure we're not doing any unnecessary work
in the unahppy cases of BatchingUploader. We might fail in three general ways:
- encounter a 412 error
- encounter another type of HTTP error
- encounter a GUID in the "failed" array

Currently, in all of these cases, we de-facto abort the session, without performing
an actual abort. E.g. we won't commit a batch, we'll refuse to upload any still-flowing
records. This patch simplifies our unhappy-case behaviour: if something failed, actually
abort the session (triggering a shutdownNow of the work queues), declare store as failed, etc.

It's important to note that our "did the synchronization fail?" login in the SynchronizerSession
depends on the store failure counts, and so this patch maintains the "record failed to store"
delegate chain. However, these counts are largely meaningless. What does it mean to fail to store
50 records, if we abort on the 51st, and prevent the other 100 from flowing (and from being counted
as failed?).

This patch also fixes an omission in the verstion tracking logic:
- prior, if we encountered a record in the "failed" array, we'd continue on with the flow, won't upload
anything, mark the synchronization as failed, but we'd also call into 'onStoreCompleted' which will
trigger an update of syncVersion for outflowing records
- with this patch, we won't call into onStoreCompleted in the case above, and so won't update syncVersion
in case of such failures
- this is the correct behaviour for batching uploads (now enabled on all but one server), but possibly
non-optimal behaviour if batching isn't enabled. However, this behaviour should be safe from a data consistency
point of view regardless of the batching mode.

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

Comment 4

[Richard Newman [:rnewman]]

Comment on attachment 8912425 [details]
Bug 1403022 - Abort session on BatchingUploader failures

https://reviewboard.mozilla.org/r/183746/#review188952

Comment 5

[:Grisha Kruglov]

Passes the tests, appears to work locally. Let's continue testing on Nightly.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/984d5493a4b3
Abort session on BatchingUploader failures r=rnewman

Comment 7

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/984d5493a4b3