refactor: use RESPParser for Redis replication by BorysTheDev · Pull Request #8211 · dragonflydb/dragonfly

BorysTheDev · 2026-09-01T14:01:42Z

Related to: #8196
Summary: Refactors Redis upstream-replication command parsing around RESPParser.

Changes:

Adds resettable parser limits plus consumed-byte and buffered-input reporting.
Bounds protocol-error payload logging to a small peer-controlled data prefix.
Adds ReadRespCommand to parse and copy flat command arrays into CommandContext.
Allows the staging I/O buffer to be drained immediately after parser input is copied.
Migrates Redis stable-stream consumption away from server-mode RedisParser.
Preserves per-command wire-byte counts used to advance replication ACK offsets.
Simplifies parser reset calls across protocol-client users and adds a command-parser reset path.
Adds unit coverage for streaming state, parser reset behavior, and array-size limits.

Technical Notes: Redis replication retains its higher array-count allowance; bulk, line, and nesting limits remain documented follow-up work.

qodo-code-review · 2026-09-01T14:02:49Z

PR Summary by Qodo

Use RESPParser for Redis replication commands

🐞 Bug fix ✨ Enhancement 🧪 Tests 🕐 40+ Minutes

AI Description

• Routes Redis replication commands through RESPParser for reliable fragmented and buffered stream
 handling.
• Tracks exact wire consumption and validates flat string-array commands before dispatch.
• Adds parser limits, reset controls, bounded diagnostics, and streaming-state tests.

Diagram

sequenceDiagram
  actor M as Redis Master
  participant S as Socket Buffer
  participant C as Protocol Client
  participant P as RESPParser
  participant R as Replica Loop
  participant D as Command Dispatch
  M->>S: RESP command bytes
  R->>C: Read command
  C->>S: Receive buffered bytes
  C->>P: Feed stream chunks
  P-->>C: Command and wire size
  C-->>R: Backed arguments
  R->>D: Dispatch batch
  R-->>M: Advance ACK offset

High-Level Assessment

The following are alternative approaches to this PR:

1. Repair the legacy RedisParser path

➕ Preserves the existing replication parsing representation
➕ Avoids constructing hiredis reply objects before copying arguments
➖ Retains known partial-read and parser-state complexity
➖ Continues maintaining two distinct parsing behaviors
➖ Makes exact fragmented-command byte accounting harder

2. Parse directly into BackedArguments

➕ Avoids the intermediate RESP object tree
➕ Could reduce allocations on the replication hot path
➖ Requires a new streaming parser or tighter hiredis integration
➖ Couples wire parsing directly to command storage
➖ Adds substantially more correctness and security risk

3. Migrate all ProtocolClient paths together

➕ Removes the legacy parser and compatibility helpers immediately
➕ Provides one consistent parser lifecycle
➖ Greatly expands scope and regression risk
➖ Mixes replication correctness changes with unrelated response handling

Recommendation: The incremental RESPParser migration is the best correctness-focused approach: it reuses a tested streaming parser while isolating the high-risk replication path. Retain the legacy response path temporarily as proposed, but benchmark reply-tree allocation overhead and consider direct argument materialization only if replication throughput regresses.

Files changed (8) +224 / -41

Bug fix (1) +11 / -19

replica.ccUse RESPParser in the Redis replication loop +11/-19

Use RESPParser in the Redis replication loop

• Builds reusable command contexts directly from RESPParser output instead of converting legacy response expressions. Replication batching now uses parser-buffer state, while exact consumed bytes drive deferred acknowledgements and offsets.

src/server/replica.cc

Refactor (2) +3 / -3

coordinator.ccAdopt unified parser reset API +1/-1

Adopt unified parser reset API
• Updates cross-shard client initialization to reset the default response parsers without specifying a legacy RedisParser mode.
src/server/cluster/coordinator.cc

outgoing_slot_migration.ccAdopt unified parser resets for migration +2/-2

Adopt unified parser resets for migration
• Updates outgoing migration flow and synchronization setup to use the mode-free response-parser reset API.
src/server/cluster/outgoing_slot_migration.cc

