src/bio.h (1)

46-46: ⚡ Quick win

Clarify content ownership in this public BIO API.

bioCreateClusterConfigSaveJob(sds content, bool do_fsync) hands data across threads; please document whether ownership of content is transferred or retained to prevent leaks/UAF at call sites.

As per coding guidelines, "Document why code exists, not just what it does; document all functions in C code".

🤖 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/bio.h` at line 46, bioCreateClusterConfigSaveJob currently hands sds data
across threads but the header prototype lacks ownership semantics; update the
public declaration for bioCreateClusterConfigSaveJob(sds content, bool do_fsync)
in src/bio.h with a clear doc comment that states whether the function takes
ownership of the sds (for example: "Takes ownership of 'content'; caller must
not free it after calling" or "Caller retains ownership; function makes an
internal copy"), describe expected life‑time/thread-transfer behavior and who is
responsible for freeing the sds, and mention any constraints on 'content'
(non-NULL, immutability expectations) and the effect of do_fsync; ensure callers
are audited to match the documented convention.

src/bio.c (1)

334-345: ⚡ Quick win

Reset the log suppressor after a successful save.

last_save_error_log never gets cleared on success, so a fail → recover → fail sequence inside 30 seconds stays silent on the second failure. That hides the transition operators usually care about most. Reset the limiter on success, or gate the warning on the previous cluster_config_bio_save_status as well.

Suggested tweak

         } else if (job_type == BIO_CLUSTER_SAVE) {
             static time_t last_save_error_log = 0;
             if (clusterSaveConfigFromBio(job->cluster_save_args.content, job->cluster_save_args.do_fsync) == C_ERR) {
                 /* Limit logging rate to 1 line per CONFIG_SAVE_LOG_ERROR_RATE seconds. */
                 if ((server.unixtime - last_save_error_log) > CONFIG_SAVE_LOG_ERROR_RATE) {
                     serverLog(LL_WARNING, "Failed to save the cluster config file in background. Cluster config "
                                           "updated even though writing the cluster config file to disk failed.");
                     last_save_error_log = server.unixtime;
                 }
                 atomic_store_explicit(&server.cluster_config_bio_save_status, C_ERR, memory_order_relaxed);
             } else {
                 atomic_store_explicit(&server.cluster_config_bio_save_status, C_OK, memory_order_relaxed);
+                last_save_error_log = 0;
             }

🤖 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/bio.c` around lines 334 - 345, The log rate limiter last_save_error_log
in the BIO_CLUSTER_SAVE branch prevents warnings after a recovery because it’s
never reset; modify the success branch in the BIO_CLUSTER_SAVE handling (the
else after clusterSaveConfigFromBio(...)) to clear or reset last_save_error_log
(e.g. set it to 0 or to server.unixtime - CONFIG_SAVE_LOG_ERROR_RATE) so a
subsequent failure will log again, and ensure
atomic_store_explicit(&server.cluster_config_bio_save_status, C_OK,
memory_order_relaxed) remains unchanged; alternatively, gate the warning on the
previous value of server.cluster_config_bio_save_status to only suppress
repeated fails without masking recoveries.

src/cluster_legacy.c (2)

1039-1047: ⚡ Quick win

Make this helper file-local.

clusterGenNodesConfContent() is only used inside src/cluster_legacy.c, so exporting the symbol unnecessarily widens the surface area.

♻️ Proposed fix

