diff --git a/scripts/ci-repro/README.md b/scripts/ci-repro/README.md index d08184897..276b5c01a 100644 --- a/scripts/ci-repro/README.md +++ b/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 diff --git a/scripts/ci-repro/slowfsync.c b/scripts/ci-repro/slowfsync.c index 20d7580f8..0b146e753 100644 --- a/scripts/ci-repro/slowfsync.c +++ b/scripts/ci-repro/slowfsync.c @@ -17,15 +17,66 @@ * 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 #include #include +#include #include #include +#include +#include +#include + +/* 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. */ @@ -33,6 +84,49 @@ 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; diff --git a/scripts/tests/test_store_siting_ledger.py b/scripts/tests/test_store_siting_ledger.py index 40366cce9..eb92cddd1 100644 --- a/scripts/tests/test_store_siting_ledger.py +++ b/scripts/tests/test_store_siting_ledger.py @@ -223,11 +223,38 @@ def test_every_ledgered_file_IMPORTS_AND_CALLS_the_shared_siting_at_least_once() as coverage is what stops anyone looking, so the name now states the weaker, true thing. - `scoped_store` is fixed. The residual gap is recorded and NOT closed here: - `test_subsystem_store_api.py` still builds store roots inline from `tmp_path` at - the sites `_DISK_ROOTED_SITES` counts below ā€” 20 of them spelled `tmp_path / - "store"` and 13 not ā€” each in one or two tests rather than a shared fixture. That - count is asserted so it can only go DOWN ā€” see the next test. + `scoped_store` is fixed, and so are the 18 sites that were spelled `tmp_path / + "store"` ā€” they take their root from the `sited_root` fixture now. The residual + gap is recorded and NOT closed here: `test_subsystem_store_api.py` still builds + 15 store roots inline from `tmp_path` under OTHER directory names. Every one of + them is enumerated, with its reason, in `_DISK_ROOTED_ALLOWLIST` below, and + `test_the_disk_rooted_census_matches_the_allowlist_EXACTLY` asserts that set in + BOTH directions. + + āš  "COVERED THERE" IS NOT WHAT THAT AMOUNTS TO, AND THIS SENTENCE USED TO SAY IT + WAS. The census reduces the residual; it does not close it. It ran over ONE of + the three files named here until this round ā€” a hardcoded + `TESTS / "test_subsystem_store_api.py"` under a docstring that talked about the + ledger ā€” and it now runs over all three, which is a real widening and still not + coverage: `_is_disk_rooted_store_expr` sees a `tmp_path / X` or a + `tmp_path.joinpath(X)` that flows into a store consumer within one function scope, + THROUGH A BINDING FORM `_assignments` RESOLVES, AND THROUGH AN EXPRESSION + `_path_base` CAN NAME A BASE FOR. The third condition is not a technicality: a + dict or list element satisfies the first two ā€” `stores = {'a': tmp_path / + 'served'}` IS an `ast.Assign` and `_assignments` does yield it ā€” and still + measures 0, because `_path_base` returns None for the `ast.Subscript` at the + consumer, so the binding is never reached. A `for` target and a comprehension + target fail the second condition. A CLOSURE fails the FIRST and neither of the + other two ā€” its binding is a plain assignment and `_path_base` names its base ā€” + because "one function scope" means `_walk_scope`'s scope, and that stops at a + nested `def` a reader sees as part of the same test. It is the scope arm one level + in, not a binding case. "Within one function + scope, and nothing else" is what this sentence used to say, and it is wider than + the code by exactly that set. The enumeration lives in + `test_the_disk_rooted_census_matches_the_allowlist_EXACTLY`'s docstring, with the + numbers. So the honest statement is that this test's "at least once" weakness is + REDUCED by the census over the sites the census can see, and the rest is written + down rather than guarded. """ offenders = [] for name in sorted(EXPECTED_SERVER_TESTS): @@ -269,54 +296,147 @@ def test_the_scan_can_actually_SEE_a_build_server_call(): # AST collapses all four ā€” quoting and spacing do not survive parsing at all. # # A scripted mass conversion of these sites was attempted and REVERTED: it silently -# skipped 9 of 19 signature edits while its own assertion still passed. Hence a ratchet -# rather than a rushed refactor. -# šŸ”“ 33, UP FROM 20, AND NOT ONE SITE WAS ADDED. The predicate below stopped being -# spelled, and 13 inline disk-backed store roots that had always been there became -# visible. Every one of them was a REAL store served by `running(...)`. The -# breakdown, RE-MEASURED 2026-09-02 and summing to 13 ā€” an earlier revision of this -# comment said "five" `served` and summed to 15, which is the sort of arithmetic -# nobody re-does: +# skipped 9 of 19 signature edits while its own assertion still passed. That is why the +# conversion that finally landed is verified by the CENSUS below rather than by the +# converting script's own assertion. # -# served 3 a `cp -a` of the store fixture, written into, served -# served-elsewhere 3 -# stage 2 the output of `run_seed`, served back -# absent 2 `running(tmp_path / "absent")` -# big 1 -# at-the-cap 1 -# unambiguous 1 -# -- -# 13 + the 20 `store`/`name`/`kind` sites = 33 +# šŸ”“ A COUNT WAS REPLACED BY AN ENUMERATED SET, AND THE REASON IS NOT TIDINESS. +# `_DISK_ROOTED_SITES = 33` could not distinguish "a site was migrated and a new one was +# written" from "nothing happened": both read 33. It also could not say WHICH sites it +# was counting, so its own history is a list of arguments about whether a number moving +# meant new debt or a wider predicate (20 -> 33 was the latter, with not one site added). +# An enumerated allowlist answers both: the failure message names the site, and a +# migration plus a regression in one commit shows up as one name arriving and another +# leaving rather than as a flat total. # -# The `absent` pair is the only debatable entry: an absent store writes nothing and -# fsyncs nothing, so counting it errs WIDE ā€” the safe direction for a ratchet, and not -# worth a second spelled exception to avoid. +# EVERY KEY IS A SITE THAT IS STILL DISK-ROOTED, AND EVERY VALUE IS THE REASON IT IS +# STILL HERE. An empty reason is a failure ā€” see `test_the_disk_rooted_census_matches_ +# the_allowlist_EXACTLY`. Adding a key is how you record debt you are choosing to leave; +# it is not a way to silence the guard, because the reason is read by a human at review +# time and the key names the exact test. # -# šŸ”“ SO THIS NUMBER MOVING UP IS NOT ALWAYS DEBT ARRIVING. The assertion below cannot -# tell "13 new store sites were written" from "the predicate got 13 sites wider", and -# they demand opposite actions. Establish WHICH before touching the constant, the same -# way the DOWN direction already demands. -_DISK_ROOTED_SITES = 33 +# The 18 sites spelled `tmp_path / "store"` are GONE ā€” they take their root from the +# `sited_root` fixture (see `test_subsystem_store_api.py`), which is +# `store_siting.store_root` with a test's lifetime. What remains is the population that +# was never spelled "store": stores built under another directory name, all of them +# served READ-ONLY. +# +# šŸ”“ SAY WHAT THE REASON CLAIMS AND WHAT IT DOES NOT. "read-only" here means the test +# drives `run_verify` / a GET route and issues no write verb, so no request reaches +# `server.py:_replace_bytes` and nothing fsyncs inside a request ā€” which is the specific +# mechanism the gate flake is made of. It does NOT mean these are harmless: they are +# still disk-backed store roots, and siting them is the obvious follow-up. They are +# recorded rather than migrated because this change was scoped to the write-path sites +# that were failing the gate, and a 33-site conversion is exactly the shape that got +# reverted last time. +# +# šŸ”“ KEYED BY FILE, BECAUSE THE CENSUS USED TO READ ONE OF THE THREE LEDGERED FILES +# WHILE ITS OWN PROSE TALKED ABOUT THE LEDGER. `TESTS / "test_subsystem_store_api.py"` +# was hardcoded in the census test and in the sited-ledger test below ā€” two functions +# under a frozenset naming three files ā€” and +# `test_every_ledgered_file_IMPORTS_AND_CALLS_the_shared_siting_at_least_once` pointed +# at the census as where its own weakness "is covered". MEASURED at the moment of the +# fix, so it is on record that this closed no live defect: api 15, `test_cairn_write.py` +# 0, `test_cairn_cli.py` 0. The two zeros are exactly why it mattered anyway ā€” a file +# contributing nothing is indistinguishable from a file never read, and +# `test_cairn_write.py` going disk-backed is the incident in this module's own header. +_DISK_ROOTED_ALLOWLIST: dict[str, dict[str, str]] = { + "test_subsystem_store_api.py": { + "TestFourStates.test_store_unreachable_is_503_and_NOT_a_200 :: tmp_path / 'absent'": + "the store deliberately does NOT exist; nothing is written and nothing is " + "fsynced. Counted at all only because the predicate errs WIDE.", + "TestFourStates.test_scope_empty_and_store_unreachable_SHARE_NOTHING :: " + "tmp_path / 'absent'": + "same absent-store shape as above.", + "TestByteIdentityVerifier.test_NEGATIVE_a_ONE_CHARACTER_divergence_FAILS_and_names_" + "the_scope :: tmp_path / 'served'": + "a `cp -a` of the store fixture served READ-ONLY to `run_verify`; no write " + "verb, so no in-request fsync.", + "TestByteIdentityVerifier.test_NEGATIVE_a_MISSING_entry_on_the_remote_FAILS :: " + "tmp_path / 'served'": + "read-only `run_verify` copy, as above.", + "TestByteIdentityVerifier.test_a_PAGINATED_index_is_REFUSED_rather_than_partially_" + "compared :: tmp_path / 'big'": + "read-only `run_verify` fixture. LISTING_PAGE_SIZE+1 entries, so it is also " + "the largest store in this population.", + "TestByteIdentityVerifier.test_a_scope_of_EXACTLY_LISTING_PAGE_SIZE_is_COMPARED_not_" + "refused :: tmp_path / 'at-the-cap'": + "read-only `run_verify` fixture, the other side of the same boundary.", + "TestByteIdentityVerifier.test_an_UNAMBIGUOUS_scope_of_the_SAME_SHAPE_still_PASSES " + ":: tmp_path / 'unambiguous'": + "read-only `run_verify` fixture.", + "TestByteIdentityVerifier.test_the_disclosure_survives_a_FAILING_run :: " + "tmp_path / 'served'": + "read-only `run_verify` copy.", + "TestByteIdentityVerifier.test_every_permitted_difference_is_ACCOUNTED_FOR_not_" + "merely_small :: tmp_path / 'served-elsewhere'": + "read-only `run_verify` copy.", + "TestByteIdentityVerifier.test_a_POD_SHAPED_remote_PASSES_when_only_the_THREE_" + "permitted_lines_differ :: tmp_path / 'served-elsewhere'": + "read-only `run_verify` copy.", + "TestByteIdentityVerifier.test_a_POD_SHAPED_remote_STILL_FAILS_on_a_real_content_" + "difference :: tmp_path / 'served-elsewhere'": + "read-only `run_verify` copy.", + "TestSeedThenVerify.test_a_seeded_copy_serves_byte_identical_digests :: " + "tmp_path / 'stage'": + "the output of `seed.sh` run as a SUBPROCESS, then served read-only to " + "`run_verify`. The writer is the seed script, not the request path.", + "TestSeedThenVerify.test_a_seed_that_MISSED_a_scope_is_caught_by_the_verifier :: " + "tmp_path / 'stage'": + "same seed-then-verify shape as above.", + "TestTheLoaderRefusesHostileEntriesByKind._recall_over_http :: tmp_path / name": + "a per-kind hostile store read through `running_subprocess`; the route under " + "test is a GET recall, and several kinds are FIFOs that must never be written.", + "TestTheLoaderRefusesHostileEntriesByKind.test_the_REFUSED_DIRECTORY_is_a_NAMED_row_" + "not_a_silent_skip :: tmp_path / kind": + "same hostile-kind read path as above.", + }, + # MEASURED, not asserted from the shape of the files: the census reports + # ZERO disk-rooted sites in each of these, so an empty allowlist is the + # correct entry and any arrival here is a NEW one. An empty dict is a + # positive statement ā€” "this file has none" ā€” where a missing key would + # only be an absence. + "test_cairn_write.py": {}, + "test_cairn_cli.py": {}, +} # Directory names that make a `tmp_path / ""` a store root ON SIGHT, with no -# need for it to flow anywhere. MEASURED 2026-09-02: of the 33 counted sites, 26 flow -# into a store consumer and would be caught without this set; 7 are counted ONLY by -# this set, so deleting it would lose those seven. +# need for it to flow anywhere. +# +# šŸ”“ RE-MEASURED, AND THE PREVIOUS MEASUREMENT'S SUBJECT NO LONGER EXISTS. It read: +# "of the 33 counted sites, 26 flow into a store consumer and would be caught without +# this set; 7 are counted ONLY by this set" ā€” and it named those seven, every one of +# them a `tmp_path / "store"` whose binding was invisible to the flow arm because +# `_walk_scope` stops at a nested-function boundary or because the consumer +# (`api.append_bullet`, `api.rc.load_index`) is not in `_ROOT_CONSUMERS`. All seven +# were among the 18 sites migrated to `sited_root`, so the propping-up they described +# is gone with them. +# +# MEASURED on the current file, by running the census with `_ROOT_NAMES` set to +# `{"store", "src"}` and again with it EMPTY: **15 both times, with an identical set of +# keys.** So on `test_subsystem_store_api.py` today this set catches nothing the flow +# arm does not already catch, and deleting it would lose nothing THERE. +# +# šŸ”“ AND IT IS INERT IN THIS FILE'S OWN PROBE SUITE TOO ā€” SAY IT, BECAUSE THE FIRST +# DRAFT OF THIS PARAGRAPH CLAIMED THE OPPOSITE. It read "two of this file's own probe +# tests depend on the on-sight arm being present", naming +# `test_the_site_index_does_not_key_on_the_DIRECTORY_being_spelled_store`. That is +# false: every probe in that test hands its path to `running(...)`, so the FLOW arm +# counts it and the directory's spelling never decides anything. MEASURED by setting +# `_ROOT_NAMES` to the empty set and running every argument-free test in this module: +# **zero behaviour changes.** # -# šŸ”“ BUT DO NOT READ THOSE 7 AS "SITES THAT GENUINELY DO NOT FLOW ANYWHERE" ā€” an -# earlier revision of this comment said exactly that and it is false of all seven. They -# are every `tmp_path / "store"` at :11310, :11765, :13099, :14086, :14201, :14227 and -# :14803 of `test_subsystem_store_api.py`, and they split two ways, each one a MASKED -# GAP in the flow arm rather than a shortcut: -# * :11310 and :13099 are `root = tmp_path / "store"` in a `_phases` helper whose -# NESTED `present()`/`absent()` closures call `_build_store(root, ā€¦)` ā€” a consumer -# `_ROOT_CONSUMERS` names. They are invisible only because `_walk_scope` stops at a -# nested function boundary, so no single scope sees both the binding and the call. -# * the other five reach `api.append_bullet` / `api.rc.load_index` ā€” real store -# consumers that `_ROOT_CONSUMERS` simply does not name (see its own comment: the -# set closes renames, not GROWTH into a new consumer name). -# So `_ROOT_NAMES` is currently propping up the flow arm on this file. Deleting it -# would not "lose seven non-flowing sites", it would expose two structural gaps. +# So today this set catches nothing, anywhere that is measured. It is KEPT rather than +# deleted, and the reason is an argument rather than a measurement, which is the +# honest way to hold it: `_ROOT_CONSUMERS`' own comment records that the flow arm +# closes RENAMES but not GROWTH into a new consumer name, and the on-sight arm is the +# only path that does not go through that set at all. Deleting it is a real option and +# a separate change; the CLOSING CONDITION for that decision is a measurement showing +# the flow arm covers a store built under a name the consumer set does not know, which +# nothing here demonstrates today. +# +# What HAS changed is that the contribution is now measurable in one command instead +# of being asserted in prose ā€” which is what caught the false sentence above. _ROOT_NAMES = {"store", "src"} @@ -342,6 +462,51 @@ def _is_disk_rooted_store_expr(node: ast.AST) -> bool: Constant arm now ALSO counts a path that flows into a store consumer, and `_ROOT_NAMES` is the residual on-sight shortcut rather than the whole test. + šŸ”“ AND THE OPERAND'S *NODE TYPE* WAS THE SAME DEFECT ONE LEVEL DOWN. Both arms + used to require `isinstance(right, (ast.Name, ast.Constant))` before consulting + the flow gate, which is a guard on the SHAPE of an ordinary spelling: `tmp_path / + f"store-{k}"` is an `ast.JoinedStr` and `tmp_path / ("store" + k)` is an + `ast.BinOp`, so neither was counted no matter where it flowed. MEASURED as a live + hole rather than imagined: swapping + `TestTheRENAMEIsFSYNCedToo.test_an_append_fsyncs_BOTH_the_file_and_its_DIRECTORY` + back to `_build_store(tmp_path / f"store-{ALLOW_SCOPE}", ā€¦)` ā€” a WRITE-PATH store + that reaches `api.append_bullet` -> `_replace_bytes` and its two in-request + fsyncs, i.e. exactly the flake population this file exists to shrink ā€” left the + ledger at **23 passed** and the api file at **760 passed**. The type gate is gone; + the flow gate is what does the discriminating. + + šŸ”“ AND THE WIDENING SHIPPED WITH NO TEST ā€” THE ONE DEFECT THIS MODULE'S WHOLE + PREMISE IS THAT A HAND MEASUREMENT CANNOT COVER. The first version of this + paragraph cited `test_a_path_that_is_never_used_as_a_store_still_does_NOT_count` + as the control that "already ruled out" round 3's false positive; that test's + probes are `ast.Constant` operands to the last one, as were both probes in + `test_the_census_can_actually_SEE_a_disk_rooted_site`. So restoring BOTH + `isinstance` gates verbatim ā€” reverting this entire widening ā€” left the file at + **23 passed**, and the measurement above was the only thing that had ever + exercised it. `test_the_operand_NODE_TYPE_is_not_what_decides_either` is the + regression coverage now: four non-Constant spellings across both arms, counted + when they flow into a consumer and 0 when they do not, and each half watched red + on its own mutant. + + āš  THE WIDENING IS NOT A GENERAL "ANY STORE ROOT" CLAIM ā€” READ THE RESIDUAL. The + left operand must still be the NAME `tmp_path`, and the expression must still be a + `/` or a `.joinpath(...)`, so these remain INVISIBLE and were each measured 0 + after the widening: `Path(tmp_path) / "store"`, `base = tmp_path` then `base / + "store"`, `os.path.join(tmp_path, "store")`, `str(tmp_path) + "/store"`, and + `tmp_path_factory.mktemp("store")`. So does any root whose only path to a consumer + crosses a `@pytest.fixture` boundary, widened or not ā€” that is the separate hole + `test_a_store_root_bound_in_a_pytest_FIXTURE_is_NOT_counted` pins, and a helper + that RETURNS `tmp_path / "holder"` is counted only when the helper is itself + CALLED in a store-flowing position, never when pytest injects it as a fixture. + + āš  AND THE FLOW GATE IS NARROWER THAN "REACHES A CONSUMER IN THIS FUNCTION" ā€” + `_used_as_a_store_root` only says yes to what `_index_store_root_uses` marked, and + that walks the binding forms `_assignments` resolves. A root reaching a real + consumer, in one scope, through a container ELEMENT (`stores['a']`, `stores[0]`), + a `for` or comprehension TARGET, or a CLOSURE measures 0 ā€” enumerated with the + numbers in `test_the_disk_rooted_census_matches_the_allowlist_EXACTLY`'s fourth + residual bullet, and pinned by nothing. + Quoting and spacing still do not matter: they do not survive parsing. """ if isinstance(node, ast.BinOp) and isinstance(node.op, ast.Div) and ( @@ -350,9 +515,7 @@ def _is_disk_rooted_store_expr(node: ast.AST) -> bool: right = node.right if isinstance(right, ast.Constant) and right.value in _ROOT_NAMES: return True - return isinstance(right, (ast.Name, ast.Constant)) and _used_as_a_store_root( - node - ) + return _used_as_a_store_root(node) if ( isinstance(node, ast.Call) and isinstance(node.func, ast.Attribute) @@ -364,9 +527,7 @@ def _is_disk_rooted_store_expr(node: ast.AST) -> bool: first = node.args[0] if isinstance(first, ast.Constant) and first.value in _ROOT_NAMES: return True - return isinstance(first, (ast.Name, ast.Constant)) and _used_as_a_store_root( - node - ) + return _used_as_a_store_root(node) return False @@ -479,9 +640,42 @@ def _assignments(scope: ast.AST): šŸ”“ SAY WHICH ONES ARE HERE. A previous revision of this docstring listed four shapes as "here" and two of them were not: a helper that RETURNS the root is resolved in `_index_store_root_uses` via `flowing_callees`, not by any binding - form; and it omitted `ast.withitem`, which the body below does handle. The walrus - IS here, but it is an INVARIANT guard rather than regression coverage ā€” it already - counted before this function existed, because a consumer's argument is walked + form; and it omitted `ast.withitem`, which the body below does handle. + + šŸ”“ AND SAY WHICH ONES ARE NOT, BECAUSE THIS SET IS HALF THE CENSUS'S FLOW REACH. + A `for` target and a comprehension target are bindings this does not yield, and a + container ELEMENT is not a target at all (`_path_base` returns None for an + `ast.Subscript`). Each measures 0 even inside one function scope with a real + consumer ā€” enumerated in + `test_the_disk_rooted_census_matches_the_allowlist_EXACTLY`'s fourth residual + bullet. + + šŸ”“ AND SAY WHAT WIDENING THIS FUNCTION WOULD ACTUALLY CLOSE, BECAUSE IT IS NOT ALL + OF THAT BULLET AND THIS SENTENCE USED TO PROMISE IT WAS ("widening this function + would close them"). Measured by adding `ast.For` / `ast.AsyncFor` / + `ast.comprehension` targets to the yields below and re-running each probe, against + an inline control of 1: + + for target 0 -> 1 CLOSED by widening this + comprehension target (isolated) 0 -> 1 CLOSED by widening this + dict element 0 -> 0 NOT ā€” it is a `_path_base` case + list element 0 -> 0 NOT ā€” it is a `_path_base` case + closure 0 -> 0 NOT ā€” it is a `_walk_scope` case + + So two rows of five, and the two it does not close are not this function's to + close: teaching `_path_base` to see through an `ast.Subscript` takes both element + rows 0 -> 1 with `_assignments` untouched, and the closure stays 0 under BOTH + widenings because `_walk_scope` stops at the nested `def`. That matters because a + maintainer told to widen, re-run the census and record what it starts seeing will + watch the census move and read the hole as closed while the element and closure + halves stay open with the suite green. + + The widening is deliberately not done here; doing it means re-running the census + and recording whatever it starts seeing ā€” which, per the table, is the first two + rows and nothing else. + + The walrus IS here, but it is an INVARIANT guard rather than regression coverage ā€” + it already counted before this function existed, because a consumer's argument is walked recursively, so the nested `tmp_path / name` was reached without the binding ever being resolved. `test_the_site_index_sees_the_BINDING_FORMS_a_plain_assignment_is_ not`'s docstring says the same thing, and the two used to disagree. @@ -611,33 +805,417 @@ def _used_as_a_store_root(node: ast.AST) -> bool: return _STORE_ROOT_PARENTS.get(id(node), False) -def test_the_inline_disk_rooted_store_sites_do_not_GROW(): - path = TESTS / "test_subsystem_store_api.py" - tree = ast.parse(path.read_text(encoding="utf-8")) +def _qualnames(tree: ast.AST) -> dict[int, str]: + """`id(node) -> dotted name of the innermost class/def it sits in`. + + šŸ”“ A LINE NUMBER IS NOT A SITE IDENTITY. The census below has to name each site + in a message a human acts on, and in a key that survives the file being edited + ANYWHERE ABOVE it. A line number survives neither: it moves when an unrelated + docstring gains a sentence, so a ledger keyed on one would demand an edit on + every commit and would be routinely re-baselined without anyone reading it. + The enclosing `Class.function` moves only when the thing itself is renamed, + which is a change a reviewer should see. + """ + owner: dict[int, str] = {} + + def walk(node: ast.AST, prefix: str) -> None: + for child in ast.iter_child_nodes(node): + if isinstance(child, (ast.FunctionDef, ast.AsyncFunctionDef, ast.ClassDef)): + name = f"{prefix}.{child.name}" if prefix else child.name + for sub in ast.walk(child): + owner[id(sub)] = name + walk(child, name) + else: + for sub in ast.walk(child): + owner.setdefault(id(sub), prefix or "") + walk(child, prefix) + + walk(tree, "") + return owner + + +def _disk_rooted_census(tree: ast.AST) -> dict[str, int]: + """Every site in `tree` that `_is_disk_rooted_store_expr` SEES, keyed + ` :: `. + + āš  That is deliberately narrower than "every disk-rooted store site in `tree`", + which is what this line used to say. The predicate's own docstring enumerates + what it cannot see, and the census inherits every one of those holes. + + The expression comes from `ast.unparse`, so quoting and spacing are normalised + away exactly as they are for the predicate itself ā€” `tmp_path/"store"` and + `tmp_path / 'store'` produce the SAME key, and neither can be used to walk + past a ledger entry. Two identical expressions in one scope get ` #2`, ` #3` + suffixes in source order; the value is the line number, for the message only. + """ _index_store_root_uses(tree) - actual = sum(1 for n in ast.walk(tree) if _is_disk_rooted_store_expr(n)) - assert actual <= _DISK_ROOTED_SITES, ( - f"{actual} inline disk-backed store roots, up from {_DISK_ROOTED_SITES}. " - "Each writes through server.py:_replace_bytes and fsyncs inside the request, " - "so a new one rejoins the contention-flake population. Use " - "testlib.store_siting.store_root() instead. šŸ”“ BUT FIRST check WHICH happened, " - "because this direction is ambiguous too: did someone WRITE new inline store " - "sites, or did _is_disk_rooted_store_expr get WIDER and start seeing sites " - "that were always there? Round 7 was the second ā€” 20 -> 33 with no site added, " - "because the predicate stopped keying on the directory being spelled 'store'. " - "Raising the constant is right for a widening and wrong for new debt." + owner = _qualnames(tree) + hits = sorted( + (n for n in ast.walk(tree) if _is_disk_rooted_store_expr(n)), + key=lambda n: (n.lineno, n.col_offset), + ) + seen: dict[str, int] = {} + census: dict[str, int] = {} + for node in hits: + base = f"{owner.get(id(node), '')} :: {ast.unparse(node)}" + seen[base] = seen.get(base, 0) + 1 + key = base if seen[base] == 1 else f"{base} #{seen[base]}" + census[key] = node.lineno + return census + + +def _census_over_the_ledgered_files() -> dict[str, int]: + """The census over EVERY file in `EXPECTED_SERVER_TESTS`, keyed with the file. + + šŸ”“ THE FILE IS IN THE KEY, AND A LINE NUMBER STILL IS NOT ā€” `_qualnames`' reason + holds unchanged. Without the file name two same-named tests in two files would + collide into one allowlist entry, which is the flat-total defect one level down: + a site arriving in one file and leaving another would read as no change at all. + + One tree at a time, deliberately: `_index_store_root_uses` keys its marks on + `id(node)` and clears them per call, so two trees alive at once could see a + recycled id vouch for the wrong node. Each file's census is finished before the + next file is parsed. + """ + census: dict[str, int] = {} + for name in sorted(EXPECTED_SERVER_TESTS): + tree = ast.parse((TESTS / name).read_text(encoding="utf-8")) + for key, lineno in _disk_rooted_census(tree).items(): + census[f"{name} :: {key}"] = lineno + return census + + +def _flat_allowlist() -> dict[str, str]: + """`_DISK_ROOTED_ALLOWLIST` flattened onto the census's ` :: ā€¦` keys.""" + return { + f"{name} :: {key}": why + for name, entries in _DISK_ROOTED_ALLOWLIST.items() + for key, why in entries.items() + } + + +def test_the_disk_rooted_census_matches_the_allowlist_EXACTLY(): + """šŸ”“ A SET, IN BOTH DIRECTIONS ā€” not a count, and not a ceiling. + + This is the guard the `store` fixture's own positive control could not be: + that one takes a single fixture and proves IT lands on tmpfs, which says + nothing about the other sites in the file. Here every store site the + predicate can see is enumerated and compared to `_DISK_ROOTED_ALLOWLIST`. + + * a name in the census that is NOT in the allowlist is a NEW disk-backed + store root ā€” the regression this exists to catch, and the shape a revert + of one of the migrated sites takes. + * a name in the allowlist that is NOT in the census means either the site + was migrated (delete the entry, in the same commit) or the predicate + NARROWED and stopped seeing it (widen it back ā€” deleting the entry would + bank a coverage loss as if it were progress). The message says both, + because this assertion cannot tell them apart and they demand opposite + actions. + + āš  IT IS A CLAIM ABOUT WHAT THE PREDICATE CAN SEE, WHICH IS NARROWER THAN "every + store root in these files" ā€” SAY THE WHOLE RESIDUAL, NOT ONE HOLE OF IT. This + paragraph used to name only the fixture case, which made it read as the complete + list. The full set ā€” and ONE OF THE FOUR IS PINNED BY A GUARD, the other three are + written down and nothing re-measures them on any run. That distinction is the + point: a hole a guard re-measures cannot silently close or widen; a hole only + written down can do both with the suite green. + + * PINNED. A root bound inside a `@pytest.fixture` and served in a test that + requests it crosses a scope boundary the AST cannot follow ā€” + `test_a_store_root_bound_in_a_pytest_FIXTURE_is_NOT_counted` asserts 0 on both + fixture forms against an inline control of 1, and two live instances are + named there; + * ONLY WRITTEN DOWN. A root not spelled `tmp_path / X` or `tmp_path.joinpath(X)` + at all ā€” `Path(tmp_path) / "store"`, an alias `base = tmp_path`, + `os.path.join(...)`, `str(tmp_path) + "/store"`, + `tmp_path_factory.mktemp(...)`. Each measured 0 by hand, both before and after + this round's widening of the operand type, and every occurrence of those + spellings in this file is inside a docstring ā€” no test exercises one; + * ONLY WRITTEN DOWN. A root that reaches its consumer through a call the flow + arm does not model ā€” `_ROOT_CONSUMERS` closes renames and not GROWTH into a + new consumer name, which its own COMMENT records, including the + `serve_store(served)` -> 0 measurement. A comment is not a guard; + * ONLY WRITTEN DOWN. šŸ”“ A root that reaches its consumer WITHIN ONE TEST + FUNCTION, with no fixture involved, and is still not counted ā€” "test function" + and not "scope", because one of the rows below is precisely a case where those + two differ. The three + bullets above did NOT cover + this and the list read as complete ā€” the fixture bullet is a scope boundary, + the spelling bullet is not `tmp_path / X`, the consumer bullet is a name + outside `_ROOT_CONSUMERS`, and each of these is none of those. + + šŸ”“ IT IS THREE MECHANISMS, NOT ONE, AND THIS BULLET USED TO NAME ONLY THE + BINDING ONE ("not through a binding form `_assignments` resolves"). Each row + below carries the mechanism that actually stops it, established by widening + ONE thing at a time and re-measuring rather than by reading the code. Measured, + all inside one `def test_probe(tmp_path)`, all reaching `running` (which IS in + `_ROOT_CONSUMERS`), all spelled `tmp_path / 'served'`, no fixture anywhere: + + inline (control) -> 1 + stores = {'a': tmp_path / 'served'}; running(stores['a']) -> 0 base + stores = [tmp_path / 'served']; running(stores[0]) -> 0 base + for served in (tmp_path / 'served',): running(served) -> 0 bind + out = [running(p) for p in (tmp_path / 'served',)] -> 0 bind + served = tmp_path / 'served'; def go(): running(served); go() -> 0 scope + + `bind` ā€” `_assignments` resolves `Assign` / `AnnAssign` / `NamedExpr` / + `withitem` and nothing else, so a `for` target and a comprehension target bind + invisibly. Adding `ast.For` / `ast.comprehension` targets to it takes both rows + 0 -> 1 and moves no other row. + + `base` ā€” `_path_base` returns None for an `ast.Subscript`, so the expression + the consumer is HANDED is one it cannot name a base for and the binding is + never reached. These two rows are unmoved by a widened `_assignments` and go + 0 -> 1 when `_path_base` is taught to see through a Subscript. + + `scope` ā€” `_walk_scope` stops at every nested function, `def go()` included, + so it is not only pytest's injection that crosses a boundary. Unmoved by + either widening. + + āš  ROW 4 USED TO BE `roots = [p for p in (tmp_path / 'served',)]; + running(roots[0])`, filed under the comprehension target. It does not + demonstrate that mechanism: it is 0 under a widened `_assignments` and 1 under + a widened `_path_base` alone, i.e. the SUBSCRIPT is what stops it and the + comprehension target is not load-bearing there at all ā€” `mark()` walks the + whole bound value, so once `roots` is reached the comprehension is transparent. + It is replaced above by a row that isolates the target. + + None of the three widenings is made here; each is written down rather than + guarded, which is the same deal the three bullets above get. + + An empty census would therefore NOT prove these files are fully sited, and the + `test_the_census_can_actually_SEE_a_disk_rooted_site` control below is what stops + an empty one reading as an all-clear. + + šŸ”“ ALL THREE LEDGERED FILES, NOT ONE. This hardcoded + `TESTS / "test_subsystem_store_api.py"` while `EXPECTED_SERVER_TESTS` two + functions above named three. Measured when that was fixed: 15 / 0 / 0, so no live + site was being missed ā€” but a file the scanner never opens reports the same zero + as a file that is clean, and this module exists because a fix applied to one file + of three was taken for a fix to all three. + """ + census = _census_over_the_ledgered_files() + allowlist = _flat_allowlist() + + # The allowlist must describe the SAME file set the census reads. Without this a + # file added to the ledger would arrive with no allowlist key at all, and the + # absence would read as "nothing to record" rather than as an unmade decision. + assert set(_DISK_ROOTED_ALLOWLIST) == set(EXPECTED_SERVER_TESTS), ( + f"_DISK_ROOTED_ALLOWLIST covers {sorted(_DISK_ROOTED_ALLOWLIST)} but the " + f"ledger names {sorted(EXPECTED_SERVER_TESTS)}. Give every ledgered file a " + "key ā€” an empty dict is the right entry for a file with no disk-rooted " + "sites, and it says so where a missing key would only be silent." + ) + + unrecorded = sorted(set(census) - set(allowlist)) + stale = sorted(set(allowlist) - set(census)) + assert not unrecorded, ( + "these disk-backed store roots are NOT in _DISK_ROOTED_ALLOWLIST:\n " + + "\n ".join(f"{k} (line {census[k]})" for k in unrecorded) + + "\n\nEach one builds its store on the contended disk. A store that is " + "written through the server fsyncs the file AND its parent directory " + "INSIDE the request (server.py:_replace_bytes), and one such fsync " + "exceeding HANG_TIMEOUT is what fails tekton/devrc-pytests on PRs whose " + "diff cannot reach the test at all. Take the root from the `sited_root` " + "fixture instead ā€” or, if this site genuinely cannot be sited, add it to " + "_DISK_ROOTED_ALLOWLIST WITH THE REASON, which a reviewer will read." + ) + assert not stale, ( + "_DISK_ROOTED_ALLOWLIST names sites the census no longer sees:\n " + + "\n ".join(stale) + + "\n\nšŸ”“ FIRST establish WHICH happened, because this assertion cannot and " + "the two demand opposite actions: (a) the site was genuinely migrated to " + "store_siting.store_root() ā€” delete the entry in the SAME commit; or (b) " + "_is_disk_rooted_store_expr NARROWED and stopped seeing a site that is " + "still there ā€” widen it back. Deleting the entry for (b) banks a coverage " + "loss as if it were progress." + ) + empty = sorted(k for k, why in allowlist.items() if not why.strip()) + assert not empty, ( + f"these allowlist entries carry no reason: {empty}. An entry without one is " + "a silenced guard: the whole point of an enumerated allowlist over a count " + "is that a human reading the diff can tell debt being recorded from debt " + "being hidden." + ) + + +def test_the_census_can_actually_SEE_a_disk_rooted_site(): + """The positive control, and it is not optional here. + + The census asserts a SET, and the day the allowlist reaches empty an assertion + of `set() == set()` is satisfied by a scanner wired to nothing just as well as + by a fully-sited file. So feed it a module that MUST produce a non-zero count + and watch the number move. Report the pair, never the zero alone. + + šŸ”“ BOTH ARMS, BECAUSE ONE PROBE ONLY EXERCISED ONE. The probe was + `root = tmp_path / 'store'`, which `_ROOT_NAMES` answers ON SIGHT with no flow + analysis at all: emptying `_ROOT_NAMES` and breaking `_index_store_root_uses` + outright would have left this control green, so the "every zero elsewhere is + interpretable" claim it makes covered half the predicate. The second probe is + named `holder`, outside `_ROOT_NAMES`, and is counted only because it flows into + a consumer ā€” so a dead flow arm now fails the control that claims to test it. + """ + on_sight = ( + "def test_probe(tmp_path):\n" + " root = tmp_path / 'store'\n" + " running(root)\n" + ) + census = _disk_rooted_census(ast.parse(on_sight)) + assert list(census) == ["test_probe :: tmp_path / 'store'"], ( + f"the census reported {census} for a module with exactly one obvious " + "disk-backed store root. Every zero it reports elsewhere is therefore " + "uninterpretable ā€” fix the scanner, not the ledger." + ) + + by_flow = ( + "def test_probe(tmp_path):\n" + " root = tmp_path / 'holder'\n" + " running(root)\n" + ) + assert "holder" not in _ROOT_NAMES, ( + "this probe's whole point is a directory name the ON-SIGHT arm does not " + f"recognise, and _ROOT_NAMES is now {sorted(_ROOT_NAMES)}. Pick another name." + ) + census = _disk_rooted_census(ast.parse(by_flow)) + assert list(census) == ["test_probe :: tmp_path / 'holder'"], ( + f"the census reported {census} for a store root that reaches `running(...)` " + "under a directory name outside _ROOT_NAMES. The FLOW arm is what counts " + "that one, and it is the arm the on-sight probe above cannot see fail." + ) + + +# šŸ”“ THE OTHER SIDE OF THE RELATIONSHIP. The census above pins the sites that are NOT +# sited; this pins the ones that ARE, so the pair fails whichever way the relationship +# is broken. +# +# šŸ”“ THE CASE THAT MAKES THIS A SECOND GUARD RATHER THAN A DUPLICATE, MEASURED BY +# MUTATION RATHER THAN ARGUED ā€” and the first draft of this comment was WRONG about +# it, in the direction that overstates the census. Three mutants, each run with +# `__pycache__` cleared: +# +# 1. a migrated TEST reverted to `tmp_path / "store"` +# -> census RED (names the test), sited-ledger green. 22 others passed. +# 2. the `sited_root` FIXTURE de-sited to `tmp_path / "store"` +# -> BOTH red. The census sees it after all, because `_ROOT_NAMES` counts a +# directory spelled "store" ON SIGHT, with no flow analysis and therefore no +# scope boundary to be blind at. +# 3. the `sited_root` FIXTURE de-sited to `tmp_path / "holder"` +# -> census GREEN, sited-ledger RED. 22 others passed. +# +# So the census's fixture blind spot is real but NARROWER than "a fixture": it needs +# the directory name to be outside `_ROOT_NAMES` as well, which is mutant 3 and is +# exactly what an author writing a holder directory would produce. +# +# šŸ”“ AND "THE ONLY GUARD THAT CAN SEE IT" IS FALSE ā€” RE-MEASURED, IN THE OTHER FILE +# THE FIRST MEASUREMENT NEVER RAN. Mutant 3's "22 others passed" was a one-FILE run of +# this 23-test module, and "every other guard in both files green" was extrapolated +# from it. Re-run over BOTH files on a host with a usable tmpfs, mutant 3 gives +# **2 failed, 781 passed** ā€” this ledger, and `test_subsystem_store_api.py:: +# TestTheStoreIsSitedOffTheContendedDisk::test_the_sited_root_fixture_ACTUALLY_lands_ +# on_tmpfs_when_one_exists`, which reads the fstype of `sited_root.parent` and finds +# the disk. The census stayed green, so mutant 3's OTHER half ā€” the census blind spot +# above ā€” is re-confirmed by the same run. +# +# THIS LEDGER STILL EARNS ITS PLACE, AND THE REASON IS THE SCOPE OF THE OTHER GUARD +# RATHER THAN ITS ABSENCE: the behavioural one SKIPS where no usable tmpfs exists ā€” +# absent, not tmpfs, under `_MIN_FREE_BYTES` free, or unwritable ā€” and the gate may +# well be such a place. This one is structural and holds everywhere. So the true claim +# is "the only guard that can see it ON A MACHINE WITH NO USABLE TMPFS", which is the +# claim the module header already makes for the pair, and it is why the arm stays. +# +# Keyed the same way as the census: ` :: `, never a line +# number, and per-file for the same reason the allowlist is ā€” this read only +# `test_subsystem_store_api.py` while `EXPECTED_SERVER_TESTS` named three, so a +# de-siting in `test_cairn_write.py`, the file whose disk-backed fixture is this +# module's founding incident, was outside it entirely. +_SITED_STORE_ROOT_CALLERS: dict[str, frozenset[str]] = { + "test_subsystem_store_api.py": frozenset( + { + # The three shared fixtures. `sited_root` is the one the 18 previously + # inline `tmp_path / "store"` sites now take their root from. + "store", + "scoped_store", + "sited_root", + # An inline `with` in a test that needs the root before `_build_store`. + "TestPUTCreatesANewEntry.test_a_scopes_FIRST_entry_creates_the_directory", + # The siting module's own fallback tests, which call it directly. + "TestTheSitingRULESThemselvesArePinned.test_mkdtemp_REFUSING_falls_back_" + "instead_of_raising", + "TestTheSitingRULESThemselvesArePinned.test_the_fallback_honours_a_custom_" + "store_NAME", + } + ), + # One sited fixture each, and they are the reason this ledger is not api-only: + # `test_cairn_write.py`'s `store` is the fixture devrc#1211 left disk-backed. + "test_cairn_write.py": frozenset({"store"}), + "test_cairn_cli.py": frozenset({"source_store"}), +} + + +def _store_root_callers(tree: ast.AST) -> set[str]: + """Enclosing class/def of every `store_siting.store_root(...)` CALL.""" + owner = _qualnames(tree) + return { + owner.get(id(node), "") + for node in ast.walk(tree) + if isinstance(node, ast.Call) + and isinstance(node.func, ast.Attribute) + and node.func.attr == "store_root" + and isinstance(node.func.value, ast.Name) + and node.func.value.id == "store_siting" + } + + +def _sited_callers_over_the_ledgered_files() -> set[str]: + """` :: ` for every `store_root(...)` call in the ledgered files.""" + found: set[str] = set() + for name in sorted(EXPECTED_SERVER_TESTS): + tree = ast.parse((TESTS / name).read_text(encoding="utf-8")) + found |= {f"{name} :: {caller}" for caller in _store_root_callers(tree)} + return found + + +def test_the_SITED_store_roots_are_a_pinned_ledger_too(): + """Fails when the sited set GROWS *or* SHRINKS, for the same reason as the census. + + SHRINKING is the regression: a `store_root(...)` call disappearing means a store + went back onto the contended disk, and if it went back inside a pytest fixture the + census above is structurally unable to notice. GROWING is not a fault, but it is a + change to the ledger this file exists to hold, so it is recorded rather than + absorbed ā€” the alternative is a `>=` that quietly stops describing the file. + """ + expected = { + f"{name} :: {caller}" + for name, callers in _SITED_STORE_ROOT_CALLERS.items() + for caller in callers + } + assert set(_SITED_STORE_ROOT_CALLERS) == set(EXPECTED_SERVER_TESTS), ( + f"_SITED_STORE_ROOT_CALLERS covers {sorted(_SITED_STORE_ROOT_CALLERS)} but " + f"the ledger names {sorted(EXPECTED_SERVER_TESTS)}. Every ledgered file needs " + "a key: `test_every_ledgered_file_IMPORTS_AND_CALLS_the_shared_siting_at_" + "least_once` already requires at least one call in each, so an empty frozenset " + "here would contradict it rather than record anything." ) - # šŸ”“ NO SLACK, and no `or actual == 0` escape. The previous version tolerated a - # drop of up to three and passed unconditionally at zero ā€” so the count could - # regrow 0 -> 18 with the constant still reading 18 and the ratchet never biting. - assert actual == _DISK_ROOTED_SITES, ( - f"only {actual} inline sites left, was {_DISK_ROOTED_SITES}. šŸ”“ FIRST check " - "WHICH happened: sites genuinely converted to store_root(), or " - "_is_disk_rooted_store_expr narrowed so it counts fewer? The counter and " - "the constant live in this file and this assertion cannot tell them apart. " - "If sites were fixed, lower _DISK_ROOTED_SITES in the SAME commit. If the " - "predicate narrowed, widen it back ā€” lowering the constant would bank a " - "coverage loss as if it were progress." + found = _sited_callers_over_the_ledgered_files() + missing = sorted(expected - found) + extra = sorted(found - expected) + assert not missing, ( + f"these no longer call store_siting.store_root(): {missing}. A store that " + "stopped being sited is back in the fsync-contention population ā€” and if it " + "was re-rooted inside a @pytest.fixture UNDER A DIRECTORY NAME OUTSIDE " + "_ROOT_NAMES, this is the only guard that can see it ON A MACHINE WITH NO " + "USABLE TMPFS: the census's flow arm stops at the fixture's scope boundary " + f"and its on-sight arm only recognises {sorted(_ROOT_NAMES)}. Where a tmpfs " + "IS usable, TestTheStoreIsSitedOffTheContendedDisk's behavioural controls in " + "test_subsystem_store_api.py fail alongside this ā€” measured, both arms, on " + "the `sited_root`-to-`tmp_path / \"holder\"` mutant. They SKIP where there is " + "no tmpfs, which is why this structural arm stays. If the removal is " + "deliberate, delete the name here in the SAME commit." + ) + assert not extra, ( + f"new store_siting.store_root() callers: {extra}. That is the right " + "direction ā€” add them to _SITED_STORE_ROOT_CALLERS so this ledger keeps " + "describing the file." ) @@ -731,6 +1309,96 @@ def test_a_path_that_is_never_used_as_a_store_still_does_NOT_count(): ) +def test_the_operand_NODE_TYPE_is_not_what_decides_either(): + """šŸ”“ THE WIDENING THAT DELETED THE `isinstance` GATES HAD NO TEST AT ALL. + + Both arms of `_is_disk_rooted_store_expr` used to require the operand to be an + `ast.Name` or an `ast.Constant` before the flow gate was ever consulted, so + `tmp_path / f"store-{k}"` and `tmp_path.joinpath("store" + k)` were uncounted no + matter where they flowed. The gates were deleted and the widening was measured BY + HAND in a commit message ā€” in the module whose entire premise is that a hand + measurement is not coverage. MEASURED at the moment this test was written: + restoring BOTH gates verbatim left this file at **23 passed**, because every + existing probe uses an `ast.Constant` operand. Nothing in the repo went red. + + āš  READ WHAT THIS PINS AND WHAT IT DOES NOT. It pins the OPERAND'S NODE TYPE, in + both arms, in both directions: four non-Constant, non-Name spellings that flow + into a consumer must be COUNTED, and the same spellings that flow nowhere must + stay 0. It says nothing about the left operand ā€” that must still be the NAME + `tmp_path` ā€” and nothing about any of the residual holes the census docstring + enumerates. The negative half is not decoration: widening the operand type + without the flow gate doing the discriminating re-creates round 3's false + accusation, `(tmp_path / name).write_text(body)` for a scratch file, in the + spelling an f-string produces. + + Watched red, both halves, `__pycache__` cleared between mutants: + + * restore both `isinstance` gates -> the four COUNTED probes report 0 and this + test fails on its own message; the four UNCOUNTED probes still pass. + * make the flow gate accept everything (`_used_as_a_store_root` -> `True`) -> + the four UNCOUNTED probes report 1 and this test fails on its own message; + the four COUNTED probes still pass. + """ + # Each spelling appears TWICE: once flowing into `_build_store`, once flowing + # nowhere. The pair is what separates "the type gate is gone" from "the predicate + # now says yes to everything", and only the pair can. + counted = { + "f-string, `/` arm": ( + "def test_probe(tmp_path, k, s):\n" + " _build_store(tmp_path / f'store-{k}', s)\n" + ), + "concatenation, `/` arm": ( + "def test_probe(tmp_path, k, s):\n" + " _build_store(tmp_path / ('store-' + k), s)\n" + ), + "call, `/` arm": ( + "def test_probe(tmp_path, k, s):\n" + " _build_store(tmp_path / k.lower(), s)\n" + ), + "subscript, joinpath arm": ( + "def test_probe(tmp_path, names, s):\n" + " _build_store(tmp_path.joinpath(names[0]), s)\n" + ), + } + uncounted = { + "f-string, `/` arm": ( + "def test_probe(tmp_path, i, b):\n" + " (tmp_path / f'scratch-{i}').write_text(b)\n" + ), + "concatenation, `/` arm": ( + "def test_probe(tmp_path, i, b):\n" + " (tmp_path / ('scratch-' + i)).write_text(b)\n" + ), + "call, `/` arm": ( + "def test_probe(tmp_path, i, b):\n" + " (tmp_path / i.lower()).write_text(b)\n" + ), + "subscript, joinpath arm": ( + "def test_probe(tmp_path, names, b):\n" + " tmp_path.joinpath(names[0]).write_text(b)\n" + ), + } + # Named individually rather than totalled: a single number would let three of the + # four regress unnoticed behind one that still works. + blind = {k: _count_disk_rooted(v) for k, v in counted.items()} + blind = {k: n for k, n in blind.items() if n != 1} + assert not blind, ( + f"these store roots flow straight into `_build_store` and are counted " + f"{blind} instead of 1. The operand's NODE TYPE is deciding again ā€” an " + "f-string, a concatenation, a call or a subscript is an ordinary spelling of " + "a store directory, and a write-path store spelled that way is exactly the " + "fsync-contention population this file exists to shrink." + ) + false_alarms = {k: _count_disk_rooted(v) for k, v in uncounted.items()} + false_alarms = {k: n for k, n in false_alarms.items() if n != 0} + assert not false_alarms, ( + f"these scratch files flow into no store consumer at all and are counted " + f"{false_alarms} instead of 0. Flowing into a consumer is the whole " + "discriminator; without it the widened operand type re-creates round 3's " + "false accusation, demanding store_root() for a file that is not a store." + ) + + def test_a_store_root_passed_BY_KEYWORD_counts_like_a_positional_one(): """šŸ”“ `node.keywords` APPEARED NOWHERE IN THIS FILE. @@ -857,16 +1525,23 @@ def test_a_store_root_bound_in_a_pytest_FIXTURE_is_NOT_counted(): inside a `@pytest.fixture` and served in a test that requests it. That is a hole, it is recorded here so it cannot be rediscovered as news, and it is pinned so that CLOSING it fails this test and forces whoever closes it to delete this guard and - move `_DISK_ROOTED_SITES` in the same commit. + add the newly-visible sites to `_DISK_ROOTED_ALLOWLIST` in the same commit. + + šŸ”“ THIS HOLE IS WHY THE CENSUS ALONE IS NOT THE WHOLE GUARD, and it is why + `_SITED_STORE_ROOT_CALLERS` exists beside it: a store re-rooted onto disk INSIDE a + fixture is invisible to `test_the_disk_rooted_census_matches_the_allowlist_EXACTLY` + and visible to the sited-ledger, because the `store_root(...)` call it deleted is + named there. Fixture-binding is this file's dominant idiom, so the hole is not exotic. TWO LIVE INSTANCES on `test_subsystem_store_api.py`, both real `cp -a` copies of a store, - written into, and served in-process by the real store server: + written into, and served in-process by the real store server ā€” cited BY NAME, + because a line citation into that file has shipped wrong repeatedly: - * `served = tmp_path / "ordered-served"` in the `shuffled_pair` fixture (:3945), - served at :4392 and four more `running(served)` sites; - * `served = tmp_path / "ambig-served"` in the `ambiguous_pair` fixture (:4294), - served at :4758 and one more. + * `served = tmp_path / "ordered-served"` in the `shuffled_pair` fixture, served + by five `running(served)` sites; + * `served = tmp_path / "ambig-served"` in the `ambiguous_pair` fixture, served + by two. šŸ”“ WHY IT IS NOT CLOSED, stated rather than implied. The flow arm is scoped per function (`_walk_scope` stops at a function boundary), and a pytest fixture crosses @@ -879,8 +1554,18 @@ def test_a_store_root_bound_in_a_pytest_FIXTURE_is_NOT_counted(): false accusations, and round 3 rejected it for exactly that. So the honest statement, which replaces the one this PR shipped: the predicate does - NOT cover the flow case generally. It covers the flow case WITHIN ONE FUNCTION - SCOPE. + NOT cover the flow case generally. It covers the flow case within one function + scope, THROUGH A BINDING FORM `_assignments` RESOLVES, AND THROUGH AN EXPRESSION + `_path_base` CAN NAME A BASE FOR. The second and third halves were both missing + from this sentence, and the third is what a `for` target and a comprehension target + do NOT need: they fail on `_assignments` alone. A dict or list element passes + `_assignments` and fails on `_path_base`, which returns None for an + `ast.Subscript`. A closure passes the last two and fails the FIRST ā€” "one function + scope" is `_walk_scope`'s scope, which stops at the nested `def` a reader counts as + part of the same test. It is the scope arm, not a binding case at all. + `test_the_disk_rooted_census_matches_the_allowlist_EXACTLY`'s fourth residual + bullet enumerates them with the numbers. Nothing here pins that set; it is written + down, not guarded. """ inline = ( "def test_probe(tmp_path):\n" @@ -921,9 +1606,9 @@ def test_a_store_root_bound_in_a_pytest_FIXTURE_is_NOT_counted(): ) assert _count_disk_rooted(via_fixture) == 0, ( "the ratchet now SEES a store root bound in a fixture. That is good news and " - "it makes this guard wrong: delete it, re-measure _DISK_ROOTED_SITES (the two " - "live instances at test_subsystem_store_api.py:3945 and :4294 will start " - "counting), and rewrite the residual in " + "it makes this guard wrong: delete it, re-run the census (the `shuffled_pair` " + "and `ambiguous_pair` fixtures in test_subsystem_store_api.py will start " + "appearing in it) and record or migrate them, and rewrite the residual in " "test_one_tests_store_root_does_not_vouch_for_ANOTHERS_scratch_directory." ) assert _count_disk_rooted(via_fixture_tuple) == 0, ( @@ -983,9 +1668,9 @@ def test_a_store_root_bound_in_a_pytest_FIXTURE_is_NOT_counted(): # whole suite, and `test_every_ledgered_file_IMPORTS_AND_CALLS_the_shared_siting_at_ # least_once` above is what keeps the ledgered files routed through the seam, so the # two together cover the population. A deselected subset run is not that claim. Nor is -# it a claim about the 33 inline sites the FIRST ratchet counts (20 of them spelled -# `tmp_path / "store"`, 13 not): those never reach `store_root`, live on disk rather -# than tmpfs, and are that ratchet's business, not this budget's. +# it a claim about the 15 inline sites `_DISK_ROOTED_ALLOWLIST` enumerates: those never +# reach `store_root`, live on disk rather than tmpfs, and are the census's business, +# not this budget's. # How much bigger than the measured peak the budget must be. šŸ”“ THE PREVIOUS BUDGET HAD # ZERO SLACK ā€” 1,875,968 was exactly (442 + 16) * 4096 where 442 was the sweep's own diff --git a/scripts/tests/test_subsystem_store_api.py b/scripts/tests/test_subsystem_store_api.py index 9afe9969b..f68f20f31 100644 --- a/scripts/tests/test_subsystem_store_api.py +++ b/scripts/tests/test_subsystem_store_api.py @@ -357,6 +357,45 @@ def store(tmp_path: Path) -> Iterator[Path]: yield root +@pytest.fixture +def sited_root(tmp_path: Path) -> Iterator[Path]: + """An EMPTY store root, sited off the contended disk. It does not exist yet. + + šŸ”“ THE SAME SITING AS `store` AND `scoped_store` ABOVE, FOR THE LONG TAIL + NEITHER OF THEM COVERS. devrc#1211 sited `store`; a later round sited + `scoped_store`; both times the tests that build their OWN root inline were + left on disk ā€” 18 of them, spelled `tmp_path / "store"`, and one of those + (`TestARefusedWriteIsIndistinguishableFromAnAbsentOne`) is the class whose + positive control kept failing the gate on docs-only PRs. Fixing a predicate + at one call site of N is the error this whole module keeps re-learning, so + the remaining sites take their root from HERE rather than from a fourth copy + of the same idea. + + šŸ”“ A FIXTURE RATHER THAN AN INLINE `with`, AND THE REASON IS LIFETIME. + `store_siting.store_root` is a context manager because a tmpfs holder is not + pytest's to clean and tmpfs is RAM. Several of the migrated sites build the + root inside a HELPER whose return value ā€” a path, or a pair of + `present()`/`absent()` closures ā€” is used throughout the test body; a `with` + wrapped around the helper CALL would tear the store down before a single + assertion ran, and a `with` wrapped around each whole test body is 30 blocks + of re-indentation that a scripted conversion has already been reverted for + once. Taking the context manager as a fixture makes its lifetime the TEST's, + per site, with nothing to get wrong per site. + + āš  ONE BEHAVIOUR DIFFERENCE FROM AN INLINE `with`, STATED RATHER THAN GLOSSED. + `store_root` skips its budget check when the body is already raising, so that + a budget violation cannot replace the error an author needs to read. pytest + does not throw a test's failure into a fixture generator, so on THIS path the + check runs even for a failing test. That is already true of `store` and + `scoped_store`, so it is the standing cost of the fixture idiom here rather + than something introduced by the migration ā€” but it is a real difference and + a `StoreBudgetExceeded` reported alongside a failing test may not be the + thing that failed. + """ + with store_siting.store_root(tmp_path) as root: + yield root + + # RFC 5737 TEST-NET-3, and three DISTINCT addresses: a test that used one # address for the client and the same one for the spoofed header could not tell # "keyed on CF-Connecting-IP" from "keyed on anything at all". @@ -12480,10 +12519,15 @@ class TestRefusedIsIndistinguishableFromAbsent: test. See the module docstring's residual-leak note. """ - def _phases(self, tmp_path: Path): + def _phases(self, root: Path): """Yields a builder for phase A (denied scope present) and phase B (it - never existed), both at ONE path so ` store: ` cannot differ.""" - root = tmp_path / "store" + never existed), both at ONE path so ` store: ` cannot differ. + + šŸ”“ THE ROOT IS PASSED IN ALREADY SITED (`sited_root`), never built from + `tmp_path` here. The closures below outlive this call, so a `with` around + it would tear the store down before the caller used either of them ā€” + which is why the siting is a fixture and not a block in this function. + """ def present(): if root.exists(): @@ -12515,9 +12559,9 @@ def _ask(self, root: Path, token, path: str): return code, _comparable(headers), body def test_RECALL_a_refused_scope_is_BYTE_IDENTICAL_to_one_that_never_existed( - self, tmp_path: Path + self, sited_root: Path ): - root, present, absent = self._phases(tmp_path) + root, present, absent = self._phases(sited_root) present() refused = self._ask(root, ZACH, f"/api/v1/recall/{DENY_SCOPE}") absent() @@ -12542,9 +12586,9 @@ def test_RECALL_a_refused_scope_is_BYTE_IDENTICAL_to_one_that_never_existed( SEARCH_QUERY = f"/api/v1/search/{DENY_SCOPE}?q=drill+head+overheats" def test_SEARCH_a_refused_scope_is_BYTE_IDENTICAL_to_one_that_never_existed( - self, tmp_path: Path + self, sited_root: Path ): - root, present, absent = self._phases(tmp_path) + root, present, absent = self._phases(sited_root) present() refused = self._ask(root, ZACH, self.SEARCH_QUERY) absent() @@ -12556,7 +12600,7 @@ def test_SEARCH_a_refused_scope_is_BYTE_IDENTICAL_to_one_that_never_existed( assert dict(refused[1])["x-store-status"] == "scope-absent" def test_POSITIVE_CONTROL_the_RECALL_comparison_CAN_see_the_difference( - self, tmp_path: Path + self, sited_root: Path ): """šŸ”“ WITHOUT THIS, THE RECALL TEST ABOVE IS SATISFIED BY A SERVER THAT ANSWERS THE SAME BYTES TO EVERYTHING. @@ -12566,7 +12610,7 @@ def test_POSITIVE_CONTROL_the_RECALL_comparison_CAN_see_the_difference( pair did not differ, the equality above would be measuring the harness rather than the fix. """ - root, present, absent = self._phases(tmp_path) + root, present, absent = self._phases(sited_root) present() seen = self._ask(root, GOOD_TOKEN, f"/api/v1/recall/{DENY_SCOPE}") absent() @@ -12578,7 +12622,7 @@ def test_POSITIVE_CONTROL_the_RECALL_comparison_CAN_see_the_difference( assert QUARTZ_NUANCE.encode() in seen[2] def test_POSITIVE_CONTROL_the_SEARCH_comparison_CAN_see_the_difference( - self, tmp_path: Path + self, sited_root: Path ): """šŸ”“ THE SEARCH PATH HAD NO POSITIVE CONTROL AT ALL, and the recall one does not cover it: they are different routes, different renderers and @@ -12592,7 +12636,7 @@ def test_POSITIVE_CONTROL_the_SEARCH_comparison_CAN_see_the_difference( an UNRESTRICTED token: present -> `search-hit` carrying the matched nuance, absent -> `scope-absent`. Both non-empty, and different. """ - root, present, absent = self._phases(tmp_path) + root, present, absent = self._phases(sited_root) present() found = self._ask(root, GOOD_TOKEN, self.SEARCH_QUERY) absent() @@ -12931,14 +12975,17 @@ class TestTheLoaderItselfTakesTheAllowlist: Neither half alone pins the loader. """ - def _store(self, tmp_path: Path, *scope_dirs: str) -> Path: + def _store(self, root: Path, *scope_dirs: str) -> Path: """A store whose scope DIRECTORY NAMES are exactly as given. Spelled by hand rather than through `_build_store`, because one test below needs a directory whose name does NOT equal its own folded form and that fixture is the whole point of it. + + šŸ”“ `root` ARRIVES SITED (`sited_root`) and does not exist yet ā€” the + `mkdir(parents=True)` below is unchanged, which is exactly the drop-in + contract `store_siting.store_root` documents. """ - root = tmp_path / "store" for name in scope_dirs: (root / name).mkdir(parents=True) (root / name / f"{name}-entry.md").write_text( @@ -12946,16 +12993,16 @@ def _store(self, tmp_path: Path, *scope_dirs: str) -> Path: ) return root - def test_POSITIVE_CONTROL_no_allowlist_loads_every_scope(self, tmp_path: Path): + def test_POSITIVE_CONTROL_no_allowlist_loads_every_scope(self, sited_root: Path): """Without this the three tests below are satisfied by a loader that returns an empty index for everything. """ - root = self._store(tmp_path, ALLOW_SCOPE, DENY_SCOPE) + root = self._store(sited_root, ALLOW_SCOPE, DENY_SCOPE) index = api.rc.load_index(root, on_malformed="collect") assert set(index.scopes) == {ALLOW_SCOPE, DENY_SCOPE} def test_an_EMPTY_allowlist_registers_NO_scope_not_EVERY_scope( - self, tmp_path: Path + self, sited_root: Path ): """šŸ”“ THE ASYMMETRY, PINNED AT THE LOADER TOO. `None` and `()` are both falsy, so a filter written `if allowed and ā€¦` treats "you may see @@ -12966,12 +13013,12 @@ def test_an_EMPTY_allowlist_registers_NO_scope_not_EVERY_scope( entirely when measured through that door. Here there is nothing in the way. """ - root = self._store(tmp_path, ALLOW_SCOPE, DENY_SCOPE) + root = self._store(sited_root, ALLOW_SCOPE, DENY_SCOPE) index = api.rc.load_index(root, on_malformed="collect", visible_scopes=()) assert index.scopes == () assert len(index) == 0 - def test_a_denied_scopes_NAME_is_not_registered_either(self, tmp_path: Path): + def test_a_denied_scopes_NAME_is_not_registered_either(self, sited_root: Path): """šŸ”“ SKIPPING THE READ IS NOT SKIPPING THE SCOPE. A filter placed one line too late still appends the directory name to `extra_scopes`, so the denied scope arrives on `index.scopes` ā€” the `known_scopes` enumeration @@ -12980,14 +13027,14 @@ def test_a_denied_scopes_NAME_is_not_registered_either(self, tmp_path: Path): Invisible through `load_store`, which drops the key again on the way out. """ - root = self._store(tmp_path, ALLOW_SCOPE, DENY_SCOPE) + root = self._store(sited_root, ALLOW_SCOPE, DENY_SCOPE) index = api.rc.load_index( root, on_malformed="collect", visible_scopes=(ALLOW_SCOPE,) ) assert index.scopes == (ALLOW_SCOPE,) def test_the_DIRECTORY_NAME_is_FOLDED_before_it_is_compared( - self, tmp_path: Path + self, sited_root: Path ): """šŸ”“ OVER-FILTERING, AND THE FIXTURE HAS TO REACH IT. Every other store in this file has directory names that are already their own folded form, @@ -13002,7 +13049,7 @@ def test_the_DIRECTORY_NAME_is_FOLDED_before_it_is_compared( "the fixture directory must NOT already equal its folded form, or " "this test measures nothing" ) - root = self._store(tmp_path, raw_dir, DENY_SCOPE) + root = self._store(sited_root, raw_dir, DENY_SCOPE) index = api.rc.load_index( root, on_malformed="collect", visible_scopes=(ALLOW_SCOPE,) ) @@ -13047,9 +13094,10 @@ class TestUnreadableEntriesInDeniedScopes: describe it by the two `RESIDUAL LEDGER` guards beside it. """ - def _store(self, tmp_path: Path, kind: str) -> Path: + def _store(self, root: Path, kind: str) -> Path: + # `root` arrives sited (`sited_root`); `_build_store` creates it. store = _build_store( - tmp_path / "store", + root, {ALLOW_SCOPE: KELP_NUANCE, DENY_SCOPE: QUARTZ_NUANCE}, ) _make_unreadable(store, DENY_SCOPE, kind) @@ -13070,9 +13118,9 @@ def test_the_FIXTURE_really_is_a_candidate_entry_glob_matches_a_dotfile( @pytest.mark.parametrize("kind", ["perm", "emacs"]) def test_a_SCOPED_caller_is_unaffected_by_an_unreadable_DENIED_entry( - self, tmp_path: Path, kind: str + self, sited_root: Path, kind: str ): - store = self._store(tmp_path, kind) + store = self._store(sited_root, kind) _s, index = api.rc.load_store( store, verb="recalled", visible_scopes=(ALLOW_SCOPE,) ) @@ -13080,7 +13128,7 @@ def test_a_SCOPED_caller_is_unaffected_by_an_unreadable_DENIED_entry( assert index.malformed == () def test_POSITIVE_CONTROL_the_same_file_in_the_CALLERS_OWN_scope_still_RAISES( - self, tmp_path: Path + self, sited_root: Path ): """šŸ”“ WITHOUT THIS THE TEST ABOVE IS SATISFIED BY A FIXTURE THAT CREATED NOTHING UNREADABLE. It also pins the half that must NOT change: the @@ -13093,18 +13141,18 @@ def test_POSITIVE_CONTROL_the_same_file_in_the_CALLERS_OWN_scope_still_RAISES( caller. Its own-scope behaviour is asserted below, as a collected malformed row. """ - store = self._store(tmp_path, "perm") + store = self._store(sited_root, "perm") with pytest.raises(api.rc.EntryUnreadableError): api.rc.load_store(store, verb="recalled", visible_scopes=(DENY_SCOPE,)) def test_the_BROKEN_LINK_in_the_CALLERS_OWN_scope_is_REPORTED_not_fatal( - self, tmp_path: Path + self, sited_root: Path ): """The other side of the guard: refusing an entry must not silently empty the scope that holds it. The caller who OWNS the hostile file still gets their good entry, and still gets told about the bad one. """ - store = self._store(tmp_path, "emacs") + store = self._store(sited_root, "emacs") _s, index = api.rc.load_store( store, verb="recalled", visible_scopes=(DENY_SCOPE,) ) @@ -13113,7 +13161,7 @@ def test_the_BROKEN_LINK_in_the_CALLERS_OWN_scope_is_REPORTED_not_fatal( assert [m.label for m in index.malformed] == [f"{DENY_SCOPE}/{EMACS_LOCK}"] def test_the_UNRESTRICTED_reading_of_a_BROKEN_LINK_no_longer_DIES( - self, tmp_path: Path + self, sited_root: Path ): """šŸ”“ THE INVERSION. This test used to be `test_the_UNRESTRICTED_reading_is_UNCHANGED_and_that_is_the_residual` @@ -13128,7 +13176,7 @@ def test_the_UNRESTRICTED_reading_of_a_BROKEN_LINK_no_longer_DIES( before `open()`, so the Emacs lock file costs its own entry and nothing else. """ - store = self._store(tmp_path, "emacs") + store = self._store(sited_root, "emacs") _s, index = api.rc.load_store(store, verb="recalled") # The OTHER scope's content survived, which is the DoS half. assert set(index.scopes) == {ALLOW_SCOPE, DENY_SCOPE} @@ -13140,7 +13188,7 @@ def test_the_UNRESTRICTED_reading_of_a_BROKEN_LINK_no_longer_DIES( assert "broken symlink" in index.malformed[0].reason def test_an_UNREADABLE_REGULAR_FILE_still_RAISES_and_THAT_is_the_residual( - self, tmp_path: Path + self, sited_root: Path ): """šŸ”“ THE HALF THAT IS **NOT** CLOSED, kept as its own named test rather than left implied by the parametrize list this used to share. @@ -13157,12 +13205,12 @@ def test_an_UNREADABLE_REGULAR_FILE_still_RAISES_and_THAT_is_the_residual( were quietly widened to refuse everything it could not read, this would go green-by-collapse and the four-state rule would be gone. """ - store = self._store(tmp_path, "perm") + store = self._store(sited_root, "perm") with pytest.raises(api.rc.EntryUnreadableError): api.rc.load_store(store, verb="recalled") def test_the_503_body_NAMED_the_denied_scope_and_its_PATH_over_HTTP( - self, tmp_path: Path + self, sited_root: Path ): """šŸ”“ THE DISCLOSURE ITSELF, DRIVEN THROUGH THE SERVER ā€” the layer where it was a leak rather than an exception type. @@ -13173,7 +13221,7 @@ def test_the_503_body_NAMED_the_denied_scope_and_its_PATH_over_HTTP( matter: the status is a denial of service he did not cause, and the body names a scope and a filename he is not allowed to know exist. """ - store = self._store(tmp_path, "perm") + store = self._store(sited_root, "perm") with running(store, tokens=(ZACH,)) as (base, _): code, headers, body = fetch( f"{base}/api/v1/recall/{ALLOW_SCOPE}", token=ZACH_TOKEN @@ -13198,7 +13246,7 @@ def test_the_503_body_NAMED_the_denied_scope_and_its_PATH_over_HTTP( assert str(store / DENY_SCOPE / LOCKED_ENTRY) not in text def test_POSITIVE_CONTROL_a_LEGACY_token_DOES_still_get_the_503( - self, tmp_path: Path + self, sited_root: Path ): """šŸ”“ THE FIXTURE MUST ACTUALLY PRODUCE AN UNREADABLE ENTRY. Without this the assertions above are a zero from a check that might see nothing ā€” a @@ -13208,7 +13256,7 @@ def test_POSITIVE_CONTROL_a_LEGACY_token_DOES_still_get_the_503( It is also the honest record of the residual: unrestricted callers still get the 503, and still get the path in it. """ - store = self._store(tmp_path, "perm") + store = self._store(sited_root, "perm") with running(store, tokens=(GOOD_TOKEN,)) as (base, _): code, headers, body = fetch( f"{base}/api/v1/recall/{ALLOW_SCOPE}", token=GOOD_TOKEN @@ -13219,7 +13267,7 @@ def test_POSITIVE_CONTROL_a_LEGACY_token_DOES_still_get_the_503( assert LOCKED_ENTRY in text and DENY_SCOPE in text def test_a_FIFO_named_md_in_a_DENIED_scope_no_longer_HANGS_the_reader( - self, tmp_path: Path + self, sited_root: Path ): """šŸ”“ THE MOST SERIOUS OF THE THREE, AND THE ONE A NORMAL TEST CANNOT ASSERT: a wedged thread produces no exception and no value, so there is @@ -13247,7 +13295,7 @@ def test_a_FIFO_named_md_in_a_DENIED_scope_no_longer_HANGS_the_reader( `open()` of the fifo itself, which is the syscall in question rather than a proxy for it. """ - store = self._store(tmp_path, "fifo") + store = self._store(sited_root, "fifo") probe = _load_store_probe( store, expr=f"None if sys.argv[1] == 'unrestricted' else ({ALLOW_SCOPE!r},)" ) @@ -13277,7 +13325,7 @@ def run(arg: str, deadline: float): assert f"MALFORMED={DENY_SCOPE}/{LOCKED_ENTRY}\n" in unrestricted.stdout def test_a_SYMLINK_to_a_FIFO_no_longer_HANGS_and_the_DEADLINE_still_SEES_one( - self, tmp_path: Path + self, sited_root: Path, tmp_path: Path ): """šŸ”“ THE SECOND INVERSION, AND THE PROBE'S OWN POSITIVE CONTROL, in one test ā€” because the two have to move together. @@ -13310,7 +13358,7 @@ def test_a_SYMLINK_to_a_FIFO_no_longer_HANGS_and_the_DEADLINE_still_SEES_one( it. """ store = _build_store( - tmp_path / "store", + sited_root, {ALLOW_SCOPE: KELP_NUANCE, DENY_SCOPE: QUARTZ_NUANCE}, ) real_fifo = tmp_path / "a-real-fifo" @@ -13393,7 +13441,7 @@ class TestTheLoaderRefusesHostileEntriesByKind: table being wrong, and no test read the documents. """ - def _hostile(self, tmp_path: Path) -> Path: + def _hostile(self, root: Path) -> Path: """One store, BOTH refused shapes, in a scope that is not the one asked for ā€” the arrangement the operator reproduced: unrestricted token, a dangling `.#lock.md` in `bravo`, and a recall for `alpha`. @@ -13406,9 +13454,11 @@ def _hostile(self, tmp_path: Path) -> Path: failing it, and with no `pytest-timeout` plugin loaded nothing would have cut it off. Every read of this store now goes through `under_deadline`, which turns that hang back into a red. + + šŸ”“ `root` ARRIVES SITED (`sited_root`); `_build_store` creates it. """ store = _build_store( - tmp_path / "store", + root, {ALLOW_SCOPE: KELP_NUANCE, DENY_SCOPE: QUARTZ_NUANCE}, ) _make_unreadable(store, DENY_SCOPE, "emacs") @@ -13416,7 +13466,7 @@ def _hostile(self, tmp_path: Path) -> Path: return store def test_an_UNRESTRICTED_recall_of_ANOTHER_scope_is_200_not_503( - self, tmp_path: Path + self, sited_root: Path, tmp_path: Path ): """šŸ”“ THE MEASURED SYMPTOM, DRIVEN THROUGH THE SERVER ON THE LIVE CREDENTIAL SHAPE. Before the guard this exact request answered `503 @@ -13435,7 +13485,7 @@ def test_an_UNRESTRICTED_recall_of_ANOTHER_scope_is_200_not_503( a socket timeout ā€” which this test converts into a NAMED failure below, because "the worker never came back" is the whole claim. """ - store = self._hostile(tmp_path) + store = self._hostile(sited_root) token_file = tmp_path / "token" token_file.write_text(GOOD_TOKEN + "\n") with running_subprocess(store, token_file) as (base, _proc): @@ -13455,7 +13505,7 @@ def test_an_UNRESTRICTED_recall_of_ANOTHER_scope_is_200_not_503( assert KELP_NUANCE in text, "the caller's own content vanished" def test_the_REFUSED_entries_are_SURFACED_not_silently_dropped( - self, tmp_path: Path + self, sited_root: Path ): """šŸ”“ A SKIP RENDERS AS "NOTHING RECORDED", which is the conflation this whole store exists to avoid ā€” so the 200 above is only correct if the @@ -13471,7 +13521,7 @@ def test_the_REFUSED_entries_are_SURFACED_not_silently_dropped( store holding a real fifo, in-process, and `read_text` never returned. The deadline is what converts that back into a `None`, i.e. a red. """ - store = self._hostile(tmp_path) + store = self._hostile(sited_root) done = under_deadline(_load_store_probe(store), 30.0) assert done is not None, ( "an UNRESTRICTED `load_store` of the hostile store HUNG ā€” a " @@ -13586,7 +13636,7 @@ def test_the_REFUSED_DIRECTORY_is_a_NAMED_row_not_a_silent_skip( assert len(index.entries(DENY_SCOPE)) == 1 def test_a_SYMLINKED_entry_is_STILL_READ_the_guard_is_NOT_the_broad_one( - self, tmp_path: Path + self, sited_root: Path, tmp_path: Path ): """šŸ”“ THE UPPER BOUND ON THE GUARD, AND THE MUTANT IT EXISTS TO KILL. @@ -13600,7 +13650,7 @@ def test_a_SYMLINKED_entry_is_STILL_READ_the_guard_is_NOT_the_broad_one( The entry's CONTENT is asserted, not merely its presence: a guard that refused it would still leave the scope registered. """ - store = _build_store(tmp_path / "store", {ALLOW_SCOPE: KELP_NUANCE}) + store = _build_store(sited_root, {ALLOW_SCOPE: KELP_NUANCE}) real = tmp_path / "outside" / "linked-entry.md" real.parent.mkdir() real.write_text( @@ -13623,7 +13673,7 @@ def test_a_SYMLINKED_entry_is_STILL_READ_the_guard_is_NOT_the_broad_one( ) def test_under_RAISE_a_refused_entry_RAISES_the_same_class_as_any_other( - self, tmp_path: Path + self, sited_root: Path ): """šŸ”“ THE POLICY IS `on_malformed`'s, NOT THE GUARD'S. The WRITER's probe loads with `RAISE` precisely because it must not modify a store it read @@ -13636,7 +13686,7 @@ def test_under_RAISE_a_refused_entry_RAISES_the_same_class_as_any_other( fifo, therefore it cannot hang" is a property of two filenames, not of the code. That is not a guarantee worth resting the suite's liveness on. """ - store = self._hostile(tmp_path) + store = self._hostile(sited_root) done = under_deadline(_load_index_raise_probe(store), 30.0) assert done is not None, ( "`load_index` under RAISE HUNG on the hostile store ā€” a REFUSE cell " @@ -13648,7 +13698,7 @@ def test_under_RAISE_a_refused_entry_RAISES_the_same_class_as_any_other( assert _probe_field(done.stdout, "SOURCE") in (EMACS_LOCK, LOCKED_ENTRY) def test_a_BOGUS_policy_is_still_a_ValueError_not_a_refusal( - self, tmp_path: Path + self, sited_root: Path ): """The guard branches on `on_malformed` BEFORE `build_index` validates it, so the predicate is shared (`_check_on_malformed`) rather than @@ -13663,11 +13713,11 @@ def test_a_BOGUS_policy_is_still_a_ValueError_not_a_refusal( alone opened. If that ordering ever changes, this test becomes a hang and must move to `under_deadline` with the others. """ - store = self._hostile(tmp_path) + store = self._hostile(sited_root) with pytest.raises(ValueError, match="on_malformed must be one of"): api.rc.load_index(store, on_malformed="collct") - def test_the_REFUSED_row_is_filed_under_the_FOLDED_scope(self, tmp_path: Path): + def test_the_REFUSED_row_is_filed_under_the_FOLDED_scope(self, sited_root: Path): """šŸ”“ THE SCOPE ON A `MalformedEntry` IS THE NORMALIZED ONE ā€” that is `MalformedEntry`'s own contract, and `malformed_in` compares against `normalize_ref(scope)`. A refusal filed under the RAW directory name @@ -13681,7 +13731,7 @@ def test_the_REFUSED_row_is_filed_under_the_FOLDED_scope(self, tmp_path: Path): """ raw_dir = "Kelp_Forest" assert api.rc.normalize_ref(raw_dir) == ALLOW_SCOPE != raw_dir - store = _build_store(tmp_path / "store", {raw_dir: KELP_NUANCE}) + store = _build_store(sited_root, {raw_dir: KELP_NUANCE}) _make_unreadable(store, raw_dir, "emacs") _s, index = api.rc.load_store(store, verb="recalled") @@ -13690,13 +13740,13 @@ def test_the_REFUSED_row_is_filed_under_the_FOLDED_scope(self, tmp_path: Path): ] assert index.malformed_outside([ALLOW_SCOPE]) == () - def test_a_CLEAN_store_is_UNCHANGED_by_the_guard(self, tmp_path: Path): + def test_a_CLEAN_store_is_UNCHANGED_by_the_guard(self, sited_root: Path): """The positive control. Every assertion above is about a hostile store; without this, a loader that refused EVERY candidate would satisfy the `.malformed` ones and only fail on content nobody asserted. """ store = _build_store( - tmp_path / "store", + sited_root, {ALLOW_SCOPE: KELP_NUANCE, DENY_SCOPE: QUARTZ_NUANCE}, ) _s, index = api.rc.load_store(store, verb="recalled") @@ -14271,8 +14321,23 @@ class TestARefusedWriteIsIndistinguishableFromAnAbsentOne: criteria 1-3 closed. """ - def _phases(self, tmp_path: Path): - root = tmp_path / "store" + def _phases(self, root: Path): + """Phase A (denied scope present) and phase B (it never existed), both + at ONE path so the store path cannot itself be the thing that differs. + + šŸ”“ THE ROOT IS PASSED IN ALREADY SITED (`sited_root`), and THIS IS THE + SITE THAT KEPT FAILING THE GATE. `test_POSITIVE_CONTROL_the_APPEND_ + comparison_CAN_see_the_difference` below is the only test in this class + that gets a `200 appended`; its siblings all assert a 404, which is + answered before any write. So it is the only one that reaches + `server.py:_replace_bytes` and executes the two in-request fsyncs, which + is why it ā€” specifically ā€” is the one that recurred on docs-only PRs. + + šŸ”“ THE CLOSURES OUTLIVE THIS CALL, which is why the siting is a fixture + and not a `with` block here: a `with store_siting.store_root(...)` around + the body of this helper would tear the store down at `return`, before + `present()` or `absent()` was ever invoked. + """ def present(): if root.exists(): @@ -14297,9 +14362,9 @@ def _post(self, root: Path, record, scope: str): return code, _comparable(headers), body def test_APPEND_to_a_refused_scope_is_BYTE_IDENTICAL_to_one_that_never_existed( - self, tmp_path: Path + self, sited_root: Path ): - root, present, absent = self._phases(tmp_path) + root, present, absent = self._phases(sited_root) present() refused = self._post(root, ZACH, DENY_SCOPE) absent() @@ -14316,7 +14381,7 @@ def test_APPEND_to_a_refused_scope_is_BYTE_IDENTICAL_to_one_that_never_existed( assert refused[2], "both bodies are empty ā€” the equality would be vacuous" def test_POSITIVE_CONTROL_the_APPEND_comparison_CAN_see_the_difference( - self, tmp_path: Path + self, sited_root: Path ): """šŸ”“ WITHOUT THIS, THE EQUALITY ABOVE IS SATISFIED BY A SERVER THAT ANSWERS 404 TO EVERYTHING ā€” and by a fail-closed one that answers two @@ -14324,7 +14389,7 @@ def test_POSITIVE_CONTROL_the_APPEND_comparison_CAN_see_the_difference( scope: present -> `appended`, absent -> `not-found`. Both non-empty, and different. """ - root, present, absent = self._phases(tmp_path) + root, present, absent = self._phases(sited_root) present() wrote = self._post(root, DANA, DENY_SCOPE) absent() @@ -14382,9 +14447,9 @@ def test_an_UNKNOWN_REF_in_an_ALLOWED_scope_is_the_SAME_404( assert missing_ref[2] def test_PUT_to_a_refused_scope_is_BYTE_IDENTICAL_to_one_that_never_existed( - self, tmp_path: Path + self, sited_root: Path ): - root, present, absent = self._phases(tmp_path) + root, present, absent = self._phases(sited_root) def put(record): with running(root, tokens=(record,)) as (base, _): @@ -15515,9 +15580,9 @@ def test_a_heading_INSIDE_a_fence_is_not_the_heading(self): assert api.nuance_insert_index(lines) == 4 def test_an_entry_with_no_NUANCE_heading_is_422_not_a_reshaped_file( - self, tmp_path: Path + self, sited_root: Path ): - root = _build_store(tmp_path / "store", {ALLOW_SCOPE: KELP_NUANCE}) + root = _build_store(sited_root, {ALLOW_SCOPE: KELP_NUANCE}) path = entry_file(root, ALLOW_SCOPE) text = path.read_text().replace(resolver.NUANCE_HEADING, "## Notes") path.write_text(text) @@ -15531,7 +15596,7 @@ def test_an_entry_with_no_NUANCE_heading_is_422_not_a_reshaped_file( assert path.read_bytes() == before def test_a_FAILED_write_leaves_the_entry_and_no_temp_file( - self, tmp_path: Path, monkeypatch + self, sited_root: Path, monkeypatch ): """šŸ”“ `os.replace`, NOT `open(path, "w")`. A truncate-then-write leaves a window in which a concurrent reader sees an EMPTY or half-written entry @@ -15543,7 +15608,7 @@ def test_a_FAILED_write_leaves_the_entry_and_no_temp_file( temp file must not be left behind either (it is invisible to every walker, so nothing would ever report or clean it up). """ - root = _build_store(tmp_path / "store", {ALLOW_SCOPE: KELP_NUANCE}) + root = _build_store(sited_root, {ALLOW_SCOPE: KELP_NUANCE}) path = entry_file(root, ALLOW_SCOPE) before = path.read_bytes() @@ -15661,20 +15726,24 @@ class TestAnAppendDoesNotREWRITETheFile: `If-Match` is invalidated for a change nobody asked for. """ - def _entry_path(self, tmp_path: Path) -> Path: - root = tmp_path / "store" / ALLOW_SCOPE + def _entry_path(self, store: Path) -> Path: + # `store` arrives sited (`sited_root`) and does not exist yet ā€” the scope + # directory is created under it exactly as it was under `tmp_path / + # "store"`. `append_bullet` below writes through `_replace_bytes`, so + # this is one of the sites whose in-request fsync the siting is for. + root = store / ALLOW_SCOPE root.mkdir(parents=True) path = root / f"{entry_ref(ALLOW_SCOPE)}.md" path.write_bytes(LOSSY_ENTRY) return path - def test_every_byte_OUTSIDE_the_inserted_line_is_IDENTICAL(self, tmp_path: Path): + def test_every_byte_OUTSIDE_the_inserted_line_is_IDENTICAL(self, sited_root: Path): """šŸ”“ THE WHOLE CLAIM, PINNED ON BYTES. The expected file is spelled here as `prefix + + suffix` over the ORIGINAL bytes, so a writer that changed anything at all ā€” an encoding, a line ending, a trailing newline ā€” fails on the equality rather than on a property somebody remembered to check.""" - path = self._entry_path(tmp_path) + path = self._entry_path(sited_root) head_end = LOSSY_ENTRY.index(_HEADING_BYTES) + len(_HEADING_BYTES) status, line, _rev = api.append_bullet( @@ -15713,11 +15782,11 @@ def test_the_hostile_byte_is_INSIDE_A_NUANCE_BULLET_which_is_what_is_hashed(self )[resolver.WHAT_HEADING] assert "\udce9" in what, what - def test_a_NON_UTF8_byte_on_an_untouched_line_SURVIVES(self, tmp_path: Path): + def test_a_NON_UTF8_byte_on_an_untouched_line_SURVIVES(self, sited_root: Path): """Named separately from the equality above because this is the one that DESTROYS content the store cannot re-derive, and because `U+FFFD` is the specific corpse to look for.""" - path = self._entry_path(tmp_path) + path = self._entry_path(sited_root) assert b"\xe9" in LOSSY_ENTRY and b"\xef\xbf\xbd" not in LOSSY_ENTRY status, line, _rev = api.append_bullet( @@ -15738,8 +15807,8 @@ def test_a_NON_UTF8_byte_on_an_untouched_line_SURVIVES(self, tmp_path: Path): "the append replaced an undecodable byte with U+FFFD" ) - def test_CRLF_line_endings_are_NOT_normalised(self, tmp_path: Path): - path = self._entry_path(tmp_path) + def test_CRLF_line_endings_are_NOT_normalised(self, sited_root: Path): + path = self._entry_path(sited_root) before_crlf = LOSSY_ENTRY.count(b"\r\n") assert before_crlf == 2, "the fixture stopped exercising CRLF" @@ -15755,8 +15824,8 @@ def test_CRLF_line_endings_are_NOT_normalised(self, tmp_path: Path): assert after.count(b"\r\n") == before_crlf - def test_a_file_with_NO_trailing_newline_does_not_gain_one(self, tmp_path: Path): - path = self._entry_path(tmp_path) + def test_a_file_with_NO_trailing_newline_does_not_gain_one(self, sited_root: Path): + path = self._entry_path(sited_root) assert not LOSSY_ENTRY.endswith(b"\n") status, line, _rev = api.append_bullet( @@ -15773,11 +15842,11 @@ def test_a_file_with_NO_trailing_newline_does_not_gain_one(self, tmp_path: Path) "the append added a trailing newline to a file that had none" ) - def test_the_bullet_INHERITS_the_headings_own_line_ending(self, tmp_path: Path): + def test_the_bullet_INHERITS_the_headings_own_line_ending(self, sited_root: Path): """A CRLF entry must not gain an LF-terminated line in the middle of it. The terminator is taken from the heading the bullet is inserted under, never assumed.""" - root = tmp_path / "store" / ALLOW_SCOPE + root = sited_root / ALLOW_SCOPE root.mkdir(parents=True) path = root / f"{entry_ref(ALLOW_SCOPE)}.md" original = ( @@ -15798,12 +15867,12 @@ def test_the_bullet_INHERITS_the_headings_own_line_ending(self, tmp_path: Path): ) def test_a_NO_trailing_newline_entry_whose_HEADING_is_the_LAST_line( - self, tmp_path: Path + self, sited_root: Path ): """The boundary the terminator rule turns on: there is no line ending to inherit, so one is introduced BEFORE the bullet and the file still does not end in a newline.""" - root = tmp_path / "store" / ALLOW_SCOPE + root = sited_root / ALLOW_SCOPE root.mkdir(parents=True) path = root / f"{entry_ref(ALLOW_SCOPE)}.md" original = ( @@ -16378,8 +16447,9 @@ class TestTheDedupeScopeIsTheINSERTIONScope: FIRST_PROSE = "the mooring pennant chafes against the fairlead" SECOND_PROSE = "the stern gland weeps a drop a minute under way" - def _twin_heading_entry(self, tmp_path: Path) -> Path: - root = tmp_path / "store" / ALLOW_SCOPE + def _twin_heading_entry(self, store: Path) -> Path: + # `store` arrives sited (`sited_root`) and does not exist yet. + root = store / ALLOW_SCOPE root.mkdir(parents=True) path = root / f"{entry_ref(ALLOW_SCOPE)}.md" path.write_text( @@ -16407,9 +16477,9 @@ def _twin_heading_entry(self, tmp_path: Path) -> Path: return path def test_a_bullet_matching_the_SECOND_section_is_APPENDED_not_swallowed( - self, tmp_path: Path + self, sited_root: Path ): - path = self._twin_heading_entry(tmp_path) + path = self._twin_heading_entry(sited_root) before = path.read_bytes() status, line, _rev = api.append_bullet( @@ -16431,12 +16501,12 @@ def test_a_bullet_matching_the_SECOND_section_is_APPENDED_not_swallowed( assert first_body.splitlines()[1] == line, after def test_a_bullet_matching_the_FIRST_section_is_STILL_a_duplicate( - self, tmp_path: Path + self, sited_root: Path ): """šŸ”“ THE POSITIVE CONTROL, and the reason the fix is a NARROWING rather than a removal: within the section the writer actually inserts into, idempotency is unchanged and not one byte is written.""" - path = self._twin_heading_entry(tmp_path) + path = self._twin_heading_entry(sited_root) before = path.read_bytes() status, _line, _rev = api.append_bullet( @@ -16447,11 +16517,11 @@ def test_a_bullet_matching_the_FIRST_section_is_STILL_a_duplicate( assert status == "duplicate" assert path.read_bytes() == before - def test_the_section_body_STOPS_at_the_next_heading(self, tmp_path: Path): + def test_the_section_body_STOPS_at_the_next_heading(self, sited_root: Path): """The narrowing is the section boundary itself, so it is asserted directly: `## Pointers` sits between the two nuance blocks and its content belongs to neither.""" - path = self._twin_heading_entry(tmp_path) + path = self._twin_heading_entry(sited_root) lines = path.read_text(encoding="utf-8").splitlines() block = api.nuance_block(lines) @@ -16493,12 +16563,12 @@ def spy(fd): return kinds def test_an_append_fsyncs_BOTH_the_file_and_its_DIRECTORY( - self, tmp_path: Path, monkeypatch + self, sited_root: Path, monkeypatch ): """šŸ”“ ONE ASSERTION PER FSYNC, so deleting EITHER one goes red ā€” a single "fsync was called" check is green with the directory one removed, which is exactly the mutant that survived.""" - root = _build_store(tmp_path / "store", {ALLOW_SCOPE: KELP_NUANCE}) + root = _build_store(sited_root, {ALLOW_SCOPE: KELP_NUANCE}) path = entry_file(root, ALLOW_SCOPE) kinds = self._fsynced_kinds( @@ -16515,11 +16585,11 @@ def test_an_append_fsyncs_BOTH_the_file_and_its_DIRECTORY( "append visible is not durable across a crash" ) - def test_a_PUT_fsyncs_BOTH_as_well(self, tmp_path: Path, monkeypatch): + def test_a_PUT_fsyncs_BOTH_as_well(self, sited_root: Path, monkeypatch): """Both write primitives go through `_replace_bytes`, and the test says so rather than assuming it: a second copy of the write would be the predicate-at-two-sites shape this module keeps finding.""" - root = _build_store(tmp_path / "store", {ALLOW_SCOPE: KELP_NUANCE}) + root = _build_store(sited_root, {ALLOW_SCOPE: KELP_NUANCE}) path = entry_file(root, ALLOW_SCOPE) data = _entry(entry_ref(ALLOW_SCOPE), ALLOW_SCOPE, nuance=f"- 2026-05-06: {BULLET_E}").encode() revision = api.entry_revision(path.read_bytes()) @@ -16535,14 +16605,14 @@ def test_a_PUT_fsyncs_BOTH_as_well(self, tmp_path: Path, monkeypatch): assert False in kinds and True in kinds, kinds def test_an_UNFSYNCABLE_directory_does_not_fail_the_write( - self, tmp_path: Path, monkeypatch + self, sited_root: Path, monkeypatch ): """Best-effort is a decision, so it is pinned: a filesystem that refuses a directory fd must not turn a completed append into a 503. Trading a rare durability gap for a certain availability one is the wrong trade, and an unasserted `try/except` is the shape that silently becomes the right one for the wrong reason.""" - root = _build_store(tmp_path / "store", {ALLOW_SCOPE: KELP_NUANCE}) + root = _build_store(sited_root, {ALLOW_SCOPE: KELP_NUANCE}) path = entry_file(root, ALLOW_SCOPE) real_open = os.open @@ -19372,6 +19442,88 @@ class TestTheStoreIsSitedOffTheContendedDisk: as flaky while every test still passed ā€” the change would be inert and indistinguishable from a working one, which is the failure mode this class exists to make impossible. + + šŸ”“ READ THE PARAGRAPH ABOVE AT THE WIDTH ITS BODY ACTUALLY HAS, BECAUSE IT WAS + WRONG BY EXACTLY THAT GAP. It says "a fixture that silently fell back to disk + EVERYWHERE", and the positive control under it took ONE fixture (`store`). That + is one site wide. When it was written this file had 18 further store roots built + inline from `tmp_path`, every one of them disk-backed unconditionally, and the + class that kept failing `tekton/devrc-pytests` on docs-only PRs was among them ā€” + behind this green. A control on one fixture cannot see the other sites, and its + docstring claiming "everywhere" is what stopped anyone looking. + + So the claim is now split, deliberately, into two guards that fail differently: + + * BEHAVIOURAL, here: `store` and `sited_root` are each shown to LAND on tmpfs + when one is usable. This is what a structural check cannot do ā€” it type- + checks past a wrong argument ā€” but it can only ever cover the fixtures it + names, and it SKIPS where there is no tmpfs, which may well be CI. + * STRUCTURAL, in `test_store_siting_ledger.py`: `_DISK_ROOTED_ALLOWLIST` plus + `test_the_disk_rooted_census_matches_the_allowlist_EXACTLY` enumerate the + store sites its predicate CAN SEE ā€” in this file and in the two other + ledgered ones ā€” and fail when that set grows or shrinks. That one holds on a + machine with no tmpfs at all. + + šŸ”“ AND THAT BULLET SAID "EVERY STORE SITE IN THIS FILE", TEN LINES UNDER THE + RETRACTION OF THE PREVIOUS OVERCLAIM. Same defect as the paragraph above, in the + replacement written for it: a description wider than the implementation, which is + exactly what stops anyone looking. What the census actually reads is a + `tmp_path / X` or `tmp_path.joinpath(X)` expression that flows into a store + consumer within one function scope, THROUGH A BINDING FORM ITS `_assignments` + RESOLVES, AND THROUGH AN EXPRESSION ITS `_path_base` CAN NAME A BASE FOR. That + third condition is load-bearing and was missing here: `stores = {'a': tmp_path / + 'served'}; running(stores['a'])` satisfies the first two and still counts 0, + because `_path_base` returns None for the `ast.Subscript` the consumer is handed. + + šŸ”“ THE HOLES, AND WHICH OF THEM A TEST RE-MEASURES ON EVERY RUN. This list used to + say "each measured and each pinned by a named guard over there rather than left to + be rediscovered as news", and that was true of ONE of them. The distinction is the + whole value of the sentence: a hole a guard re-measures cannot silently close or + widen, and a hole only written down can do both while every suite stays green. So + the label comes first, and it is checked rather than asserted: + + * PINNED ā€” a root bound in a `@pytest.fixture` and served in a test that + requests it. `test_a_store_root_bound_in_a_pytest_FIXTURE_is_NOT_counted` + runs every time, asserts 0 for both the plain and the tuple-returning fixture + form against an inline control of 1, and names two live instances here + (`served = tmp_path / "ordered-served"` in `shuffled_pair`, `served = + tmp_path / "ambig-served"` in `ambiguous_pair` ā€” both verified present). + * ONLY WRITTEN DOWN ā€” a root not spelled off the NAME `tmp_path` at all: + `Path(tmp_path) / "store"`, an alias `base = tmp_path`, + `os.path.join(...)`, `str(tmp_path) + "/store"`, + `tmp_path_factory.mktemp(...)`. NO TEST ANYWHERE exercises any of these ā€” + every occurrence of those spellings in `test_store_siting_ledger.py` is + inside a docstring. The 0s were measured by hand and nothing re-measures + them, so this hole can close (the predicate widens and sites appear + uncounted-but-visible) or widen with the suite green. + * ONLY WRITTEN DOWN ā€” a root reaching a consumer whose name is not in + `_ROOT_CONSUMERS`. Its own clause says it: recorded by "that set's own + comment", which is a comment, not a guard. The `serve_store(served)` -> 0 + control it cites is prose in both places it appears in that file ā€” the + `_ROOT_CONSUMERS` comment and the fourth residual bullet ā€” and no test runs + it. + * ONLY WRITTEN DOWN ā€” šŸ”“ AND THIS ONE WAS MISSING FROM THE LIST ENTIRELY, WHICH + IS THE SAME DEFECT ONE LEVEL UP. A root reaching a REAL consumer, in ONE + test function, with no fixture involved, and blocked by none of the three + arms above. It is THREE mechanisms, not one, and an earlier revision of this + bullet grouped all of it under "a binding the census does not resolve": + a `for` or comprehension TARGET is that binding case, closed by widening + `_assignments`; a dict or list ELEMENT (`running(stores['a'])`) is a + `_path_base` case and is NOT ā€” it stays 0 under a widened `_assignments`, and + goes to 1 only when `_path_base` sees through an `ast.Subscript`; and a + CLOSURE is the SCOPE case ā€” one test function to a reader, two scopes to + `_walk_scope`, which stops at the nested `def` ā€” and stays 0 under both + widenings. Measured 0 each against an inline control of 1; the + numbers are in + `test_the_disk_rooted_census_matches_the_allowlist_EXACTLY`'s fourth residual + bullet. + + So: one hole of four is guarded. "WITHIN ONE FUNCTION SCOPE" is what the sentence + above used to say, full stop, and that is wider than the code by the fourth + bullet ā€” a fix round's own explanatory sentence has repeatedly been the next + round's defect, so read this one at the width its code has. + + Neither half is the guard. The pair is ā€” and the pair is still not everything. """ def test_the_fstype_is_resolved_by_LONGEST_mount_point_not_by_prefix(self): @@ -19451,6 +19603,57 @@ def test_the_store_fixture_is_still_a_correct_store_wherever_it_lands( "no front matter" ) + def test_the_sited_root_fixture_ACTUALLY_lands_on_tmpfs_when_one_exists( + self, sited_root: Path + ): + """šŸ”“ THE SECOND FIXTURE NEEDS ITS OWN CONTROL, AND THAT IS THE WHOLE + LESSON OF THIS CLASS'S HEADER. + + `sited_root` carries the 18 store roots that used to be built inline from + `tmp_path`, one of which is the write path that kept failing the gate. A + control on `store` says nothing about it: they are separate fixtures with + separate bodies, and a `sited_root` that had been written to fall back + unconditionally would leave every one of those 18 sites back on the + contended disk with `store`'s control still green. + + The root does not exist yet ā€” that is `store_root`'s documented contract ā€” + so the filesystem is resolved from its PARENT, which is the tmpfs holder + directory itself. Asserting on `sited_root` directly would read the + fstype of a path that is not there. + """ + available = store_siting.tmpfs_dir() + if available is None: + pytest.skip( + "no USABLE tmpfs: absent, not tmpfs, under _MIN_FREE_BYTES " + "free, or unwritable. The fallback path is exercised. Check " + "WHICH cause applies before reading this as a bare absence." + ) + assert not sited_root.exists(), ( + "sited_root exists before the test created it ā€” `store_root`'s " + "contract is that callers mkdir their own scopes under it, and the " + "parent-directory assertion below is written against that contract" + ) + landed = store_siting.mount_fstype(sited_root.parent) + assert landed == "tmpfs", ( + f"sited_root's holder landed on {landed!r} while a tmpfs at " + f"{available} was available ā€” the 18 sites that take their root from " + "this fixture are back on the contended disk and the fix is inert" + ) + + def test_the_sited_root_fixture_is_a_USABLE_store_root_wherever_it_lands( + self, sited_root: Path + ): + """The other half, and it is not a duplicate of the `store` version: this + fixture yields an EMPTY path rather than a populated tree, so what has to + be true of it is that a scope can be created under it and read back. A + `store_root` that yielded something un-mkdir-able would fail every + migrated site at once, and the tmpfs assertion above would not notice.""" + (sited_root / SCOPE).mkdir(parents=True) + (sited_root / SCOPE / "thing-alpha.md").write_text( + _entry("thing-alpha", SCOPE) + ) + assert (sited_root / SCOPE / "thing-alpha.md").is_file() + class TestTheSitingRULESThemselvesArePinned: """šŸ”“ THE THREE FIXES OF ROUND 2 ARE PINNED HERE ā€” NOT every guard in the module.