refactor(facade): separate redis data/control paths (phase 2, final) #6477

glevkovich · 2026-01-27T04:34:39Z

Separates Redis pipeline processing from the SendAsync control path to
improve architecture and allow distinct monitoring.

Fixes pipeline_buffer_limit enforcement by introducing
QueueBackpressure::pipeline_bytes to track global per-thread memory
usage, resolving a bug where limits were applied per-connection.
Adds a safety barrier in ClearPipelinedMessages to wait for in-flight
async commands during teardown, preventing use-after-free races.
Fixes potential underflow assertions in ReleaseParsedCommand.
Optimizes fiber notification logic based on protocol specific needs.

augmentcode · 2026-01-27T05:21:14Z

🤖 Augment PR Summary

Summary: Refactors RESP pipeline dispatching by separating pipelined commands from control/admin events to simplify async processing and migration ordering.

Changes:

Removed pipelined Redis commands from `dispatch_q_` and introduced a dedicated parsed-command queue (linked list via `ParsedCommand::next`).
Added `HasPendingMessages()` / `GetPendingMessageCount()` helpers and updated migration/close logic to use them.
Reworked `DispatchSingle` and parsing to enqueue pipeline commands and optionally yield to the async consumer.
Split async processing into `ProcessAdminMessage` and `ProcessPipelineCommand`, and updated `AsyncFiber` to drain admin vs pipeline paths explicitly.
Adjusted backpressure, flushing behavior, and memory accounting (including adding queued pipeline bytes into `GetMemoryUsage()`).

Technical Notes: Migration requests are intentionally deferred behind outstanding RESP pipeline work to avoid priority inversion and to keep reply flushing deterministic.

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

augmentcode

Review completed. 2 suggestions posted.

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

src/facade/dragonfly_connection.cc

Copilot

Pull request overview

Refactors Dragonfly connection dispatching by separating Redis pipelined command handling from the admin/control dispatch queue, aiming to improve queuing policy and latency accounting.

Changes:

Move Redis pipelined commands off dispatch_q_ into a dedicated parsed-command linked list and update async fiber draining logic accordingly.
Update pipeline backpressure/latency accounting to use per-pipeline counters (pending_pipeline_*) and parsed_cycle as the enqueue timestamp.
Adjust connection stats semantics/documentation to reflect protocol-specific counting (Redis admin-only vs Memcache all).

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.

File	Description
src/facade/parsed_command.h	Clarifies that `parsed_cycle` is also used as enqueue timestamp for pipeline wait latency.
src/facade/facade_types.h	Updates/clarifies dispatch queue stats semantics and improves comment formatting.
src/facade/dragonfly_connection.h	Updates message variant, dispatch API signatures, and adds helpers for pending-queue tracking.
src/facade/dragonfly_connection.cc	Main refactor: splits control vs pipeline paths, changes async fiber processing, and rewires stats/backpressure and cleanup logic.

src/facade/dragonfly_connection.cc

Copilot

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.

src/facade/dragonfly_connection.cc

src/facade/facade_types.h

src/facade/dragonfly_connection.h

src/facade/dragonfly_connection.cc

Copilot

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

src/facade/facade_types.h

src/facade/dragonfly_connection.cc

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

src/server/server_family.cc

src/facade/dragonfly_connection.cc

Separates Redis pipeline processing from the SendAsync control path to improve architecture and allow distinct monitoring. * Fixes pipeline_buffer_limit enforcement by introducing QueueBackpressure::pipeline_bytes to track global per-thread memory usage, resolving a bug where limits were applied per-connection. * Adds a safety barrier in ClearPipelinedMessages to wait for in-flight async commands during teardown, preventing use-after-free races. * Fixes potential underflow assertions in ReleaseParsedCommand. * Optimizes fiber notification logic based on protocol specific needs. Signed-off-by: Gil Levkovich <[email protected]>

Signed-off-by: Gil Levkovich <[email protected]>

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

src/server/server_family.cc

src/facade/dragonfly_connection.cc

Signed-off-by: Gil Levkovich <[email protected]>

Copilot

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

