fix(test): scope published memory limits so no test leaks one (#856)

[TinDang97]

Closes #856. (#974 is a duplicate; the reporter is closing it.)

What was actually wrong

It is not a race. It is a permanent leak through production code.

command::config::config_set publishes to five process-global atomics on the CONFIG SET
path — MAXMEMORY_GLOBAL, MAXMEMORY_HINT, MAXMEMORY_PER_SHARD_HINT,
MAXMEMORY_POLICY_GLOBAL (src/storage/eviction.rs) and DB_MAXMEMORY_ANY_SET
(src/storage/db_quota.rs). The unit tests that drove it never put them back. Under
cargo test --lib — one process for the whole suite, unlike nextest's process-per-test —
whichever test later READ that state failed.

That is why the VICTIM looked order-dependent while the CAUSE was not, and why isolating the
red test showed green and proved nothing.

Five writers, each individually sufficient (each proven by running it alone with the victim
under --test-threads=1):

test	leaks
config_set_maxmemory_accepts_redis_memory_units	MAXMEMORY_GLOBAL = 1048576
config_set_db_maxmemory_accepts_memory_units	DB_MAXMEMORY_ANY_SET = true
test_config_set_maxmemory	MAXMEMORY_GLOBAL = 1048576
test_config_set_multiple_params	MAXMEMORY_GLOBAL = 2048
test_config_set_db_maxmemory	DB_MAXMEMORY_ANY_SET = true

Attribution control: -- --skip command::config → 5541 passed / 0 failed.
Negative control: config_set_maxmemory_rejects_what_redis_rejects takes the reject path and
leaks nothing. test_config_set_db_maxmemory reads green under a prefix filter only because
a sibling republishes false after it; --exact exposes it.

The fix

1. PublishedLimits — a #[cfg(test)] RAII guard in src/storage/eviction.rs, beside the
   existing ForceWriteGate. Snapshots all five atomics on capture(), restores them on Drop.
   db_quota gains a #[cfg(test)] restore_db_maxmemory_any_set(bool) rather than widening the
   static, because the public publisher derives the flag from a whole RuntimeConfig and cannot
   express "put back what was there". Nesting is LIFO-correct.

   Drop-on-unwind is the point. Four tests restored MANUALLY at the end of their bodies
   (eviction.rs ×2, db_quota.rs, server/conn/tests.rs), so a panic mid-body skipped the
   restore entirely and left the poison behind for a run that had already failed — worse than
   never restoring. db_quota.rs's own comment conceded this. All four are converted.

2. Un-forgettable, not a convention. command::config::tests calls a module-local
   config_set_scoped, and a module-local shadow of the glob-imported config_set means
   nothing in that module can reach the unguarded publisher without spelling out
   super::config_set. A new test cannot forget — that would be a compile-visible act, not an
   omission. (The attack below uses super:: precisely because it is the only spelling that
   works.)

3. Waiver RETIRED. scripts/libtest-singleproc-gate.sh defaulted LIBTEST_KNOWN_FAILURES=1
   and named this exact test in LIBTEST_KNOWN_FAILURE. Both are now empty: the gate waives
   nothing and any failure at all fails it.

4. Spin deleted. The victim's 100-attempt retry loop — sampling the globals around each gate
   call and continue-ing when a sibling had a limit published — is what turned a deterministic
   leak into something that read as a flake for months. It now ESTABLISHES the unset state under
   the guard, asserts that precondition, and fails immediately naming the values it observed.

Adoption: 16 tests

12 in src/command/config.rs (the brief listed 10; test_config_set_invalid_policy and
test_config_set_unknown_param also call config_set and are routed too, since the point is
that the module has no unguarded path), 2 in src/storage/eviction.rs, 1 in
src/storage/db_quota.rs, 1 in src/server/conn/tests.rs. The conn/tests.rs one applied
cleanly and its manual restore is removed; the guard covers the panic-inside-run case the
manual line could not.

---

Verification

All on macOS aarch64, default features, CARGO_TARGET_DIR=$HOME/n1-target.

1. BEFORE — reproduced on clean main (a8eb2efc)

test result: FAILED. 5558 passed; 1 failed; 9 ignored; 0 measured; 0 filtered out; finished in 9.64s

---- scripting::bridge::tests::gate_is_skipped_with_spill_sender_when_no_limit_is_configured stdout ----
thread '...gate_is_skipped_with_spill_sender_when_no_limit_is_configured' (11854686) panicked at src/scripting/bridge.rs:833:9:
could not observe an unset maxmemory in 100 attempts; check for a test leaking a published limit

failures:
    scripting::bridge::tests::gate_is_skipped_with_spill_sender_when_no_limit_is_configured

2. AFTER — cargo test --release --lib

test result: ok. 5559 passed; 0 failed; 9 ignored; 0 measured; 0 filtered out; finished in 8.75s
EXIT=0

3. The one the prefix filter hides — --exact

running 1 test
test command::config::tests::test_config_set_db_maxmemory ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 5567 filtered out; finished in 0.00s

4. scripts/libtest-singleproc-gate.sh — green with ZERO waivers

  libtest (single process): 5559 passed / 0 failed / 9 ignored
    baselines for macOS aarch64, default features re-captured with
      moon#856 fixed (a8eb2efc + the fix, 2026-09-16):
      5559 / 0   default features  (was 5352 / 1 at 7a87f69f)
      4470 / 1   runtime-tokio,jemalloc -- PRE-fix; the 1 is gone now
    A branch adding N unit tests reads as baseline+N passed / 0 failed.
      Since moon#856 this gate waives nothing: any failure fails it.
GATE_EXIT=0

5. The gate's own self-test — 14/14

libtest-singleproc-gate self-test:
  ok    clean run passes (rc=0)
  ok    the retired waiver no longer waives moon#856's victim (rc=1)
  ok    any single failure FAILS with no waiver armed (rc=1)
  ok    two failures FAIL with no waiver armed (rc=1)
  ok    truncated run (no summary line) FAILS (rc=1)
  ok    short run (200 < 5000 tests) FAILS (rc=1)
  ok    short run that ALSO failed is not waived (rc=1)
  ok    a branch adding tests still passes (rc=0)
  ok    the Linux count (+21 cfg tests) is not a failure (rc=0)
  ok    a re-armed waiver waives its NAMED failure (rc=0)
  ok    a re-armed waiver refuses a DIFFERENT failure (rc=1)
  ok    a re-armed waiver refuses a SECOND failure (rc=1)
  ok    a smaller feature set passes its own floor (rc=0)
  ok    unexplained non-zero cargo exit is NOT swallowed (rc=1)

SELF-TEST PASSED: the gate can fail, and only waives what it should.

the retired waiver no longer waives moon#856's victim is the retirement check: if that line
ever reads rc=0, the waiver has crept back in. The three re-armed waiver cases keep the
waiver mechanism proven — the knobs stay (empty) so a future known failure can be tolerated
deliberately, and count-only waiving stays refused.

6. ATTACK THE GUARD — mandatory, and here is the red

A guard that cannot be shown to fail is not evidence. Two runs, same binary shape, one variable.

A wrinkle worth stating plainly: after this fix the victim ESTABLISHES its own precondition, so
removing the guard from a config test would no longer redden it. That would make the guard
look decorative when it is not. So the attack uses a temporary probe with the victim's
pre-fix shape — it READS the published atomics without establishing them — which isolates
the guard as the only variable:

// TEMPORARY, NOT MERGED
#[test]
fn attack_probe_no_test_leaks_a_published_limit() {
    assert!(
        !maxmemory_is_set() && !crate::storage::db_quota::db_maxmemory_any_set(),
        "ATTACK PROBE: a test leaked a published limit — maxmemory={} db_quota={}",
        maxmemory_bytes(), crate::storage::db_quota::db_maxmemory_any_set()
    );
}

CONTROL — probe added, guards intact → GREEN. Nothing in the entire lib binary leaves a
limit published:

test storage::eviction::tests::attack_probe_no_test_leaks_a_published_limit ... ok
test result: ok. 5560 passed; 0 failed; 9 ignored; 0 measured; 0 filtered out; finished in 9.91s
GATE_EXIT=0

ATTACK — guard removed from ONE test → RED. The single change, in
test_config_set_maxmemory:

-        let result = config_set_scoped(&mut rt, &args);
+        let result = super::config_set(&mut rt, &args);

failures:

---- storage::eviction::tests::attack_probe_no_test_leaks_a_published_limit stdout ----

thread 'storage::eviction::tests::attack_probe_no_test_leaks_a_published_limit' (12636743) panicked at src/storage/eviction.rs:1874:9:
ATTACK PROBE: a test leaked a published limit — maxmemory=1048576 db_quota=false


failures:
    storage::eviction::tests::attack_probe_no_test_leaks_a_published_limit

test result: FAILED. 5559 passed; 1 failed; 9 ignored; 0 measured; 0 filtered out; finished in 7.35s

GATE FAIL: 1 failure(s); this gate waives nothing (moon#856 is fixed).
GATE_EXIT=1

maxmemory=1048576 is exactly the value test_config_set_maxmemory sets. One guard removed,
one leak, the gate red — and the retired waiver does not wave it through. Both mutations were
reverted before the commit; git diff --stat HEAD is empty.

A permanent version of this probe was considered and rejected: it only catches leakers that are
scheduled before it, so it would be an order-dependent canary — the config_set shadow is the
structural guarantee instead.

7. Both runtimes + clippy + fmt

cargo check --all-targets                                                   EXIT=0
cargo check --all-targets --no-default-features --features runtime-tokio,jemalloc   EXIT=0
cargo clippy --all-targets -- -D warnings                                   EXIT=0
cargo fmt --check                                                           clean

(cargo fmt --check caught one line in bridge.rs after the first commit; fixed and amended,
then clippy and both check legs re-run on the amended tree.)

---

Scope note: two CI files changed, comments only

.github/workflows/ci.yml and scripts/ci-local.sh are outside the original brief. Both edits
are comment-only — no stage, no run: line, no run_step, no matrix entry moves; the
run: lines are byte-identical on both sides of each hunk. Each file asserted in the present
tense that the single-process stage "waives one known failure (moon#856)", which this PR makes
false. Retiring a waiver in the script while three files keep advertising it is exactly the
divergence #913 is about, so the prose is retired with it.

Not done (deliberate)

• No std::sync → parking_lot conversion, no config_set refactor.
• scripts/ci-local.sh NOT run — the merge bar is the reviewer's to run.
• One adjacent leak observed but NOT fixed, out of scope for lib test gate_is_skipped_with_spill_sender_when_no_limit_is_configured fails on main under parallel execution #856: config_set's
  notify-keyspace-events arm calls crate::notify::publish_flags, another process-global
  publisher with no test-side restore. No test in command::config::tests currently exercises
  it, so nothing is red today — worth its own issue rather than widening this PR.

[coderabbitai]

Important

Draft PR not reviewed

Draft PRs are not automatically reviewed by default.

• Trigger a manual review

To automatically review draft PRs, update your CodeRabbit configuration:

reviews:
  auto_review:
    drafts: true

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[TinDang97]

Independently verified the fix. One open item on the merge bar, attribution in progress.

The fix works — red→green confirmed at the merge-base

I ran the same single-process gate on main (the merge-base) and on this branch:

leg	main (a8eb2ef)	this branch
tokio, single-process	scripting::bridge::tests::gate_is_skipped_with_spill_sender_when_no_limit_is_configured FAILED	passes
default features, cargo test --release --lib	5546 passed / 1 failed	5559 passed / 0 failed

So the leak reproduces at the merge-base and is gone here. Test count went 5546 → 5559: 13 tests added, none removed.

Anti-regression checks, because a green suite can also mean the victim was weakened:

• --exact scripting::bridge::tests::gate_is_skipped_with_spill_sender_when_no_limit_is_configured → still exists, passes alone.
• --exact command::config::tests::test_config_set_db_maxmemory → passes. This is the one that read green under a prefix filter because a sibling republished false after it; --exact is the only spelling that exposed it.

The shadow is better than what was asked for

The brief asked for a scoped helper so no test would forget the guard. This PR makes it so no test can: the module-local config_set shadows the glob-imported super::config_set, so a forgotten call still picks up PublishedLimits, and reaching the unguarded publisher requires explicitly spelling super::config_set. That turns a convention — which is exactly what #856 was, a convention a new test silently broke — into a compile-level property. Keeping that.

One open item

A full gate run on this branch reported:

GATE FAIL: 1 failure(s); this gate waives nothing (moon#856 is fixed).
  admin::footprint::footprint_tests::footprint_is_phys_footprint_not_resident_size

Probably load-induced, not this PR — but not yet proven. Evidence each way:

For environmental: my direct cargo test --release --lib on this exact branch returned 5559/0 with footprint passing. The gate run that flagged it happened while two other agents were cold-building (load ~50–115). The test maps 64 MiB and compares phys_footprint against resident_size, which is memory-pressure sensitive, and jemalloc on macOS has no background_thread.

Against dismissing it: this PR touches src/storage/eviction.rs, and that test is specifically about the eviction correction — "the eviction correction computed a ratio of ~1.0 and did nothing." There is a plausible causal path, so it does not get waved off.

The gate's own guidance is right — re-running the test alone proves nothing, since the mechanism is order-dependent process-global state. I will re-run the full gate on a quiet host and report. If it is environmental, this PR is clean as-is.

Note for anyone reproducing the attribution

Running the gate on the tokio leg by hand needs MIN_LIBTEST_TESTS=4000, matching ci.yml:245-246 — that leg drops graph and text-index and legitimately executes ~4692 tests, below the default 5000 floor. Without the override the gate fails on a truncation check that is not a real failure. (I hit this myself and briefly mistook it for a script bug; it is documented behaviour.)

Still DRAFT, nothing merged.

[TinDang97]

Attribution of the footprint_is_phys_footprint_not_resident_size failure: environmental, not this PR

I said I would re-run the full gate on a quiet host and report. A quiet host never
arrived — the load on this machine is other people's work — so I did what the gate
script itself prescribes instead:

To attribute: run this same gate on the merge-base.

Both legs, same worktree, same target dir, same host, back to back:

	base a8eb2efc	head e671268c (this PR)
total	5558 passed, 1 failed	5559 passed, 0 failed
scripting::bridge::tests::gate_is_skipped_with_spill_sender_when_no_limit_is_configured	FAILED	ok
admin::footprint::footprint_tests::footprint_is_phys_footprint_not_resident_size	ok	ok

Two conclusions.

1. The fix is proven red→green on the bug it targets, in my own A/B rather than on
the agent's word. The merge-base reproduces exactly the failure this PR exists to fix,
and the head commit clears it. The totals also land precisely where the gate script's
own header predicts — 5559 / 0 default features (was 5352 / 1 at 7a87f69f).

2. The footprint failure is not attributable to this PR. It passed on both legs here.

Why it can fail on an unrelated run

Reading src/admin/footprint.rs:490, the test asserts two process-wide absolute
deltas measured across a window:

• rss_growth > 32 MiB — the 64 MiB clean file-backed mapping must fault in
• fp_growth < 16 MiB — phys_footprint must not absorb it

TEST_LOCK serialises only the eight tests inside footprint.rs, not the other ~5,550
tests in the binary, and cargo test --lib runs them on parallel threads. So:

• any concurrent test thread allocating >16 MiB of anonymous memory during the window
  breaks the second assertion; and
• under memory pressure the kernel reclaims clean file-backed pages — the cheapest
  thing it can evict — as fast as the probe loop faults them in, breaking the first.

Neither path involves PublishedLimits or maxmemory, which is the entire surface this
PR touches. That is consistent with it being order- and load-dependent, which is what
the gate's own message says about single-process failures.

Honest caveat on what I ran: my A/B used --release and --test-threads=8, where
the gate uses a debug build and the default thread count. So I did not reproduce the
footprint failure and cannot claim to have diagnosed it from a live repro — I showed it
does not reproduce on either leg under my configuration, and that this PR's target bug
is fixed. The mechanism above is read from the source, not measured.

If the footprint test is worth hardening — the two assertions are genuinely racy against
a parallel --lib run — that is its own issue, not a blocker here.