Bug 582083 - Should inspect POST responses for failed WBOs [r=Mardak]

Make sure records that failed to upload continue to be marked in the tracker so that they'll be uploaded again in the next sync.

services/sync/modules/engines.js

@@ -639,6 +639,7 @@ SyncEngine.prototype = {
 
   // Upload outgoing records
   _uploadOutgoing: function SyncEngine__uploadOutgoing() {
+    let failed = {};
     let outnum = [i for (i in this._tracker.changedIDs)].length;
     if (outnum) {
       this._log.trace("Preparing " + outnum + " outgoing records");
@@ -662,6 +663,18 @@ SyncEngine.prototype = {
         if (modified > this.lastSync)
           this.lastSync = modified;
 
+        // Remember changed IDs and timestamp of failed items so we
+        // can mark them changed again.
+        let failed_ids = [];
+        for (let id in resp.obj.failed) {
+          failed[id] = this._tracker.changedIDs[id];
+          failed_ids.push(id);
+        }
+        if (failed_ids.length)
+          this._log.debug("Records that will be uploaded again because "
+                          + "the server couldn't store them: "
+                          + failed_ids.join(", "));
+
         up.clearRecords();
       });
 
@@ -690,6 +703,11 @@ SyncEngine.prototype = {
         doUpload(count >= MAX_UPLOAD_RECORDS ? "last batch" : "all");
     }
     this._tracker.clearChangedIDs();
+
+    // Mark failed WBOs as changed again so they are reuploaded next time.
+    for (let id in failed) {
+      this._tracker.addChangedID(id, failed[id]);
+    }
   },
 
   // Any cleanup necessary.

services/sync/tests/unit/head_http_server.js

@@ -153,7 +153,7 @@ ServerCollection.prototype = {
   post: function(input) {
     input = JSON.parse(input);
     let success = [];
-    let failed = [];
+    let failed = {};
 
     // This will count records where we have an existing ServerWBO
     // registered with us as successful and all other records as failed.
@@ -164,10 +164,11 @@ ServerCollection.prototype = {
         wbo.modified = Date.now() / 1000;
         success.push(record.id);
       } else {
-        failed.push(record.id);
+        failed[record.id] = "no wbo configured";
       }
     }
-    return {success: success,
+    return {modified: Date.now() / 1000,
+            success: success,
             failed: failed};
   },
 

services/sync/tests/unit/test_syncengine_sync.js

@@ -837,6 +837,65 @@ function test_uploadOutgoing_toEmptyServer() {
 }
 
 
+function test_uploadOutgoing_failed() {
+  _("SyncEngine._uploadOutgoing doesn't clear the tracker of objects that failed to upload.");
+
+  Svc.Prefs.set("clusterURL", "http://localhost:8080/");
+  Svc.Prefs.set("username", "foo");
+  let crypto_steam = new ServerWBO('steam');
+  let collection = new ServerCollection();
+  // We only define the "flying" WBO on the server, not the "scotsman"
+  // and "peppercorn" ones.
+  collection.wbos.flying = new ServerWBO('flying');
+
+  let server = sync_httpd_setup({
+      "/1.0/foo/storage/crypto/steam": crypto_steam.handler(),
+      "/1.0/foo/storage/steam": collection.handler()
+  });
+  createAndUploadKeypair();
+  createAndUploadSymKey("http://localhost:8080/1.0/foo/storage/crypto/steam");
+
+  let engine = makeSteamEngine();
+  engine._store.items = {flying: "LNER Class A3 4472",
+                         scotsman: "Flying Scotsman",
+                         peppercorn: "Peppercorn Class"};
+  // Mark one of these records as changed 
+  const FLYING_CHANGED = 12345;
+  const SCOTSMAN_CHANGED = 23456;
+  const PEPPERCORN_CHANGED = 34567;
+  engine._tracker.addChangedID('flying', FLYING_CHANGED);
+  engine._tracker.addChangedID('scotsman', SCOTSMAN_CHANGED);
+  engine._tracker.addChangedID('peppercorn', PEPPERCORN_CHANGED);
+
+  try {
+
+    // Confirm initial environment
+    do_check_eq(collection.wbos.flying.payload, undefined);
+    do_check_eq(engine._tracker.changedIDs['flying'], FLYING_CHANGED);
+    do_check_eq(engine._tracker.changedIDs['scotsman'], SCOTSMAN_CHANGED);
+    do_check_eq(engine._tracker.changedIDs['peppercorn'], PEPPERCORN_CHANGED);
+
+    engine._uploadOutgoing();
+
+    // Ensure the 'flying' record has been uploaded and is no longer marked.
+    do_check_true(!!collection.wbos.flying.payload);
+    do_check_eq(engine._tracker.changedIDs['flying'], undefined);
+
+    // The 'scotsman' and 'peppercorn' records couldn't be uploaded so
+    // they weren't cleared from the tracker.
+    do_check_eq(engine._tracker.changedIDs['scotsman'], SCOTSMAN_CHANGED);
+    do_check_eq(engine._tracker.changedIDs['peppercorn'], PEPPERCORN_CHANGED);
+
+  } finally {
+    server.stop(function() {});
+    Svc.Prefs.resetBranch("");
+    Records.clearCache();
+    CryptoMetas.clearCache();
+    syncTesting = new SyncTestingInfrastructure(makeSteamEngine);
+  }
+}
+
+
 function test_uploadOutgoing_MAX_UPLOAD_RECORDS() {
   _("SyncEngine._uploadOutgoing uploads in batches of MAX_UPLOAD_RECORDS");
 
@@ -1034,6 +1093,7 @@ function run_test() {
   test_processIncoming_reconcile();
   test_processIncoming_fetchNum();
   test_uploadOutgoing_toEmptyServer();
+  test_uploadOutgoing_failed();
   test_uploadOutgoing_MAX_UPLOAD_RECORDS();
   test_syncFinish_noDelete();
   test_syncFinish_deleteByIds();