[daily-ci] FLAKY-TEST: create_nodes_conf_folder races the BIO cluster-config save that recreates nodes.conf

[madolson]

The create_nodes_conf_folder test helper deletes nodes.conf and then creates a directory in its place using two separate exec calls, and in best-effort mode the server legitimately recreates nodes.conf via rename() on a BIO thread whenever gossip changes the topology. When a save lands in that window the following mkdir -p fails EEXIST, because mkdir -p tolerates an existing directory but not an existing regular file. This is a [exception], so it kills the test client and fails the whole job rather than one test. The server is behaving correctly; the helper is the bug.

Details

Job: test-fedorarawhide-jemalloc, run 35039408935
https://github.com/valkey-io/valkey/actions/runs/35039408935/job/104615694004

Seen once across seven consecutive failing Daily runs on unstable (2026-09-10 to 2026-09-16). Source read at d6415e766.

[exception]: Executing test client: mkdir: cannot create directory '/__w/valkey/valkey/tests/tmp/server.7554.137/nodes.conf': File exists.
mkdir: cannot create directory '/__w/valkey/valkey/tests/tmp/server.7554.137/nodes.conf': File exists
    while executing
"exec mkdir -p $cluster_conf_path"
    (procedure "create_nodes_conf_folder" line 4)
    invoked from within
"create_nodes_conf_folder 1"
    ("uplevel" body line 81)

start_cluster opens at tests/unit/cluster/misc.tcl:225, so body line 81 is tests/unit/cluster/misc.tcl:306, inside cluster-config-save-behavior best-effort mode - node continues running when config save fails.

The helper

tests/unit/cluster/misc.tcl:192

proc create_nodes_conf_folder {srv_idx} {
    set cluster_conf_path [get_nodes_conf_path $srv_idx]
    if {[file exists $cluster_conf_path]} { exec rm -rf $cluster_conf_path }
    exec mkdir -p $cluster_conf_path
}

Two forks, tens of milliseconds apart, against a file the server owns.

Why the server refills the hole

clusterSaveConfigImpl renames a temp file into place at src/cluster_legacy.c:1163. In best-effort mode the save is queued, not inline: src/cluster_legacy.c:7013 calls clusterSaveConfigBackground (src/cluster_legacy.c:1243) which calls clusterSaveConfig(true, ...) (src/cluster_legacy.c:1207), handing the content to bioCreateClusterConfigSaveJob; the worker lands in clusterSaveConfigFromBio (src/cluster_legacy.c:1230). CLUSTER_TODO_SAVE_CONFIG is set from many gossip paths (src/cluster_legacy.c:3379, :3407, :4276, :4407, :4436, :4459), so a save can fire at any moment. A rename() onto a path the test just deleted succeeds and logs nothing, so the recreation is invisible in the server log.

Sequence that guarantees the window

tests/unit/cluster/misc.tcl:284-306

1. 284-285 remove_nodes_conf_folder 0 / 1, nodes.conf deleted.
2. 287 R 1 cluster failover takeover, save succeeds, nodes.conf recreated as a regular file.
3. 292-303 wait for cluster_config_save_status ok on both, i.e. the test explicitly waits for the file to be back.
4. 305 create_nodes_conf_folder 0, ok.
5. 306 create_nodes_conf_folder 1, rm -rf then a background save recreates the file, mkdir -p fails.

From the run (pid 37708 is R1 / srv -1, pid 37808 is R0 / srv 0):

00:22:16.774  37708 Taking over the primary                    <- misc.tcl:240
00:22:16.777  37708 Could not rename tmp cluster config file: Is a directory
00:22:16.799  37808 Taking over the primary                    <- misc.tcl:262
00:22:16.916  37708 Could not rename tmp cluster config file: Is a directory
00:22:16.940  37708 Taking over the primary                    <- misc.tcl:287, after the removes
00:22:17.092  37808 Could not rename tmp cluster config file: Is a directory   <- after misc.tcl:305
00:22:17.116  37708 Client closed connection ... cmd=config|get <- get_nodes_conf_path for :306
00:22:17.17         mkdir fails

R1 logs no rename failure between 16.940 and shutdown, confirming its saves were succeeding right up to line 306.

Introduced by

f1addd8f3 "Use BIO thread for cluster config saving in cluster-config-save-behavior best-effort mode (#2555)", 2026-09-01. It added the remove-save-recreate cycle at 284-306 and moved the best-effort save onto a BIO thread, so the main thread returns from CLUSTER FAILOVER TAKEOVER and CLUSTER INFO while the write is still pending. The helper predates it (82d7b7e40, 2e28af6fd), but before #2555 it was only called once per file on a quiescent cluster, so the window was never reached.

Suggested fix

Retry until the directory sticks.

proc create_nodes_conf_folder {srv_idx} {
    set p [get_nodes_conf_path $srv_idx]
    wait_for_condition 50 100 {
        [catch {file delete -force $p; file mkdir $p}] == 0 && [file isdirectory $p]
    } else {
        fail "could not turn $p into a directory"
    }
}

Related

