fix(sync-service): Fix keepalive acks advancing past unflushed small transactions#4731
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4731 +/- ##
==========================================
+ Coverage 60.04% 60.05% +0.01%
==========================================
Files 397 397
Lines 43766 43766
Branches 12588 12590 +2
==========================================
+ Hits 26278 26283 +5
+ Misses 17407 17401 -6
- Partials 81 82 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Failing regression test for a durability hole: flush_up_to_date? is only cleared when an event is applied while the decoder is mid-transaction, which never happens for a transaction that fits into a single commit-carrying fragment (fewer changes than max_batch_size, no Relation message). With the flag stale-true, the keepalive handler advances flushed_wal to the keepalive's wal_end, acknowledging WAL that no shape has durably stored — a crash before the async storage flush then loses the transaction, and a stuck flush pipeline gets acked past instead of retaining WAL. Regression from the operation-batching refactor (33fbc85); the original implementation (871d4e9) cleared the flag as soon as any transaction data started accumulating. The existing "flushes are not advancing" test misses this because its only relevant transaction is the session's first write to the table: that txn carries a Relation message, dispatched as a standalone mid-transaction event, which clears the flag by accident.
The flag was only cleared when an event was applied while the MessageConverter was still mid-transaction, which never happens for a transaction decoded into a single commit-carrying fragment (fewer changes than max_batch_size, no Relation message). With the flag stale-true, the keepalive handler advanced flushed_wal to the keepalive's wal_end, acknowledging WAL that no shape has durably stored — see the regression test in the previous commit. Track the flush tracker's confirmations in the new monotonic last_confirmed_flush_lsn field and recompute the flag whenever either side moves: the client is up to speed exactly when the tracker has confirmed the last seen transaction. The recompute after acknowledge_transaction matters for transactions that affect no shapes: their confirmation is emitted during event processing and reaches the client before the reply that updates last_seen_txn_lsn, so recomputing only on notification arrival would leave the flag false and stall the keepalive-driven slot advance. Covered by the new "confirmed during processing" test.
alco
force-pushed
the
alco/flush-ack-bypasses-flush-tracker
branch
from
July 20, 2026 22:35
42985e8 to
a40ee2c
Compare
alco
changed the title
alco
changed the title
alco
marked this pull request as ready for review
July 20, 2026 22:45
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
2 participants
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.
Keepalives could acknowledge WAL past a transaction whose storage flush hadn't been confirmed yet:
flush_up_to_date?was only cleared while the decoder was mid-transaction, which never happens for a transaction that fits into a single commit-carrying fragment (fewer changes thanmax_batch_size, noRelationmessage). A crash in the window between such a transaction's commit and its async storage flush loses it permanently — the slot'sconfirmed_flush_lsnis already past it. It also means a stuck flush pipeline gets acked past instead of retaining WAL under small-transaction-only traffic.Introduced by the operation batching refactor in #3483 (
33fbc857f, first shipped in@core/sync-service@1.2.7): before it, the flag was cleared on every transaction's first protocol message; the refactor's buffering path skips the flag update, leaving dispatch-at-commit as the only touchpoint — when the decoder is already out of the transaction.The fix makes the flag derived state: the client is up to speed exactly when the flush tracker has confirmed the last seen transaction, recomputed whenever either LSN moves. The first commit adds a regression test that fails on
main; the second commit fixes it and adds a test for the confirmation-during-processing ordering that keeps no-shape transactions from stalling the keepalive-driven slot advance.🤖 Generated with Claude Code