Make create_nodes_conf_folder tolerate the server recreating nodes.conf

[madolson]

create_nodes_conf_folder deletes nodes.conf and then creates a directory in its place using two separate exec calls. The server owns that path and legitimately recreates it via rename() whenever gossip changes the topology, so a save landing between the two calls leaves a regular file behind and the following mkdir -p fails EEXIST. That is an [exception], so it kills the test client and fails the whole job rather than one test. This replaces the pair with a wait_for_condition that retries delete-then-mkdir until the directory sticks.

Details

Problem

tests/unit/cluster/misc.tcl:108 (upstream d6415e766 has the same helper at tests/unit/cluster/misc.tcl:192):

proc create_nodes_conf_folder {srv_idx} {
    ...
    if {[file exists $cluster_conf_path]} { exec rm -f $cluster_conf_path }
    exec mkdir -p $cluster_conf_path
}

Two forks against a path the server writes. mkdir -p tolerates an existing directory but not an existing regular file, so if rename() fills the hole in between, it fails:

[exception]: Executing test client: 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)

The save side is clusterSaveConfigImpl renaming a temp file into place at src/cluster_legacy.c:1163. In best-effort mode it runs on a BIO thread (src/cluster_legacy.c:7013 -> :1243 -> :1207 -> clusterSaveConfigFromBio at :1230), so the main thread returns from CLUSTER FAILOVER TAKEOVER while the write is still pending. CLUSTER_TODO_SAVE_CONFIG is set from ~50 sites in src/cluster_legacy.c, many of them gossip paths (: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, which is why the recreation is invisible in the server log.

Reproduction

The window is a couple of milliseconds wide, so CI hits it rarely. It goes to 100% by forcing a save to be in flight across it: issue CLUSTER BUMPEPOCH on a deferring client and do not read the reply, then run the helper. clusterBumpConfigEpochWithoutConsensus sets CLUSTER_TODO_SAVE_CONFIG | CLUSTER_TODO_FSYNC_CONFIG at src/cluster_legacy.c:2605, which queues the BIO save while Tcl is between its two forks.

Repro suite, built from the two helper procs copied verbatim out of upstream misc.tcl (sed -n '183,203p' tests/unit/cluster/misc.tcl) plus this loop:

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
    }
}

Run at d6415e766 with the unfixed helper, ./runtest --single unit/cluster/repro36, five invocations (--loops cannot be used because an [exception] aborts the 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)

5/5. Same stack as CI:

[exception]: Executing test client: 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"

Appending the same loop to the real upstream misc.tcl reproduces it too, on the first run, through the real helper.

With the fix applied to the helper:

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!

5/5.

Fix

file delete -force plus file mkdir are Tcl builtins rather than forks, which shrinks the window on its own, and file mkdir on an existing directory is a no-op so the retry is safe to repeat. The wait_for_condition closes the window: 50 attempts at 100ms is generous against any save rate the server can produce.

Alternative considered

Draining the BIO queue first (DEBUG BIO-DRAIN BIO_CLUSTER_SAVE) before the delete. That loses, because gossip can set CLUSTER_TODO_SAVE_CONFIG again immediately after the drain returns; there is no point at which the test can know the server will not write. Retrying is the only correct shape.

Note on this branch

agents/unstable predates f1addd8f3 (#2555), the commit that put the best-effort save on a BIO thread and added the remove-save-recreate cycle to the test. The helper on this branch is the older variant (rm -f, inline path lookup) but has the identical delete-then-create shape, so it gets the identical fix. The reproduction above was run against upstream d6415e766 where the BIO save exists.

Testing

./runtest --single unit/cluster/misc --loops 5 on this branch: All tests passed without errors!, 75 [ok].

Fixes #36

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