Abort sync cycles when download step fails

Abort sync cycles when download step fails

This change causes the syncer to exit from its download then commit
function if the download fails. This helps prevent the creation of
server-side conflicts, which would be more likely to happen if a
download failed but the following commit succeeded.

The main changes are as follows:
- Rather than proceed to the next step when a download updates failure
is detected, the syncer will exit. This should cause the
SyncScheduler to schedule a retry at a later time. 
- The definition of a download updates failure is now based on the
return code of the download attempt, rather than checking the contents
of the (possible non-existent) returned protobuf. This makes the error
detection logic used here more closely match the logic used to decide
whether or not to schedule retries.

This implementation has a side-effect on configure sync cycles. The old
behaviour was to handle failures by applying whatever updates we had
downloaded at that point. The new behaviour will leave updates
unapplied if any error occurs. This better matches a nearby comment's
assertion which states that we should not attempt to apply updates until
we have downloaded all of them. I believe the author of that comment
would approve of this change.

This change moves around some of the ExtensionActivityMonitor logic.
It's important that we not take the data from the extensions acitivity
monitor at the start of the cycle. Restoring that data is handled in
the commit building and response code, which might not be executed if we
need to break out early. This also fixes issue 123270.

Most of the diffs for this change are concentrated in the unit tests:
- Expose more of the SyncScheduler to the SyncerTest test harness.
- Add functions to SyncerTest for testing specific types of sync jobs.
- Add some functions that allow us to better control failures in
MockConnectionManager.
- Added tests for configure job success and failure.
- Added tests for update then commit job success and failure.
- Removed some redundant tests.
- Removed unused test infrastructure.

This change allows the integration tests to expose a bug that was
introduced back in r118572.  That commit assumed it was OK to not
retry a job that failed due to a MIGRATION_DONE response from the
server.  That is incorrect, and this error has been fixed.

BUG=122033, 123270
TEST=sync_unit_tests, specifically:
SyncerTest.UpdateThenCommit,
SyncerTest.UpdateFailsThenDontCommit,
SyncerTest.ConfigureDownloadsTwoBatchesSuccess,
SyncerTest.ConfigureFailsDontApplyUpdates


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133754

[rlarocque, 2012-04-17 01:16:05]

This is the second attempt at http://codereview.chromium.org/10006046/.  Here's
why the original failed:

First of all, the integration tests were not run as part of the CQ.  That's an
issue that still needs to be investigated.

