Investigate m9ren sync data loss

[yuliswe]

  self.applyChanges = function (changes, newProjects, deletedProjects, updatedProjects, settings) {
    return self.database.transaction('rw', self.database.files, self.database.changelog, self.database.projects, self.database.settings, function () {
      Dexie.currentTransaction.on('abort', function(ev) {
        console.log("applyChanges transaction aborted", ev);
        throw ev.target.error;
      });
      newProjects.forEach(function (project) {
        self.newProject(project);
      });
      changes.forEach(function (change) {
        if (change.type === "deleteFile") {
          self.deleteFile(change.file.project, change.file.file, true);
        } else if (change.type === "editFile") {
          self.writeFile(change.file.project, change.file.file, change.contents, change.history, change.file.checksum);
        } else if (change.type === "newFile") {
          self.newFile(change.file.project, change.file.file, change.contents, undefined, undefined, change.file.checksum);
        } else {
          throw sprintf("applyChanges: unknown change %s!", change);
        }
      });
      deletedProjects.forEach(function (project) {
        self.deleteProject(project, true);
      });
      updatedProjects.forEach(function(project) {
        self.updateProject(project);
      });
      if(settings) {
        self.saveSettings(JSON.parse(settings));
      }
      self.database.changelog.clear();
    });
  };

https://github.com/cs136/seashell/blob/master/src/frontend/js/storage-service.js#L340 I don't think you should be calling changelog.clear() here. What if the sync has failed?

[kpalway]

The clear is the last thing that occurs in the transaction.. if we reach that line then the sync was successful. Unless there's something different about IndexedDB transactions that makes this not the case?

[yuliswe]

I think this was what happened to m9ren since edward couldn't find her work in the changelog. Chrome was throwing indexeddb error.

[kpalway]

This is the logically correct place to clear the changelog, though. It is possible that there is something wrong with the logic of the rest of the transaction, which is causing it to proceed as normal when something bad has occurred. If so, the solution is to fix that bug instead of just leaving the changelog un-cleared.

Was that the student who ran into the IndexedDB error that Ed submitted a fix for? Or someone else?

[yuliswe]

Of course we still need to clear the changlog. I mean maybe you want to clear the log here in .then(), when the transaction is successful. https://github.com/cs136/seashell/blob/master/src/frontend/js/websocket-service.js#L447 Just so that if other error happens we don't lose the data.

[yuliswe]

Or maybe even here, when syncAll is successful https://github.com/cs136/seashell/blob/master/src/frontend/js/websocket-service.js#L460

[kpalway]

Transactions are atomic, so if an error occurs all changes are rolled back. We want to make sure to clear the changelog within the transaction so we don't run into concurrency issues. Anything else opens us up to actually lose data due to when we clear the changelog.

[yuliswe]

It's my theory. What do you think that caused the changelog to be lost for Molan?

[kpalway]

The change log wasn't exactly "lost," it was cleared at the end of a sync transaction. This tells us that the transaction completed without any errors. There are a few other ways that data may have been lost.. maybe it was never added to the changelog, maybe there is a logic error in the sync code that caused a conflict to be resolved incorrectly without actually throwing an exception.. there are different possible points of failure in the code. I haven't looked through the logs that student sent.

I can guarantee that moving parts of what should be a single transaction into separate transactions will only introduce new bugs, not fix any. That's simply how databases work. Again: it's possible there's an issue with the syncing code that we need to fix, but moving the changelog.clear won't fix it.

[yuliswe]

Transactions are atomic, so if an error occurs all changes are rolled back. We want to make sure to clear the changelog within the transaction so we don't run into concurrency issues. Anything else opens us up to actually lose data due to when we clear the changelog.

but moving the changelog.clear won't fix it.

Yeah I agree.

The error message was "IPC message size exceeded". It most likely happened when trying to dump the entire table in chrome. Given that she had "lost 3 hours of work" in offline mode, I think it happened when dumping the changelog table. Since each entry had the entire file content and the total size is large enough, Chrome complained.

The reason then the changelog is gone is still puzzling. But I think according to the symptom, the changelogs should be in the table at that time.

[kpalway]

I merged Ed's pull request that addressed that issue earlier today.

[yuliswe]

Cool. Our implementation must be robust enough to handle unanticipated errors like this.

What happened next was that she wrote something new on the original data, then a successful sync just rewrote her previous work. This is a problem but it's probably a "won't fix" until we're using dexie protocol supported backend that recognizes revision.

Awesome. Otherwise I don't see there's any other problem with the code.