fix: PUBLISH leaks a TransactionExecution on every call - #527
Merged
Conversation
RedisServiceImpl::Publish created a TransactionExecution for the cross-node PublishTxRequest but never committed or aborted it. The engine handler only finishes the request result, so the txm never reached Finished and was never returned to the free list: one txm leaked per PUBLISH, ext_active_tx_cnt_ grew monotonically, and AllTxFinished() could never hold again (blocking graceful node-group leader transfer). Release the txm with AbortTx after the request completes. The publish txm has an empty read/write set, so the abort takes the empty-set fast path: an asynchronous enqueue+enlist that adds no blocking work to the hot path. Verified with a Debug build and a gdb probe over the TxProcessor counters: pre-fix, 10k PUBLISHes grow TOTAL_EXT from 4 to 10004 with zero drain; post-fix it stays at 0 immediately. PUBLISH throughput improves 42k -> 73k req/s (txms now recycle instead of allocating fresh ones), and the pubsub/multi/string TCL suites pass. Fixes #505 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough
ChangesPUBLISH transaction cleanup
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
tests/unit/eloq/flaky_test/pubsub.tcl runs in CI but its failures are swallowed, so no gating test exercises PUBLISH at all - which is how the txm leak (#505) shipped unnoticed. Add a deterministic subset directly under tests/unit/eloq/ where failures fail CI: publish with no subscribers, SUBSCRIBE/PSUBSCRIBE delivery + reply counts, and publish via Lua EVAL. Every assertion is a synchronous request or a blocking read of a confirmation the server sends after updating the registry under pub_sub_mu_, so there are no timing sleeps. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The framework's unsubscribe/punsubscribe helpers consume zero confirmation messages when called without channel arguments. Upstream Redis is single-threaded so the next command is serialized after the unsubscribe anyway, but EloqKV processes connections concurrently: nothing orders the pub/sub registry update before another connection's PUBLISH, so the publish-returns-0 assertions raced it. The file papered over this with 'after 100' sleeps, which still flake on slow or loaded machines - the reason it lives in flaky_test/. Read all per-channel confirmations instead: EloqKV flushes one per channel inside the single pub_sub_mu_ critical section that performs the erasures, and PUBLISH takes the same mutex, so once the last confirmation is read a subsequent publish deterministically counts 0. Reproduced and verified on a Debug build: a 300-iteration tight loop of subscribe -> unsubscribe-all -> publish under concurrent benchmark load failed every round before this change and passes 3/3 after; the full file passes 5/5 loaded + 5/5 unloaded rounds. Kept in flaky_test/ for now so the CI matrix can demonstrate stability before promotion. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The unsubscribe-all race fix (previous commit) held up: the file passed every phase of every CI matrix job non-gating, including the 3-node cluster log-replay phase on the slowest arm64/ELOQSTORE variant, with zero swallowed failures, plus loaded/unloaded local stress rounds. Merge it into tests/unit/eloq/pubsub.tcl where failures fail CI, absorbing the smoke file's two unique cases (publish with no subscribers; publish via Lua EVAL), and delete the flaky_test copy. Also fix the remaining teardown races before gating: three tests closed deferring clients while still subscribed, and server-side removal happens asynchronously in the connection destructor, so a later publish to the same channel could still count them — unsubscribe explicitly (confirmations consumed) before close. Merged suite passes 3/3 rounds under concurrent benchmark load and 3/3 unloaded. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/eloq/pubsub.tcl`:
- Line 186: Remove the pasted failed-test diagnostic comment near the affected
test in pubsub.tcl; retain the surrounding test logic and assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f67469c4-ffe7-4b34-982f-a6023346adc3
📒 Files selected for processing (1)
tests/unit/eloq/pubsub.tcl
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/unit/eloq/pubsub.tcl`:
- Line 186: Remove the pasted failed-test diagnostic comment near the affected
test in pubsub.tcl; retain the surrounding test logic and assertions unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f67469c4-ffe7-4b34-982f-a6023346adc3
📒 Files selected for processing (1)
tests/unit/eloq/pubsub.tcl
🛑 Comments failed to post (1)
tests/unit/eloq/pubsub.tcl (1)
186-186: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove debug artifact comment.
This line appears to be error output from a failed test run pasted as a comment. It references an internal assertion failure in
consume_subscribe_messagesand adds no documentation value.🧹 Proposed fix
- -# Expected 'subscribe' to be equal to 'unknown' (context: type source line 838 file /home/eloq/workspace/eloqkv/tests/support/util.tcl cmd {assert_equal $type [lindex $msg 0]} proc ::consume_subscribe_messages level 2) # 'CLIENT REPLY OFF' unsupported📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.# 'CLIENT REPLY OFF' unsupported🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/eloq/pubsub.tcl` at line 186, Remove the pasted failed-test diagnostic comment near the affected test in pubsub.tcl; retain the surrounding test logic and assertions unchanged.
thweetkomputer
approved these changes
Jul 11, 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
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.
Fixes #505
Problem
RedisServiceImpl::Publishcreates an external txm to carry the cross-nodePublishTxRequest, but no path ever commits or aborts it. The engine handler (ProcessTxRequest(PublishTxRequest&)) finishes only the request result — it never drives the txm toFinished— and an external txm is returned to the free list only viaRemoveExternActiveTxm, which fires only whenForward()reportsFinished.SendTxRequestAndWaitResult's internal abort covers onlyObjectCommandTxRequest/MultiObjectCommandTxRequestwithauto_commit_, whichPublishTxRequestis not.Result: one
TransactionExecutionleaked per PUBLISH (including Luaredis.call('publish', ...)),ext_active_tx_cnt_grows monotonically, andAllTxFinished()never holds again — the concrete consumer is node-group leader transfer, which spins inwhile (!AllTxFinished())(cc_node_service.cpp:2502).Fix
Release the txm with
AbortTx(txm)after the request wait — the caller-owns-lifecycle pattern every otherNewTxmsite in this layer uses (e.g. the FaultInject path). The publish txm has an empty read/write set, so the abort takes the empty-set fast path ofTransactionExecution::CommitTx(tx_execution.cpp:574-587): an asynchronous enqueue + enlist, no blocking work added to the hot path. The call is safe unconditionally: the FIFO request queue orders it after the in-flight publish request,PublishTxRequestmatches neitherdynamic_castin theTX_INIT_FAILpath (no double abort), and aborting an init-failed txm takes the establishedtx_term_ < 0branch.docs/04-scripting-pubsub-blocking.mdis updated in the same change per the docs maintenance rule (PUBLISH-flow prose + line-reference refreshes in the Pub/Sub section).Verification (Debug build, ELOQDSS_ROCKSDB, single node)
Leak oracle: gdb probe summing
ext_active_tx_cnt_+active_tx_cnt_across TxProcessors.redis-cli -r 10000 publish c mredis-benchmark -n 100000 -c 10 publish c mThroughput improves ~74% because txms now recycle through the free list instead of allocating a fresh
TransactionExecutionper publish.Functional: SUBSCRIBE/PUBLISH delivery + reply count, Lua
EVAL redis.call('publish'), and theunit/eloq/flaky_test/pubsub,unit/eloq/multi,unit/eloq/stringTCL suites all pass.Notes
tests/unit/eloq/*.tclhas none;pubsub.tclis quarantined underflaky_test/), so this ships without an automated leak regression test — there is also no protocol-surface observable for active txms. Suggested follow-ups: promote a deterministic pubsub smoke test into CI, and expose an active-transaction gauge (e.g.TxService::AllTxFinished()) via INFO/metrics.TX_INIT_FAILdrain loop and a concurrently enqueued abort exists for all caller-sideAbortTx-after-init-failure patterns (pre-existing, nanosecond window, not a regression of this change).🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests
Documentation