Repository navigation
✨ Add retry middleware action - #761
dawidreedsy wants to merge 3 commits into
Conversation
When a commit loses the race to another op, `SubmitRequest.retry()` re-runs `submit()` on the same request. That re-fetches the snapshot and resets fixups, but keeps any properties middleware set on the request during the failed attempt, so they can describe a stale snapshot. The new `retry` action is triggered before each retry, giving middleware a place to reset that state. An error passed to `next` rejects the op. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
dawidreedsy
left a comment
There was a problem hiding this comment.
🤖 PR panel
Roster: Correctness, Tests & conventions, Readability, Compatibility, Integration, Intent. This is an additive public API and a change to the submit lifecycle, and it is driven by a request in reedsy-sharedb#1687.
Verdict: Nothing blocks. The library change is correct, and with no 'retry' middleware registered the behaviour is identical (trigger calls back synchronously, and the timing-event order is unchanged). Three should items: the docs example doesn't justify the hook (F1), there is no test covering the hook's purpose (F2), and the PR body should say why this is an action rather than an event (F3).
Outside the diff
- F6 · Readability · nit:
docs/middleware/op-submission.md:70. The existing warning that'apply'may run more than once ends with "be careful" but gives no next step. Append: "Use the retry hook to reset any state stored on thecontextduring the failed attempt." - F9 · Intent · nit: PR body. The test plan says "New tests fail without the change", but only 3 of 5 do. The two negative tests (first-time commit, past
maxSubmitRetries) pass on the old code, becausebackend.useaccepts any action name. Reword to "the positive tests (ordering,retries, error) fail without the change".
Checked, not an issue
- The
retryplacement inMIDDLEWARE_ACTIONSwas challenged and dropped. The map isn't alphabetical (receivePresence/sendPresencecome afterreply). - Sync→async: with no middleware,
triggercalls back on the same tick, soretry()→submit()timing is unchanged. request.actionis briefly'retry', then reset bytrigger('apply').$fixupcan't misfire.- An error from
'retry'propagates through the same callback chain asapply/commiterrors (submitRequestEnd, client rollback). The callback runs once. retries++, the max-retries check and_resetFixups()all run before the hook. The PR body's claims check out.- Re-entrancy has the same shape as before.
triggercopies the middleware array. - Leaving
'retry'out of the "submit lifecycle" options test is correct, because a lone create can never retry. - Tests follow file conventions (
forceRetrymirrors the hold-both-commits technique in test/client/submit.js). Lint is clean, and 905 tests pass. - reedsy-sharedb can adopt the hook cleanly and drop its
order = -Infinityworkaround (consumer-side enum value plus typings; see F8).
- Explain why the example captures the snapshot first-write-wins - Note that snapshot and channels are still the failed attempt's in the hook - Say "retried" rather than "submitted again" - Point the 'apply' re-run warning at the retry hook Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
|
🤖 PR panel follow-up. F6 (outside the diff) was fixed in |
Summary
When a commit loses the race to another op,
SubmitRequest.retry()callssubmit()again on the same request. That re-fetchessnapshotand resets fixups, but any property middleware set on the request during the failed attempt is still there. If that state was derived from the old snapshot, the retry silently runs against stale data.For example, middleware that captures the pre-apply snapshot once per request:
Several
applymiddlewares can share one lazily-taken clone this way, so only the first that needs it pays for the copy. On a retry, though, this keeps the first attempt's snapshot rather than the re-fetched one. (A plain=would re-capture on every attempt, but then each middleware would need its own copy.) Today the only places to hook in are anapplymiddleware that tries to run first, or the synchronoustimingevent (submit.retry). Neither is really meant for this.This adds a
retrymiddleware action, triggered inSubmitRequest.retry()after the max-retries check and fixup reset, and before the next attempt:retriesis already incremented).nextrejects the op.maxSubmitRetries.applyandcommitrun again after it;submitmiddleware does not (unchanged behaviour, now documented).snapshotandchannelsare still those of the failed attempt; they're re-fetched/reset beforeapply.Why a middleware action rather than an event
An event (e.g.
backend.emit('submitRequestRetry', request), likesubmitRequestEnd) would be a smaller API surface, and I'm happy to switch if you'd prefer it. An action seemed the better fit because it:next(err);backend.useregistration, context and ordering asapply/commit;retrymiddleware,triggercalls back synchronously, soretry()→submit()timing is unchanged.Compatibility notes
backend.use(Object.values(backend.MIDDLEWARE_ACTIONS), fn)) will now also be called withaction: 'retry'when an op is retried. Nothing else changes, so this should be a minor release.@types/sharedbwill needretry: SubmitContextadded tomiddleware.ActionContextMap([sharedb] Add 'retry' middleware action DefinitelyTyped/DefinitelyTyped#75750).Changes
lib/backend.js: addretrytoMIDDLEWARE_ACTIONSlib/submit-request.js: trigger it before re-submittingdocs/middleware/actions.md,docs/middleware/op-submission.md: document the action and where it sits in the lifecycletest/middleware.js: ordering relative to the other submit actions,retriescount, state reset before the nextapply(which sees the re-fetched snapshot), not triggered on a first-time commit or pastmaxSubmitRetries, errors reject the opTest plan
npm test(906 passing)npm run lintsubmit-request.jschange (the 2 negative ones pass either way, sincebackend.useaccepts any action name)🤖 Generated with Claude Code