#33 is also fallout from #2555 (valgrind definite loss on the BIO cluster-config save content in a fork child) but is a different failure. No valkey-io/valkey issue tracks this one.

This was generated by AI but verified, with love, by a human.

[madolson]

Reproduced at 100% by forcing a cluster-config save to be in flight across the helper's rm/mkdir window: issue CLUSTER BUMPEPOCH on a deferring client and do not read the reply, then call create_nodes_conf_folder. clusterBumpConfigEpochWithoutConsensus sets CLUSTER_TODO_SAVE_CONFIG | CLUSTER_TODO_FSYNC_CONFIG at src/cluster_legacy.c:2605, so the BIO save queued at src/cluster_legacy.c:7013 renames a regular file into place while Tcl sits between its two forks, and mkdir -p then fails EEXIST with the same stack as CI. This is a test bug, not a product bug: the server's rename() at src/cluster_legacy.c:1163 onto a path the test just deleted is correct behavior. Fix in #54, which replaces the two exec calls with a wait_for_condition retry; 5/5 runs fail before it and 5/5 pass after.

Details

Repro

Built from the two helper procs copied verbatim out of upstream misc.tcl at d6415e766 (sed -n '183,203p' tests/unit/cluster/misc.tcl, giving get_nodes_conf_path and remove_nodes_conf_folder plus the unfixed create_nodes_conf_folder), written to tests/unit/cluster/repro36.tcl with this appended:

start_cluster 1 1 {tags {external:skip cluster} overrides {cluster-config-save-behavior best-effort}} {
    test {REPRO-36 create_nodes_conf_folder races the BIO cluster-config save} {
        set d0 [valkey_deferring_client 0]
        set d1 [valkey_deferring_client -1]
        remove_nodes_conf_folder 0
        remove_nodes_conf_folder 1
        for {set i 0} {$i < 300} {incr i} {
            # Queue a config save on both nodes without waiting for the reply, so the
            # BIO save is in flight while the helper does its rm/mkdir pair.
            $d0 cluster bumpepoch
            $d1 cluster bumpepoch
            create_nodes_conf_folder 1
            remove_nodes_conf_folder 1
            $d0 read
            $d1 read
        }
        $d0 close
        $d1 close
    }
}

./runtest --single unit/cluster/repro36, five separate invocations. --loops is useless here because an [exception] aborts the whole run.

run 1 EXIT=1 1 exception(s)
run 2 EXIT=1 1 exception(s)
run 3 EXIT=1 1 exception(s)
run 4 EXIT=1 1 exception(s)
run 5 EXIT=1 1 exception(s)

[exception]: Executing test client: mkdir: cannot create directory /tmp/wt-36-up/tests/tmp/server.19245.35/nodes.conf: File exists.
mkdir: cannot create directory /tmp/wt-36-up/tests/tmp/server.19245.35/nodes.conf: File exists
    while executing
"exec mkdir -p $cluster_conf_path"
    (procedure "create_nodes_conf_folder" line 4)
    invoked from within
"create_nodes_conf_folder 1"

The knob is the deferred, unread CLUSTER BUMPEPOCH. Reading the reply makes it never reproduce, because beforeSleep has already queued and the BIO thread has usually already finished the rename by the time Tcl gets the reply. Leaving it unread is what puts the write concurrent with the forks.

Appending the same loop to the real upstream misc.tcl rather than a copy reproduces on the first run, through the real helper, confirming the copy is faithful.

After the fix

Helper replaced with a retry:

wait_for_condition 50 100 {
    [catch {file delete -force $cluster_conf_path; file mkdir $cluster_conf_path}] == 0 &&
    [file isdirectory $cluster_conf_path]
} else {
    fail "Could not turn $cluster_conf_path into a directory"
}

run 1 EXIT=0 exceptions=0 All tests passed without errors!
run 2 EXIT=0 exceptions=0 All tests passed without errors!
run 3 EXIT=0 exceptions=0 All tests passed without errors!
run 4 EXIT=0 exceptions=0 All tests passed without errors!
run 5 EXIT=0 exceptions=0 All tests passed without errors!

Real misc.tcl with the fix and the repro loop still appended: All tests passed without errors!.

Why not a product bug

Nothing in the server promises that nodes.conf stays absent after something else unlinks it. clusterSaveConfigImpl at src/cluster_legacy.c:1163 renames a fully written temp file over the target, which is the correct atomic-write pattern; it has no way to know a test wants a directory there. The test is asserting on a path it does not own.

Why not drain BIO first

DEBUG BIO-DRAIN BIO_CLUSTER_SAVE before the delete does not fix it. Gossip can set CLUSTER_TODO_SAVE_CONFIG again immediately after the drain returns (src/cluster_legacy.c:4276, :4407, :4436, :4459), so there is no instant at which the test can know no further write is coming. Retrying is the only correct shape.

Note

agents/unstable predates f1addd8f3 (#2555), so the reproduction was run against upstream d6415e766. The helper on agents/unstable is the older variant (rm -f, inline path lookup) with the identical delete-then-create shape, and #54 applies the identical fix there.

This was generated by AI but verified, with love, by a human.