Doc._opAcknowledged transforms the wrong pendingOps entry when handling fixup ops

[alecgibson]

Summary

Doc.prototype._opAcknowledged loops over this.pendingOps with j but indexes it with i, the fixup op index. As a result, when a client with queued pending ops receives fixup ops on an ack:

• pendingOps[0] is transformed once per pending op instead of once (and transformX mutates in place, so the transforms compound)
• every other pending op is never transformed at all
• if there are more fixup ops than pending ops, transformX is handed undefined and throws

Reproduced on master (lib/client/doc.js there is byte-identical to the published 6.0.0).

Root cause

https://github.com/share/sharedb/blob/master/lib/client/doc.js#L970-L977

if (message[ACTIONS.fixup]) {
  for (var i = 0; i < message[ACTIONS.fixup].length; i++) {
    var fixupOp = message[ACTIONS.fixup][i];

    for (var j = 0; j < this.pendingOps.length; j++) {
      var transformErr = transformX(this.pendingOps[i], fixupOp);   // <- i, should be j
      if (transformErr) return this._hardRollback(transformErr);
    }

transformX mutates both arguments in place:

https://github.com/share/sharedb/blob/master/lib/client/doc.js#L568-L576

var clientOp = client.type.transform(client.op, server.op, 'left');
var serverOp = client.type.transform(server.op, client.op, 'right');
client.op = clientOp;
server.op = serverOp;

so the repeated call on pendingOps[0] isn't idempotent.

The single-pending-op case is correct by coincidence, which is presumably why this has survived.

Repro

No dependencies beyond sharedb's own. Save as repro-fixup-pendingops.js in the repo root and node repro-fixup-pendingops.js:

const Backend = require('./lib/backend');
const json0 = require('ot-json0').type;

const COLLECTION = 'c';

function makeBackend(fixups) {
  const backend = new Backend();
  backend.use('apply', (request, next) => {
    if (!('op' in request.op)) return next();
    for (const fixup of fixups) request.$fixup(fixup);
    next();
  });
  return backend;
}

// Case 1: more fixup ops than pending ops -> this.pendingOps[i] is undefined -> TypeError.
function case1() {
  return new Promise((resolve) => {
    console.log('=== Case 1: 2 fixup ops on an op with 1 pending op behind it ===');
    const backend = makeBackend([[{p: ['a'], na: 1}], [{p: ['b'], na: 1}]]);
    const doc = backend.connect().get(COLLECTION, 'case1');
    doc.preventCompose = true;

    let timer;
    const done = (line) => {
      clearTimeout(timer);
      process.removeAllListeners('uncaughtException');
      console.log(line);
      resolve();
    };

    process.once('uncaughtException', (error) => done(
      `  TypeError: ${error.message}\n` +
      '  (transformX called with this.pendingOps[1] === undefined)'));

    doc.create({a: 0, b: 0, n: 0}, json0.uri, () => {
      doc.submitOp([{p: ['n'], na: 1}]); // inflight
      doc.submitOp([{p: ['n'], na: 1}]); // pendingOps[0]; there is no pendingOps[1]
      timer = setTimeout(() => done(`  no crash; doc.data = ${JSON.stringify(doc.data)}`), 300);
    });
  });
}

// Case 2: 1 fixup op, 2 pending ops -> pendingOps[0] is transformed twice (transformX
// mutates in place) and pendingOps[1] is never transformed. Client and server diverge.
function case2() {
  return new Promise((resolve) => {
    console.log('\n=== Case 2: 1 fixup op, 2 pending ops ===');
    const backend = makeBackend([[{p: ['list', 0], li: 'F'}]]);
    const doc = backend.connect().get(COLLECTION, 'case2');
    doc.preventCompose = true;

    doc.create({list: ['a', 'b', 'c']}, json0.uri, () => {
      doc.submitOp([{p: ['list', 3], li: 'inflight'}]);
      doc.submitOp([{p: ['list', 4], li: 'pending0'}]);
      doc.submitOp([{p: ['list', 5], li: 'pending1'}]);
      setTimeout(() => {
        backend.db.getSnapshot(COLLECTION, 'case2', null, null, (error, snapshot) => {
          const client = JSON.stringify(doc.data);
          const server = JSON.stringify(snapshot.data);
          console.log(`  client doc.data = ${client}`);
          console.log(`  server snapshot = ${server}`);
          console.log(`  converged? ${client === server}`);
          resolve();
        });
      }, 400);
    });
  });
}

case1().then(case2).then(() => process.exit(0));

Actual

=== Case 1: 2 fixup ops on an op with 1 pending op behind it ===
  TypeError: Cannot read properties of undefined (reading 'del')
  (transformX called with this.pendingOps[1] === undefined)

=== Case 2: 1 fixup op, 2 pending ops ===
  client doc.data = {"list":["F","F","F","a","b","c","inflight","pending0","pending1"]}
  server snapshot = {"list":["F","F","F","a","b","c","inflight","pending1","pending0"]}
  converged? false

(Three Fs is correct — the middleware fixups each of the three ops. The bug signal is converged? false: the client and the server disagree on the order of the two pending ops.)

Expected

No throw in case 1, and converged? true in case 2.

Reachability

_tryCompose collapses queued ops into one, so pendingOps.length > 1 needs one of:

• doc.preventCompose = true
• differing op.source values while connection.submitSource is set (_tryCompose bails on a source mismatch)
• a pending op that has already been flushed (last.sentAt set)

An offline client flushing a queue on reconnect is the natural shape for this, which is also when a server is most likely to want to fix up an incoming op.

Suggested fix

this.pendingOps[i] -> this.pendingOps[j]. Given how easy the two indices are to confuse here, renaming them to something like fixupIndex / pendingIndex would be worth doing at the same time.

Tests worth adding alongside: a doc with preventCompose and two pending ops receiving a single fixup op (asserting client/server convergence), and an op receiving two fixup ops with one pending op behind it (asserting no throw).

---

Found alongside #716 (also $fixup, but an independent defect with an independent fix).