Repository navigation
fix(offset): markProcessed runs outside OFFSET_COMMIT_LOCK, leaking the flush semaphore - #1447
Merged
Merged
Conversation
…he flush semaphore
acknowledgeRecords() serializes only markBatchFinished() (via
markBatchFinishedSafely) and leaves markProcessed() outside the lock. That is
not sufficient: markProcessed() mutates the OffsetStorageWriter's
pending-offset map, which is exactly the state beginFlush() snapshots, and
neither is thread-safe. Debezium also builds a NEW RecordCommitter per batch
(EmbeddedEngine.buildRecordCommitter), so the committer's own synchronized
methods lock different monitors for different batches and provide no mutual
exclusion across worker threads.
Once two worker threads collide, EmbeddedEngine.commitOffsets returns early
when doFlush returns null WITHOUT calling cancelFlush, permanently leaking the
flushInProgress semaphore. Every subsequent beginFlush() in the JVM throws
ConnectException: OffsetStorageWriter is already flushing.
Two further behaviours turn a stuck writer into SILENT DIVERGENCE rather than
a visible failure:
- ClickHouseBatchRunnable routes the failure through
ClickHouseErrorClassifier, which sees no ClickHouse error code and
defaults it to UNKNOWN/retriable. With errors.max.retries = -1 it retries
forever.
- JdbcOffsetBackingStore.set is asynchronous, so ClickHouse inserts keep
succeeding against a frozen binlog position.
Observed in production on a MySQL -> ClickHouse deployment running 2.11.0: the
connector logged healthy batches for ~8h while its committed offset never
advanced past an event 8 hours old, and on restart the binlog client resumed
nine binlog files (~10 GiB) behind the position the dying process had been
reading. 3,930 occurrences of the exception in that window. Three sibling
connectors on 2.9.1 against the same ClickHouse host logged zero, as did a
second 2.11.0 connector running roughly a tenth of the volume -- the defect
needs two batch completions in flight.
Changes:
- DebeziumOffsetManagement: markProcessed() and markBatchFinished() now run
inside ONE OFFSET_COMMIT_LOCK critical section in both acknowledgeRecords
variants. markBatchFinishedSafely is replaced by a public acknowledgeRecord
entry point so every offset-committing path funnels through the same lock.
- ClickHouseBatchWriter: its second, unserialized acknowledgement path --
which called markProcessed()/markBatchFinished() directly, bypassing the
lock entirely -- now routes through acknowledgeRecord. A poisoned writer is
rethrown rather than logged and swallowed, and an InterruptedException
re-sets the interrupt flag instead of continuing to acknowledge.
- ClickHouseBatchRunnable: a poisoned writer is detected BEFORE the
ClickHouse classifier and stops the task, so it can never be retried
forever as UNKNOWN. A restart resumes from the last committed offset.
No lock-order change: acknowledgeRecord takes only OFFSET_COMMIT_LOCK, never
the class monitor.
Testing (2.11.0 @ af198d9, java-17, maven 3.9.11):
OffsetCommitSerializationTest reproduces the defect deterministically with a
committer that fails if any two offset-mutating calls overlap.
with this fix: 3 run, 0 failures
acknowledgeRecord reverted to
the partial-lock shape: 3 run, 2 FAILURES, both naming
markProcessed as the overlapping call
Full module suite, same tree, no Docker available on the runner:
unmodified 2.11.0: Tests run: 344, Failures: 0, Errors: 20
with this fix: Tests run: 347, Failures: 0, Errors: 20
The 20 errors are identical in both runs and are all
"Previous attempts to find a Docker environment failed" from the
container-backed tests; they are pre-existing and unrelated.
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.
fix(offset): markProcessed runs outside OFFSET_COMMIT_LOCK, leaking the flush semaphore
acknowledgeRecords() serializes only markBatchFinished() (via
markBatchFinishedSafely) and leaves markProcessed() outside the lock. That is
not sufficient: markProcessed() mutates the OffsetStorageWriter's
pending-offset map, which is exactly the state beginFlush() snapshots, and
neither is thread-safe. Debezium also builds a NEW RecordCommitter per batch
(EmbeddedEngine.buildRecordCommitter), so the committer's own synchronized
methods lock different monitors for different batches and provide no mutual
exclusion across worker threads.
Once two worker threads collide, EmbeddedEngine.commitOffsets returns early
when doFlush returns null WITHOUT calling cancelFlush, permanently leaking the
flushInProgress semaphore. Every subsequent beginFlush() in the JVM throws
ConnectException: OffsetStorageWriter is already flushing.
Two further behaviours turn a stuck writer into SILENT DIVERGENCE rather than
a visible failure:
ClickHouseErrorClassifier, which sees no ClickHouse error code and
defaults it to UNKNOWN/retriable. With errors.max.retries = -1 it retries
forever.
succeeding against a frozen binlog position.
Observed in production on a MySQL -> ClickHouse deployment running 2.11.0: the
connector logged healthy batches for ~8h while its committed offset never
advanced past an event 8 hours old, and on restart the binlog client resumed
nine binlog files (~10 GiB) behind the position the dying process had been
reading. 3,930 occurrences of the exception in that window. Three sibling
connectors on 2.9.1 against the same ClickHouse host logged zero, as did a
second 2.11.0 connector running roughly a tenth of the volume -- the defect
needs two batch completions in flight.
Changes:
inside ONE OFFSET_COMMIT_LOCK critical section in both acknowledgeRecords
variants. markBatchFinishedSafely is replaced by a public acknowledgeRecord
entry point so every offset-committing path funnels through the same lock.
which called markProcessed()/markBatchFinished() directly, bypassing the
lock entirely -- now routes through acknowledgeRecord. A poisoned writer is
rethrown rather than logged and swallowed, and an InterruptedException
re-sets the interrupt flag instead of continuing to acknowledge.
ClickHouse classifier and stops the task, so it can never be retried
forever as UNKNOWN. A restart resumes from the last committed offset.
No lock-order change: acknowledgeRecord takes only OFFSET_COMMIT_LOCK, never
the class monitor.
Testing (2.11.0 @ af198d9, java-17, maven 3.9.11):
OffsetCommitSerializationTest reproduces the defect deterministically with a
committer that fails if any two offset-mutating calls overlap.
Full module suite, same tree, no Docker available on the runner:
The 20 errors are identical in both runs and are all
"Previous attempts to find a Docker environment failed" from the
container-backed tests; they are pre-existing and unrelated.