Skip to content

✨ Add retry middleware action - #761

Open
dawidreedsy wants to merge 3 commits into
mainfrom
submit-retry-hook
Open

dawidreedsy wants to merge 3 commits into
mainfrom
submit-retry-hook

Conversation

@dawidreedsy

@dawidreedsy dawidreedsy commented Oct 9, 2026 •

Copy link
Copy Markdown
Contributor

Summary

When a commit loses the race to another op, SubmitRequest.retry() calls submit() again on the same request. That re-fetches snapshot and 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:

backend.use('apply', (context, next) => {
  context.snapshotBeforeApply ??= structuredClone(context.snapshot)
  next()
})

Several apply middlewares 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 an apply middleware that tries to run first, or the synchronous timing event (submit.retry). Neither is really meant for this.

This adds a retry middleware action, triggered in SubmitRequest.retry() after the max-retries check and fixup reset, and before the next attempt:

backend.use('retry', (context, next) => {
  delete context.snapshotBeforeApply
  next()
})
  • The context is the same as the other submit actions (retries is already incremented).
  • Passing an error to next rejects the op.
  • Not triggered when the op has used up maxSubmitRetries.
  • Only apply and commit run again after it; submit middleware does not (unchanged behaviour, now documented).
  • In the hook, snapshot and channels are still those of the failed attempt; they're re-fetched/reset before apply.

Why a middleware action rather than an event

An event (e.g. backend.emit('submitRequestRetry', request), like submitRequestEnd) 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:

  • can be async, e.g. to clean up external state before the next attempt;
  • can reject the op via next(err);
  • uses the same backend.use registration, context and ordering as apply/commit;
  • costs nothing when unused: with no retry middleware, trigger calls back synchronously, so retry() → submit() timing is unchanged.

Compatibility notes

  • Middleware registered for every action (e.g. backend.use(Object.values(backend.MIDDLEWARE_ACTIONS), fn)) will now also be called with action: 'retry' when an op is retried. Nothing else changes, so this should be a minor release.
  • @types/sharedb will need retry: SubmitContext added to middleware.ActionContextMap ([sharedb] Add 'retry' middleware action DefinitelyTyped/DefinitelyTyped#75750).

Changes

  • lib/backend.js: add retry to MIDDLEWARE_ACTIONS
  • lib/submit-request.js: trigger it before re-submitting
  • docs/middleware/actions.md, docs/middleware/op-submission.md: document the action and where it sits in the lifecycle
  • test/middleware.js: ordering relative to the other submit actions, retries count, state reset before the next apply (which sees the re-fetched snapshot), not triggered on a first-time commit or past maxSubmitRetries, errors reject the op

Test plan

  • npm test (906 passing)
  • npm run lint
  • The 4 positive tests fail without the submit-request.js change (the 2 negative ones pass either way, since backend.use accepts any action name)

🤖 Generated with Claude Code

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>
@coveralls

coveralls commented Oct 9, 2026 •

Copy link
Copy Markdown

Coverage Status

coverage: 97.913% (+0.001%) from 97.912% — submit-retry-hook into main

@dawidreedsy dawidreedsy left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 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 the context during 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, because backend.use accepts any action name. Reword to "the positive tests (ordering, retries, error) fail without the change".

Checked, not an issue

  • The retry placement in MIDDLEWARE_ACTIONS was challenged and dropped. The map isn't alphabetical (receivePresence/sendPresence come after reply).
  • Sync→async: with no middleware, trigger calls back on the same tick, so retry() → submit() timing is unchanged.
  • request.action is briefly 'retry', then reset by trigger('apply'). $fixup can't misfire.
  • An error from 'retry' propagates through the same callback chain as apply/commit errors (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. trigger copies 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 (forceRetry mirrors 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 = -Infinity workaround (consumer-side enum value plus typings; see F8).

Comment thread docs/middleware/op-submission.md
Comment thread test/middleware.js
Comment thread lib/backend.js
Comment thread docs/middleware/actions.md
Comment thread docs/middleware/op-submission.md Outdated
Comment thread lib/submit-request.js
Comment thread lib/backend.js
dawidreedsy and others added 2 commits October 9, 2026 14:07
- 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>
@dawidreedsy

Copy link
Copy Markdown
Contributor Author

🤖 PR panel follow-up. F6 (outside the diff) was fixed in 2850978: the apply re-run warning now links to the retry hook. F9 was fixed in the PR body: the test plan now says the 4 positive tests fail without the change.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants