Emit latency metrics for round trip time for cluster nodes - #4093
Emit latency metrics for round trip time for cluster nodes#4093ydsakshi wants to merge 20 commits into
Conversation
Signed-off-by: ydsakshi <ydsakshi023@gmail.com>
Signed-off-by: ydsakshi <ydsakshi023@gmail.com>
Signed-off-by: ydsakshi <ydsakshi023@gmail.com>
Signed-off-by: ydsakshi <ydsakshi023@gmail.com>
Signed-off-by: ydsakshi <ydsakshi023@gmail.com>
Signed-off-by: ydsakshi <ydsakshi023@gmail.com>
Signed-off-by: ydsakshi <ydsakshi023@gmail.com>
Signed-off-by: ydsakshi <ydsakshi023@gmail.com>
Signed-off-by: ydsakshi <ydsakshi023@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR adds cluster node RTT tracking. Ping extensions carry echo timestamps; PONG processing computes per-node maximum and sliding-window average RTT values, exposes them through ChangesRTT latency stats feature
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant writePingExtensions
participant clusterProcessPingExtensions
participant clusterProcessPacket
participant updateCommandLatencyStats
writePingExtensions->>clusterProcessPingExtensions: Send PING_ECHO_TIME
clusterProcessPingExtensions->>clusterProcessPacket: Store link->ping_echo_time
writePingExtensions->>clusterProcessPacket: Echo timestamp in PONG
clusterProcessPacket->>updateCommandLatencyStats: Compute and record RTT
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 10
🧹 Nitpick comments (2)
src/cluster_legacy.c (1)
3479-3514: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun clang-format-18 on this hunk.
The indentation and brace placement here (e.g. Lines 3479, 3481-3485, 3513) don't match the surrounding style and will fail the CI clang-format check. As per coding guidelines: "CI uses clang-format-18 ... run clang-format-18 -i on the modified files before finalizing."
🤖 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 `@src/cluster_legacy.c` around lines 3479 - 3514, The block in cluster_legacy.c needs to be reformatted to match the project’s clang-format-18 style, since the current indentation and brace placement around the server.cluster_nodes_latency_stats_enabled, CLUSTERMSG_TYPE_PING/MEET, and CLUSTERMSG_TYPE_PONG logic are inconsistent with surrounding code. Reformat this hunk in the affected function so the nesting, braces, and alignment follow the existing style and will pass CI’s clang-format check.Source: Coding guidelines
tests/unit/cluster/cluster-shards.tcl (1)
96-127: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest only checks field presence, not RTT correctness.
The fields are emitted unconditionally whenever
cluster-nodes-latency-stats-enabledis on (addNodeDetailsToShardReply), so this passes even if RTT is never computed — which is currently the case given the echo-back defect incluster_legacy.c. Consider asserting that at least primaries report a plausible non-zeroavg-round-trip-timeafterwait_for_cluster_propagation, so the test actually exercises the measurement path. Also, thecatch {}around SET/GET silently hides real failures. End-to-end behavior changes like this should be covered by meaningful integration assertions.🤖 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/cluster/cluster-shards.tcl` around lines 96 - 127, The current test in cluster-shards.tcl only verifies that max-round-trip-time and avg-round-trip-time fields exist, so it can pass even when RTT is never measured. Update the CLUSTER SHARDS assertions in test "Verify latency metrics are present per node in CLUSTER SHARDS output" to check that primaries report a plausible non-zero avg-round-trip-time after wait_for_cluster_propagation, using the existing R, CLUSTER SHARDS, and shard_dict/node_dict parsing flow. Also remove the broad catch wrappers around the SET/GET setup so real failures surface instead of being hidden.Source: Coding guidelines
🤖 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 `@src/cluster_legacy.c`:
- Around line 3626-3641: The ping echo time handling in cluster_legacy.c still
contains a large commented-out RTT flow directly after the active assignment in
the ping echo time extension case. Remove that dead commented block around the
logic that sets ping_echo_time_ext and link->ping_echo_time, leaving only the
active code and break so the behavior is clear and the switch case is not
cluttered with unused alternatives.
- Around line 7409-7418: The `node->max_round_trip_time` and
`node->avg_round_trip_time` conditional checks in the reply-building block are
commented out dead code. Clean up this section by either removing those `if`
guards entirely and keeping the unconditional
`addReplyBulkCString`/`addReplyLongLong` calls, or restoring the guards as real
conditionals in the same block. Use the existing reply logic around
`reply_count` in `src/cluster_legacy.c` to keep the behavior consistent.
- Line 1375: The RTT handling in clusterDoBeforeSleep should not trigger
CLUSTER_TODO_SAVE_CONFIG on every PONG, since RTT is runtime telemetry and this
forces unnecessary nodes.conf rewrites. Remove the
clusterDoBeforeSleep(CLUSTER_TODO_SAVE_CONFIG) call from the RTT update path in
the cluster node message handling logic so persisted config changes are only
scheduled for actual topology/state updates.
- Line 1367: The RTT update path in the node latency logic is still emitting a
debug printf from the hot path, which writes directly to stdout on every
PONG-driven update. Remove the printf in the latency update code, or replace it
with serverLog(LL_DEBUG, ...) if this trace must remain, and keep the change
localized around the node latency update logic that uses current_rtt and
node->name.
- Around line 1370-1374: The RTT smoothing logic in node -> avg_round_trip_time
uses an unsafe and inconsistent recurrence: it can divide by
server.sliding_window_length_rtt_latency_stats when that value is 0, and it
mixes load_factor_historic_rtt_latency with an unrelated denominator so the
average is not a proper smoothing update. Update this branch in the
avg_round_trip_time calculation to use a guaranteed non-zero denominator and a
consistent EMA-style recurrence based on the intended historical factor, keeping
the special case for a zero initial avg_round_trip_time.
- Around line 3493-3512: The PONG handling in clusterMsgType processing is
reading extensions from the outbound PONG header instead of the received PING,
so it never finds the echoed timestamp. Update the CLUSTERMSG_TYPE_PONG path in
src/cluster_legacy.c to thread the parsed PING echo value through from the
receive side into writePingExtensions()/writePingTimeExtIfNonzero, using the
existing helpers getInitialPingExt, getNextPingExt, and
clusterMsgPingExtEchoTime to source the timestamp from the incoming PING
extension rather than hdr->extensions on the new send buffer.
- Around line 4511-4516: Clamp the RTT calculation in the CLUSTERMSG_TYPE_PONG
handling so negative values cannot reach updateCommandLatencyStats; when
computing current_rtt from mstime() and link->ping_echo_time, guard against a
backward clock step or stale ping_echo_time and force any negative sample to
zero before using it. Keep the fix in the pong-processing block that touches
link->ping_echo_time and updateCommandLatencyStats, so max_round_trip_time
cannot be polluted by an implicit uint32_t cast.
- Line 3629: The RTT handling in `writePingTimeExtIfNonzero()`/the
`ping_echo_time_ext` assignment is using the echoed timestamp without converting
it back to host order first. Normalize `link->ping_echo_time` with `ntohu64()`
before using it in `mstime() - link->ping_echo_time`, so the value stays
consistent with the `htonu64()` encoding path and works correctly across
mixed-endian peers.
In `@src/config.c`:
- Line 3452: The sliding_window_length_rtt_latency_stats config is currently
allowed to be 0, which can later be used as a divisor in
updateCommandLatencyStats and crash the process. Update the createIntConfig
entry for sliding-window-length-rtt-latency-stats so its minimum is 1 instead of
0, keeping the existing symbols and config name unchanged. Ensure the validation
matches the divisor usage in updateCommandLatencyStats so invalid zero values
cannot be set at runtime.
In `@tests/unit/cluster/cluster-shards.tcl`:
- Around line 108-111: Remove the debug logging from the cluster shard test: the
`puts` calls in the `cluster-shards` test are printing the full `CLUSTER SHARDS`
output for every node and should be deleted or guarded behind an existing
verbosity flag. Update the test block that iterates over nodes and prints `NODE
$i CLUSTER SHARDS OUTPUT` so it no longer emits these debug messages during
normal runs.
---
Nitpick comments:
In `@src/cluster_legacy.c`:
- Around line 3479-3514: The block in cluster_legacy.c needs to be reformatted
to match the project’s clang-format-18 style, since the current indentation and
brace placement around the server.cluster_nodes_latency_stats_enabled,
CLUSTERMSG_TYPE_PING/MEET, and CLUSTERMSG_TYPE_PONG logic are inconsistent with
surrounding code. Reformat this hunk in the affected function so the nesting,
braces, and alignment follow the existing style and will pass CI’s clang-format
check.
In `@tests/unit/cluster/cluster-shards.tcl`:
- Around line 96-127: The current test in cluster-shards.tcl only verifies that
max-round-trip-time and avg-round-trip-time fields exist, so it can pass even
when RTT is never measured. Update the CLUSTER SHARDS assertions in test "Verify
latency metrics are present per node in CLUSTER SHARDS output" to check that
primaries report a plausible non-zero avg-round-trip-time after
wait_for_cluster_propagation, using the existing R, CLUSTER SHARDS, and
shard_dict/node_dict parsing flow. Also remove the broad catch wrappers around
the SET/GET setup so real failures surface instead of being hidden.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f4caddcd-48f1-469a-a0b8-755cedcf3932
📒 Files selected for processing (5)
src/cluster_legacy.csrc/cluster_legacy.hsrc/config.csrc/server.htests/unit/cluster/cluster-shards.tcl
| if (type == CLUSTERMSG_TYPE_PONG && link && link->ping_echo_time != 0) { | ||
| mstime_t current_rtt = mstime() - link->ping_echo_time; | ||
| if (link->node) { | ||
| updateCommandLatencyStats(link->node, current_rtt); | ||
| } | ||
| link->ping_echo_time = 0; /* Clear immediately */ |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the reported region
sed -n '4488,4535p' src/cluster_legacy.c
echo '--- updateCommandLatencyStats references ---'
rg -n "updateCommandLatencyStats|max_round_trip_time|round_trip_time" src includeRepository: valkey-io/valkey
Length of output: 4022
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the surrounding cluster ping/pong logic and latency API signatures.
sed -n '3475,3535p' src/cluster_legacy.c
echo '--- declaration/definition of updateCommandLatencyStats ---'
rg -n "updateCommandLatencyStats\s*\(" -n src includeRepository: valkey-io/valkey
Length of output: 3482
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "ping_echo_time|mstime\(" src/cluster_legacy.c src/*.hRepository: valkey-io/valkey
Length of output: 4552
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show all ping_echo_time assignments/usages with context.
rg -n -C 3 "ping_echo_time" src/cluster_legacy.c src/cluster_legacy.h
# Inspect mstime() declaration/implementation if present in repo.
rg -n -C 2 "mstime\s*\(" src includeRepository: valkey-io/valkey
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show where link->ping_echo_time is written, reset, or compared.
rg -n -C 4 "ping_echo_time" src/cluster_legacy.c src/cluster_legacy.h
# Find mstime() type/behavior declarations if they live in-tree.
rg -n -C 2 "mstime_t|mstime\s*\(" src include depsRepository: valkey-io/valkey
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect mstime() implementation and the ping_echo_time lifecycle.
rg -n -C 4 "mstime\s*\(" .
echo '---'
rg -n -C 4 "ping_echo_time" src/cluster_legacy.c src/cluster_legacy.hRepository: valkey-io/valkey
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate mstime() and mstime_t definitions.
rg -n -C 2 "typedef .*mstime_t|mstime_t\b|mstime\s*\(" src includeRepository: valkey-io/valkey
Length of output: 50373
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '3478,3520p' src/cluster_legacy.c
echo '---'
sed -n '3618,3642p' src/cluster_legacy.c
echo '---'
sed -n '1,80p' src/util.hRepository: valkey-io/valkey
Length of output: 7197
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the sender-side helper and the PONG handling function boundaries.
sed -n '3468,3515p' src/cluster_legacy.c
echo '---'
sed -n '3560,3650p' src/cluster_legacy.cRepository: valkey-io/valkey
Length of output: 7927
Clamp RTT before updating latency stats. A backward clock step or stale ping_echo_time can make mstime() - link->ping_echo_time negative, and the implicit cast to uint32_t will turn that into a huge sample that skews max_round_trip_time.
🤖 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 `@src/cluster_legacy.c` around lines 4511 - 4516, Clamp the RTT calculation in
the CLUSTERMSG_TYPE_PONG handling so negative values cannot reach
updateCommandLatencyStats; when computing current_rtt from mstime() and
link->ping_echo_time, guard against a backward clock step or stale
ping_echo_time and force any negative sample to zero before using it. Keep the
fix in the pong-processing block that touches link->ping_echo_time and
updateCommandLatencyStats, so max_round_trip_time cannot be polluted by an
implicit uint32_t cast.
| // if (node->max_round_trip_time) { | ||
| addReplyBulkCString(c, "max-round-trip-time"); | ||
| addReplyLongLong(c, node->max_round_trip_time); | ||
| reply_count++; | ||
| // } | ||
| // if (node->avg_round_trip_time) { | ||
| addReplyBulkCString(c, "avg-round-trip-time"); | ||
| addReplyLongLong(c, node->avg_round_trip_time); | ||
| reply_count++; | ||
| // } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Drop the commented-out conditional guards.
The // if (node->max_round_trip_time) / // if (node->avg_round_trip_time) lines are dead. Either emit unconditionally (current effective behavior) or restore the guards, but don't leave them commented.
🤖 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 `@src/cluster_legacy.c` around lines 7409 - 7418, The
`node->max_round_trip_time` and `node->avg_round_trip_time` conditional checks
in the reply-building block are commented out dead code. Clean up this section
by either removing those `if` guards entirely and keeping the unconditional
`addReplyBulkCString`/`addReplyLongLong` calls, or restoring the guards as real
conditionals in the same block. Use the existing reply logic around
`reply_count` in `src/cluster_legacy.c` to keep the behavior consistent.
| # Print the raw structure to your terminal log | ||
| puts "=== NODE $i CLUSTER SHARDS OUTPUT ===" | ||
| puts $shards | ||
| puts "=====================================" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove debug puts from the test.
Printing full CLUSTER SHARDS output for every node clutters CI logs. Drop these puts lines (or gate behind a verbosity flag).
🤖 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/cluster/cluster-shards.tcl` around lines 108 - 111, Remove the
debug logging from the cluster shard test: the `puts` calls in the
`cluster-shards` test are printing the full `CLUSTER SHARDS` output for every
node and should be deleted or guarded behind an existing verbosity flag. Update
the test block that iterates over nodes and prints `NODE $i CLUSTER SHARDS
OUTPUT` so it no longer emits these debug messages during normal runs.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #4093 +/- ##
============================================
+ Coverage 76.84% 76.86% +0.02%
============================================
Files 162 162
Lines 81479 81533 +54
============================================
+ Hits 62610 62669 +59
+ Misses 18869 18864 -5
🚀 New features to boost your workflow:
|
![valkey-review-bot[bot]](https://avatars.githubusercontent.com/in/3633497?s=60&v=4){kind=small}
| if(msg_type == CLUSTERMSG_TYPE_PING || msg_type == CLUSTERMSG_TYPE_MEET) { | ||
| //pass on fresh ts | ||
| uint64_t current_time = mstime(); | ||
| extensions += writePingTimeExtIfNonzero(&totlen, &cursor, CLUSTERMSG_EXT_TYPE_PING_ECHO_TIME, current_time); |
There was a problem hiding this comment.
Peers only advertise generic extension support today, so this line sends CLUSTERMSG_EXT_TYPE_PING_ECHO_TIME to every extension-capable node. Older peers that haven't learned this specific type do not negotiate it away — they only hit the Received unknown extension type warning path in clusterProcessPingExtensions() (src/cluster_legacy.c:3643-3644), so a mixed-version cluster will log a warning on every PING/MEET once this config is enabled. The existing light-header features avoid this with a dedicated capability bit first (src/cluster_legacy.h:63-69); the RTT extension needs the same kind of gate before it is sent.
| clusterMsgPingExtEchoTime *ping_echo_time_ext = | ||
| (clusterMsgPingExtEchoTime *)&(ext->ext[0].ping_echo_time); | ||
| link -> ping_echo_time = ping_echo_time_ext->ping_echo_time; | ||
| break; |
There was a problem hiding this comment.
break exits the entire extension loop the first time we see PING_ECHO_TIME. writePingExtensions() appends this RTT extension before the forgotten-node and shard-id extensions (src/cluster_legacy.c:3516-3548), so with latency stats enabled we stop processing those later extensions on every PING/MEET. That silently drops blacklisted-node gossip and shard-id updates; store link->ping_echo_time here and continue scanning the remaining extensions instead.
|
|
||
| if(server.cluster_nodes_latency_stats_enabled) { | ||
| // if (node->max_round_trip_time) { | ||
| addReplyBulkCString(c, "max-round-trip-time"); |
There was a problem hiding this comment.
These reply members change the CLUSTER SHARDS node shape, but src/commands/cluster-shards.json still declares the node object with additionalProperties: false and only the old fields. The test harness uses COMMAND DOCS reply schemas for validation (tests/support/response_transformers.tcl:8-15), so once cluster-nodes-latency-stats-enabled is on this output no longer matches the documented schema. Please add max-round-trip-time and avg-round-trip-time to the command schema in the same patch.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cluster_legacy.c (1)
973-1000: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject malformed migration tokens in
src/cluster_legacy.c:987-999.atoi(argv[j] + 1)accepts non-numeric slots as 0, and the]check still ignores trailing garbage, so[x->-<valid-id>]junkcan load as slot 0 instead of failing. Parse the slot withstrtoland require]to terminate the token.🤖 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 `@src/cluster_legacy.c` around lines 973 - 1000, In the migration-token parsing block, replace atoi(argv[j] + 1) with strict strtol parsing, validating that the slot text is numeric, within range, and followed by the expected migration syntax. Update the closing-bracket handling after strchr in the relevant parser function to require ']' as the token terminator with no trailing characters (such as “]junk”), rejecting malformed input before loading it.Source: Linters/SAST tools
🤖 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 `@src/cluster_legacy.c`:
- Around line 8686-8690: In the node parsing logic around verifyClusterNodeId,
validate that k equals CLUSTER_NAMELEN and that *s is a comma before calling
verifyClusterNodeId; return C_ERR for either failure, then validate the fully
initialized node_name.
- Around line 2405-2408: Guard every currentEpoch increment in the relevant bump
logic, including the collision-handler path, by checking against UINT64_MAX
before incrementing; update the symbols handling configEpoch/currentEpoch so no
currentEpoch++ can wrap to zero while preserving the existing strict-ordering
behavior.
---
Outside diff comments:
In `@src/cluster_legacy.c`:
- Around line 973-1000: In the migration-token parsing block, replace
atoi(argv[j] + 1) with strict strtol parsing, validating that the slot text is
numeric, within range, and followed by the expected migration syntax. Update the
closing-bracket handling after strchr in the relevant parser function to require
']' as the token terminator with no trailing characters (such as “]junk”),
rejecting malformed input before loading it.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19be4529-ca04-4c90-8042-c85aed288834
📒 Files selected for processing (4)
src/cluster_legacy.csrc/cluster_legacy.hsrc/config.csrc/server.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/config.c
- src/server.h
- src/cluster_legacy.h
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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cluster_legacy.c (1)
973-1000: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReject malformed migration tokens in
src/cluster_legacy.c:987-999.atoi(argv[j] + 1)accepts non-numeric slots as 0, and the]check still ignores trailing garbage, so[x->-<valid-id>]junkcan load as slot 0 instead of failing. Parse the slot withstrtoland require]to terminate the token.🤖 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 `@src/cluster_legacy.c` around lines 973 - 1000, In the migration-token parsing block, replace atoi(argv[j] + 1) with strict strtol parsing, validating that the slot text is numeric, within range, and followed by the expected migration syntax. Update the closing-bracket handling after strchr in the relevant parser function to require ']' as the token terminator with no trailing characters (such as “]junk”), rejecting malformed input before loading it.Source: Linters/SAST tools
🤖 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 `@src/cluster_legacy.c`:
- Around line 8686-8690: In the node parsing logic around verifyClusterNodeId,
validate that k equals CLUSTER_NAMELEN and that *s is a comma before calling
verifyClusterNodeId; return C_ERR for either failure, then validate the fully
initialized node_name.
- Around line 2405-2408: Guard every currentEpoch increment in the relevant bump
logic, including the collision-handler path, by checking against UINT64_MAX
before incrementing; update the symbols handling configEpoch/currentEpoch so no
currentEpoch++ can wrap to zero while preserving the existing strict-ordering
behavior.
---
Outside diff comments:
In `@src/cluster_legacy.c`:
- Around line 973-1000: In the migration-token parsing block, replace
atoi(argv[j] + 1) with strict strtol parsing, validating that the slot text is
numeric, within range, and followed by the expected migration syntax. Update the
closing-bracket handling after strchr in the relevant parser function to require
']' as the token terminator with no trailing characters (such as “]junk”),
rejecting malformed input before loading it.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 19be4529-ca04-4c90-8042-c85aed288834
📒 Files selected for processing (4)
src/cluster_legacy.csrc/cluster_legacy.hsrc/config.csrc/server.h
🚧 Files skipped from review as they are similar to previous changes (3)
- src/config.c
- src/server.h
- src/cluster_legacy.h
🛑 Comments failed to post (2)
src/cluster_legacy.c (2)
2405-2408: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -C5 'clusterGetMaxEpoch|currentEpoch\+\+|configEpoch' src/cluster_legacy.c rg -n 'BUMPEPOCH|configEpoch|currentEpoch' tests || trueRepository: valkey-io/valkey
Length of output: 30993
Guard
currentEpochbefore incrementing. If it reachesUINT64_MAX,currentEpoch++wraps to0and breaks the strict ordering guarantee. Add the same bound check in both bump paths, including the collision handler.🤖 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 `@src/cluster_legacy.c` around lines 2405 - 2408, Guard every currentEpoch increment in the relevant bump logic, including the collision-handler path, by checking against UINT64_MAX before incrementing; update the symbols handling configEpoch/currentEpoch so no currentEpoch++ can wrap to zero while preserving the existing strict-ordering behavior.
8686-8690: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash rg -n -C8 'char node_name\[CLUSTER_NAMELEN\]|verifyClusterNodeId\(node_name' src/cluster_legacy.c rg -n 'clusterDecodeOpenSlotsAuxField|open slots.*aux' tests src || trueRepository: valkey-io/valkey
Length of output: 1298
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the validator and the surrounding decode logic. rg -n -C20 'int verifyClusterNodeId|verifyClusterNodeId\(' src/cluster_legacy.c src/*.c src/*.h sed -n '1,220p' src/cluster_legacy.c | cat -n sed -n '8661,8710p' src/cluster_legacy.c | cat -nRepository: valkey-io/valkey
Length of output: 50372
Check the node length before calling
verifyClusterNodeId.
node_nameis only filled up tok, soverifyClusterNodeId(node_name, CLUSTER_NAMELEN)can read uninitialized stack bytes when the ID is short. Rejectk != CLUSTER_NAMELEN(and the missing comma) first, then validate the ID.🤖 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 `@src/cluster_legacy.c` around lines 8686 - 8690, In the node parsing logic around verifyClusterNodeId, validate that k equals CLUSTER_NAMELEN and that *s is a comma before calling verifyClusterNodeId; return C_ERR for either failure, then validate the fully initialized node_name.
Signed-off-by: ydsakshi <ydsakshi023@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/cluster_legacy.c (1)
3520-3556: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRun
clang-format-18on this block.The indentation and brace placement in the latency-stats block (mixed leading whitespace, dangling
else{, stray blank/whitespace lines) don't match the surrounding style and will fail the clang-format CI check.As per coding guidelines: "When touching C/C++ sources or headers, run
clang-format-18 -ion the modified files before finalizing when the tool is available."🤖 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 `@src/cluster_legacy.c` around lines 3520 - 3556, Run clang-format-18 on the modified C source so the latency-stats block in the cluster message extension logic is consistently indented and its braces and whitespace match project style. Preserve the existing behavior of the server.cluster_nodes_latency_stats_enabled handling and its hdr, message-type, and ping_echo_time branches.Source: Coding guidelines
🤖 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 `@src/cluster_legacy.c`:
- Around line 3534-3554: Update the CLUSTERMSG_TYPE_PONG handling to use the
reply peer/link supplied by the surrounding message-processing path instead of
calling clusterLookupNode with hdr->sender. Retrieve the ping_echo_time from
that peer’s inbound_link or link, emit the CLUSTERMSG_EXT_TYPE_PING_ECHO_TIME
extension, and reset the selected link’s ping_echo_time after emission.
---
Nitpick comments:
In `@src/cluster_legacy.c`:
- Around line 3520-3556: Run clang-format-18 on the modified C source so the
latency-stats block in the cluster message extension logic is consistently
indented and its braces and whitespace match project style. Preserve the
existing behavior of the server.cluster_nodes_latency_stats_enabled handling and
its hdr, message-type, and ping_echo_time branches.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 5d5baf16-b6be-4dff-9338-b10643b887f2
📒 Files selected for processing (2)
src/cluster_legacy.csrc/cluster_legacy.h
🚧 Files skipped from review as they are similar to previous changes (1)
- src/cluster_legacy.h
| else if(msg_type == CLUSTERMSG_TYPE_PONG) { | ||
|
|
||
| clusterNode *node = clusterLookupNode(hdr->sender, CLUSTER_NAMELEN); | ||
| if (node) { | ||
| uint64_t extracted_echo_time = 0; | ||
| clusterLink *active_link = NULL; | ||
|
|
||
| if (node -> inbound_link && node -> inbound_link -> ping_echo_time != 0) { | ||
| active_link = node -> inbound_link; | ||
| extracted_echo_time = node -> inbound_link -> ping_echo_time; | ||
| } | ||
| else if (node -> link && node -> link -> ping_echo_time != 0) { | ||
| active_link = node -> link; | ||
| extracted_echo_time = node -> link -> ping_echo_time; | ||
| } | ||
|
|
||
| if(extracted_echo_time != 0) { | ||
| extensions += writePingTimeExtIfNonzero(&totlen, &cursor, CLUSTERMSG_EXT_TYPE_PING_ECHO_TIME, extracted_echo_time); | ||
| active_link -> ping_echo_time = 0; //reset the echo time after sending it in PONG | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Confirm hdr->sender is set to myself before writePingExtensions() runs.
rg -n "memcpy\(hdr->sender|clusterBuildMessageHdr|writePingExtensions\(" src/cluster_legacy.c
echo "--- clusterBuildMessageHdr body ---"
ast-grep run --pattern 'void clusterBuildMessageHdr($$$) { $$$ }' --lang c src/cluster_legacy.cRepository: valkey-io/valkey
Length of output: 1061
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- writePingExtensions ---"
sed -n '3490,3565p' src/cluster_legacy.c
echo "--- clusterBuildMessageHdr ---"
sed -n '4888,4925p' src/cluster_legacy.c
echo "--- ping echo time handling ---"
sed -n '3655,3680p' src/cluster_legacy.c
echo "--- RTT computation path ---"
sed -n '4535,4555p' src/cluster_legacy.c
echo "--- send path around writePingExtensions ---"
sed -n '5038,5140p' src/cluster_legacy.cRepository: valkey-io/valkey
Length of output: 11914
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- clusterNode / clusterLink definitions ---"
rg -n "typedef struct clusterNode|clusterLink|inbound_link|link;" src/cluster_legacy.c src/*.h
echo "--- myself initialization / link assignment ---"
rg -n "myself->link|myself->inbound_link|node->link =|node->inbound_link =" src/cluster_legacy.c src/*.c
echo "--- places where ping_echo_time is set or cleared ---"
rg -n "ping_echo_time" src/cluster_legacy.cRepository: valkey-io/valkey
Length of output: 9423
Use the peer link when emitting the PONG echo timestamp.
clusterLookupNode(hdr->sender, CLUSTER_NAMELEN) resolves to myself here because clusterBuildMessageHdr() already overwrites hdr->sender with myself->name. That leaves inbound_link/link unset for the lookup result, so extracted_echo_time stays 0 and the PONG echo extension is never written. Thread the reply peer/link into this path instead of deriving it from hdr->sender.
🤖 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 `@src/cluster_legacy.c` around lines 3534 - 3554, Update the
CLUSTERMSG_TYPE_PONG handling to use the reply peer/link supplied by the
surrounding message-processing path instead of calling clusterLookupNode with
hdr->sender. Retrieve the ping_echo_time from that peer’s inbound_link or link,
emit the CLUSTERMSG_EXT_TYPE_PING_ECHO_TIME extension, and reset the selected
link’s ping_echo_time after emission.
1 participant
Resolves #1917
Description
tba