Skip to content

fix(pglite-sync): commit must-refetch truncation with the refetched rows; report failed commits - #1126

Open
jeromerg wants to merge 1 commit into
electric-sql:mainfrom
jeromerg:fix/pglite-sync-must-refetch-commit
Open

jeromerg wants to merge 1 commit into
electric-sql:mainfrom
jeromerg:fix/pglite-sync-must-refetch-commit

Conversation

@jeromerg

@jeromerg jeromerg commented Oct 8, 2026

Copy link
Copy Markdown

Summary

Fixes #1125.

After a must-refetch, syncShapesToTables can commit a table's truncation on its own, which leaves the table empty until every shape has refetched (or for good, if one shape never catches up). It can also truncate under a commit that is already queued, and it drops a failed commit as an unhandled rejection. This PR fixes all three in packages/pglite-sync/src/index.ts.

Changes

  • lastCommittedLsn advances. It was a const that was never updated after a commit. It is now set to the commit's target LSN when commitUpToLsn collects its messages. Without this, isCommitNeeded was true on every batch once any data had arrived. More importantly, isMustRefetchAndCatchingUp (lowest >= lastCommittedLsn) was -1 >= -1 right after a must-refetch, which committed the truncation alone.
  • A must-refetch commits once every shape has caught up again (truncateNeeded.size > 0 && lowestCommittedLsn > -1). The truncation and the refetched rows then land in one transaction, and the table keeps its old rows until that point. I left out the >= lastCommittedLsn comparison on purpose. A refetch can follow a server-side reset whose LSNs are lower than the stored last_lsn, and with that comparison such a refetch would never commit.
  • Truncations are taken with the messages. commitUpToLsn takes a snapshot of truncateNeeded (and clears it) when it collects its messages, not when its transaction runs. A must-refetch that arrives while a commit is queued is applied by the next commit. Before, the queued commit applied it and inserted the old rows, and the refetched snapshot then failed with duplicate key.
  • A failed commit is reported. commitUpToLsn(...) gets a .catch. Its messages have already been taken from the buffer, so carrying on would leave the table silently diverged. The handler therefore stops the subscription (unsubscribe()) and calls onError. Without an onError it rethrows, which keeps today's unhandled rejection. An error after the user has unsubscribed is ignored.

Behaviour for subscriptions that never see a must-refetch or a failing commit stays the same, except that empty commits stop: an empty commit used to run on every batch because lastCommittedLsn was stuck.

Fail-stop on a failed commit is a behaviour change. If you would rather keep the subscription running and only call onError, I can change that.

Testing

Three new tests in test/sync.test.ts (describe('commits around must-refetch')), using the existing MultiShapeStream mock:

  • keeps a refetched table populated until every shape has caught up: two shapes, both get must-refetch, and only one has refetched. Both tables keep their rows, then switch to the refetched rows together.
  • does not truncate under a commit that was queued before a must-refetch: the database is held by a transaction while an insert commit and then a must-refetch + snapshot commit queue up. The table ends with the snapshot, and onError is not called.
  • reports a failed commit through onError and stops syncing: a row the column type rejects. onError is called with the error, the stream is unsubscribed, and later messages are not applied.

Before the fix (main's src/index.ts with the new tests): 3 failed, 19 passed, and 2 unhandled rejections (duplicate key value violates unique constraint "todo_pkey", invalid input syntax for type integer). After the fix: 22 passed, across three repeated runs.

  • pnpm --filter @electric-sql/pglite-sync test
  • pnpm --filter @electric-sql/pglite-sync typecheck
  • pnpm --filter @electric-sql/pglite-sync stylecheck
  • pnpm --filter @electric-sql/pglite-sync build

I ran these locally against the published @electric-sql/pglite 0.5.8 build in place of a local WASM build. I did not run the e2e suite (test-e2e, Docker).

We have run the same change as a local patch of 0.4.1 in our application: Electric 1.8.1, ten forced 409 rounds × five tabs, with @electric-sql/client 1.2.0 and 1.5.28. The result was 0 of 50 tab-rounds empty or stale per client, with the subscription's last_lsn advancing in every round. Without the patch, about 1 in 15 tab-rounds ended with an empty table.

A changeset (@electric-sql/pglite-sync: patch) is included.

🤖 Generated with Claude Code

…ows; report failed commits

- advance lastCommittedLsn when a commit collects its messages (it was a const)
- commit a must-refetch only once every shape has a complete LSN, so the
  truncation and the refetched rows land in one transaction
- take the truncation set with the messages, not inside the transaction, so a
  must-refetch arriving while a commit is queued is applied by the next commit
- catch a failed commit: stop the subscription and call onError instead of an
  unhandled rejection with the collected rows lost

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

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.

[BUG]: pglite-sync: after a must-refetch a synced table can stay empty; commit errors are swallowed

1 participant