[fix][txn] PIP-473: v5 transaction timeout applied 1000x too long#25959
Merged
merlimat merged 1 commit intoJun 8, 2026
Conversation
Motivation: Scalable-topics (v5) transactions never timed out at the requested value: a 1-minute timeout was applied as ~16.7 hours, so the coordinator's timeout sweep effectively never aborted a dangling (never-committed) transaction, leaving its buffer pinned indefinitely. Root cause: the NEW_TXN handler's v5 branch multiplied the wire ttl by 1000. But the ttl field has always carried milliseconds — the client sends unit.toMillis(...), and both the legacy coordinator (TransactionMetadataStoreService.newTransaction, param timeoutInMills) and the v5 coordinator (TransactionCoordinatorV5.newTransaction, Duration.ofMillis) consume milliseconds. The field's legacy name (txn_ttl_seconds) was a misnomer that invited the bug. Modifications: - ServerCnx.handleNewTxn: drop the * 1000 on the v5 path; pass the millisecond value straight through, matching the legacy path. - Rename the wire field CommandNewTxn.txn_ttl_seconds -> txn_ttl_millis to stop the unit confusion recurring, and update Commands.newTxn accordingly. The field number (2) and type (uint64) are unchanged, so the binary protocol is unaffected; only the generated accessor names change. - Add V5TransactionTimeoutTest: a never-committed v5 transaction must be aborted by the broker timeout sweep, unpinning the buffer so a later non-transactional message becomes visible. Assisted-by: Claude Code
void-ptr974
approved these changes
Jun 8, 2026
dao-jun
approved these changes
Jun 8, 2026
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.
Motivation
Scalable-topics (v5) transactions never timed out at the requested value: a 1-minute timeout was applied as ~16.7 hours, so the coordinator's timeout sweep effectively never aborted a dangling (never-committed) transaction, leaving its transaction buffer pinned indefinitely (which also blocks visibility of later writes on the same segment).
Root cause. The
NEW_TXNhandler's v5 branch inServerCnxmultiplied the wire ttl by 1000. But the ttl field has always carried milliseconds — the client sendsunit.toMillis(...), and both the legacy coordinator (TransactionMetadataStoreService.newTransaction, parametertimeoutInMills) and the v5 coordinator (TransactionCoordinatorV5.newTransaction,Duration.ofMillis(...)) consume milliseconds. The field's legacy name,txn_ttl_seconds, was a misnomer that invited the bug.Modifications
ServerCnx.handleNewTxn: drop the* 1000on the v5 path; pass the millisecond value straight through, matching the legacy path.CommandNewTxn.txn_ttl_seconds→txn_ttl_millisso the unit confusion can't recur, and updateCommands.newTxnaccordingly.V5TransactionTimeoutTest: a never-committed v5 transaction must be aborted by the broker timeout sweep, unpinning the buffer so a later non-transactional message becomes visible.Compatibility
The binary protocol is unchanged: the field number (
2) and type (uint64) are identical — only the field's name and the generated Java accessor names change (getTxnTtlSeconds→getTxnTtlMillis). Older and newer clients/brokers interoperate exactly as before.Verifying this change
V5TransactionTimeoutTestreproduces the bug (failed before the fix, passes after).V5TransactionTest7/7,V5TransactionScalableTest8/8,V5TransactionRecoveryTest3/3,TransactionCoordinatorV5Test22/22,CommandsScalableTxnFlagTest6/6; checkstyle clean.Does this pull request potentially affect one of the following parts: