Add DENYOOM flag to SUBSCRIBE, PSUBSCRIBE, SSUBSCRIBE, WATCH#4230
Add DENYOOM flag to SUBSCRIBE, PSUBSCRIBE, SSUBSCRIBE, WATCH#4230enjoy-binbin wants to merge 1 commit into
Conversation
These commands previously lacked the DENYOOM flag, letting clients bypass the maxmemory limit and exhaust server memory. Follow-up to valkey-io#3362, which made the memory used by watched keys and pubsub channel/pattern names count toward each client's overhead so the maxmemory-clients eviction can reclaim it. However, that only helps when maxmemory-clients is configured. This change marks them as DENYOOM so that the server will reject these commands with an OOM error instead of accepting new subs and watches that consume additional memory. Closing the gap where users could otherwise bypass maxmemory by subscribing/watching. Part of valkey-io#3365. Signed-off-by: Binbin <binloveplay1314@qq.com>
📝 WalkthroughWalkthroughThe command definitions for ChangesDENYOOM command coverage
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/unit/maxmemory.tcl (2)
663-667: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReplace potential missing helper procedures with standard client methods.
If the
subscribe,psubscribe, andssubscribehelper procedures are not globally available in the test suite (they are typically confined totests/unit/pubsub.tcl), this test will fail with aninvalid command nameerror.Consider using the standard deferring client method calls (
$rd command ...followed by$rd read) to ensure the test is self-contained and avoids dependencies on external unit test files. This also allows you to assert against the exact RESP array returned by the server.♻️ Proposed refactor
- set rd [valkey_deferring_client] - assert_equal {1} [subscribe $rd test_channel] - assert_equal {2} [psubscribe $rd test_pattern*] - assert_equal {1} [ssubscribe $rd test_shardchannel] - $rd close + set rd [valkey_deferring_client] + + $rd subscribe test_channel + assert_equal {subscribe test_channel 1} [$rd read] + + $rd psubscribe test_pattern* + assert_equal {psubscribe test_pattern* 1} [$rd read] + + $rd ssubscribe test_shardchannel + assert_equal {ssubscribe test_shardchannel 1} [$rd read] + + $rd close🤖 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/maxmemory.tcl` around lines 663 - 667, Replace the subscribe, psubscribe, and ssubscribe helper calls in this test with standard deferring-client command and read operations on $rd. Assert the exact RESP arrays returned for each subscription, then close $rd as before, keeping the test independent of globally defined helper procedures.
663-667: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReplace potential missing helper procedures with standard client methods.
If the
subscribe,psubscribe, andssubscribehelper procedures are not globally available in the test suite (they are typically confined totests/unit/pubsub.tcl), this test will fail with aninvalid command nameerror.Consider using the standard deferring client method calls (
$rd command ...followed by$rd read) to ensure the test is self-contained and avoids dependencies on external unit test files. This also allows you to assert against the exact RESP array returned by the server.♻️ Proposed refactor
- set rd [valkey_deferring_client] - assert_equal {1} [subscribe $rd test_channel] - assert_equal {2} [psubscribe $rd test_pattern*] - assert_equal {1} [ssubscribe $rd test_shardchannel] - $rd close + set rd [valkey_deferring_client] + + $rd subscribe test_channel + assert_equal {subscribe test_channel 1} [$rd read] + + $rd psubscribe test_pattern* + assert_equal {psubscribe test_pattern* 2} [$rd read] + + $rd ssubscribe test_shardchannel + assert_equal {ssubscribe test_shardchannel 1} [$rd read] + + $rd close🤖 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/maxmemory.tcl` around lines 663 - 667, Replace the subscribe, psubscribe, and ssubscribe helper calls in this deferring-client setup with standard `$rd command ...` requests followed by `$rd read` assertions. Keep the existing channel and pattern arguments, and assert the exact RESP arrays returned for each subscription before closing `$rd`, removing reliance on globally defined test helpers.
🤖 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.
Nitpick comments:
In `@tests/unit/maxmemory.tcl`:
- Around line 663-667: Replace the subscribe, psubscribe, and ssubscribe helper
calls in this test with standard deferring-client command and read operations on
$rd. Assert the exact RESP arrays returned for each subscription, then close $rd
as before, keeping the test independent of globally defined helper procedures.
- Around line 663-667: Replace the subscribe, psubscribe, and ssubscribe helper
calls in this deferring-client setup with standard `$rd command ...` requests
followed by `$rd read` assertions. Keep the existing channel and pattern
arguments, and assert the exact RESP arrays returned for each subscription
before closing `$rd`, removing reliance on globally defined test helpers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4ea99de2-6ef5-4e2d-9367-6cb78bababf6
📒 Files selected for processing (6)
src/commands.defsrc/commands/psubscribe.jsonsrc/commands/ssubscribe.jsonsrc/commands/subscribe.jsonsrc/commands/watch.jsontests/unit/maxmemory.tcl
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #4230 +/- ##
============================================
+ Coverage 76.69% 76.83% +0.14%
============================================
Files 162 162
Lines 81461 81477 +16
============================================
+ Hits 62474 62606 +132
+ Misses 18987 18871 -116
🚀 New features to boost your workflow:
|
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
| "function": "subscribeCommand", | ||
| "command_flags": [ | ||
| "PUBSUB", | ||
| "DENYOOM", |
There was a problem hiding this comment.
SUBSCRIBE is also the command Sentinel uses for its internal __sentinel__:hello pubsub link (src/sentinel.c:2411-2413). With DENYOOM here, an instance that is already over maxmemory rejects that subscription in processCommand() (src/server.c:4550-4570), so Sentinel never actually joins the hello channel. The callback on that link only handles pubsub message frames and ignores the error reply (src/sentinel.c:2879-2900), so the connection sits unsubscribed until the inactivity timeout closes and retries it (src/sentinel.c:4472-4474).
This needs an exemption for Sentinel’s internal hello subscription instead of making SUBSCRIBE deny-oom unconditionally.
1 participant
These commands previously lacked the DENYOOM flag, letting clients
bypass the maxmemory limit and exhaust server memory.
Follow-up to #3362, which made the memory used by watched keys and
pubsub channel/pattern names count toward each client's overhead so
the maxmemory-clients eviction can reclaim it. However, that only
helps when maxmemory-clients is configured.
This change marks them as DENYOOM so that the server will reject
these commands with an OOM error instead of accepting new subs and
watches that consume additional memory. Closing the gap where users
could otherwise bypass maxmemory by subscribing/watching.
Part of #3365 and closes #3365.