-sds clusterGenNodesConfContent(void) {
+static sds clusterGenNodesConfContent(void) {

As per coding guidelines, `src/**/*.{c,h}`: `Use static keyword for file-local functions in C code`

🤖 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 1039 - 1047, The function
clusterGenNodesConfContent is used only within this translation unit so make it
file-local by adding the static keyword to its declaration/definition; update
the function signature for clusterGenNodesConfContent(...) to be static sds
clusterGenNodesConfContent(void) so the symbol is not exported while keeping the
implementation (calls to clusterGenNodesDescription, sdscatfmt and uses of
server.cluster->currentEpoch/lastVoteEpoch) unchanged.

---

47-47: Please get @core-team sign-off on this cluster persistence change set.

This rewires cluster config durability behavior in src/cluster_legacy.c, so it should get an architectural pass from the core team before merge.

As per coding guidelines, src/{cluster*.c,replication.c,rdb.c,aof.c}: Request @core-team architectural review for changes to cluster*.c, replication.c, rdb.c, or aof.c

🤖 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` at line 47, This change touches cluster durability
behavior in src/cluster_legacy.c and requires an explicit architectural sign-off
from the core team; update the PR and code to request that review by adding a
clear reviewer request in the PR description and add an in-file TODO comment
near the changed include or the related functions (e.g., any cluster
init/persistence functions in src/cluster_legacy.c) stating "REQ: `@core-team`
sign-off per coding guidelines for cluster/replication persistence changes", and
add the corresponding JIRA/issue tag or checklist entry so CI/reviewers cannot
merge without core-team approval.

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

src/cluster_legacy.c (3)

6390-6399: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

CLUSTER_TODO_FSYNC_CONFIG still broadcasts before persistence in best-effort mode.

When cluster-config-save-behavior=best-effort, Line 6399 only queues the BIO write and CLUSTER_TODO_BROADCAST_ALL can still run immediately afterward. Callers such as clusterBumpConfigEpochWithoutConsensus() explicitly require “persist the configuration on disk before sending packets”, so this still regresses their contract. These paths need a synchronous save or a separate flag that preserves persist-before-advertise semantics.

🤖 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 6390 - 6399, The best-effort branch
currently calls clusterSaveConfigBackground(fsync) which queues persistence but
still allows CLUSTER_TODO_BROADCAST_ALL to run immediately, breaking callers
that require on-disk persistence (e.g., clusterBumpConfigEpochWithoutConsensus).
Fix by honoring the persist-before-advertise contract: when flags include
CLUSTER_TODO_FSYNC_CONFIG (or introduce a dedicated flag like
CLUSTER_TODO_PERSIST_BEFORE_BROADCAST) call clusterSaveConfigOrDie(fsync)
(synchronous save + fsync) instead of clusterSaveConfigBackground, otherwise
keep the best-effort background save; update callers/flag checks accordingly to
reference CLUSTER_TODO_SAVE_CONFIG, CLUSTER_TODO_FSYNC_CONFIG,
clusterSaveConfigOrDie, clusterSaveConfigBackground, and
CLUSTER_TODO_BROADCAST_ALL.

---

1647-1650: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't ignore the final synchronous save failure during shutdown.

After the BIO drain, Line 1650 still lets shutdown continue on C_ERR. That can exit with a stale nodes.conf, which defeats the explicit persistence guarantee for shutdown.

Suggested fix

     serverLog(LL_NOTICE, "Saving the cluster configuration file before exiting.");
     bioDrainWorker(BIO_CLUSTER_SAVE);
-    clusterSaveConfig(false, true);
+    clusterSaveConfigOrDie(true);

🤖 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 1647 - 1650, The code calls
bioDrainWorker(BIO_CLUSTER_SAVE) and then clusterSaveConfig(false, true) but
ignores a C_ERR return; change this so the return value of clusterSaveConfig is
checked and a failed save is handled (e.g., log an error with serverLog
including the failure context and prevent/abort normal shutdown or return a
non-zero exit) to guarantee nodes.conf is persisted; update the block around
bioDrainWorker/clusterSaveConfig to detect clusterSaveConfig(...) == C_ERR and
take a failure path instead of continuing shutdown.

---

7385-7406: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

CLUSTER INFO is still missing cluster_config_last_save_time.

This adds cluster_config_save_status, but not the last successful save timestamp promised by the PR contract. Without that field, an old "ok" is indistinguishable from a fresh one, so operators still can't tell whether BIO persistence is stalled.

🤖 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 7385 - 7406, Load the last-save timestamp
from server.cluster_config_last_save_time (using atomic_load_explicit with
memory_order_relaxed like config_bio_save_status), and add a new
"cluster_config_last_save_time:%U\r\n" entry to the sdscatfmt call that builds
info (using the same formatting convention as other epoch/timestamp fields, e.g.
casting to (unsigned long long)). Keep the existing cluster_config_save_status
logic but append the last-save time field so operators can see when the config
was last successfully persisted; update the sdscatfmt format string and its
argument list (in the same block where config_bio_save_status and info are
handled).