Tests (1) +45 / -0

resp_parser_test.ccTest streaming state and array limits +45/-0

Test streaming state and array limits

• Covers fragmented arrays, multiple buffered replies, large replies, consumed-byte accounting, resets, and buffered-input state. Verifies that configured array limits reject oversized arrays.

src/facade/resp_parser_test.cc

Other (4) +165 / -19

resp_parser.ccAdd resettable limits and consumed-byte tracking +37/-6

Add resettable limits and consumed-byte tracking

• Adds configurable parser construction and reset support, applies hiredis array limits, and reports exact bytes consumed across fragmented or buffered replies. Parser errors now log only a bounded prefix of peer-controlled input.

src/facade/resp_parser.cc

resp_parser.hExpose RESPParser streaming-state controls +20/-2

Expose RESPParser streaming-state controls

• Introduces array-length limits, reset overloads, consumed-byte output, and buffered-input inspection. Copy construction and assignment are explicitly disabled for reader ownership safety.

src/facade/resp_parser.h

protocol_client.ccParse replication commands with RESPParser +94/-8

Parse replication commands with RESPParser

• Adds streaming command reads that validate flat string arrays, copy arguments into owned storage, and return exact wire sizes plus buffered-data state. Separates ordinary response-parser resets from replication command-parser setup and applies a larger replication array limit.

src/server/protocol_client.cc

protocol_client.hDefine the RESP command-reading interface +14/-3

Define the RESP command-reading interface

• Adds command-read result metadata and the ReadRespCommand API for owned arguments. Splits response and replication parser reset methods while documenting the incremental migration from the legacy parser.

src/server/protocol_client.h

qodo-free-for-open-source-projects · 2026-09-01T14:04:28Z

Code Review by Qodo

🐞 Bugs (4) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

🟠 Medium

1. Header lines grow unbounded 🐞 Bug ⛨ Security

Description

RESPParser has no line-length limit, so an upstream that streams an unterminated bulk or array
length header causes hiredis's internal input buffer to grow until memory exhaustion. The previous
parser rejected fragmented length headers once they exceeded its fixed 32-byte buffer.

Code

src/server/protocol_client.cc[R563-564]

+  // TODO: Add bulk-length, line-length, and nesting-depth limits to RESPParser.
+  resp_parser_.Reset({.max_array_len = max_array_len});

Relevance

●●● Strong
Unterminated peer-controlled headers can grow parser buffering; adding a deterministic line-length
cap is consistent with security hardening.
PR-#6649

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Hiredis appends every feed to its internal SDS buffer, while bulk-header processing simply waits
when seekNewline cannot find a terminator. In contrast, RedisParser::ParseLen returned BAD_ARRAYLEN
when a fragmented header no longer fit its 32-byte small_buf_.
src/redis/read.c[397-407]
src/redis/read.c[710-733]
src/facade/redis_parser.cc[267-287]
src/facade/redis_parser.h[119-125]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new replication parser buffers unterminated RESP length lines without a maximum. A malformed upstream can continuously send a header without CRLF and exhaust replica memory before parsing produces an error.
## Issue Context
The old RedisParser capped fragmented length headers through its 32-byte small buffer. Add a RESPParser/hiredis line-length limit and reject an incomplete header as soon as it exceeds that bound.
## Fix Focus Areas
- src/server/protocol_client.cc[558-564]
- src/facade/resp_parser.h[85-105]
- src/facade/resp_parser.cc[28-34]
- src/redis/read.c[397-407]
- src/redis/read.c[710-733]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Replication bulk size unbounded 🐞 Bug ⛨ Security

Description

ResetCommandParser configures only an array-element limit and omits FLAGS_max_bulk_len, so
ReadRespCommand accepts peer-declared bulk strings beyond the previous configured bound. Hiredis
copies and retains received fragments until the complete payload is buffered, then allocates the
parsed string, creating a newly introduced remote memory-exhaustion path for a malformed or
compromised upstream peer.

