feat(server): Serializer bucket states by dranikpg · Pull Request #7027 · dragonflydb/dragonfly

dranikpg · 2026-03-30T16:11:38Z

Add bucket dependency counter to track "async" serialization operations (like big value streaming or tiered loads). Resolves #6830

Why did I choose just a latch over a struct BucketState { vector<DelayedEntry> }?

Because multiple fibers can serialize delayed entries, we need a mechanism to not serialize the same value concurrently (i.e. pop before wait as before). This means that we need a signalling mechanism to wait for the last popped item to finish serializing (as the vector can be empty), which is either a bool or a counter. And then we also need a wakeup primitive to unblock waiters -> we need just a latch or condvar.

What is more, we need some kind of latch or condvar for big value streaming. We can unite all that into a unified notion of "dependency" and use a single latch.

dranikpg · 2026-04-02T11:11:01Z

augment review

augmentcode · 2026-04-02T11:13:21Z

🤖 Augment PR Summary

Summary: This PR adds bucket-level dependency tracking to represent in-flight async serialization work during snapshot/replication.

Changes:

Introduced BucketDependencies (per-bucket LocalLatch counters) with Increment/Decrement and a Wait() helper.
Integrated dependency tracking with tiered offload serialization by incrementing on enqueue and decrementing after delayed entry serialization.
Refactored SerializerBase to inherit BucketDependencies and mark buckets busy for the duration of ProcessBucket().
Removed the previous bucket state-machine tracking code and the corresponding unit test target.

Technical Notes: Bucket staleness is now detected via the snapshot version, with touched stale buckets flushing tiered delayed entries and asserting no remaining dependencies under big_value_mu_.

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

augmentcode

Review completed. 5 suggestions posted.

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

augmentcode · 2026-04-02T11:14:54Z

