feat(server): add --flush_on_promotion to flush data when promoted with REPLICAOF NO ONE

[nmass-betpawa]

Summary

Adds an opt-in --flush_on_promotion flag that makes an instance drop its whole dataset the
moment it is promoted to master with REPLICAOF NO ONE. This is for cache deployments that must
never serve stale data after an unplanned failover: the promoted replica can be behind the master
that just died, and today there is no way to have the new master start empty without racing the
promotion from the outside.

Concretely, on Kubernetes the operator reacts to a dead master by sending REPLICAOF NO ONE to a
replica and only afterwards flipping the role: master pod label that the Service selects on.
Flushing from a sidecar or an external controller therefore always lands somewhere in that window
and can wipe writes that the new master has already accepted. Doing it inside ReplicaOfNoOne
removes the race entirely: the dataset is gone before anything outside the process learns about the
new role.

Changes

• New flag --flush_on_promotion (default false) in src/server/server_family.cc.
• ServerFamily::ReplicaOfNoOne tracks whether it actually promoted the instance and, when the
  flag is set, calls the existing ServerFamily::FlushAll() helper on the default namespace.
• Integration test test_flush_on_promotion in tests/dragonfly/replication_test.py.

Behavior

• Default is false, so there is no change for existing deployments.
• Only the REPLICAOF NO ONE path is affected. REPLTAKEOVER promotions are deliberately left
  alone: a takeover is coordinated and loses no data, and it is what the operator uses for rolling
  updates -- flushing there would give a cold cache and a backend load spike on every upgrade.
• REPLICAOF NO ONE stays idempotent. The promoted guard means that issuing it against an
  instance that is already a master is still a no-op, so operator reconciliation re-sending the
  command cannot wipe a live dataset.
• The flush is done after SwitchState(LOADING, ACTIVE) so the FLUSHALL transaction does not run
  while the instance is still in LOADING.

Testing

DRAGONFLY_PATH=build-dbg/dragonfly \
  python3 -m pytest tests/dragonfly/replication_test.py::test_flush_on_promotion -xvs

The test covers both directions: a replica with the flag set has dbsize == 0 right after
REPLICAOF NO ONE, and a second REPLICAOF NO ONE against the now-master keeps its data.

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Flush dataset on replica promotion via REPLICAOF NO ONE (opt-in flag)

✨ Enhancement 🧪 Tests ⚙️ Configuration changes 🕐 20-40 Minutes

---

AI Description

• Add --flush_on_promotion to drop cached/stale data when a replica is promoted via REPLICAOF NO
 ONE.
• Ensure REPLICAOF NO ONE remains side-effect free when already a master.
• Add replication test coverage validating flush-on-promotion and idempotent behavior.

Diagram

graph TD
  A["Failover automation"] --> B["REPLICAOF NO ONE"] --> C["ServerFamily::ReplicaOfNoOne"] --> D{"Promoted & flag on?"}
  D -- "yes" --> E["FlushAll(default ns)"] --> F[("Dataset")]
  D -- "no" --> F
  F --> G["SwitchState → ACTIVE"] --> H["Serve traffic"]

  subgraph Legend
    direction LR
    _p["Process"] ~~~ _d{"Decision"} ~~~ _db[("Data")]
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Make flushing an explicit command/workflow step

• ➕ Avoids tying data-loss semantics to REPLICAOF NO ONE
• ➕ Operators can control timing and scope (FLUSHDB vs FLUSHALL)
• ➖ Harder to make failover safe-by-default for cache deployments
• ➖ More moving parts for automation during an already sensitive operation

2. Block reads/writes until re-seeded instead of flushing

• ➕ Prevents serving stale data without necessarily destroying local state
• ➕ Can be gentler on latency than a synchronous full flush
• ➖ Requires additional state/guardrails in request path
• ➖ More complex to reason about and test than an opt-in flush hook

3. Flush all namespaces (not just default) on promotion

• ➕ More complete stale-data protection in multi-namespace deployments
• ➖ Potentially larger blast radius and unexpected data loss in multi-tenant setups
• ➖ Might be incompatible with current expectations around namespace isolation