Code

src/server/protocol_client.cc[R561-564]

+  // An upstream master's commands may contain more arguments than regular client requests.
+  uint32_t max_array_len = max(GetFlag(FLAGS_max_multi_bulk_len), 1u << 20);
+  // TODO: Add bulk-length, line-length, and nesting-depth limits to RESPParser.
+  resp_parser_.Reset({.max_array_len = max_array_len});

Relevance

●●● Strong
Missing peer-controlled payload bound is a concrete memory-exhaustion risk; repository history
accepts comparable untrusted-input guards.
PR-#6649

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new replication reset sets only max_array_len and records bulk limits as missing, whereas the
retired RedisParser explicitly compared each declared bulk length with max_bulk_len_ and
rejected oversized payloads before consuming them. Hiredis checks only that the declared length fits
its numeric types, appends each feed to its internal SDS buffer, waits for the full declared
content, and then allocates len+1 bytes for the reply, demonstrating that the new path has no
application-level bulk payload bound and may duplicate the buffered data.

src/server/protocol_client.cc[561-564]
src/facade/redis_parser.cc[380-395]
src/redis/read.c[417-447]
src/redis/read.c[710-738]
src/server/protocol_client.cc[552-564]
src/redis/read.c[397-447]
src/redis/hiredis.c[125-143]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new `RESPParser` replication path does not enforce `FLAGS_max_bulk_len`, allowing a peer-controlled bulk payload to accumulate without the previous configured bound. Hiredis buffers the complete declared payload and then allocates storage for the parsed string, allowing a malformed or compromised upstream master to exhaust replica memory.
## Issue Context
The previous server-mode `RedisParser` rejected oversized declared bulk lengths before consuming their payload, while `RESPParser`/hiredis retains supplied input until a complete bulk reply is available. Add a bulk-length limit to `RESPParser::Limits`, enforce it while parsing the bulk header, and configure it from `FLAGS_max_bulk_len` in `ResetCommandParser()`.
## Fix Focus Areas
- src/server/protocol_client.cc[558-564]
- src/facade/resp_parser.h[85-105]
- src/facade/resp_parser.cc[28-34]
- src/facade/resp_parser.cc[65-99]
- src/redis/read.c[397-447]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. Replication parser loses limits 🐞 Bug ☼ Reliability

Description

ResetCommandParser switches the peer-controlled replication stream to RESPParser even though it
lacks the previous bulk-length, line-length, and nesting limits. An upstream can therefore make the
replica buffer an arbitrarily long incomplete value or materialize deeply nested data, causing
excessive memory use before the command is rejected.

Code

src/server/protocol_client.cc[R563-564]

+  // TODO: Add bulk-length, line-length, and nesting-depth limits to RESPParser.
+  resp_parser_.Reset({.max_array_len = max_array_len});

Evidence
ResetCommandParser explicitly acknowledges that RESPParser lacks bulk, line, and nesting limits and
configures only max_array_len. The previous RedisParser rejects oversized bulk strings, caps
nesting, and in server mode rejects nested aggregate commands; hiredis instead waits for the
complete declared bulk payload while Feed retains all supplied bytes.
src/server/protocol_client.cc[558-564]
src/facade/resp_parser.h[87-105]
src/facade/resp_parser.cc[78-88]
src/facade/redis_parser.cc[330-359]
src/facade/redis_parser.cc[380-395]
src/redis/read.c[397-466]
src/redis/read.c[469-505]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new RESPParser-based replication command path omits limits previously enforced by RedisParser. Preserve the existing parser for replication until RESPParser can reject oversized bulk strings, overlong lines, and excessive nesting incrementally, or add equivalent limits to RESPParser before enabling this path.
## Issue Context
RESPParser currently limits only each aggregate's element count. Hiredis otherwise continues buffering incomplete bulk strings and grows its nesting stack, while the previous RedisParser rejected oversized bulk lengths and nested server-mode input.
## Fix Focus Areas
- src/server/protocol_client.cc[558-564]
- src/facade/resp_parser.h[85-116]
- src/facade/resp_parser.cc[65-99]
- src/facade/redis_parser.cc[330-359]
- src/facade/redis_parser.cc[380-395]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

