Repository navigation
fix(pglite-sync): commit must-refetch truncation with the refetched rows; report failed commits - #1126
Open
jeromerg wants to merge 1 commit into
Open
fix(pglite-sync): commit must-refetch truncation with the refetched rows; report failed commits#1126jeromerg wants to merge 1 commit into
jeromerg wants to merge 1 commit into
Conversation
…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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #1125.
After a
must-refetch,syncShapesToTablescan 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 inpackages/pglite-sync/src/index.ts.Changes
lastCommittedLsnadvances. It was aconstthat was never updated after a commit. It is now set to the commit's target LSN whencommitUpToLsncollects its messages. Without this,isCommitNeededwas true on every batch once any data had arrived. More importantly,isMustRefetchAndCatchingUp(lowest >= lastCommittedLsn) was-1 >= -1right after amust-refetch, which committed the truncation alone.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>= lastCommittedLsncomparison on purpose. A refetch can follow a server-side reset whose LSNs are lower than the storedlast_lsn, and with that comparison such a refetch would never commit.commitUpToLsntakes a snapshot oftruncateNeeded(and clears it) when it collects its messages, not when its transaction runs. Amust-refetchthat 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 withduplicate key.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 callsonError. Without anonErrorit rethrows, which keeps today's unhandled rejection. An error after the user has unsubscribed is ignored.Behaviour for subscriptions that never see a
must-refetchor a failing commit stays the same, except that empty commits stop: an empty commit used to run on every batch becauselastCommittedLsnwas 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 existingMultiShapeStreammock:keeps a refetched table populated until every shape has caught up: two shapes, both getmust-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, andonErroris not called.reports a failed commit through onError and stops syncing: a row the column type rejects.onErroris called with the error, the stream is unsubscribed, and later messages are not applied.Before the fix (main's
src/index.tswith 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 testpnpm --filter @electric-sql/pglite-sync typecheckpnpm --filter @electric-sql/pglite-sync stylecheckpnpm --filter @electric-sql/pglite-sync buildI ran these locally against the published
@electric-sql/pglite0.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/client1.2.0 and 1.5.28. The result was 0 of 50 tab-rounds empty or stale per client, with the subscription'slast_lsnadvancing 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