Recommendation: The PR’s opt-in flag gated on an actual promotion is a pragmatic approach for cache-style deployments: it keeps default behavior unchanged, avoids side effects when already master, and flushes at the safest point (before serving traffic). The main follow-up to consider is whether flushing should target all namespaces or remain default-namespace-only; the current choice is reasonable if multi-namespace promotion semantics are intentionally limited.

Files changed (2) +49 / -0

Enhancement (1) +16 / -0

server_family.ccAdd --flush_on_promotion and flush on actual REPLICAOF NO ONE promotion +16/-0

Add --flush_on_promotion and flush on actual REPLICAOF NO ONE promotion

• Introduces a new absl flag (flush_on_promotion) and wires it into ReplicaOfNoOne(). The code tracks whether the call performed a real promotion (replica→master) and, if so and the flag is enabled, executes FlushAll on the default namespace before returning OK/serving traffic.

src/server/server_family.cc

Tests (1) +33 / -0

replication_test.pyAdd test ensuring data is flushed only when promotion actually occurs +33/-0

Add test ensuring data is flushed only when promotion actually occurs

• Adds an async replication test that creates a master/replica pair with flush_on_promotion enabled. Verifies that REPLICAOF NO ONE clears the promoted replica’s dataset, and that calling REPLICAOF NO ONE again when already master does not clear data.

tests/dragonfly/replication_test.py

[qodo-free-for-open-source-projects]

Code Review by Qodo

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

---

🟡 Remediation Recommended

1. Stale reads after promotion 🐞 Bug ≡ Correctness

Description

ReplicaOfNoOne() switches the instance to ACTIVE and then triggers FlushAll(), but FlushAll
dispatches an async per-shard flush (wait=false) and returns immediately, so the promoted master can
accept and serve stale reads while the background flush is still running.

Code

src/server/server_family.cc[R3407-3410]

+  if (promoted && absl::GetFlag(FLAGS_flush_on_promotion)) {
+    LOG(INFO) << "Flushing all data after promotion to master";
+    FlushAll(&namespaces->GetDefaultNamespace());
+  }

Evidence

The promotion path calls FlushAll() after switching to ACTIVE. FlushAll() calls Drakarys(...,
wait=false), and Drakarys explicitly detaches flush fibers when wait=false, so the command can
return while keys are still present. main_service.cc shows global state gates command execution, so
switching to ACTIVE before flush enables commands during the flush window.

src/server/server_family.cc[3378-3413]
src/server/server_family.cc[1425-1431]
src/server/server_family.cc[1928-1946]
src/server/main_service.cc[1361-1396]
src/server/main_service.cc[2904-2922]

Agent prompt

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

## Issue description
`ReplicaOfNoOne()` enables `--flush_on_promotion` by calling `FlushAll()`, but:
1) the server is already switched to `ACTIVE` before flushing, and
2) `FlushAll()` uses `Drakarys(..., wait=false)` which detaches flush fibers.
This creates a window where the newly promoted master can execute normal commands and potentially serve stale data.
### Issue Context
- `Service::SwitchState()` updates the global state used to allow/deny command execution.
- `Drakarys(..., wait=false)` detaches the per-shard flush fibers.
- There is already a concept of synchronous flushing (`FLUSHALL SYNC`) in `FlushDb()`.
### Fix Focus Areas
- src/server/server_family.cc[3378-3413]
- src/server/server_family.cc[1425-1431]
- src/server/server_family.cc[1928-1946]
- src/server/main_service.cc[1361-1396]
- src/server/main_service.cc[2904-2922]
### Concrete fix direction
- Ensure the flush completes before the instance is considered ready for traffic:
- Either run the promotion flush synchronously (join shard flush fibers / `wait=true`) and only then switch to `ACTIVE`, OR
- Temporarily move the service to a blocking state (e.g., LOADING/TAKEN_OVER) during flush and switch back to `ACTIVE` only after completion.
- Avoid returning `OK` from `REPLICAOF NO ONE` until the flush has finished (otherwise orchestrators may start routing traffic early).
- Consider releasing `replicaof_mu_` before a potentially long synchronous flush (do the promotion state changes under the lock, then unlock, then flush) if safe for your concurrency model.

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

---

2. Non-default namespaces remain 🐞 Bug ≡ Correctness

Description

The promotion-time flush clears only namespaces->GetDefaultNamespace(), so any data stored in other
namespaces survives promotion even with --flush_on_promotion enabled.