View medium (1)

4. Nested commands exhaust parser memory 🐞 Bug ⛨ Security

Description

The replacement replication RESPParser limits each aggregate's element count but has no aggregate
nesting-depth limit, so repeated one-element *1 arrays can grow hiredis's parsing task stack and
allocate nested reply objects without bound. Unlike the legacy parser, which rejected nested
aggregates in server mode and capped general nesting at 64 levels, the new path rejects a malformed
flat command only after the nested structure has already been allocated, allowing a malformed
upstream to exhaust replica memory.

Code

src/server/protocol_client.cc[R561-564]

+  // An upstream master's commands may contain more arguments than regular client requests.
+  uint32_t max_array_len = max(GetFlag(FLAGS_max_multi_bulk_len), 1u << 20);
+  // TODO: Add bulk-length, line-length, and nesting-depth limits to RESPParser.
+  resp_parser_.Reset({.max_array_len = max_array_len});

Relevance

●● Moderate
Potential parser-stack exhaustion is plausible, but nested RESP behavior and intended replication
limits require architectural confirmation.
ⓘ Recommendations generated based on similar findings in past PRs

Evidence
RESPParser::Limits contains only max_array_len, so each nested *1 remains within the
configured element limit. Hiredis expands its parser task array as nesting increases, advances to
another task for every nonempty aggregate, and allocates an aggregate reply object for each header,
while the previous parser rejected nested aggregates in replication's former server mode and
enforced a general depth cap of 64.
src/server/protocol_client.cc[561-564]
src/redis/read.c[494-559]
src/redis/hiredis.c[161-176]
src/facade/redis_parser.cc[18-22]
src/facade/redis_parser.cc[357-360]
src/facade/resp_parser.h[85-105]
src/redis/read.c[469-505]
src/redis/read.c[549-560]
src/facade/redis_parser.cc[336-360]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Replication command parsing no longer bounds aggregate nesting. A peer can keep every array within `max_array_len` by sending arbitrarily many nested `*1` aggregates, causing hiredis parser-task and reply-object memory to grow continuously before flat-command validation rejects the input.
## Issue Context
The configured per-aggregate element cap does not constrain aggregate depth. The previous server-mode parser rejected nested aggregates entirely, and its general aggregate parser capped nesting at 64 levels; add a suitable nesting-depth limit to `RESPParser::Limits`, enforce it before hiredis grows its task stack or allocates nested reply objects, and configure it for replication command mode.
## Fix Focus Areas
- src/server/protocol_client.cc[558-564]
- src/facade/resp_parser.h[85-105]
- src/facade/resp_parser.cc[28-99]
- src/redis/read.c[469-560]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Tip of the day

💡 Did you know, you can ask Qodo to dismiss a finding you disagree with, with your reason on record

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Powered by Qodo

augmentcode · 2026-09-01T14:05:20Z

🤖 Augment PR Summary

Summary: Refactors Redis upstream-replication command parsing around RESPParser.

Changes:

Adds resettable parser limits plus consumed-byte and buffered-input reporting.
Bounds protocol-error payload logging to a small peer-controlled data prefix.
Adds ReadRespCommand to parse and copy flat command arrays into CommandContext.
Allows the staging I/O buffer to be drained immediately after parser input is copied.
Migrates Redis stable-stream consumption away from server-mode RedisParser.
Preserves per-command wire-byte counts used to advance replication ACK offsets.
Simplifies parser reset calls across protocol-client users and adds a command-parser reset path.
Adds unit coverage for streaming state, parser reset behavior, and array-size limits.

Technical Notes: Redis replication retains its higher array-count allowance; bulk, line, and nesting limits remain documented follow-up work.

_{🤖 Was this summary useful? React with 👍 or 👎}

augmentcode

Review completed. No suggestions at this time.

Comment augment review to trigger a new review at any time.

