test(bench): integrity-check acceptance benchmark — NRN-83 closed on evidence#107
Merged
Conversation
Emit a stable `norn trace: integrity_check` stderr marker per PRAGMA integrity_check execution when NORN_TRACE_INTEGRITY_CHECK is set. Gated on the env var so normal output and the byte-identical routing proofs are untouched. Gives an out-of-process harness a deterministic, cross-process count of how many times a code path pays the O(db-size) check: a warm daemon opens-once/verifies-once (one marker across N routed reads) versus N markers for N direct reopens.
Committed, re-runnable acceptance benchmark proving the daemon initiative's founding-bug fix (ADR 0005) end-to-end at 50k-doc scale: routed reads pay zero per-invocation integrity_check while direct no-daemon reads still verify every call. - tests/bench_util/: deterministic synthetic-vault generator (SplitMix64, pure function of (N, seed) — no wall-clock/thread RNG), with determinism self-tests that run in the default pass. - tests/integrity_benchmark.rs: #[ignore]-gated harness. Structural acceptance assertion (not timing): exactly ONE integrity_check marker in the daemon's stderr across all routed reads vs one-per-call direct, cross-checked against served markers so it cannot pass vacuously. Scale/seed/iters env-overridable; prints an evidence table. 50k run: direct read median ~410ms vs routed warm ~147ms; direct=15 integrity_check markers, daemon=1.
- src/cache/open.rs: make the trace hook's contract explicit — opt-in diagnostic off by default; enabling it intentionally diverges routed vs direct stderr; never enable where norn's stderr is asserted on. Documents why it stays release-available (pure observability) in contrast to the debug-gated NORN_CACHE_LOCK_TIMEOUT_MS (which changes timing behavior). CHANGELOG gains the same clarification. - integrity_benchmark: demote the routed-faster-than-direct timing check from a hard assert to a WARN println — the structural marker assertions are the only acceptance gates; timing is evidence. - integrity_benchmark: locate cache.db via the production identity mapping instead of a recursive walk, failing loudly if absent. Adds a #[doc(hidden)] resolve_cache_dir seam (cache.rs, re-exported in lib.rs) that stringifies the error so the crate-private CacheError does not leak into public visibility; same posture as Cache::conn. - bench_util: drop the blanket #![allow(dead_code)] (everything compiles into the #[ignore] test binary; no per-item allows needed).
The harness set XDG_CACHE_HOME via std::env::set_var so the in-process resolver would match the children's explicit .env() — reintroducing the process-global env-mutation class NRN-215 removed; under --include-ignored the benchmark runs concurrently with the bench_util unit tests, where set_var races libtest's own getenv on any variable. Make the resolution explicit instead: identity::cache_dir_for now delegates to a new cache_dir_in(cache_home, vault_root) — the same mapping as a pure function of its arguments — and the #[doc(hidden)] seam becomes resolve_cache_dir_in(cache_home, vault_root). The harness computes the path from the same private home value it already passes to children; zero env mutation anywhere in the test binary.
Two review rounds (engine=code-review, agents pinned to Opus): 7 findings — 5 fixed (trace-hook contract documented with the release-availability position, timing assert demoted to evidence, cache-dir resolution via explicit-home pure function replacing both the recursive walk and a set_var UB reintroduction, blanket dead_code allow removed), 2 dismissed (suppression-scan prose false positive; the benchmark's #[ignore] is the harness design — explicitly-run acceptance gate, not suite erosion). Adversarial-Review: run engine=code-review tier=high findings=7 fixed=5 dismissed=2 deferred=0
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The daemon initiative's founding bug (NRN-83:
Cache::openpaysPRAGMA integrity_checkper process, O(db-size), dwarfing queries at 50k docs) is closed on structural evidence, not timing. This PR adds the committed, re-runnable acceptance harness that proves the ADR-0005 resolution end-to-end.Evidence (this machine, 50k synthetic docs, 58MB cache.db):
15 routed reads across three shapes share exactly one integrity check; 15 direct reads pay 15 (trust preserved on the no-daemon path). The assertion is marker-counting cross-checked against served markers — it cannot pass vacuously, and it demonstrably fails under the cold hypothesis.
What's committed
tests/bench_util/— deterministic synthetic-vault generator: content is a pure function of(index, seed)via SplitMix64, no wall-clock or thread RNG; determinism self-tests run in the default suite.tests/integrity_benchmark.rs—#[ignore]-gated acceptance harness (explicit run:cargo test --release --test integrity_benchmark -- --ignored --nocapture), size/seed/iters env-overridable, isolated cache home + own daemon, prints the evidence table. Timing is evidence only; the structural marker gates are the assertions.src/cache/open.rs— opt-inNORN_TRACE_INTEGRITY_CHECKstderr marker (off by default; contract documented: enabling it intentionally reveals where the check runs, so stderr byte-identity does not hold under trace — deliberately release-available, unlike the debug-gated timing knob, because it changes no behavior and the release benchmark needs it).resolve_cache_dir_in) — the harness mutates no process environment.Verification
Quartet clean:
check --locked/ 2329 tests, 0 failed / clippy-D warnings/fmt;Cargo.lockuntouched. Benchmark run green at 50k and 2k scales with identical structural results.Adversarial review
Two rounds: 7 findings — 5 fixed, 2 dismissed (suppression-scan prose false positive; the benchmark's
#[ignore]is the harness design). Notable: round 1 demoted a self-contradicting timing assert to evidence and took the documented position on the trace hook's release availability; round 2 caught the harness reintroducing theset_varUB class NRN-215 eliminated — fixed by pure-function decomposition.Adversarial-Review: run engine=code-review tier=high findings=7 fixed=5 dismissed=2 deferred=0