@@ -108,21 +133,7 @@ class SerializerBase : public DelayedEntryHandler {

 private:
  friend class SerializerBaseTest;


src/server/serializer_base.h:135: Since the prior SerializerBaseTest was removed in this PR, there doesn’t appear to be replacement unit coverage for the new BucketDependencies accounting (especially failure paths like tiered read errors). That increases the risk of regressions where buckets remain permanently “busy”.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

augmentcode

Review completed. 4 suggestions posted.

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

augmentcode · 2026-04-02T11:16:31Z

    PrimeValue pv{*value};
    SerializeFetchedEntry(*entry, pv);
+
+    deps_.Decrement(it->first);


DelayedEntryHandler::ProcessDelayedEntries: if !value.has_value() we return without removing the delayed entry or calling deps_.Decrement(...), which can leave the bucket permanently “busy” (and repeatedly re-hit the error on subsequent passes). This becomes observable now that EnqueueOffloaded() increments dependencies for each offloaded key.

Severity: high

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

augmentcode · 2026-04-02T11:16:31Z

-    : db_slice_(slice), base_cntx_(cntx) {
-  DCHECK(db_slice_);
+    : DelayedEntryHandler(static_cast<BucketDependencies&>(*this)),
+      db_slice_(slice),


SerializerBase::SerializerBase: the previous DCHECK(db_slice_) was removed, so a null slice would now crash later (e.g. in RegisterChangeListener()/ProcessBucket()) without a clear precondition failure.

Severity: medium

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

augmentcode · 2026-04-02T11:16:31Z

 // State tracking is purely observational in early PRs: it drives DCHECKs and
 // stats but does not alter the serialization control flow.
-class SerializerBase : public DelayedEntryHandler {
+class SerializerBase : public BucketDependencies, public DelayedEntryHandler {


The comment block above SerializerBase still describes a per-bucket state-machine stored in a map, but this PR removed bucket_states_ in favor of BucketDependencies, so the documentation (and BucketPhase description) looks out of sync with the implementation.

Severity: low

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

augmentcode · 2026-04-02T11:16:31Z

@@ -1,63 +0,0 @@
-// Copyright 2026, DragonflyDB authors.  All rights reserved.


This PR removes the only unit tests covering SerializerBase’s bucket bookkeeping; with the new BucketDependencies semantics, it may be worth adding a small focused test to guard against regressions (e.g., increment/decrement and error paths).

Severity: low

_{🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.}

…tableFieldJson test (dragonflydb#7094) Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

Copilot

Pull request overview

Adds bucket-level dependency tracking to SerializerBase to coordinate async serialization work (tiered reads / big value streaming) and avoid racing bucket updates during snapshotting/streaming. Resolves the need for a per-bucket “completion” signal described in #6830.

Changes:

Introduces BucketDependencies (per-bucket latch/counter) and wires it into SerializerBase and delayed tiered-entry handling.
Refactors ProcessDelayedEntries to decrement bucket dependencies when tiered reads complete.
Adjusts a search test ordering and removes serializer_base_test from the CMake test list.

Reviewed changes

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

File	Description
`src/server/serializer_base.h`	Adds `BucketDependencies` and integrates it with serializer/delayed-entry infrastructure.
`src/server/serializer_base.cc`	Implements dependency tracking and refactors delayed-entry processing to signal dependency completion.
`src/server/search/search_family_test.cc`	Reorders setup calls in a JSON search test.
`src/server/CMakeLists.txt`	Stops building/running `serializer_base_test`.

Comments suppressed due to low confidence (1)

src/server/serializer_base.cc:82

On tiered read failure (!value.has_value()), the code decrements the bucket dependency but returns without removing the entry from delayed_entries_. This can cause repeated retries and eventually a CHECK failure/double-Decrement when the same entry is processed again; remove the failed entry (and ensure its dependency is released exactly once).

    if (!value.has_value()) {
      deps_.Decrement(it->first);
      cntx->ReportError(make_error_code(std::errc::io_error),
                        absl::StrCat("Failed to read tiered key: ", entry->key.ToString()));
      return;
    }

Copilot · 2026-04-08T16:48:25Z

+  auto serialize_entry = [&](decltype(delayed_entries_)::iterator it) {
+    auto& entry = it->second;
    auto value = entry->value.Get();

    if (!value.has_value()) {
+      deps_.Decrement(it->first);
      cntx->ReportError(make_error_code(std::errc::io_error),
                        absl::StrCat("Failed to read tiered key: ", entry->key.ToString()));
      return;
    }

    PrimeValue pv{*value};
    SerializeFetchedEntry(*entry, pv);
+
+    deps_.Decrement(it->first);
+    delayed_entries_.erase(it++);
+  };


ProcessDelayedEntries holds a multimap iterator/reference across entry->value.Get() and SerializeFetchedEntry(), both of which can suspend; if another fiber also processes delayed_entries_ concurrently, the same element can be erased/processed twice, leading to use-after-free/UB. Consider extracting/moving the node (or unique_ptr) out of delayed_entries_ before any potentially-blocking calls, then erase/commit after serialization finishes to guarantee single processing per entry.

Copilot · 2026-04-08T16:48:26Z

 helio_cxx_test(cluster/cluster_family_test dfly_test_lib LABELS DFLY)
 helio_cxx_test(acl/acl_family_test dfly_test_lib LABELS DFLY)
 helio_cxx_test(engine_shard_set_test dfly_test_lib LABELS DFLY)


serializer_base_test.cc still exists but references removed SerializerBase APIs (BucketPhase/MarkBucketSerializing/FinishBucketIteration); removing the test target here hides a now-broken test instead of fixing/updating it. Either update the test to cover the new BucketDependencies behavior or delete the obsolete test file to keep the test suite consistent.

dranikpg · 2026-04-08T19:25:03Z

-  // Check if this bucket should be serialized
-  if (!ShouldProcessBucket(it)) {
+  // Check if this bucket is stale
+  if (it.is_done() || it.GetVersion() >= snapshot_version_) {
+    stats_.buckets_skipped++;
+


romange · 2026-04-14T14:14:03Z

 helio_cxx_test(cluster/cluster_family_test dfly_test_lib LABELS DFLY)
 helio_cxx_test(acl/acl_family_test dfly_test_lib LABELS DFLY)
 helio_cxx_test(engine_shard_set_test dfly_test_lib LABELS DFLY)
-helio_cxx_test(serializer_base_test dfly_test_lib LABELS DFLY)


why is this deleted?

because the test uses a bucket state machine that I completely replaced

I can add some other tests

ah, my bad, did not delete the file

romange · 2026-04-14T14:16:20Z

is it synced with latest?

dranikpg · 2026-04-14T16:18:28Z

is it synced with latest?

Yes

Signed-off-by: Vladislav <vlad@dragonflydb.io>

romange · 2026-04-15T16:57:14Z

+  // Flush all entries of bucket
+  if (flush_bucket) {
+    auto range = delayed_entries_.equal_range(flush_bucket);
+    for (auto it = range.first; it != range.second;) {


nit: why not increment in the "for" loop, like a true professional?

dranikpg force-pushed the serializer-bucket-states branch 5 times, most recently from 71c9ad5 to 383ac33 Compare April 2, 2026 07:54

dranikpg marked this pull request as ready for review April 2, 2026 11:07

dranikpg requested review from kostasrim and romange April 2, 2026 11:08

augmentcode Bot reviewed Apr 2, 2026

View reviewed changes

vyavdoshenko and others added 3 commits April 8, 2026 18:43

fix: move FT.CREATE before data insertion in SearchSortByOptionNonSor…

a4d9d4e

…tableFieldJson test (dragonflydb#7094) Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

small fixes

e7b2add

Signed-off-by: Vladislav Oleshko <vlad@dragonflydb.io>

more fixes

97d8133

dranikpg force-pushed the serializer-bucket-states branch from 1b1f0b0 to 97d8133 Compare April 8, 2026 16:43

Copilot AI review requested due to automatic review settings April 8, 2026 16:43

Copilot started reviewing on behalf of dranikpg April 8, 2026 16:44 View session

Copilot AI reviewed Apr 8, 2026

View reviewed changes

dranikpg commented Apr 8, 2026

View reviewed changes

dranikpg added 2 commits April 9, 2026 09:26

Merge branch 'main' into serializer-bucket-states

7ce7792

fix is done case

5c9b177

romange reviewed Apr 14, 2026

View reviewed changes

dranikpg added 2 commits April 15, 2026 13:09

remove test file

eaed492

Merge branch 'main' into serializer-bucket-states

bfc12fc

Signed-off-by: Vladislav <vlad@dragonflydb.io>

dranikpg requested a review from romange April 15, 2026 12:35

romange approved these changes Apr 15, 2026

View reviewed changes

romange reviewed Apr 15, 2026

View reviewed changes

kostasrim approved these changes Apr 16, 2026

View reviewed changes

dranikpg merged commit 96ad549 into dragonflydb:main Apr 16, 2026
13 checks passed

dranikpg deleted the serializer-bucket-states branch April 16, 2026 07:19

		@@ -108,21 +133,7 @@ class SerializerBase : public DelayedEntryHandler {

		private:
		friend class SerializerBaseTest;

		@@ -1,63 +0,0 @@
		// Copyright 2026, DragonflyDB authors. All rights reserved.

Conversation

dranikpg commented Mar 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dranikpg commented Apr 2, 2026

Uh oh!

augmentcode Bot commented Apr 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

augmentcode Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

augmentcode Bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot left a comment

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

augmentcode Bot Apr 2, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

dranikpg Apr 8, 2026

Choose a reason for hiding this comment

Uh oh!

romange Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

dranikpg Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

dranikpg Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

dranikpg Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

romange commented Apr 14, 2026

Uh oh!

dranikpg commented Apr 14, 2026

Uh oh!

romange Apr 15, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dranikpg commented Mar 30, 2026 •

edited

Loading

augmentcode Bot commented Apr 2, 2026 •

edited

Loading