src/facade/dragonfly_connection.cc

Signed-off-by: Gil Levkovich <[email protected]>

romange · 2026-01-29T17:14:18Z

src/server/server_family.cc

                            MetricType::GAUGE, &resp->body());
-  AppendMetricWithoutLabels("pipeline_queue_length", "", conn_stats.dispatch_queue_entries,
+  AppendMetricWithoutLabels("pipeline_queue_length", "",
+                            conn_stats.dispatch_queue_entries + conn_stats.pipeline_queue_entries,


I know you preserve the original behavior, but here lets actually measure pipeline_queue_entries

romange · 2026-01-29T17:15:46Z

src/server/server_family.cc

-    append("pipeline_queue_length", m.facade_stats.conn_stats.dispatch_queue_entries);
-
+    // Sum of dispatch and pipeline queues.
+    append("pipeline_queue_length", m.facade_stats.conn_stats.dispatch_queue_entries +


similarly - pipeline only

romange · 2026-01-29T17:19:26Z

src/facade/dragonfly_connection.h


+  // Processes a single Admin/Control message from dispatch_q_.
+  // Returns true if the fiber should terminate (e.g. Migration).
+  bool ProcessAdminMessage(MessageHandle msg, AsyncOperations& async_op);


pls pass mutable arguments by pointer

Admin is a little misleading now, because it also handles monitor/pubsub - which is "technically" not admin

dranikpg · 2026-01-30T08:11:37Z

src/facade/dragonfly_connection.h


+  // Processes a single Admin/Control message from dispatch_q_.
+  // Returns true if the fiber should terminate (e.g. Migration).
+  bool ProcessAdminMessage(MessageHandle msg, AsyncOperations& async_op);


Admin is a little misleading now, because it also handles monitor/pubsub - which is "technically" not admin

dranikpg · 2026-01-30T08:14:57Z

src/facade/dragonfly_connection.cc

+        if (ProcessAdminMessage(std::move(msg), async_op)) {
+          return;  // don't set conn closing flag
+        }
+      } else {  // Process Pipeline Queue
+        ProcessPipelineCommand();


So non-commands have always higher priority? I'm trying to think of possible problems... For example, a pubsub client that always runs with messages pending (at limit), but never overfills it, so it stays active for a long time. Imagine it wants to unsubscribe from a channel - it sends an "UNSUBSCRIBE" command. It won't be processed

I wrote it 100 times, we should verify that we don't break anything

#6423 (comment)

dranikpg · 2026-01-30T08:24:07Z

src/facade/dragonfly_connection.cc

+    if (GetQueueBackpressure().IsPipelineBufferOverLimit(
+            stats_->dispatch_queue_bytes + stats_->pipeline_queue_bytes,
+            GetPendingMessageCount())) {
+      cc_->conn_closing = true;


You use it always just with pipeline_queue_bytes. Do we ignore dispatch_queue_bytes now? I assume it should be the same formula everywhere

glevkovich marked this pull request as ready for review January 27, 2026 05:13

Copilot AI review requested due to automatic review settings January 27, 2026 05:13

Copilot started reviewing on behalf of glevkovich January 27, 2026 05:13 View session

glevkovich self-assigned this Jan 27, 2026

augmentcode bot reviewed Jan 27, 2026

View reviewed changes

src/facade/dragonfly_connection.cc Outdated Show resolved Hide resolved

src/facade/dragonfly_connection.cc Outdated Show resolved Hide resolved

Copilot AI reviewed Jan 27, 2026

View reviewed changes

glevkovich force-pushed the glevkovich/refactor_redis_dispatch_queuing_policy3 branch from 3eabc83 to 31fa1c2 Compare January 27, 2026 06:17

Copilot AI review requested due to automatic review settings January 27, 2026 09:01

glevkovich force-pushed the glevkovich/refactor_redis_dispatch_queuing_policy3 branch from 31fa1c2 to 9373fb0 Compare January 27, 2026 09:01

Copilot started reviewing on behalf of glevkovich January 27, 2026 09:01 View session

glevkovich changed the title ~~Glevkovich/refactor redis dispatch queuing policy3~~ refactor(facade): separate redis data/control paths (phase 2, final) Jan 27, 2026

glevkovich requested review from dranikpg, kostasrim and romange January 27, 2026 09:08

Copilot AI reviewed Jan 27, 2026

View reviewed changes

src/facade/dragonfly_connection.cc Outdated Show resolved Hide resolved

src/facade/facade_types.h Outdated Show resolved Hide resolved

src/facade/dragonfly_connection.h Outdated Show resolved Hide resolved

src/facade/dragonfly_connection.cc Show resolved Hide resolved

Copilot AI review requested due to automatic review settings January 27, 2026 09:41

Copilot started reviewing on behalf of glevkovich January 27, 2026 09:41 View session

Copilot AI reviewed Jan 27, 2026

View reviewed changes

src/facade/facade_types.h Outdated Show resolved Hide resolved

src/facade/dragonfly_connection.cc Outdated Show resolved Hide resolved

src/facade/dragonfly_connection.cc Outdated Show resolved Hide resolved

glevkovich force-pushed the glevkovich/refactor_redis_dispatch_queuing_policy3 branch from 96155a8 to fa75cdc Compare January 27, 2026 11:31

Copilot AI review requested due to automatic review settings January 27, 2026 15:14

glevkovich force-pushed the glevkovich/refactor_redis_dispatch_queuing_policy3 branch from fa75cdc to 988e2fb Compare January 27, 2026 15:14

Copilot started reviewing on behalf of glevkovich January 27, 2026 15:15 View session

Copilot AI reviewed Jan 27, 2026

View reviewed changes

src/server/server_family.cc Show resolved Hide resolved

src/facade/dragonfly_connection.cc Show resolved Hide resolved

glevkovich added 6 commits January 29, 2026 19:08

chore: fix pr comments

c36fd75

Signed-off-by: Gil Levkovich <[email protected]>

chore: fix pr comments 2

378477a

Signed-off-by: Gil Levkovich <[email protected]>

chore: fix pr comments 3

e63269c

Signed-off-by: Gil Levkovich <[email protected]>

chore: fix pr comments and failing tests 4

0869291

Signed-off-by: Gil Levkovich <[email protected]>

chore: fix pr comments 5

2a8e16e

Signed-off-by: Gil Levkovich <[email protected]>

chore: fix pr comments

5de84b4

Signed-off-by: Gil Levkovich <[email protected]>

Copilot AI review requested due to automatic review settings January 29, 2026 12:09

glevkovich force-pushed the glevkovich/refactor_redis_dispatch_queuing_policy3 branch from 0a71915 to 5de84b4 Compare January 29, 2026 12:09

Copilot started reviewing on behalf of glevkovich January 29, 2026 12:09 View session

Copilot AI reviewed Jan 29, 2026

View reviewed changes

chore: fix pr comments

ace4994

Signed-off-by: Gil Levkovich <[email protected]>

glevkovich requested a review from Copilot January 29, 2026 12:42

Copilot started reviewing on behalf of glevkovich January 29, 2026 12:43 View session

chore: fix pr comments

c07bbf1

Signed-off-by: Gil Levkovich <[email protected]>

Copilot AI reviewed Jan 29, 2026

View reviewed changes

src/facade/dragonfly_connection.cc Show resolved Hide resolved

src/facade/dragonfly_connection.cc Outdated Show resolved Hide resolved

src/facade/dragonfly_connection.cc Outdated Show resolved Hide resolved

chore: fix pr comments

710db1d

Signed-off-by: Gil Levkovich <[email protected]>

romange reviewed Jan 29, 2026

View reviewed changes

dranikpg reviewed Jan 30, 2026

View reviewed changes

refactor(facade): separate redis data/control paths (phase 2, final) #6477

Are you sure you want to change the base?

refactor(facade): separate redis data/control paths (phase 2, final) #6477

Conversation

glevkovich commented Jan 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

augmentcode bot commented Jan 27, 2026

Uh oh!

augmentcode bot left a comment

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

glevkovich commented Jan 27, 2026 •

edited

Loading