My change broke the integration tests because they exposed a bug introduced in
r118572 (http://codereview.chromium.org/9185031).  In that CL, I decided to make
a MIGRATION_DONE a non-retryable error.  A nudge that received such a response
would be dropped, regardless of whether or not it completed its task.  

The integration tests used to fail to detect this.  On receipt of a
MIGRATION_DONE response, the syncer would try to commit anyway.  When running
against the test server, it would succeed*.  This meant that we would have no
unsynced handles by the time that we needed to AwaitDataSynced(), even though we
dropped the NUDGE without retrying.

Once my patch was applied, the syncer would abort on receipt of a MIGRATION_DONE
response.  The syncer is not given the opportunity to commit.  Just as before,
the NUDGE would be dropped.  However, since we missed out on the chance to
commit, this meant that we would be stuck with unsynced handles and wait
indefinitely during AwaitDataSynced().

The solution is simple: MIGRATION_DONE errors should be considered a reason to
retry.  I've uploaded a copy of the current CL as patch set 6 in the old code
review in case you would like to see that change in context.  

Note: Running the 'AllTypesIndividually' test correctly takes a lot of time.  I
hit the 45 second timeout when running cold on my desktop.  I imagine that the
trybots and buildbots would be even more at risk of hitting this timeout.  It
wouldn't surprise me if some of the migration tests turn out to be flaky.  

* For all I know, this is the behaviour of the real server, too.  That's
something we should investigate as part of our goal of making sure that the real
server and test server are as similar as possible.

[rlarocque, 2012-04-17 18:18:24]

On 2012/04/17 01:16:05, rlarocque wrote:
> This is the second attempt at http://codereview.chromium.org/10006046/. 
Here's
> why the original failed:
> 
> First of all, the integration tests were not run as part of the CQ.  That's an
> issue that still needs to be investigated.
> 
> My change broke the integration tests because they exposed a bug introduced in
> r118572 (http://codereview.chromium.org/9185031).  In that CL, I decided to
make
> a MIGRATION_DONE a non-retryable error.  A nudge that received such a response
> would be dropped, regardless of whether or not it completed its task.  
> 
> The integration tests used to fail to detect this.  On receipt of a
> MIGRATION_DONE response, the syncer would try to commit anyway.  When running
> against the test server, it would succeed*.  This meant that we would have no
> unsynced handles by the time that we needed to AwaitDataSynced(), even though
we
> dropped the NUDGE without retrying.
> 
> Once my patch was applied, the syncer would abort on receipt of a
MIGRATION_DONE
> response.  The syncer is not given the opportunity to commit.  Just as before,
> the NUDGE would be dropped.  However, since we missed out on the chance to
> commit, this meant that we would be stuck with unsynced handles and wait
> indefinitely during AwaitDataSynced().
> 
> The solution is simple: MIGRATION_DONE errors should be considered a reason to
> retry.  I've uploaded a copy of the current CL as patch set 6 in the old code
> review in case you would like to see that change in context.  
> 
> Note: Running the 'AllTypesIndividually' test correctly takes a lot of time. 
I
> hit the 45 second timeout when running cold on my desktop.  I imagine that the
> trybots and buildbots would be even more at risk of hitting this timeout.  It
> wouldn't surprise me if some of the migration tests turn out to be flaky.  
> 
> * For all I know, this is the behaviour of the real server, too.  That's
> something we should investigate as part of our goal of making sure that the
real
> server and test server are as similar as possible.

I'm seeing slightly different trybot failures now.  Don't bother reviewing yet. 
I'll ping this list again when it's actually ready for commit.

[rlarocque, 2012-04-18 01:44:15]

On 2012/04/17 18:18:24, rlarocque wrote:
> On 2012/04/17 01:16:05, rlarocque wrote:
> > This is the second attempt at http://codereview.chromium.org/10006046/. 
> Here's
> > why the original failed:
> > 
> > First of all, the integration tests were not run as part of the CQ.  That's
an
> > issue that still needs to be investigated.
> > 
> > My change broke the integration tests because they exposed a bug introduced
in
> > r118572 (http://codereview.chromium.org/9185031).  In that CL, I decided to
> make
> > a MIGRATION_DONE a non-retryable error.  A nudge that received such a
response
> > would be dropped, regardless of whether or not it completed its task.  
> > 
> > The integration tests used to fail to detect this.  On receipt of a
> > MIGRATION_DONE response, the syncer would try to commit anyway.  When
running
> > against the test server, it would succeed*.  This meant that we would have
no
> > unsynced handles by the time that we needed to AwaitDataSynced(), even
though
> we
> > dropped the NUDGE without retrying.
> > 
> > Once my patch was applied, the syncer would abort on receipt of a
> MIGRATION_DONE
> > response.  The syncer is not given the opportunity to commit.  Just as
before,
> > the NUDGE would be dropped.  However, since we missed out on the chance to
> > commit, this meant that we would be stuck with unsynced handles and wait
> > indefinitely during AwaitDataSynced().
> > 
> > The solution is simple: MIGRATION_DONE errors should be considered a reason
to
> > retry.  I've uploaded a copy of the current CL as patch set 6 in the old
code
> > review in case you would like to see that change in context.  
> > 
> > Note: Running the 'AllTypesIndividually' test correctly takes a lot of time.

> I
> > hit the 45 second timeout when running cold on my desktop.  I imagine that
the
> > trybots and buildbots would be even more at risk of hitting this timeout. 
It
> > wouldn't surprise me if some of the migration tests turn out to be flaky.  
> > 
> > * For all I know, this is the behaviour of the real server, too.  That's
> > something we should investigate as part of our goal of making sure that the
> real
> > server and test server are as similar as possible.
> 
> I'm seeing slightly different trybot failures now.  Don't bother reviewing
yet. 
> I'll ping this list again when it's actually ready for commit.

It turns out the previous approach was a dead-end.  The details can be found in
crbug.com/122033, which is probably a WontFix.  This patch is an alternative
solution.  

Instead of trying to handle all the problems created by treating MIGRATION_DONE
errors as errors, we will instead try to emulate the old behaviour of continuing
as if nothing had happened when we receive a MIGRATION_DONE response.  This
doesn't solve part (2) of Issue 123954, but it seems to be enough to make the
migration tests pass.  

I still need to run a few more tests and remove a FIXME before submitting this,
so this is not quite ready for a review yet.

[rlarocque, 2012-04-18 01:46:10]

> It turns out the previous approach was a dead-end.  The details can be found
in
> crbug.com/122033, which is probably a WontFix.  This patch is an alternative
> solution.  

Correction: crbug.com/122033 -> crbug.com/123954.

[rlarocque, 2012-04-19 23:05:03]

On 2012/04/18 01:46:10, rlarocque wrote:
> > It turns out the previous approach was a dead-end.  The details can be found
> in
> > crbug.com/122033, which is probably a WontFix.  This patch is an alternative
> > solution.  
> 
> Correction: crbug.com/122033 -> crbug.com/123954.

I've decided that fixing migration (or migration tests) is too big a challenge
to take on at this point.  The work-around is still our best bet to land this
change, which fixes a higher priority issue than any migration-related bugs.  

Please review.  I'll send out additional information offline to highlight some
of the more significant changes since the previous attempt.

[tim (not reviewing), 2012-04-20 16:41:07]

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer.cc#newc...
sync/engine/syncer.cc:163: session->status_controller().updates_response();
You're doing this just to get changes_remaining, right? StoreTimestamps already
process this field and stores it in the status controller right now. Syncer
shouldn't be privy to full raw proto message types.

I'm surprised you didn't have to add an include for ClientToServerResponse, but
it looks like it's because of ProcessClientCommand still living in here.

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer.cc#newc...
sync/engine/syncer.cc:173: // The MIGRATION_DONE case is special.  The migration
test code relies
As far as I can tell it is actually handled naturally by the block above, since
changes_remaining *must* be zero if the server sends migration done. If it's not
we should set it to zero, because whatever the number is it cannot reflect the
state in our progress markers by definition!  I may be missing something, but it
seems like we're making things more complicated with an unnecessary branch for
the same benefit we could get by simply tweaking your DCHECK on 166 to have an
|| for return_migration_done, alongside your nice comment.

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer.cc#newc...
sync/engine/syncer.cc:177: // attempt to commit will be counted as failures most
of the time,
I'm having a hard time groking this: "... we will count downloads that encounter
MD errors as non-failures, while jobs that do attempt to commit..." - something
feels apples-to-oranges about that statement. Can you reword it?

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer_unittes...
File sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer_unittes...
sync/engine/syncer_unittest.cc:3985: // It is the "happy path" alternative to
the test below.
nit - s/It is/This is, and also s/test below/UpdateFailsThenDontCommit below.
(Same with other tests)

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer_unittes...
sync/engine/syncer_unittest.cc:3986: TEST_F(SyncerTest, UpdateThenCommit) {
Probably not a bad idea to add a really dumb test that no updates returned from
the server works as expected, if we don't already have it.

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer_unittes...
sync/engine/syncer_unittest.cc:4016:
mock_server_->FailNextPostBufferToPathCall();
Would it make sense to write one of these tests with a specific failure type
(migration)?

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer_unittes...
sync/engine/syncer_unittest.cc:4064: TEST_F(SyncerTest,
ConfigureFailsDontApplyUpdates) {
Good test.

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer_unittes...
sync/engine/syncer_unittest.cc:4093: EXPECT_FALSE(n2.good());
Can we also check that initial_sync_ended is not set? I believe
AddUpdateDirectory does shove BookmarkSpecifics in there at some point, so you
can use bookmarks as the type.

http://codereview.chromium.org/10103017/diff/21001/sync/test/engine/mock_conn...
File sync/test/engine/mock_connection_manager.h (right):

http://codereview.chromium.org/10103017/diff/21001/sync/test/engine/mock_conn...
sync/test/engine/mock_connection_manager.h:129: class
ResponseCodeOverrideRequestor {
This isn't so related to your change except that you're adding different failure
mode controls, but it doesn't look like ResponseCodeOverrideRequestor and
associated control methods below are used anywhere.  Can you confirm and remove
so that we don't have a melting pot of these methods?

[rlarocque, 2012-04-23 20:16:48]

I'm flip-flopping back to the other approach.  Your comments about testing the
migration-specific workarounds made me realize that this current revision can't
be tested correctly.  I think that's unacceptable, so I'm going to try doing
this the right way.  

I landed a separate commit to try to fix migration-specific issues.  The next
revision of this patch will build on that work.

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer.cc
File sync/engine/syncer.cc (right):

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer.cc#newc...
sync/engine/syncer.cc:163: session->status_controller().updates_response();
On 2012/04/20 16:41:07, timsteele wrote:
> You're doing this just to get changes_remaining, right? StoreTimestamps
already
> process this field and stores it in the status controller right now. Syncer
> shouldn't be privy to full raw proto message types.
> 
> I'm surprised you didn't have to add an include for ClientToServerResponse,
but
> it looks like it's because of ProcessClientCommand still living in here. 

The next revision of this patch should avoid this issue.

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer.cc#newc...
sync/engine/syncer.cc:173: // The MIGRATION_DONE case is special.  The migration
test code relies
On 2012/04/20 16:41:07, timsteele wrote:
> As far as I can tell it is actually handled naturally by the block above,
since
> changes_remaining *must* be zero if the server sends migration done. If it's
not
> we should set it to zero, because whatever the number is it cannot reflect the
> state in our progress markers by definition!  I may be missing something, but
it
> seems like we're making things more complicated with an unnecessary branch for
> the same benefit we could get by simply tweaking your DCHECK on 166 to have an
> || for return_migration_done, alongside your nice comment.

Same as above.

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer.cc#newc...
sync/engine/syncer.cc:177: // attempt to commit will be counted as failures most
of the time,
On 2012/04/20 16:41:07, timsteele wrote:
> I'm having a hard time groking this: "... we will count downloads that
encounter
> MD errors as non-failures, while jobs that do attempt to commit..." -
something
> feels apples-to-oranges about that statement. Can you reword it? 

This will also be simplified in the next revision.

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer_unittes...
File sync/engine/syncer_unittest.cc (right):

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer_unittes...
sync/engine/syncer_unittest.cc:3985: // It is the "happy path" alternative to
the test below.
On 2012/04/20 16:41:07, timsteele wrote:
> nit - s/It is/This is, and also s/test below/UpdateFailsThenDontCommit below.
> (Same with other tests)
> 

Done.

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer_unittes...
sync/engine/syncer_unittest.cc:3986: TEST_F(SyncerTest, UpdateThenCommit) {
On 2012/04/20 16:41:07, timsteele wrote:
> Probably not a bad idea to add a really dumb test that no updates returned
from
> the server works as expected, if we don't already have it.

I think TestGetUnsyncedAndSimpleCommit handles that case.

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer_unittes...
sync/engine/syncer_unittest.cc:4016:
mock_server_->FailNextPostBufferToPathCall();
On 2012/04/20 16:41:07, timsteele wrote:
> Would it make sense to write one of these tests with a specific failure type
> (migration)?

Hopefully not.  The less special cases we have, the better.  

The next revision of this patch will should not require any migration-specific
testing.

http://codereview.chromium.org/10103017/diff/21001/sync/engine/syncer_unittes...
sync/engine/syncer_unittest.cc:4093: EXPECT_FALSE(n2.good());
On 2012/04/20 16:41:07, timsteele wrote:
> Can we also check that initial_sync_ended is not set? I believe
> AddUpdateDirectory does shove BookmarkSpecifics in there at some point, so you
> can use bookmarks as the type.

Good idea.  Done.

http://codereview.chromium.org/10103017/diff/21001/sync/test/engine/mock_conn...
File sync/test/engine/mock_connection_manager.h (right):

http://codereview.chromium.org/10103017/diff/21001/sync/test/engine/mock_conn...
sync/test/engine/mock_connection_manager.h:129: class
ResponseCodeOverrideRequestor {
On 2012/04/20 16:41:07, timsteele wrote:
> This isn't so related to your change except that you're adding different
failure
> mode controls, but it doesn't look like ResponseCodeOverrideRequestor and
> associated control methods below are used anywhere.  Can you confirm and
remove
> so that we don't have a melting pot of these methods?

Done.

[rlarocque, 2012-04-23 20:39:01]

Please review.

Here's the new patch.  It's similar to the one I tried to land at the beginning
of last week, but this time around it's being applied on top of r133498.  That
should be enough to fix the integration tests.  

I've also responded to Tim's comments on the unit tests.

[rlarocque, 2012-04-24 17:25:36]

Patch set 7 is meant to fix some integration test flakiness I've been seeing on
Windows.  That failure is unrelated to this patch.  I've seen it happen on other
CLs, too.  

I'm adding the workaround in this CL only because I don't think this change
would make it through the CQ otherwise.

[tim (not reviewing), 2012-04-24 17:39:55]

This patch looks great.  Nice work.

LGTM

[commit-bot: I haz the power, 2012-04-24 18:45:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rlarocque@chromium.org/10103017/41001

[commit-bot: I haz the power, 2012-04-24 20:29:09]

Change committed as 133754