629463 - Follow on: take action on server records that cause local failures

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Richard Newman [:rnewman]]

Follow on from Bug 622762, to decide if and when to re-upload a server record that we're having trouble processing.

Comment 1

[Richard Newman [:rnewman]]

... and to decide (probably on a per-engine basis) whether to delete the server record directly. That's what we ought to do for client records with HMAC mismatches, at least.

Comment 2

[Mike Connor [:mconnor]]

We should at least delete broken Client records, or add similar exception-eating logic there.  But really, busted client records should be nuked for sure.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Proposed patch. v1

Tests and crossweave pass. Uses existing HMAC error call! Hooray!

Comment 4

[Philipp von Weitershausen [:philikon]]

Comment on attachment 515965 [details] [diff] [review]
Proposed patch. v1

At first I thought I wasn't going to find anything to criticize, but then I did and the world made sense again!

>+    _("Records now: " + clientsColl.get({}));
>+    _("Change our keys and our client ID, reupload keys.");
>+    Clients.localID = Utils.makeGUID();
>+    CollectionKeys.generateNewKeys();
>+    let serverKeys = CollectionKeys.asWBO("crypto", "keys");
>+    serverKeys.encrypt(Weave.Service.syncKeyBundle);
>+    do_check_true(serverKeys.upload(Weave.Service.cryptoKeysURL).success);
>+
>+    _("Sync.");
>+    do_check_true(!deleted);
>+    Clients._resetClient();

Why call _resetClient instead of resetClient here? Also (small nit), this line should probably go next to the reassignment of Clients.localID.

Comment 5

[Richard Newman [:rnewman]]

Comment on attachment 515965 [details] [diff] [review]
Proposed patch. v1

mconnor, a?

This allows users to break out of the "constant errors as a result of a bad client record" loop.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 515965 [details] [diff] [review]
Proposed patch. v1

a=beltzner

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 515965 [details] [diff] [review]
Proposed patch. v1

Phil r-'d but forgot to change the flag; that changes my decision!

Comment 8

[Philipp von Weitershausen [:philikon]]

(In reply to comment #7)
> Phil r-'d but forgot to change the flag; that changes my decision!

Huh?!? I r+'d!

Comment 9

[Philipp von Weitershausen [:philikon]]

(My comments were just nits... I was being a bit sarcastic in the first line, sorry if that was misleading. I was commenting on the fact that there's nothing wrong with the patch which can't obviously be! See I did it again...)

Comment 10

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 515965 [details] [diff] [review]
Proposed patch. v1

a=beltzner

Comment 11

[Richard Newman [:rnewman]]

Pushed to fx-sync:

http://hg.mozilla.org/services/fx-sync/rev/5622ff3299b4

Comment 12

[Philipp von Weitershausen [:philikon]]

Merged to s-c: http://hg.mozilla.org/services/services-central/rev/76784798d9a7

Comment 13

[Philipp von Weitershausen [:philikon]]

Merged to m-c: http://hg.mozilla.org/mozilla-central/rev/76784798d9a7