qodo-code-review · 2026-09-01T14:05:58Z

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

🔴 High

1. Header lines grow unbounded 🐞 Bug ⛨ Security

Description

RESPParser has no line-length limit, so an upstream that streams an unterminated bulk or array
length header causes hiredis's internal input buffer to grow until memory exhaustion. The previous
parser rejected fragmented length headers once they exceeded its fixed 32-byte buffer.

Code

src/server/protocol_client.cc[R563-564]

+  // TODO: Add bulk-length, line-length, and nesting-depth limits to RESPParser.
+  resp_parser_.Reset({.max_array_len = max_array_len});

Relevance

●●● Strong
Unterminated peer-controlled headers can grow parser buffering; adding a deterministic line-length
cap is consistent with security hardening.
PR-#6649

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
Hiredis appends every feed to its internal SDS buffer, while bulk-header processing simply waits
when seekNewline cannot find a terminator. In contrast, RedisParser::ParseLen returned BAD_ARRAYLEN
when a fragmented header no longer fit its 32-byte small_buf_.
src/redis/read.c[397-407]
src/redis/read.c[710-733]
src/facade/redis_parser.cc[267-287]
src/facade/redis_parser.h[119-125]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new replication parser buffers unterminated RESP length lines without a maximum. A malformed upstream can continuously send a header without CRLF and exhaust replica memory before parsing produces an error.

## Issue Context
The old RedisParser capped fragmented length headers through its 32-byte small buffer. Add a RESPParser/hiredis line-length limit and reject an incomplete header as soon as it exceeds that bound.

## Fix Focus Areas
- src/server/protocol_client.cc[558-564]
- src/facade/resp_parser.h[85-105]
- src/facade/resp_parser.cc[28-34]
- src/redis/read.c[397-407]
- src/redis/read.c[710-733]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

2. Replication bulk size unbounded 🐞 Bug ⛨ Security

Description

ResetCommandParser configures only an array-element limit and omits FLAGS_max_bulk_len, so
ReadRespCommand accepts peer-declared bulk strings beyond the previous configured bound. Hiredis
copies and retains received fragments until the complete payload is buffered, then allocates the
parsed string, creating a newly introduced remote memory-exhaustion path for a malformed or
compromised upstream peer.

Code

src/server/protocol_client.cc[R561-564]

+  // An upstream master's commands may contain more arguments than regular client requests.
+  uint32_t max_array_len = max(GetFlag(FLAGS_max_multi_bulk_len), 1u << 20);
+  // TODO: Add bulk-length, line-length, and nesting-depth limits to RESPParser.
+  resp_parser_.Reset({.max_array_len = max_array_len});

Relevance

●●● Strong
Missing peer-controlled payload bound is a concrete memory-exhaustion risk; repository history
accepts comparable untrusted-input guards.
PR-#6649

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The new replication reset sets only max_array_len and records bulk limits as missing, whereas the
retired RedisParser explicitly compared each declared bulk length with max_bulk_len_ and
rejected oversized payloads before consuming them. Hiredis checks only that the declared length fits
its numeric types, appends each feed to its internal SDS buffer, waits for the full declared
content, and then allocates len+1 bytes for the reply, demonstrating that the new path has no
application-level bulk payload bound and may duplicate the buffered data.

src/server/protocol_client.cc[561-564]
src/facade/redis_parser.cc[380-395]
src/redis/read.c[417-447]
src/redis/read.c[710-738]
src/server/protocol_client.cc[552-564]
src/redis/read.c[397-447]
src/redis/hiredis.c[125-143]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new `RESPParser` replication path does not enforce `FLAGS_max_bulk_len`, allowing a peer-controlled bulk payload to accumulate without the previous configured bound. Hiredis buffers the complete declared payload and then allocates storage for the parsed string, allowing a malformed or compromised upstream master to exhaust replica memory.

