-
Notifications
You must be signed in to change notification settings - Fork 553
UN-2857 [MISC] Fix Redis unacked hash memory leak in log consumer worker #1566
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
UN-2857 [MISC] Fix Redis unacked hash memory leak in log consumer worker #1566
Conversation
Added write_only=True parameter to KombuManager in log_consumer worker to prevent accumulation of unacked messages in Redis.
Summary by CodeRabbit
WalkthroughUpdated Socket.IO client manager initialization in workers/log_consumer/tasks.py to instantiate KombuManager with write_only=True. No other logic or control flow changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
workers/log_consumer/tasks.py (2)
31-31
: Consider clarifying the comment about configuration difference.The comment states "uses same KombuManager as backend" but this worker now uses
write_only=True
while the backend uses the defaultwrite_only=False
. Consider updating the comment to reflect this configuration difference.Apply this diff to clarify the comment:
-# Socket.IO client for emitting events (uses same KombuManager as backend) +# Socket.IO client for emitting events (uses KombuManager with write_only=True for emit-only worker)
40-40
: Approve write-only KombuManager for emit-only worker
write_only=True
is correct since this worker only emits events (no handlers) and prevents the Redis unacked hash memory leak.- Optional: update the module comment to reflect the write-only configuration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to Reviews > Disable Cache setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base
setting
📒 Files selected for processing (1)
workers/log_consumer/tasks.py
(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
|
|
@muhammad-ali-e will this change cause messages to be dropped by any chance? Will simply adding a TTL for these messages help instead? |
Tested this with integration. Also it seems socketio creating single hashmap instaed of each key. So TTL might not work on each internal elements. |
What
write_only=True
parameter to KombuManager in log_consumer workerunacked
hash for emit-only workersWhy
unacked
hash was consuming 145 MB with 110K entries in integration environmentunacked
entries to track message acknowledgments, but has no TTLHow
write_only=True
insocketio.KombuManager()
constructor inworkers/log_consumer/tasks.py
write_only=False
Can this PR break any existing features. If yes, please list possible items. If no, please explain why.
No, this PR cannot break existing features:
write_only=True
only affects the worker's internal tracking, not the message flowwrite_only=True
is the recommended configuration for emit-only workers per python-socketio documentationDatabase Migrations
Env Config
Relevant Docs
workers/log_consumer/tasks.py
- KombuManager configurationRelated Issues or PRs
unacked
hash (internal investigation)Dependencies Versions
Notes on Testing
Testing strategy:
unacked
hash size - should stop growingSuccess criteria:
unacked
hash stops growing beyond current 110K entriesRollback plan:
write_only=True
parameter if any issues detectedScreenshots
N/A - Backend optimization, no UI changes
Checklist
✅ I have read and understood the Contribution Guidelines.
🤖 Generated with Claude Code