fix(tests): site the 18 remaining store roots off the contended disk, and census the set

scripts/ci-repro/README.md

@@ -108,6 +108,94 @@ seed/ordering hypothesis is the CI evidence itself: the suite's own classifier n
 `SERVER_BLOCKED_IN_FSYNC` on the failing run. The reproducer's job is to make that
 mechanism testable on demand rather than to eliminate rivals.
 
+### `SLOWFSYNC_SKIP_TMPFS=1` — measuring a SITING fix, which the default mode cannot
+
+🔴 **The default mode stalls tmpfs too, so it cannot tell a fixed store from a broken
+one.** The shim interposes on `fsync(2)` in libc; the filesystem behind the fd is not
+consulted. Measured, and it is the whole reason this mode exists:
+
+```
+default mode, fd on ext4   -> stalls 65.0s (fs magic=0xef53)
+default mode, fd on tmpfs  -> stalls 65.0s (fs magic=0x1021994)
+```
+
+So a run against a store that `testlib/store_siting.py` has moved onto tmpfs goes red
+in the default mode, and the red is the **shim's**, not the code's. Both arms of the
+comparison fail and the instrument answers nothing.
+
+`SLOWFSYNC_SKIP_TMPFS=1` models the mechanism this file documents — node-local
+**device** contention, which an fsync with no backing device does not wait for — by
+passing a `TMPFS_MAGIC` fd straight through and stalling everything else. Two
+properties are deliberate: the pass-through does **not** consume the one-shot latch (a
+pass-through that spent it would turn "the store moved to tmpfs" into "the shim ran out
+of ammunition", which is a green that means nothing), and a *failing* `fstatfs()`
+stalls rather than skips. The stall line prints the fd's fs magic so the filesystem
+that was stalled is readable from the run rather than inferred.
+
+🔴 **THE VALUE SELECTS THE MODE — `SLOWFSYNC_SKIP_TMPFS=0` USED TO TURN IT ON.** The
+shim tested `getenv(...) != NULL`, which is presence, not value, so the spelling an
+operator reaches for to switch the mode **off** switched it on instead. Measured on this
+branch, whose store is on tmpfs, at the test selection below:
+
+```
+before the fix:  SLOWFSYNC_SKIP_TMPFS=0  -> two pass-through lines, 1 passed in  3.18s
+after  the fix:  SLOWFSYNC_SKIP_TMPFS=0  -> stall line magic=0x1021994, 1 failed in 65.91s
+after  the fix:  SLOWFSYNC_SKIP_TMPFS=1  -> two pass-through lines, 1 passed in  5.10s
+```
+
+The `=0` row before the fix is exactly what the paragraph above warns about — a shim
+that quietly stopped firing, reporting a pass. The convention now is: **ON** for `1`,
+`true`, `yes`, `on` (case-insensitive); **OFF** for `0`, `false`, `no`, `off`, the empty
+string, and for the variable being unset; and an unrecognised value resolves **OFF** —
+i.e. the shim fires — with a one-shot line on stderr naming the value. Sixteen
+spellings were watched on a one-fsync tmpfs probe (`1 true TRUE yes YES on On` →
+pass-through in 0.00s; `0 false no off Off` and the empty string and the variable unset
+→ the 65s stall; `maybe` and `2` → the warning line, then the stall).
+
+```bash
+SO=/tmp/slowfsync-$USER-$$.so
+gcc -shared -fPIC -o "$SO" scripts/ci-repro/slowfsync.c -ldl
+T='scripts/tests/test_subsystem_store_api.py::TestARefusedWriteIsIndistinguishableFromAnAbsentOne::test_POSITIVE_CONTROL_the_APPEND_comparison_CAN_see_the_difference'
+
+# reproduction on a DISK-backed store — expect 1 failed, ~65s, fs magic=0xef53
+nix develop . --command env SLOWFSYNC_SKIP_TMPFS=1 LD_PRELOAD="$SO" \
+  python3 -m pytest "$T" -q -s
+
+# the same test with the store SITED — expect 1 passed, ~4s, two pass-through lines
+```
+
+🔴 **Use `-s`.** pytest captures stderr and only prints it for a FAILING test, so a
+passing run shows none of the shim's own output — which is indistinguishable from a
+shim that never attached. `-s` makes the pass-through lines visible, and they are the
+positive control: you should see exactly **two**, the file and its parent directory,
+which is `_replace_bytes`'s pair.
+
+**Measured on the store-siting branch** (`fix/site-every-store-off-the-contended-disk`),
+every row watched:
+
+| tree | mode | store lands on | result |
+|---|---|---|---|
+| `origin/main` | none (control) | ext4 | whole class `5 passed in 8.76s` |
+| `origin/main` | default | ext4 | `1 failed in 65.52s`, `TimeoutError` at `socket.py:720`, `MECHANISM = SERVER_BLOCKED_IN_FSYNC` |
+| `origin/main` | `SKIP_TMPFS=1` | ext4 | `1 failed in 63.96s`, same, stall line `fs magic=0xef53` |
+| branch | `SKIP_TMPFS=1` | tmpfs | **`1 passed in 3.67s`**, two `pass-through … magic=0x1021994` lines, latch untouched |
+| branch | default | tmpfs | `1 failed in 63.69s`, stall line `fs magic=0x1021994` |
+| branch | `SKIP_TMPFS=1`, siting forced to fall back | ext4 | `1 failed in 64.29s`, stall line `fs magic=0xef53` |
+
+The last two rows are the controls that make the fourth mean something, and they are
+different claims. Row 5 says the shim can **still** kill this test on the branch, so the
+green is not "the reproducer no longer reaches this code". Row 6 says forcing
+`store_siting.tmpfs_dir()` to `None` reproduces `main`'s red exactly, so the green is
+the **siting** and not a property of the branch's other edits. Row 6 was produced with a
+throwaway pytest plugin that sets `store_siting._DEFAULT_CANDIDATE` to a non-existent
+path and asserts `tmpfs_dir() is None` before any test runs; it is deliberately not in
+the repo, because a lever that forces the fallback is a lever that can be left on.
+
+⚠ **This does not measure CI.** It measures that the failure mode is filesystem-
+dependent on this host, at this test. The gate's own environment may have no usable
+tmpfs at all — `store_siting` then falls back to disk by design and the flake is
+untouched there. `store_root`'s docstring enumerates the five ways that happens.
+
 ### Why LD_PRELOAD and not the narrower tool already in the repo
 
 `test_subsystem_store_api.py` monkeypatches `api._fsync_dir` inside

scripts/ci-repro/slowfsync.c

@@ -17,22 +17,116 @@
  * stall short and print an identical line, turning an under-delivered stall
  * into a PASSING run that reads as "not reproducible".
  *
+ * 🔴 `SLOWFSYNC_SKIP_TMPFS=1` — THE FILESYSTEM-AWARE MODE, AND IT EXISTS
+ * BECAUSE THE DEFAULT MODE CANNOT MEASURE A SITING FIX AT ALL. This shim
+ * intercepts fsync(2) in libc, so it stalls whatever the fd is backed by. That
+ * is right for "does a slow fsync fail this test" and WRONG for "does siting
+ * the store off the contended disk fix it": with the default mode both arms of
+ * that comparison go red, and the red on the fixed arm is the shim's, not the
+ * code's. The mechanism the README documents is *device* contention — an fsync
+ * on tmpfs has no backing device to wait for and does not block — so this mode
+ * models the mechanism instead of modelling "every fsync is slow": an fd whose
+ * filesystem reports TMPFS_MAGIC is passed straight through.
+ *
+ * Two properties of that pass-through are deliberate:
+ *   * it does NOT consume the one-shot latch, so a later fsync on a real disk
+ *     still gets the full stall. A pass-through that spent the latch would turn
+ *     "the store moved to tmpfs" into "the shim ran out of ammunition", which
+ *     is a green that means nothing.
+ *   * a *failing* fstatfs() stalls rather than skips. The conservative
+ *     direction for a reproducer is to fire: a shim that quietly stops firing
+ *     reports a pass.
+ * The stall line prints the fd's fs magic, so which filesystem was stalled is
+ * readable from the run rather than inferred.
+ *
+ * 🔴 THE MODE IS SELECTED BY THE VARIABLE'S VALUE, NOT BY ITS PRESENCE, AND THAT
+ * IS A FIX RATHER THAN A STYLE CHOICE. This read `getenv(...) != NULL`, which
+ * tests presence: `SLOWFSYNC_SKIP_TMPFS=0` — the spelling an operator reaches
+ * for to turn the mode OFF — turned it ON. Measured on the store-siting branch,
+ * whose store is on tmpfs: `=0` gave two pass-through lines and `1 passed in
+ * 3.18s`, while the default (unset) mode on the same selection gave `1 failed
+ * in 64.38s`. So an operator disabling the mode got exactly what the paragraph
+ * above warns about — a shim that quietly stopped firing, reporting a pass.
+ *
+ * The convention, and it is enforced by `skip_tmpfs_enabled()` below:
+ *   * ON  for `1`, `true`, `yes`, `on` (case-insensitive);
+ *   * OFF for `0`, `false`, `no`, `off`, the empty string, and for the variable
+ *     being unset entirely;
+ *   * anything else is a TYPO, and it is resolved in the firing direction with a
+ *     one-shot line on stderr. Silently choosing either mode for an unrecognised
+ *     value re-creates this same defect one spelling over; choosing the
+ *     non-firing one would additionally hide it behind a pass.
+ *
  * Build:  gcc -shared -fPIC -o slowfsync.so slowfsync.c -ldl
  * Use:    LD_PRELOAD=/abs/path/slowfsync.so pytest ...
+ *         SLOWFSYNC_SKIP_TMPFS=1 LD_PRELOAD=... pytest ...
  */
 #define _GNU_SOURCE
 #include <dlfcn.h>
 #include <unistd.h>
 #include <stdio.h>
+#include <stdlib.h>
 #include <time.h>
 #include <errno.h>
+#include <string.h>
+#include <strings.h>
+#include <sys/vfs.h>
+
+/* linux/magic.h is not guaranteed on every toolchain this may be built with,
+ * and the value is a stable part of the kernel ABI. */
+#ifndef TMPFS_MAGIC
+#define TMPFS_MAGIC 0x01021994
+#endif
 
 /* The server under test is multi-threaded, so the latch is atomic: a plain int
  * is a C11 data race and could spend a second 65s stall on another thread. */
 static volatile int stalled = 0;
 
 #define STALL_SECONDS 65
 
+/* Warned about an unrecognised SLOWFSYNC_SKIP_TMPFS value already? One line per
+ * process, not one per fsync: a stalling reproducer that prints a warning on
+ * every call buries the stall line it exists to make readable. */
+static volatile int warned_bad_value = 0;
+
+/* Is the filesystem-aware mode ON? Reads the VALUE, not the variable's presence.
+ *
+ * 🔴 `getenv(...) != NULL` was the bug: `SLOWFSYNC_SKIP_TMPFS=0` enabled the
+ * mode. Read on every call rather than cached, which is the pre-existing
+ * behaviour and keeps a mid-run `setenv` from being silently ignored. */
+static int skip_tmpfs_enabled(void) {
+    const char *v = getenv("SLOWFSYNC_SKIP_TMPFS");
+    if (v == NULL) {
+        return 0;
+    }
+    if (!strcasecmp(v, "1") || !strcasecmp(v, "true") || !strcasecmp(v, "yes")
+        || !strcasecmp(v, "on")) {
+        return 1;
+    }
+    if (v[0] == '\0' || !strcasecmp(v, "0") || !strcasecmp(v, "false")
+        || !strcasecmp(v, "no") || !strcasecmp(v, "off")) {
+        return 0;
+    }
+    if (!__atomic_test_and_set(&warned_bad_value, __ATOMIC_SEQ_CST)) {
+        fprintf(stderr, "[slowfsync] SLOWFSYNC_SKIP_TMPFS=%s is not a recognised "
+                "value; the filesystem-aware mode stays OFF and tmpfs fds WILL be "
+                "stalled. Use 1/true/yes/on to enable it, 0/false/no/off to "
+                "disable it. pid=%d\n", v, (int)getpid());
+        fflush(stderr);
+    }
+    return 0;
+}
+
+/* The fd's filesystem magic, or 0 when it could not be read. 0 is NOT tmpfs,
+ * so an unreadable fd stalls — see the header comment. */
+static unsigned long fs_magic(int fd) {
+    struct statfs sb;
+    if (fstatfs(fd, &sb) != 0) {
+        return 0UL;
+    }
+    return (unsigned long)sb.f_type;
+}
+
 int fsync(int fd) {
     static int (*real)(int) = NULL;
     if (!real) {
@@ -50,10 +144,20 @@ int fsync(int fd) {
             return -1;
         }
     }
+    unsigned long magic = fs_magic(fd);
+    if (skip_tmpfs_enabled() && magic == TMPFS_MAGIC) {
+        /* Deliberately BEFORE the latch, and deliberately loud: a silent skip
+         * is indistinguishable from a shim that never attached. */
+        fprintf(stderr, "[slowfsync] pass-through fsync(%d): tmpfs (magic=0x%lx), "
+                "latch untouched, pid=%d\n", fd, magic, (int)getpid());
+        fflush(stderr);
+        return real(fd);
+    }
     if (!__atomic_test_and_set(&stalled, __ATOMIC_SEQ_CST)) {
         struct timespec t0, t1, rem;
-        fprintf(stderr, "[slowfsync] stalling fsync(%d) for %ds (HANG_TIMEOUT=60), pid=%d\n",
-                fd, STALL_SECONDS, (int)getpid());
+        fprintf(stderr, "[slowfsync] stalling fsync(%d) for %ds (HANG_TIMEOUT=60), "
+                "fs magic=0x%lx, pid=%d\n",
+                fd, STALL_SECONDS, magic, (int)getpid());
         fflush(stderr);
         clock_gettime(CLOCK_MONOTONIC, &t0);
         rem.tv_sec = STALL_SECONDS;