## Issue Context
The previous server-mode `RedisParser` rejected oversized declared bulk lengths before consuming their payload, while `RESPParser`/hiredis retains supplied input until a complete bulk reply is available. Add a bulk-length limit to `RESPParser::Limits`, enforce it while parsing the bulk header, and configure it from `FLAGS_max_bulk_len` in `ResetCommandParser()`.

## Fix Focus Areas
- src/server/protocol_client.cc[558-564]
- src/facade/resp_parser.h[85-105]
- src/facade/resp_parser.cc[28-34]
- src/facade/resp_parser.cc[65-99]
- src/redis/read.c[397-447]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

3. Nested commands exhaust parser memory 🐞 Bug ⛨ Security

Description

The replacement replication RESPParser limits each aggregate's element count but has no aggregate
nesting-depth limit, so repeated one-element *1 arrays can grow hiredis's parsing task stack and
allocate nested reply objects without bound. Unlike the legacy parser, which rejected nested
aggregates in server mode and capped general nesting at 64 levels, the new path rejects a malformed
flat command only after the nested structure has already been allocated, allowing a malformed
upstream to exhaust replica memory.

Code

src/server/protocol_client.cc[R561-564]

+  // An upstream master's commands may contain more arguments than regular client requests.
+  uint32_t max_array_len = max(GetFlag(FLAGS_max_multi_bulk_len), 1u << 20);
+  // TODO: Add bulk-length, line-length, and nesting-depth limits to RESPParser.
+  resp_parser_.Reset({.max_array_len = max_array_len});

Relevance

●● Moderate
Potential parser-stack exhaustion is plausible, but nested RESP behavior and intended replication
limits require architectural confirmation.
ⓘ Recommendations generated based on similar findings in past PRs

Evidence
RESPParser::Limits contains only max_array_len, so each nested *1 remains within the
configured element limit. Hiredis expands its parser task array as nesting increases, advances to
another task for every nonempty aggregate, and allocates an aggregate reply object for each header,
while the previous parser rejected nested aggregates in replication's former server mode and
enforced a general depth cap of 64.
src/server/protocol_client.cc[561-564]
src/redis/read.c[494-559]
src/redis/hiredis.c[161-176]
src/facade/redis_parser.cc[18-22]
src/facade/redis_parser.cc[357-360]
src/facade/resp_parser.h[85-105]
src/redis/read.c[469-505]
src/redis/read.c[549-560]
src/facade/redis_parser.cc[336-360]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Replication command parsing no longer bounds aggregate nesting. A peer can keep every array within `max_array_len` by sending arbitrarily many nested `*1` aggregates, causing hiredis parser-task and reply-object memory to grow continuously before flat-command validation rejects the input.

## Issue Context
The configured per-aggregate element cap does not constrain aggregate depth. The previous server-mode parser rejected nested aggregates entirely, and its general aggregate parser capped nesting at 64 levels; add a suitable nesting-depth limit to `RESPParser::Limits`, enforce it before hiredis grows its task stack or allocates nested reply objects, and configure it for replication command mode.

## Fix Focus Areas
- src/server/protocol_client.cc[558-564]
- src/facade/resp_parser.h[85-105]
- src/facade/resp_parser.cc[28-99]
- src/redis/read.c[469-560]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

Context sources

✅ Cross-repo context — repo relationships

Explored: repo: romange/helio (sha: 7ea50945)

Review mode: 🧠 Deep: This is a substantial behavioral refactor across RESP parsing, protocol buffering, replication command handling, and parser limits, with many independent edit sites and plausible subtle state/streaming defects.

Tip of the day

💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

Powered by Qodo

Copilot

Pull request overview

This PR refactors the Redis replication command stream handling to parse incoming commands via facade::RESPParser (hiredis-based) instead of the legacy RedisParser/RespExpr response parsing path, aiming to improve correctness around streaming/buffering.

Changes:

Switch Replica::ConsumeRedisStream() to read replication commands using ProtocolClient::ReadRespCommand() and batch/dispatch based on parser-buffered data.
Extend ProtocolClient with ReadRespCommand(), ResetParser(), and ResetCommandParser() to support separate parsing modes for client replies vs replication command streams.
Enhance facade::RESPParser with limits plumbing, reset helpers, and “consumed bytes” accounting; add unit tests for streaming state and array limits.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
src/server/replica.cc	Use `ReadRespCommand()` for replication stream consumption and batching decisions based on parser buffered input.
src/server/protocol_client.h	Add `ReadCommandRes`, `ReadRespCommand()`, and split parser reset APIs.
src/server/protocol_client.cc	Implement RESPParser-based command reads and introduce command-parser reset with a higher array-length cap.
src/server/cluster/outgoing_slot_migration.cc	Update to new `ResetParser()` signature.
src/server/cluster/coordinator.cc	Update to new `ResetParser()` signature.
src/facade/resp_parser.h	Add parser limits, reset API, buffered-input query, and consumed-bytes tracking.
src/facade/resp_parser.cc	Implement limits-aware reset and consumed-bytes accounting with bounded error logging.
src/facade/resp_parser_test.cc	Add tests for streaming consumption semantics and array length limiting.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

dranikpg

What is the goal? Get rid of old parser?

BorysTheDev · 2026-09-09T07:17:16Z

What is the goal? Get rid of old parser?

yes. We have 3 parsers; we want to have at least 2 and maybe 1 in the future

refactor: use RESPParser for Redis replication

561a2ba

Copilot AI lite review requested due to automatic review settings September 1, 2026 14:01

Copilot started reviewing on behalf of BorysTheDev September 1, 2026 14:02 View session

qodo-free-for-open-source-projects Bot reviewed Sep 1, 2026

View reviewed changes

Comment thread src/server/protocol_client.cc

augmentcode Bot reviewed Sep 1, 2026

View reviewed changes

qodo-code-review Bot reviewed Sep 1, 2026

View reviewed changes

Comment thread src/server/protocol_client.cc

Comment thread src/server/protocol_client.cc

Comment thread src/server/protocol_client.cc

Copilot AI reviewed Sep 1, 2026

View reviewed changes

Comment thread src/server/protocol_client.cc

BorysTheDev requested review from dranikpg and romange September 2, 2026 07:45

dranikpg approved these changes Sep 8, 2026

View reviewed changes

BorysTheDev merged commit 11d6ae9 into main Sep 14, 2026
24 of 26 checks passed

BorysTheDev deleted the use_resp_parser_for_replication branch September 14, 2026 09:40

Navigation Menu

refactor: use RESPParser for Redis replication - #8211

refactor: use RESPParser for Redis replication#8211
BorysTheDev merged 1 commit into
mainfrom
use_resp_parser_for_replication

BorysTheDev commented Sep 1, 2026 •

edited

Loading

Uh oh!

qodo-code-review Bot commented Sep 1, 2026 •

edited by qodo-free-for-open-source-projects Bot

Loading

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 1, 2026 •

edited

Loading

Uh oh!

Uh oh!

augmentcode Bot commented Sep 1, 2026

Uh oh!

augmentcode Bot left a comment

Uh oh!

qodo-code-review Bot commented Sep 1, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

dranikpg left a comment

Uh oh!

BorysTheDev commented Sep 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

BorysTheDev commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

qodo-code-review Bot commented Sep 1, 2026 • edited by qodo-free-for-open-source-projects Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary by Qodo

Uh oh!

qodo-free-for-open-source-projects Bot commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Uh oh!

Uh oh!

augmentcode Bot commented Sep 1, 2026

Uh oh!

augmentcode Bot left a comment

Choose a reason for hiding this comment

Uh oh!

qodo-code-review Bot commented Sep 1, 2026

Code Review by Qodo

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

dranikpg left a comment

Choose a reason for hiding this comment

Uh oh!

BorysTheDev commented Sep 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

BorysTheDev commented Sep 1, 2026 •

edited

Loading

qodo-code-review Bot commented Sep 1, 2026 •

edited by qodo-free-for-open-source-projects Bot

Loading

qodo-free-for-open-source-projects Bot commented Sep 1, 2026 •

edited

Loading