From 03c1f2d175f875762b7a50fdf4b49af0d6f9ee24 Mon Sep 17 00:00:00 2001 From: Alec Gibson <12036746+alecgibson@users.noreply.github.com> Date: Fri, 31 Jul 2026 11:58:59 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=90=9B=20Restore=20the=20op=20before=20fi?= =?UTF-8?q?xups=20when=20retrying=20a=20submit?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes https://github.com/share/sharedb/issues/716 At the moment, `$fixup()` composes its op permanently into `op.op` (or `op.create.data`), but the `apply` middleware is re-triggered on every submit retry. `apply()` resets the fixup *bookkeeping* on each attempt, while leaving `op.op` carrying the abandoned attempt's fixup, and nothing restores it. This causes two problems for a submit that loses a commit race. First, `op.op` accumulates fixups: with N retries the fixup lands N+1 times. Second, the ack under-reports what was committed, since `Agent` only sends the surviving attempt's `_fixupOps`. So we can commit `na: 2` while telling the client `na: 1` — a client/server divergence at the same version, produced entirely by our own bookkeeping. It happens even for a middleware whose fixup is a pure function of the request; existing `$fixup` users tend to write idempotent metadata, which masks it. Restoring at the top of `apply()` isn't enough, because `_transformOp` legitimately mutates `op.op` between attempts, and we want the *original* op transformed forward, not the fixed-up one. So this change stashes the op as it was before the attempt's first fixup, and resets all of the fixup state — the bookkeeping, and the op itself — in `retry()` before re-submitting. The middleware then recomputes its fixup against the newer snapshot, and `_fixupOps` matches what we actually commit. The stash is taken lazily in `$fixup()` rather than unconditionally in `apply()`, so submits that don't fix anything up don't pay to clone their op. Cloning is necessary because OT types make no guarantees about mutating their inputs. It's wrapped in an object so that its own truthiness tells us whether this attempt has stashed yet: the stashed value can legitimately be falsy, since `checkOp()` only requires that `op.op` is present, and `op.create.data` is whatever the type's `create()` returned. Note the `delete op.m.fixup` is *not* made redundant by this: `commit()` only sets `m.fixup` when `_fixupOps` is non-empty, so an attempt that fixes nothing up would otherwise commit an op still carrying the previous attempt's `m.fixup`. Resetting in `retry()` rather than `apply()` also covers the retry that finds our own op already committed, since that bails out during transform without ever reaching `apply()`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- docs/middleware/op-submission.md | 6 +++ lib/submit-request.js | 21 ++++++++- test/middleware.js | 73 ++++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 2 deletions(-) diff --git a/docs/middleware/op-submission.md b/docs/middleware/op-submission.md index c09971ed0..10f6e2b0b 100644 --- a/docs/middleware/op-submission.md +++ b/docs/middleware/op-submission.md @@ -65,6 +65,9 @@ backend.use('apply', (context, next) => { }) ``` +{: .warn :} +The `'apply'` hook may be triggered more than once for a single submission. If another client wins the race to commit, the op is transformed over the winning op and applied again to the newer snapshot, re-triggering the hook. Be careful, therefore, with any side effects that assume the hook only runs once per op. + ### Commit The [`commit`]({{ site.baseurl }}{% link middleware/actions.md %}#commit) hook is triggered after the op has been applied to the snapshot in memory, and both the op and snapshot are about to be written to the database. @@ -158,6 +161,9 @@ backend.use('apply', (request, next) => { {: .warn :} The `request.$fixup()` method may throw an error, which should be handled appropriately, usually by passing directly to the `next()` callback. +{: .info :} +Since the [`'apply'`](#apply) hook can be triggered more than once for a single submission, `request.$fixup()` may also be called more than once. Fixups from an abandoned attempt are discarded along with it, and the hook is expected to fix the op up again against the newer snapshot. Only the fixups from the attempt that is actually committed are sent to the client. + ## Comparing old snapshot version with new version Frequently, it becomes necessary to verify the changes made. This can be accomplished by leveraging two hooks, `apply` and `commit`, and creating a snapshot clone within the `apply` hook. diff --git a/lib/submit-request.js b/lib/submit-request.js index 8e6715123..d2cac8d99 100644 --- a/lib/submit-request.js +++ b/lib/submit-request.js @@ -3,6 +3,7 @@ var projections = require('./projections'); var ShareDBError = require('./error'); var types = require('./types'); var protocol = require('./protocol'); +var util = require('./util'); var ERROR_CODE = ShareDBError.CODES; @@ -43,6 +44,7 @@ function SubmitRequest(backend, agent, index, id, op, options) { this.ops = []; this.channels = null; this._fixupOps = []; + this._opBeforeFixups = null; } module.exports = SubmitRequest; @@ -70,6 +72,12 @@ SubmitRequest.prototype.$fixup = function(op) { ); } + if (!this._opBeforeFixups) { + this._opBeforeFixups = this.op.create ? + {data: util.clone(this.op.create.data)} : + {op: util.clone(this.op.op)}; + } + if (this.op.create) this.op.create.data = type.apply(this.op.create.data, op); else this.op.op = type.compose(this.op.op, op); @@ -186,8 +194,6 @@ SubmitRequest.prototype.apply = function(callback) { // Always set the channels before each attempt to apply. If the channels are // modified in a middleware and we retry, we want to reset to a new array this.channels = this.backend.getChannels(this.collection, this.id); - this._fixupOps = []; - delete this.op.m.fixup; var request = this; this.backend.trigger(this.backend.MIDDLEWARE_ACTIONS.apply, this.agent, this, function(err) { @@ -254,10 +260,21 @@ SubmitRequest.prototype.retry = function(callback) { if (this.maxRetries != null && this.retries > this.maxRetries) { return callback(this.maxRetriesError()); } + this._resetFixups(); this.backend.emit('timing', 'submit.retry', Date.now() - this.start, this); this.submit(callback); }; +// The fixups must be undone before the op is transformed forward again +SubmitRequest.prototype._resetFixups = function() { + this._fixupOps = []; + delete this.op.m.fixup; + if (!this._opBeforeFixups) return; + if (this.op.create) this.op.create.data = this._opBeforeFixups.data; + else this.op.op = this._opBeforeFixups.op; + this._opBeforeFixups = null; +}; + SubmitRequest.prototype._transformOp = function(ops) { var type = this.snapshot.type; for (var i = 0; i < ops.length; i++) { diff --git a/test/middleware.js b/test/middleware.js index 66e50b2fa..61dd17ffc 100644 --- a/test/middleware.js +++ b/test/middleware.js @@ -702,6 +702,79 @@ describe('middleware', function() { }); }); + it('applies the fixup once when the submit is retried', function(done) { + var remoteDoc = backend.connect().get('dogs', 'fido'); + + doc.submitOp([{p: ['fixups'], oi: 0}], function(error) { + if (error) return done(error); + remoteDoc.fetch(function(error) { + if (error) return done(error); + + backend.use('apply', function(request, next) { + request.$fixup([{p: ['fixups'], na: 1}]); + next(); + }); + + var commitDoc; + var commitRemoteDoc; + backend.use('commit', function(request, next) { + // Once we've forced the race, let the retry through untouched + if (commitDoc && commitRemoteDoc) return next(); + if (request.op.src === doc.connection.id) commitDoc = next; + else commitRemoteDoc = next; + // Both ops have now been applied to the same snapshot, so + // committing one of them will force the other to retry + if (commitDoc && commitRemoteDoc) commitDoc(); + }); + + doc.submitOp([{p: ['name', 0], si: 'a'}], function(error) { + if (error) return done(error); + commitRemoteDoc(); + }); + + remoteDoc.submitOp([{p: ['name', 0], si: 'b'}], function(error) { + if (error) return done(error); + backend.db.getSnapshot('dogs', 'fido', null, null, function(error, snapshot) { + if (error) return done(error); + expect(snapshot.data.fixups).to.equal(2); + expect(snapshot.data).to.eql(remoteDoc.data); + expect(snapshot.v).to.equal(remoteDoc.version); + done(); + }); + }); + }); + }); + }); + + it('applies the fixup once when a create is retried', function(done) { + backend.use('apply', function(request, next) { + request.$fixup([{p: ['fixups'], na: 1}]); + next(); + }); + + // A create can only be retried if the database reports a failed commit + // without another op having landed, since a create that loses a genuine + // race is rejected during transform + var commit = backend.db.commit; + var lostRace = false; + sinon.stub(backend.db, 'commit').callsFake(function(collection, id, op, snapshot, options, callback) { + if (lostRace) return commit.call(this, collection, id, op, snapshot, options, callback); + lostRace = true; + process.nextTick(callback, null, false); + }); + + doc = connection.get('dogs', 'rover'); + doc.create({name: 'rover', fixups: 0}, function(error) { + if (error) return done(error); + backend.db.getSnapshot('dogs', 'rover', null, null, function(error, snapshot) { + if (error) return done(error); + expect(snapshot.data.fixups).to.equal(1); + expect(snapshot.data).to.eql(doc.data); + done(); + }); + }); + }); + it('applies two fixups', function(done) { backend.use('apply', function(request, next) { request.$fixup([{p: ['tricks', 0], li: 'sit'}]);