Code

src/server/server_family.cc[R3408-3410]

+    LOG(INFO) << "Flushing all data after promotion to master";
+    FlushAll(&namespaces->GetDefaultNamespace());
+  }

Evidence

ServerFamily sets the per-connection namespace based on credentials using Namespaces::GetOrInsert,
and Namespaces explicitly supports multiple named namespaces. Therefore flushing only
GetDefaultNamespace cannot clear all data for non-default namespaces.

src/server/server_family.cc[3405-3410]
src/server/server_family.cc[2037-2042]
src/server/namespaces.h[22-71]

Agent prompt

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

## Issue description
`--flush_on_promotion` currently calls `FlushAll(&namespaces->GetDefaultNamespace())`, which does not clear data in non-default namespaces.
### Issue Context
Dragonfly supports multiple named namespaces (`Namespaces::GetOrInsert()`), selected during AUTH. A failover/promotion flush that only targets the default namespace can leave stale data visible for tenants using non-default namespaces.
### Fix Focus Areas
- src/server/server_family.cc[3405-3410]
- src/server/server_family.cc[2037-2042]
- src/server/namespaces.h[22-71]
### Concrete fix direction
- Provide a way to enumerate all existing namespaces (e.g., add a `ForEachNamespace(...)` API to `Namespaces` under appropriate locking).
- During promotion with `--flush_on_promotion`, iterate and flush every namespace (and every DB if applicable), not just the default.
- Ensure the iteration is safe under concurrent namespace creation (and decide whether new namespaces created during promotion should also be flushed/blocked).

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

---

3. No tracking invalidations 🐞 Bug ☼ Reliability

Description

The promotion-time flush calls FlushAll() directly and does not invoke SendInvalidationMessages(),
so CLIENT TRACKING consumers may not get the standard flush invalidation signal even though the
dataset is being dropped.

Code

src/server/server_family.cc[R3407-3410]

+  if (promoted && absl::GetFlag(FLAGS_flush_on_promotion)) {
+    LOG(INFO) << "Flushing all data after promotion to master";
+    FlushAll(&namespaces->GetDefaultNamespace());
+  }

Evidence

FlushDb (used for FLUSHALL/FLUSHDB commands) calls SendInvalidationMessages(), but FlushAll helper
does not, and the new promotion-time code uses FlushAll directly, so tracking invalidations are
skipped on promotion flush.

src/server/server_family.cc[3405-3410]
src/server/server_family.cc[2018-2028]
src/server/server_family.cc[1999-2016]
src/server/server_family.cc[1425-1431]

Agent prompt

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

## Issue description
Promotion-time flushing bypasses the normal `FlushDb()` command handler path, so it does not call `SendInvalidationMessages()`. This can leave client-side caches (CLIENT TRACKING) unaware that a flush happened.
### Issue Context
`FlushDb()` explicitly calls `SendInvalidationMessages()` after scheduling `Drakarys(...)`.
### Fix Focus Areas
- src/server/server_family.cc[3405-3410]
- src/server/server_family.cc[2018-2028]
- src/server/server_family.cc[1999-2016]
### Concrete fix direction
- After initiating (or completing, if you make it synchronous) the promotion flush, call `SendInvalidationMessages()` to match the FLUSHALL/FLUSHDB behavior.
- If you keep the flush async, decide whether invalidations should be sent immediately on dispatch or after completion (sync promotion path is usually simpler/safer).

ⓘ 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 group findings by type and pick your Finding display, from Minimal to Full

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

---

Powered by Qodo

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[qodo-code-review]

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

[kostasrim]

Hi @nmass-betpawa, thank you for this but before I follow up I have a question:

Concretely, on Kubernetes the operator reacts to a dead master by sending REPLICAOF NO ONE to a
replica and only afterwards flipping the role: master pod label that the Service selects on.
Flushing from a sidecar or an external controller therefore always lands somewhere in that window
and can wipe writes that the new master has already accepted

I am not sure how the operator works but it seems this should not be a dragonfly flag.

step 1. replicaof no one
step 2. change the role

Why not add the functionality to the operator ? In other words, the operator could also do I guess:

step 1. replicaof no one
step 2. flush
step 3. change the role

All in all based on my understanding this should not be the responsibility of dragonfly