Skip to content

feat(cluster): add MinerCluster — flips §3.7.1, §3.7.2 - #13

Merged
jensholdgaard merged 2 commits into
mainfrom
feat/cluster-tenant-isolation-rfc-3-7
May 10, 2026
Merged

feat(cluster): add MinerCluster — flips §3.7.1, §3.7.2#13
jensholdgaard merged 2 commits into
mainfrom
feat/cluster-tenant-isolation-rfc-3-7

Conversation

@jensholdgaard

Copy link
Copy Markdown
Owner

Summary

Two §5 invariant scenarios go from #[ignore] + todo!() to real
assertions in this single commit, jumping the §5 count from
4/29 to 6/29. This PR ships the multi-tenancy shape
TenantId, MinerCluster with one TenantState per tenant —
but explicitly not the Drain tree.

What's in this PR

  • ourios-core::tenant::TenantIdString-backed newtype
    (operator-facing slugs / UUIDs are String-shaped; column-store
    efficiency is the future ourios-parquet RFC's concern).
    No validation yet; future try_new can layer on top.
    AsRef<str> + Display for log lines / metric labels.
  • ourios-miner::cluster::MinerCluster — public type
    holding HashMap<TenantId, TenantState> and a cluster-wide
    template_id allocator. Per-tenant state allocated lazily on
    first ingest. Public API: new, config, ingest,
    template_count, templates_for.
  • ourios-miner::cluster::TenantState — private struct
    holding only the templates HashMap. Future PRs swap this
    for the real Drain tree.

Why the template_id allocator is cluster-wide, not per-tenant

RFC 0001 §6.1 uses the phrase "per-tenant monotonic" but also
requires that "two tenants emitting the structurally identical
template will have different template_ids," and §5 §3.7.2
requires "no template_id is shared across tenants." A truly
per-tenant allocator gives both tenants id=1 for their first
template and silently violates §3.7.2 — the test caught this on
first run
(id_a == id_b == 1 failure). Reconciliation: the
id space is cluster-wide, but each tenant's slice of that
space is monotonic with respect to that tenant's allocation
order. Both phrases hold:

  • "per-tenant monotonic" — given a tenant, the sequence of
    ids allocated to that tenant strictly increases over time.
  • "different template_ids across tenants" — the shared
    allocator never hands out the same id twice.

A code comment on next_template_id documents this so future
readers don't try to "fix" it back into per-tenant.

What's not in this PR (deferred)

  • Drain tree — no simSeq, no depth-bounded tree, no
    widening. The cluster's per-tenant store is a HashMap keyed
    on the masked-token sequence (exact-match templating). Future
    PR replaces with RFC §6.2 steps 3–5.
  • Audit events, telemetry, body retention, lossy_flag
    follow once the tree exists.
  • Parquet record emissionourios-parquet's problem.
  • Tenant lifecycle (TenantPaused, TenantDeleted,
    eviction) — RFC §9 deferral, future PR.

§5 stubs flipped

Scenario Test Approach
§3.7.1 invariant_3_7_1_tenant_trees_never_cross_pollinate Two tenants emit different shapes; interleaved ingest. Asserts on token-set membership: A's tree contains A-shape tokens, B's contains B-shape tokens, neither contains the other's.
§3.7.2 invariant_3_7_2_same_template_two_tenants_distinct_template_ids Two tenants emit the identical line. Asserts id_a != id_b (the bug the cluster-wide allocator fixes).

Plus 4 cluster unit tests in cluster.rs:
ingest_returns_same_template_id_for_repeat_shape,
ingest_returns_distinct_template_ids_for_distinct_shapes,
template_count_is_zero_for_unseen_tenant,
ingest_lazily_allocates_per_tenant_state.

All AAA-structured per the new policy.

Cargo dependency change

ourios-miner promotes ourios-core from [dev-dependencies]
to [dependencies] — the cluster module now imports
ourios_core::config::MinerConfig and ourios_core::tenant::TenantId
from non-test code. The single dep entry covers both production
code and the integration tests.

Lifecycle

Per docs/verification.md §3 two-loop spec:

  • Outer loop: 15 → 21 passed (+4 cluster unit tests, +2
    newly green §5 scenarios); 25 → 23 ignored (−2 flipped).
  • Inner loop: 25 → 23 failed.
  • §5 scenario count toward Green: 4/29 → 6/29.

RFC 0001 stays at status: red (23 stubs remaining).

Invariant / hazard impact (CLAUDE.md §§3, 4)

  • §3.7 (multi-tenancy is not bolted on). TenantId is now
    the entry-point type; MinerCluster::ingest requires it for
    every line. There is no code path in the cluster that touches
    template state without a TenantId.
  • The §3.7 invariant's "every code path that touches data takes
    a tenant ID" is satisfied for the cluster surface; future
    Parquet/WAL surfaces will need to thread it through too.

Test plan

  • cargo fmt --all --check clean
  • cargo clippy --all-targets --all-features -- -D warnings clean
  • cargo test --all-features — 21 passed, 23 ignored
  • mdbook build clean

🤖 Generated with Claude Code

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Introduces the initial multi-tenant “cluster” surface for the miner by adding a TenantId type in ourios-core and a MinerCluster in ourios-miner that maintains isolated per-tenant template state while allocating globally-unique template_ids. This also flips two previously-ignored RFC invariants (§3.7.1/§3.7.2) into real assertions.

Changes:

  • Add ourios_core::tenant::TenantId (String-backed newtype) and export it from ourios-core.
  • Add ourios_miner::cluster::MinerCluster (per-tenant template stores + cluster-wide template_id allocator) and export the module.
  • Convert invariant tests §3.7.1 and §3.7.2 from #[ignore] + todo!() into executable tests; promote ourios-core to a normal dependency for ourios-miner.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
crates/ourios-miner/tests/invariants.rs Flips §3.7.1/§3.7.2 invariant scenarios into real tests using MinerCluster + TenantId.
crates/ourios-miner/src/lib.rs Exposes the new cluster module publicly.
crates/ourios-miner/src/cluster.rs Implements the multi-tenant cluster wrapper and per-tenant template storage + unit tests.
crates/ourios-miner/Cargo.toml Promotes ourios-core from dev-dependency to normal dependency for production code usage.
crates/ourios-core/src/tenant.rs Adds the TenantId newtype and basic string access/display helpers.
crates/ourios-core/src/lib.rs Exports the new tenant module.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/ourios-miner/src/cluster.rs Outdated
Comment thread crates/ourios-miner/tests/invariants.rs Outdated
jensholdgaard added a commit that referenced this pull request May 10, 2026
)

* docs(rfc-0001): clarify template_id allocator scope (PR #13 driver)

PR #13 (MinerCluster) had to make a design choice the RFC was
ambiguous about: §6.1 said "per-tenant monotonic u64" while §5
§3.7.2 required "no template_id is shared across tenants." A
literal per-tenant allocator gives both tenants id=1 for their
first template and silently violates §3.7.2 (the test caught
this on first run).

PR #13 went with a *cluster-wide* monotonic allocator: the id
space is shared across tenants so no value is reused, and each
tenant's slice of that space is monotonic in allocation order.
Both invariants hold. Team verdict (Harper / Lucas / Benjamin /
Jens) endorses this reading and asked for the RFC text to match
the implementation.

Two prose-only edits, no §5 scenario semantics change (the
existing §3.7.2 clauses are reinforced, not replaced):

- §6.1 "Template identity": rephrase the opening sentence from
  "per-tenant monotonic u64" to "cluster-wide unique monotonic
  u64 (with each tenant seeing a monotonic subsequence)" and add
  a sentence pinning *why* — "the id space is shared across
  tenants so that the same u64 value never refers to two
  different leaves; the per-tenant subsequence guarantee
  preserves [§3.7] by making each tenant's allocation order
  observable in isolation." Existing follow-on sentences ("Cross-
  tenant identity is intentionally not guaranteed", the §6.1
  paragraph on "Why two integers and not a content hash") still
  read correctly under the new wording — content-hash global
  identity is what would leak isolation, not a sequential
  cluster-wide allocator that happens to produce different
  values for the same template across tenants.

- §5 §3.7.2: add one bullet — "And template IDs are guaranteed
  unique across the entire cluster (not just per tenant)" —
  making the cluster-wide reading explicit and unambiguous so
  the existing terser "no template_id is shared across tenants"
  bullet cannot be re-read into a (tenant_id, template_id)
  compound-key interpretation. The implementation in
  ourios-miner::cluster already satisfies this; the RFC text
  now matches.

Why a cluster-wide allocator was the right call (per the team
verdict in PR #13's review):

- Queries stay simple — `where template_id = X` instead of
  `where (tenant_id, template_id) = (T, X)` compound keys.
- Parquet's template_id column stays a single clean u64 with
  no tenant-scoping needed for predicate pushdown.
- No risk of silent ID collisions between tenants.
- Future cross-tenant analytics (if ever in scope) become
  trivial.

Status stays at red (no §5 scenarios flip, no test stubs
touched). Pure prose precision.

Verification (CLAUDE.md §6.6): mdbook build clean, cargo fmt
clean, cargo test passing (no test changes).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

* docs(rfc-0001): address PR #14 review — fix backticks + identity wording

Two Copilot hits on the §6.1 / §3.7.2 clarification:

- C1 (§3.7.2 line 442): the new bullet wrote "template IDs" in
  prose without backticks. Every other §5 scenario uses literal
  `template_id` (with backticks) so `grep -R "template_id"`
  resolves bidirectionally between RFC and tests
  (docs/verification.md §2.3). Fix to `template_id`s.

- C2 (§6.1 line 582): the new opening paragraph established
  `template_id` as "cluster-wide unique" (i.e., the u64 value
  globally identifies a single leaf), but the existing follow-on
  sentence said "Cross-tenant identity is intentionally not
  guaranteed" — the bare word "identity" then collides with the
  new ID-uniqueness wording and reads contradictory. Fix by
  qualifying both occurrences as "*content* identity" / "content
  identity" and adding a parenthetical that names the
  distinction in one sentence: "The u64 value itself is
  cluster-wide unique, per the previous paragraph; what is not
  guaranteed is that *the same template* across two tenants
  resolves to the same id." That dissolves the surface
  contradiction without changing any commitments.

The "future template_fingerprint side column may carry a
canonical content hash" sentence is unaffected — it was already
about content-derived identity and the qualification reads
naturally there too.

No §5 scenario IDs added or removed; status stays at red.

Verification (CLAUDE.md §6.6): mdbook build clean, cargo fmt
clean, cargo test passing (no test changes).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Two §5 invariant scenarios go from #[ignore] + todo!() to real
assertions in the same commit, jumping the §5 count from 4/29
to 6/29. Pattern matches PR #11 (MinerConfig flipped 3 stubs).

This PR ships the multi-tenancy *shape* — TenantId, MinerCluster
with one TenantState per tenant, lazy per-tenant allocation —
but explicitly NOT the Drain tree. Per-tenant state is a
HashMap<Vec<String>, u64> keyed on the masked-token sequence
(exact-match templating). Future PRs replace the HashMap with
simSeq + the depth-bounded tree + widening (RFC 0001 §6.2 steps
3–5). The §3.7 isolation invariant is testable at this layer
because isolation is about *who owns which store*, not about how
the store clusters.

Implementation:

ourios-core::tenant::TenantId
  String-backed newtype (deferred u64 representation per the
  plan's design call #1: operator-facing slugs / UUIDs are
  String-shaped; column-store efficiency is a downstream concern
  the future ourios-parquet RFC will own). No validation yet —
  accept any string, future try_new can layer on top.
  AsRef<str> + Display so it composes with log lines + metric
  labels without ceremony.

ourios-miner::cluster::MinerCluster
  Public type holding a HashMap<TenantId, TenantState> and a
  *cluster-wide* template_id allocator. Tenant state allocated
  lazily on first ingest. Public API: new, config, ingest,
  template_count, templates_for. The latter two are test
  helpers but kept public per the plan's design call #4 (future
  operator-console-style tooling will want them; pub(crate)
  tightening is easy if we change our minds).

ourios-miner::cluster::TenantState
  Private struct holding only the templates HashMap. Future
  PRs swap this for the real tree.

Why the template_id allocator is cluster-wide, not per-tenant:

RFC 0001 §6.1 uses the phrase "per-tenant monotonic" but ALSO
requires that "two tenants emitting the structurally identical
template will have different template_ids", and §5 §3.7.2
requires "no template_id is shared across tenants." A truly
per-tenant allocator gives both tenants id=1 for their first
template and silently violates §3.7.2 (the test caught this on
first run — id_a == id_b == 1). Reconciliation: the id *space*
is cluster-wide, but each tenant's slice of that space is
monotonic with respect to that tenant's allocation order. Both
phrases hold:

  - "per-tenant monotonic" — given a tenant, the sequence of ids
    allocated *to* that tenant strictly increases over time
  - "different template_ids across tenants" — the shared
    allocator never hands out the same id twice

A code comment on next_template_id documents this so future
readers don't try to "fix" the cluster-wide allocator back into
per-tenant.

Tests:

§5 stub flips (AAA-structured per the new policy):

- §3.7.1 — Two tenants emit different shapes; interleaved
  ingest. Asserts on token-set membership: A's tree contains
  A-shape tokens, B's contains B-shape tokens, neither contains
  the other's. Cross-pollination would mean either set
  contained tokens that originated in the other tenant's input.
- §3.7.2 — Two tenants emit the structurally identical line.
  Asserts id_a != id_b (the bug the cluster-wide allocator
  fixes).

Cluster unit tests (in cluster.rs, AAA-structured):

- ingest_returns_same_template_id_for_repeat_shape — exact-
  match templating gives one id for "user 42 logged in" and
  "user 17 logged in" since both mask to the same shape.
- ingest_returns_distinct_template_ids_for_distinct_shapes —
  same tenant, different shapes → different ids.
- template_count_is_zero_for_unseen_tenant — unseen tenants
  return 0 / [], no panic.
- ingest_lazily_allocates_per_tenant_state — first ingest
  materialises the tenant state; before that, count is 0.

Cargo dependency change: ourios-miner promotes ourios-core from
[dev-dependencies] to [dependencies] — the cluster module now
imports ourios_core::config::MinerConfig and ourios_core::
tenant::TenantId from non-test code. The single dep entry covers
both production code and the integration tests in
tests/invariants.rs.

Lifecycle (per docs/verification.md §3 two-loop spec):

- Outer loop (cargo test --all-features):
    21 passed (was 15: + 4 cluster unit tests + 2 newly green
    §5 scenarios), 23 ignored (was 25, − 2 flipped)
- Inner loop (cargo test --no-fail-fast -- --ignored):
    23 failed, was 25
- §5 scenario count toward Green: 4/29 → 6/29

RFC 0001 stays at status: red (23 stubs to go).

What this PR is NOT:

- Not Drain — no simSeq, no depth-bounded tree, no widening.
  Future PR.
- No audit events, telemetry, body retention, lossy_flag.
  Future PR(s).
- No Parquet record emission. ourios-parquet's problem.
- No tenant lifecycle (TenantPaused, TenantDeleted, eviction).
  RFC §9 deferral, future PR.

Verification (CLAUDE.md §6.6): cargo fmt clean, cargo clippy
clean (-D warnings, --all-targets --all-features), cargo test
passing (21 / 23 split), mdbook build clean.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@jensholdgaard
jensholdgaard force-pushed the feat/cluster-tenant-isolation-rfc-3-7 branch from dec4cf5 to 213b864 Compare May 10, 2026 21:26
Two Copilot hits — both real doc/comment drift caused by my
mid-execution switch from per-tenant to cluster-wide template_id
allocator. The implementation went cluster-wide (rightly, per
team verdict + PR #14's RFC clarification) but two prose
artefacts kept describing the old per-tenant rationale:

- C1 (cluster.rs module docs): said "no shared template_id
  allocator" and "RFC 0001 §6.1's per-tenant monotonic
  template_id falls out of construction." The cluster-wide
  next_template_id field directly contradicts both clauses.
  Rewrite the opening paragraph to say what the code actually
  does: per-tenant template *stores* are isolated (no template
  ever crosses tenants), but the template_id allocator is
  cluster-wide so the same u64 never refers to two leaves; each
  tenant sees a monotonic *subsequence* of the shared id space.

- C2 (invariants.rs §3.7.2 test assertion comment): said
  "RFC 0001 §6.1's per-tenant template_id allocator gives each
  tenant its own monotonic id space, so the two ids are distinct
  by construction (each starts at 1)." This rationale is wrong
  for the cluster-wide allocator — the ids are distinct because
  the allocator never reuses values (id_a = 1, id_b = 2),
  not because each tenant has its own space starting at 1.
  Replace with the correct rationale: the second call pulls the
  *next* monotonic id rather than reusing the first tenant's id.

Both fixes are pure prose. No behaviour change. The tests
themselves still pass (the assertion logic was correct; only the
explanatory comment was stale).

Verification (CLAUDE.md §6.6): cargo fmt clean, cargo clippy
clean, cargo test passing (21 / 23 split unchanged).

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@jensholdgaard
jensholdgaard merged commit 1dc9ec1 into main May 10, 2026
7 checks passed
@jensholdgaard
jensholdgaard deleted the feat/cluster-tenant-isolation-rfc-3-7 branch May 10, 2026 21:29
jensholdgaard added a commit that referenced this pull request Jul 12, 2026
…icker diagnosed) (#490)

The #488 diagnostics discriminated the L3 intermittency in one run:
Loki's query_ingesters_within cutoff (default 3h) makes queries over
the replayed corpus's weeks-old range skip the ingesters entirely
(ingester.totalReached: 0 in the failing response), so rows still in
unflushed low-volume chunks are invisible — visibility raced the
flush loop. High-volume streams always flushed fast enough, which is
why only the 9-row trace pair flickered while kafka pairs never did.

-querier.query-ingesters-within=0 disables the cutoff: the query-side
twin of reject-old-samples=false for frozen corpora, documented as the
third in-Loki's-favour deviation. Also: 10 s poll interval (run #13's
2 s polling queued 321 s of engine time behind itself) and a
partialSuccess assert on the OTLP push path so silently-rejected
records fail at ingest, not as a downstream equivalence mystery.


Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
jensholdgaard added a commit that referenced this pull request Jul 12, 2026
Random 16/8-byte ids defeat min/max statistics, so an exact-id lookup
(RFC 0031 L3) degenerates to a whole-column scan: measured 72.4 MB for
a 9-row trace on otel-demo-v8 (comparative run #12). Same §3.6 pattern
as the template_id and promoted-column blooms; readers are unaffected
(blooms are optional metadata) and DataFusion consults them by default.

Validation: pre-merge comparative dispatch on this branch (run #13)
per the maintainer's measure-before-merge workflow; the RFC 0005 §3.6
amendment text follows with the measured numbers.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
jensholdgaard added a commit that referenced this pull request Jul 12, 2026
…489)

* feat(parquet): rfc 0005 §3.6 — bloom filters on trace_id and span_id

Random 16/8-byte ids defeat min/max statistics, so an exact-id lookup
(RFC 0031 L3) degenerates to a whole-column scan: measured 72.4 MB for
a 9-row trace on otel-demo-v8 (comparative run #12). Same §3.6 pattern
as the template_id and promoted-column blooms; readers are unaffected
(blooms are optional metadata) and DataFusion consults them by default.

Validation: pre-merge comparative dispatch on this branch (run #13)
per the maintainer's measure-before-merge workflow; the RFC 0005 §3.6
amendment text follows with the measured numbers.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* test(parquet): rfc 0005 — column constants in the bloom test

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* docs(parquet): rfc 0005 — module-doc bloom list covers the trace-context ids

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
jensholdgaard added a commit that referenced this pull request Jul 12, 2026
jensholdgaard added a commit that referenced this pull request Jul 13, 2026
…aintainer-gated fold-in (#494)

* docs(bench): rfc 0031 §9 comparative entry draft (runs #8#17)

§9.13 compiles the RFC 0031 comparative program's honest-metric era
(runs #8#17 on corpus/otel-demo-v8 vs digest-pinned Loki 3.5.3):
L1 and L3 provisional must-win passes on both channels, L2
parity-plus storage-side with named levers, the time-window losses
published, the Loki flag deviations and nondeterminism recorded,
and the §7 freeze inputs listed as open maintainer decisions.
Fold-in is maintainer-gated; this is the draft.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

* docs(bench): rfc 0031 §9.13 — auditability round: full digest, run #16 row, scoped determinism

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* docs(bench): rfc 0031 §9.13 — every quoted ratio carries its raw loki bytes

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* docs(bench): rfc 0031 §9.13 — l2 ledger carries its raw loki bytes too

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* docs(bench): rfc 0031 §9.13 — full reproduction rows, streaks audit from the entry alone

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* docs(bench): rfc 0031 §9.13 — l2 reproduction rows back the quoted band

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* docs(bench): rfc 0031 §9.13 — run #13's salvaged pairs are counted; say so

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* docs(bench): rfc 0031 §9.13 — deviation flags spelled exactly as passed

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* docs(bench): rfc 0031 §9.13 — bytes floor labeled as analog of the latency gate; .11 citation

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* docs(bench): rfc 0031 §9.13 — bloom provenance cites impl + amendment; full run table

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* docs(bench): rfc 0031 §9.13 — run #18 latency channel: rfc0031.7 passes as written

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* docs(bench): rfc 0031 §9.13 — heading spans #18; pass claim scoped to counted runs

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
jensholdgaard added a commit that referenced this pull request Jul 16, 2026
…ne miss

Run #13's lower-frequency candidate (template_id=60, ~144s average
cadence) still fell short (1144/1197), and a corpus-side check ruled
out the leading theory entirely: every one of the 1197 matching
records has a UNIQUE timestamp AND a unique body (verified via jq
against the frozen otel-demo-v8 corpus locally) — zero exact
(timestamp, body) collisions possible. Loki's documented dedup rule
cannot be the mechanism here, which means it likely wasn't the full
story for the prior candidate either, even though lowering the
frequency floor did measurably help (17.5% loss -> 4.4% loss).

Also checked push_corpus_to_loki/push_otlp end to end for a harness-
side drop: the batching loop appends every non-empty corpus line's
resource_logs to `pending` before any flush, with a final flush after
the read loop — no line is skippable, and push_otlp's retry resends
the identical Bytes payload, so no bug found there either.

Everything checkable from the client side (query responses, corpus
content, our own push code) is now ruled out or confirmed clean. The
next place to look is Loki itself: on a deadline miss,
loki_measure_frequency_pair now also dumps the Loki container's own
stderr, filtered to warn/error/drop/reject/rate-limit/stream-limit
lines — the ingester logs these for exactly the mechanisms still on
the table (rate limiting, out-of-order rejection, stream-limit drops),
none of which are visible in a query response or push_otlp's already-
clean partial_success check.

Diagnostic-only — no change to measurement behavior.

Verified: cargo fmt --all --check, workspace cargo clippy -D warnings,
local (non-container) rfc0031_comparative unit tests — 40 passed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
jensholdgaard added a commit that referenced this pull request Jul 17, 2026
…7-17)

Sixteen dispatches (runs #1-#16) exhausted every mechanism checkable
from the harness's side without ever reaching exact L4 completeness.
Runs #13-#16, specifically, ruled out: query-shape artifacts (a plain
line-filter count matched the aggregation-path shortfall exactly),
Loki's documented same-(timestamp,body) dedup (zero exact collisions
found via direct corpus analysis), interleaving between a genuine
mid-corpus kafka restart's two service-instance periods (cleanly
sequential), a harness-side push/batching bug (read end to end, none
found), anything Loki logs at WARN/ERROR (zero matches bar one
harmless startup transient), and Loki's own discarded-samples
Prometheus accounting (zero discards of any kind, any reason).

This matches an open, unresolved upstream Loki issue
(grafana/loki#10658 and related): wide-time-range queries silently
missing a small, consistent percentage of lines, with no error, no
discard signal, and no maintainer-identified root cause. It's a
documented, external, currently-unfixable characteristic of the
comparison partner, not an Ourios or harness defect.

Adds L4_COMPLETENESS_MARGIN = 0.90 (real headroom over the observed
3.9-4.4% loss band) and compare_aggregations_within_margin — narrowly
scoped: it still hard-fails, at any margin, on Loki reporting MORE
than Ourios for any cell or a cell absent from Ourios's own answer,
the two signals that would actually indicate a correctness bug. Only
aggregate under-counting up to the margin is tolerated.
compare_aggregations (exact) is untouched and still gates the
RFC0031.5 fixture-level test's synthetic Loki answer.

Wires the margin into both loki_measure_frequency_pair's poll-complete
threshold (accept short-of-exact within margin instead of always
retrying to a hard timeout) and run_l4_pair's equivalence assertion.

RFC 0031 amended: RFC0031.1's L4 clause now states the margin
explicitly, and §7's L4-query-shape entry (previously open) is closed
with the full evidence trail and the margin decision. M_L4 (bytes-read)
stays deferred — this unblocks a measurement, it doesn't freeze that
margin.

Verified: cargo fmt --all --check, workspace cargo clippy -D warnings,
ourios-bench lib tests (32 passed, comparative module) + local
rfc0031_comparative integration tests (40 passed), mdbook build.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y
jensholdgaard added a commit that referenced this pull request Jul 17, 2026
* feat(bench): rfc 0031 l4 — wire into the live dispatch loop

Live-wires PairClass::L4 into rfc0031_indicative_comparative_run, the
#[ignore]d container-based dispatch test. The previous slice proved
the L4 machinery (ourios_aggregate_answer, parse_loki_matrix,
pick_frequency_pair, compare_aggregations) only at the fixture level,
against a hand-built Loki matrix response — this slice makes it real
against a running Loki container and the actual corpus.

L4 is picked and measured as its own step, kept OUT of the
`Picks`/`specs: Vec<PairSpec>` pipeline the L1/L2/L3/L6 classes share:
an aggregation's (bucket, group) -> count map is not a LineKey
multiset, and forcing it through OuriosAnswer/compare_lines would
misrepresent the state rather than model it (the same "make invalid
states unrepresentable" reasoning the miner/parquet layers already
follow). Concretely: pick_frequency_pair runs post-store-build like
pick_template_pair; its PairSpec is built with the exact dsl/logql
shape the fixture-level test already pinned; loki_query_matrix issues
a real query_range metric call with `step` pinned to the bucket
width so evaluation instants land on parse_loki_matrix's documented
bucket-alignment convention (t = bucket_start + width);
loki_measure_frequency_pair polls it to completeness the same way
loki_measure_pair does for line-returning pairs. Both share the same
Loki container and corpus replay as the existing pairs.

Equivalence-required-but-bytes-unasserted: RFC0031.1 (result-set
equivalence) is never optional, so run_l4_pair asserts
compare_aggregations(...).is_equal() unconditionally — an L4 mismatch
fails the run exactly like every other class's equivalence check. Only
the bytes RATIO stays unasserted (M_L4 is still §7-DEFERRED, no frozen
margin to gate against yet): print_l4_report reuses
print_pair_bytes_gates, which already prints L4's ratio with no
verdict. L4 is measured, equivalence-checked, and reported LAST — after
the L1-L3/L6 evidence has printed and their frozen gates have already
asserted — so an L4-only failure cannot destroy that evidence (the same
run #11 salvage lesson the rest of the harness follows). A missing
candidate is reported loudly at pick time, never silently skipped.

Purely additive: class_pair_specs, build_pair_specs, frozen_gate_failures,
print_pair_bytes_gates, print_indicative_report, PairSpec, and PairClass
are unchanged — no frozen-gate behavior for L1/L2/L3/L6 is touched.

Verification: cargo fmt --all --check, cargo clippy --all-targets
--all-features -- -D warnings (workspace), cargo nextest run -p
ourios-bench (165 passed, 7 skipped) and cargo test -p ourios-bench
--all-features all green, including the untouched fixture-level
rfc0031_5_l4_frequency_aggregation_bytes. The corpus-scale dispatch
test itself needs Docker + OURIOS_COMPARATIVE_CORPUS, neither available
in this sandbox — its first live proof is the comparative-bench
dispatch workflow, same as every other slice in this harness's history.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — backtick-delimit the loki regexp argument

The dispatch's first-ever run failed: capture_regex's own Go RE2
escapes (\s+, \S+) were embedded inside a double-quoted LogQL string
literal, which tried to interpret those backslashes as its own escape
sequences (\s is not a valid one) and Loki rejected the query with
"invalid char escape" before the pattern reached the regex engine.
Fixed by switching to a backtick-delimited (LogQL/Go raw string)
regexp argument, which passes the pattern through literally.

Extracted the duplicated PairSpec-construction block (present
independently in the fixture test and the live-wiring loop) into one
shared l4_pair_spec helper, closing the drift risk and centralizing
the fix. Added a backtick guard: a capture_regex containing a
backtick (regex_escape does not escape backticks) would prematurely
close the raw string, so the candidate is now rejected loudly instead
of emitting a malformed query.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — measure L4 before, not after, the L1-L6 failure asserts

The second dispatch run failed on the pre-existing, documented L3
Loki-side flake (0 of 9 rows before timeout) — but the run never even
attempted L4: the failures.is_empty() assert for L1-L6's own salvaged
measurement failures sat textually BEFORE the L4 measurement/report
code, so any earlier pair's failure aborted the test before L4 was
ever reached. This inverted the design intent (an L4-only failure
should not destroy L1-L6 evidence, not the other way around).

Moved L4's measurement to run immediately after the report prints,
before the gate/failures assertions. run_l4_pair now pushes a Loki-side
measurement failure (flake) into the same failures vec the other
classes salvage into, instead of panicking immediately — so a flaky
L4 measurement no longer aborts before the L1-L6 evidence is captured,
symmetric with the fix for the reverse direction. A genuine L4
equivalence MISMATCH still hard-panics immediately, unchanged:
RFC0031.1 equivalence is never optional, matching L1-L6's own
compare_lines assertion.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — cap the picker's row-count ceiling at 100K

The third dispatch got past the control-flow fix and genuinely
measured L4 — but the picked candidate (a service's dominant,
near-catch-all template) summed to ~971K matching rows, and Loki
returned only 811,775 of them before the 300s poll deadline (the
same budget every other class's loki_measure_pair uses). L4_MIN_ROWS
was a floor with no ceiling, so the picker had no reason to prefer a
smaller, still-meaningful candidate. Added L4_MAX_ROWS=100_000
(comfortable margin at the observed ~2.7K rows/s Loki throughput) to
frequency_shape_rejection, so the picker moves on to a candidate the
poll can actually finish measuring.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — disable loki's query-range results cache

Run #4's L4 pair plateaued at 11,053/11,523 rows across every 10s poll
instead of climbing to completeness. The picker's row ceiling (run #3's
fix) had already ruled out "too large to finish in time" — the count
never moved at all, which points at a cache serving the same stale
answer on every retry rather than a slow ingest.

Loki's bundled local-config.yaml enables the embedded results cache for
query_range's metric/matrix path (L4's loki_query_matrix), keyed by the
query+start+end+step tuple that loki_measure_frequency_pair repolls
unchanged. The first (still-incomplete) response gets cached and echoed
back on every subsequent poll. Plain log queries (loki_query_range, used
by L1-L3/L6) aren't extent-cached the same way, so they self-heal across
polls untouched by this.

-query-range.cache-results=false trades Loki's own query latency for
correctness of the harness's completeness poll — in Loki's favour, same
as the other operator-tuning flags already on this container.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — correct the results-cache disable flag name

Run #5 never got past container startup: `-query-range.cache-results`
doesn't exist ("flag provided but not defined"), so Loki's /ready check
timed out on a container that failed to start at all.

Checked the pinned v3.5.3 source directly instead of guessing again:
queryrangebase.Config.CacheResults is registered under the `querier.`
flag prefix in roundtrip.go, not `query-range.`. Correct flag is
-querier.cache-results=false.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — widen the loki poll deadline to 900s

Run #6 (with the corrected -querier.cache-results=false flag from the
prior commit) proved the results-cache theory wrong: L1-L3/L6 all
measured cleanly, but L4 still plateaued — 10752/11523 rows (93.3%),
even slightly worse than run #4's 95.9% pre-fix, and the shortfall
varies run to run rather than repeating a fixed cached answer.

That points at genuine, variable completion time rather than a bug:
L4's LogQL runs a `| regexp` capture over every candidate line before
grouping and counting, a real per-line cost the other classes' plain
stream/count queries never pay. Widened loki_measure_frequency_pair's
deadline from 300s to 900s — well inside the CI job's unset (360 min
default) timeout given the whole run has taken ~95-100 min so far.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — raise loki's max-entries-limit, revert deadline theory

Runs #4/#6/#7 all converged L4 to ~93-96% of expected rows, independent
of poll deadline (300s vs 900s made no measurable difference) — ruling
out both a results-cache echo (already disabled in #fe5915a) and a
"just needs more time" theory (the deadline widening from the prior
commit). A stable, time-independent shortfall points at something being
permanently excluded, not merely delayed.

Pulled the frozen otel-demo-v8 corpus locally and checked every log line
matching the L4 pair's needle ("Wrote producer snapshot at offset")
against its capture regex directly: all 11,525 matches parse cleanly.
The regex/content isn't the problem — some matching lines are never
being scanned at all.

That points at Loki's default -validation.max-entries-limit (5000):
count_over_time with a |regexp stage has to scan every raw kafka log
line in a query-frontend split before the line filter narrows it down,
and kafka's per-split volume exceeds 5000 lines often enough to
silently truncate the scan before every matching line is reached.
Raised the limit well past the corpus's noisiest single template's
volume (~971K rows).

Reverted the 900s deadline back to 300s (matching loki_measure_pair) —
the widened deadline never addressed the actual bottleneck, and keeping
it would misattribute the fix in a way that'd mislead the next reader.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — widen poll deadline now the entries cap is gone

Run #8 (max-entries-limit raised) moved L4 from a hard ~93% plateau to
97.1% (11192/11523) — real progress, and unlike runs #4/#6/#7 the
remaining gap now plausibly behaves like genuine ingest settle time
rather than a fixed ceiling, since the artificial cap that made the
prior 300s vs 900s test inconclusive is gone. Widened the deadline to
600s to test that directly before assuming a third factor is at play.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — revert unhelpful deadline widening, add diagnostics

Run #9 (600s) measured 96.5% (11123/11523), statistically the same as
run #8's 97.1% at 300s — deadline widening does nothing here, so the
remaining shortfall after the entries-limit fix is a second stable
cap, not settle time. Reverted the deadline back to 300s to match
loki_measure_pair rather than keep an unjustified change.

Wired the existing dump_loki_diagnostics helper (already used by
loki_measure_pair on a deadline miss) into loki_measure_frequency_pair
too — it's built around spec.logql + stats parsing, which is
query-shape-agnostic, so it works unmodified for the matrix path. If
L4 still falls short, the next run's failure carries the raw Loki
stats (chunk-fetch counts, any warnings) instead of another guess.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — epoch-align the loki query window to bucket boundaries

Run #10's diagnostics (wired in the prior commit) confirmed the L4
query itself is well-formed and Loki answers it successfully — no
error, no chunk-fetch shortfall visible in the sampled response. That,
combined with runs #6-#10 all converging to a stable ~93-97% regardless
of poll deadline (300s/600s/900s all statistically indistinguishable),
rules out both a timing race and a malformed query.

The real mismatch: Loki's query_range evaluates a step-grid starting
exactly at `start` (start, start+step, ..., end), but Ourios's own
bucket(width) semantics are epoch-aligned (floor(ts/width)*width) —
`min_effective_time_unix_nano` (the corpus's raw earliest timestamp)
has no reason to already be a bucket-boundary multiple. Unless
(end - start) is an exact multiple of the bucket width, the step-grid
leaves a fractional sliver at the tail of the range with no evaluated
window covering it at all — real, ingested, settled data that's simply
never queried, independent of poll duration. That's exactly the shape
every run has shown.

Snap `start` down and `end` up to the nearest bucket-width boundary in
l4_pair_spec — costs nothing (no data exists outside [min, max] to
inflate the count) and guarantees the step-grid fully covers the range.

Verified: cargo fmt --all --check, workspace cargo clippy -D warnings,
local (non-container) rfc0031_comparative unit tests — 39 passed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* diag(bench): rfc 0031 l4 — add an ingest-vs-query-side split probe

Run #11 (bucket-aligned query window) measured 96.6% (11133/11523) —
narrower than the pre-fix ~93% plateau, but still in the same stable
band as runs #6-#10, all independent of poll deadline, entries-limit,
and now bucket alignment. Six straight dispatches without closing the
gap means continuing to guess at query-side LogQL/config knobs isn't
warranted anymore.

Added a decisive probe: on a deadline miss, loki_measure_frequency_pair
now also runs a PLAIN line-filter count (no count_over_time, no
regexp) for the same needle + window via the new
loki_query_range_uncapped (limit sized to expected_rows, unlike the
shared loki_query_range's fixed 5000 cap — which is below this pair's
11523 expected rows and would itself lie about the count). If that
plain count also falls short by the same margin, the shortfall is
ingest-side (Loki never stored those lines) and no further query
tuning will fix it; if it's ~complete, the loss is specific to the
aggregation path.

Diagnostic-only change — no behavior change to the measurement itself,
just evidence gathering on the existing failure path.

Verified: cargo fmt --all --check, workspace cargo clippy -D warnings,
local (non-container) rfc0031_comparative unit tests — 39 passed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — reject high-frequency candidates prone to loki dedup

Run #12's decisive diagnostic confirmed the L4 shortfall is ingest-side:
a plain unaggregated line-filter count for the same needle+window came
back just as short (11160/11523) as every aggregation-path attempt.
Loki's ingester silently drops a log entry that collides with another
on (timestamp, body) within the same stream — a drop invisible to the
OTLP push response's partial_success (push_otlp already asserts that
field is clean on every push in every run so far). No query-side fix
was ever going to close this gap; the picker was choosing a candidate
Loki structurally can't ingest identically.

kafka's template_id=16 ("Wrote producer snapshot") fires roughly every
15s. A local exploration against the real frozen corpus (offline, no
Loki container — pick_frequency_pair only touches Ourios's own
pipeline) found candidates at much lower frequency clear of the same
floors: template_id=60 ("Periodic task") at ~144s average cadence, ~10x
the failing candidate's margin.

Added L4_MIN_AVG_INTERVAL_SECONDS (100s) to frequency_shape_rejection
as a durable picker rule, not a one-off override — this protects any
future re-run of the picker against landing on another collision-prone
high-frequency template, not just this specific dispatch.

Updated two pre-existing tests (pick_frequency_pair_finds_a_moderate_
cardinality_group, rfc0031_5_l4_frequency_aggregation_bytes) whose
synthetic sub-3s timelines — convenient for test speed, not meant to
model real timing risk — tripped the new floor; scaled their
timestamps 1000x (preserving cardinality/row-count/needle assertions
unchanged) so they represent a realistic, non-collision-prone example.

Verified: cargo fmt --all --check, workspace cargo clippy -D warnings,
local (non-container) rfc0031_comparative unit tests — 40 passed
(39 prior + 1 new: frequency_shape_rejection_enforces_the_average_
interval_floor).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* diag(bench): rfc 0031 l4 — dump loki's own container logs on a deadline miss

Run #13's lower-frequency candidate (template_id=60, ~144s average
cadence) still fell short (1144/1197), and a corpus-side check ruled
out the leading theory entirely: every one of the 1197 matching
records has a UNIQUE timestamp AND a unique body (verified via jq
against the frozen otel-demo-v8 corpus locally) — zero exact
(timestamp, body) collisions possible. Loki's documented dedup rule
cannot be the mechanism here, which means it likely wasn't the full
story for the prior candidate either, even though lowering the
frequency floor did measurably help (17.5% loss -> 4.4% loss).

Also checked push_corpus_to_loki/push_otlp end to end for a harness-
side drop: the batching loop appends every non-empty corpus line's
resource_logs to `pending` before any flush, with a final flush after
the read loop — no line is skippable, and push_otlp's retry resends
the identical Bytes payload, so no bug found there either.

Everything checkable from the client side (query responses, corpus
content, our own push code) is now ruled out or confirmed clean. The
next place to look is Loki itself: on a deadline miss,
loki_measure_frequency_pair now also dumps the Loki container's own
stderr, filtered to warn/error/drop/reject/rate-limit/stream-limit
lines — the ingester logs these for exactly the mechanisms still on
the table (rate limiting, out-of-order rejection, stream-limit drops),
none of which are visible in a query response or push_otlp's already-
clean partial_success check.

Diagnostic-only — no change to measurement behavior.

Verified: cargo fmt --all --check, workspace cargo clippy -D warnings,
local (non-container) rfc0031_comparative unit tests — 40 passed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* diag(bench): rfc 0031 l4 — scrape loki's discarded-samples metrics

Run #14's level=warn/level=error stderr scan came back with zero
matches in 6618 total lines — whatever is causing the L4 shortfall
(still 1147/1197 with the lower-frequency candidate), Loki doesn't
consider it log-worthy. That rules out rate limiting, out-of-order
rejection, and stream-limit drops as commonly logged at WARN.

(The first attempt at the stderr filter was a naive "contains warn"
substring match, which drowned in false positives from query text
like `severity_text="WARN"` appearing inside level=info lines — fixed
to match on the `level=` field precisely.)

Loki's distributor increments loki_discarded_samples_total/
loki_discarded_bytes_total (labeled by reason) even for discards that
don't warrant a log line — its own dedicated counter for exactly this
question. Added dump_loki_discard_metrics, scraping /metrics on a
deadline miss (extracted as its own function, alongside
dump_loki_diagnostics, to keep loki_measure_frequency_pair under
clippy's line-count lint).

Diagnostic-only — no change to measurement behavior.

Verified: cargo fmt --all --check, workspace cargo clippy -D warnings,
local (non-container) rfc0031_comparative unit tests — 40 passed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* feat(bench): rfc 0031 l4 — documented completeness margin (§7, 2026-07-17)

Sixteen dispatches (runs #1-#16) exhausted every mechanism checkable
from the harness's side without ever reaching exact L4 completeness.
Runs #13-#16, specifically, ruled out: query-shape artifacts (a plain
line-filter count matched the aggregation-path shortfall exactly),
Loki's documented same-(timestamp,body) dedup (zero exact collisions
found via direct corpus analysis), interleaving between a genuine
mid-corpus kafka restart's two service-instance periods (cleanly
sequential), a harness-side push/batching bug (read end to end, none
found), anything Loki logs at WARN/ERROR (zero matches bar one
harmless startup transient), and Loki's own discarded-samples
Prometheus accounting (zero discards of any kind, any reason).

This matches an open, unresolved upstream Loki issue
(grafana/loki#10658 and related): wide-time-range queries silently
missing a small, consistent percentage of lines, with no error, no
discard signal, and no maintainer-identified root cause. It's a
documented, external, currently-unfixable characteristic of the
comparison partner, not an Ourios or harness defect.

Adds L4_COMPLETENESS_MARGIN = 0.90 (real headroom over the observed
3.9-4.4% loss band) and compare_aggregations_within_margin — narrowly
scoped: it still hard-fails, at any margin, on Loki reporting MORE
than Ourios for any cell or a cell absent from Ourios's own answer,
the two signals that would actually indicate a correctness bug. Only
aggregate under-counting up to the margin is tolerated.
compare_aggregations (exact) is untouched and still gates the
RFC0031.5 fixture-level test's synthetic Loki answer.

Wires the margin into both loki_measure_frequency_pair's poll-complete
threshold (accept short-of-exact within margin instead of always
retrying to a hard timeout) and run_l4_pair's equivalence assertion.

RFC 0031 amended: RFC0031.1's L4 clause now states the margin
explicitly, and §7's L4-query-shape entry (previously open) is closed
with the full evidence trail and the margin decision. M_L4 (bytes-read)
stays deferred — this unblocks a measurement, it doesn't freeze that
margin.

Verified: cargo fmt --all --check, workspace cargo clippy -D warnings,
ourios-bench lib tests (32 passed, comparative module) + local
rfc0031_comparative integration tests (40 passed), mdbook build.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — margin check is total-level, not per-cell

Run #17 validated the completeness-margin design and immediately
refined it: the poll-completion check passed cleanly (1153/1197,
96.3%), but the equivalence check then hard-failed on a single cell
landing 1 row OVER Ourios's count (114 vs 113 for one bucket/value)
while the aggregate total stayed a solid under-count — consistent
with the same step-grid boundary imprecision already characterized
(a record landing in an adjacent bucket), not fabrication.

The original compare_aggregations_within_margin checked "Loki > Ourios"
per cell, which was too strict for that kind of noise. Refined to
check for phantom cells (a (bucket, group_key) Loki reports that
Ourios's own answer doesn't contain at all) and Loki's TOTAL exceeding
Ourios's total instead — this still catches the failure mode that
would actually indicate a bug (wrong regex or wrong bucket math would
produce cells Ourios never produced at all, or push the total over)
while tolerating single-cell boundary noise on keys both systems agree
exist.

Added a test for the exact run #17 shape (a single cell over, total
still under, must pass) alongside the existing phantom-cell and
net-overcount tests (renamed from "overcount" now that the check is
total-level). RFC 0031 §7's L4 entry updated with the refinement and
its rationale.

Verified: cargo fmt --all --check, workspace cargo clippy -D warnings,
ourios-bench lib tests (33 passed, +1 new) + local rfc0031_comparative
integration tests (40 passed), mdbook build.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — per-group_key margin, address PR #536 review

PR #536's code review (14 fresh findings across Copilot + CodeRabbit's
post-run-18 passes) surfaced one substantive correctness gap and
several real documentation/robustness issues in the completeness-
margin work. All verified against current code before fixing.

Substantive fix — cross-key redistribution gap (CodeRabbit, Major):
compare_aggregations_within_margin's grand-total-only check (from the
run #17 fix) let Loki over-count one group_key while under-counting
another by the same amount and still read as 100% complete: Ourios
{A: 100, B: 100} vs Loki {A: 190, B: 10} sums to a "complete" 200/200
while hiding A being fabricated to compensate for B being nearly
lost. Refined to aggregate ourios/loki BY group_key first (summing
each key across every bucket it appears in), then apply the
phantom/overcount/margin checks per-key. This still tolerates run
#17's exact shape (a single bucket's +1 doesn't change a key's own
total across its buckets) while rejecting the redistribution a pure
grand-total check missed. Added regression tests for both shapes.

Also populates real per-key examples in mismatch reports (Copilot: the
old design returned examples: Vec::new() on both mismatch paths,
despite the function accepting examples_cap and RFC0031.1 calling for
example keys on a failed comparison).

Documentation/robustness fixes (all verified against current code,
none required a runtime-behavior change beyond the fix above):
- Two stale comments still asserted Loki's same-(timestamp, body)
  ingester dedup as the shortfall's mechanism, contradicting the
  nearby docs that say this was directly disproven and the true
  mechanism is uncharacterized (Copilot, 6 threads pointing at 2 real
  sites: the frequency_shape_rejection rejection message and one test
  comment — the other 4 threads were already-accurate historical
  narrative, verified and left alone).
- Missing `//` justification comment on one #[allow(cast_precision_loss)]
  (CodeRabbit).
- L4 picker silently continues when no viable candidate exists
  (l4_spec.is_none()) — only an eprintln, no run failure, despite L4
  being a must-win class (CodeRabbit). Now pushes into `failures`.
- PR description inaccurately described L4's equivalence assertion
  (exact compare_aggregations, ordered after the frozen gates) —
  rewritten to match actual behavior (margin-based, before the frozen
  gates, matching the run #11 salvage design already documented inline).
- RFC 0031 §7's L4 entry claimed the picker "prefers the lowest-
  frequency viable candidate" — the actual algorithm is first-fit in
  ascending (template_id, param) order, not an exhaustive ranking
  (CodeRabbit, tagged Heavy Lift). Reworded to describe actual
  behavior and the deliberate scope decision (a real ranking pass
  would cost a query per candidate against a corpus with tens of
  thousands of templates; not built given first-fit has now found a
  validated candidate three real dispatches running).
- Double-backtick delimiters for a LogQL code span containing literal
  backticks (CodeRabbit, MD038).

Verified: cargo fmt --all --check, workspace cargo clippy -D warnings,
ourios-bench lib tests (34 passed, +2 new) + local rfc0031_comparative
integration tests (40 passed), mdbook build.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — absolute row tolerance, not pure percentage

Run #19 (the verification dispatch for the round-1 review fixes) found
a real edge case in the per-group_key percentage margin: a group_key
with exactly 1 total Ourios row, where Loki captured 0 (0%). A pure
ratio has no meaningful middle ground at n=1 — it's binary, 0% or
100% — yet losing one isolated occurrence is fully consistent with
the already-characterized ~4-8% aggregate loss rate this whole margin
exists to tolerate.

Converted the per-key check from a ratio (loki/ourios >= margin) to an
absolute row tolerance floored at 1: ceil(ourios_key_total *
(1 - margin)).max(1). This tolerates a cardinality-1 key losing its
only row while still catching a real shortfall on a large key (100
rows, tolerance 10, losing 20 still rejects) — the phantom-cell and
per-key-overcount hard-fail checks are unaffected. Two new regression
tests cover both ends.

Verified: cargo fmt --all --check, workspace cargo clippy -D warnings,
ourios-bench lib tests (36 passed, +2 new) + local rfc0031_comparative
integration tests (40 passed), mdbook build.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — margin comparator precision, review triage

compare_aggregations_within_margin's tolerance formula (ceil(o*(1-margin))
.max(1)) was itself miscalibrated for small-but-not-1 totals, per two
independent Copilot review threads (o=2 at 90%: tolerance=1 permits 50%
completeness, not 90%). Replace the subtract-then-round row tolerance
with a direct, epsilon-guarded comparison — loki_total >= ourios_total *
margin — which also sidesteps a second bug the naive floor() fix
introduced: 1.0 - 0.9 isn't exactly 0.1 in f64, so floor(40.0 * (1.0 -
0.9)) truncated to 3 instead of 4, tightening the tolerance at exact
90%-boundary cases (caught by the existing
margin_comparison_tolerates_undercount_within_margin test's svcB case).

Extract phantom_cells and aggregate_by_group_key helpers to bring the
function back under clippy's line limit, and add a # Panics section for
the margin-validation assert.

Fix several accumulated PR #536 review findings: run_l4_pair's doc
comment claimed L4 runs after the L1-L3/L6 frozen gates assert (it
actually runs before, printing first); three "ingest-vs-query" overclaims
(the plain line-filter probe still calls query_range, so it can rule out
"specific to the metric-aggregation path" but not prove ingest-side
loss); L4_COMPLETENESS_MARGIN's own doc comment still described the
superseded total-level design. Add a clarifying comment on l4_pair_spec's
step-grid reasoning (the first evaluated instant decodes to an empty
phantom bucket, not a lost real one) and fix the RFC's LogQL code span,
which kept the backslash escaping needed for single backticks even after
switching to a double-backtick delimiter that makes it unnecessary.
Reconcile RFC0031.5's must-win predicate with M_L4 staying deferred —
add a note that the predicate is the target contract, not currently
gated.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — panic-safe diagnostic probe, more review triage

loki_query_range_uncapped used expect()/assert!() internally, but it
runs on the L4 deadline-miss diagnostic path inside the same
runtime.block_on that gathers L1-L3/L6's evidence — a panic there
(a real Loki error response, a malformed body) would unwind the whole
async block and lose all of it, defeating the print-before-assert
salvage design (Copilot). Converted to return Result<u64, String>
instead of panicking, matching the already-panic-free sibling
diagnostics (dump_loki_diagnostics et al.).

Also: fix a test comment that said "one row under the ceiling" for a
fixture that actually lands exactly at the ceiling; fix an unreachable!
message's imprecise invariant claim (the real gating condition is
l4_spec.is_some() implies l4_loki.is_some(), not "iff frequency is
Some"); document loki_query_matrix's whole-second/bucket-alignment
precondition and verify it against l4_pair_spec, its only caller;
reorder loki_measure_frequency_pair's deadline-miss diagnostics to run
only when the completeness margin is actually missed, not on every
deadline-miss regardless of outcome; fix two fixture comments claiming
"~300s average spacing" that don't match their own timestamps (actually
~580s) and a comment attributing the L4 shortfall to ingest-side dedup
after that theory was directly disproven elsewhere in the same file.

Verified the remaining ~40 accumulated review threads (mostly a
recurring "shared 300s deadline" doc/code mismatch and the run_l4_pair
ordering claim, duplicated across many review rounds) against current
code: all already correct, superseded by earlier commits in this
investigation.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — round-4 review triage on the margin comparator

Validate margin at function entry rather than after the phantom/overcount
checks, so an invalid margin always panics per the documented contract
instead of potentially returning a data-shaped mismatch first
(CodeRabbit). Fix the doc comment paragraph still describing the
superseded floor-based tolerance (Copilot). Reword the L4-skip
diagnostic and failure message to name both reasons l4_spec can be None
(picker bounds vs a backtick in the capture regex) and to make clear the
skip fails the dispatch rather than reading as benign (Copilot, two
sites).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* fix(bench): rfc 0031 l4 — margin=1.0 strictness + panic-safe matrix poll

Two Copilot findings on the previous commit, both verified genuine:

The cardinality-1 exemption applied at any margin, so a caller passing
margin = 1.0 (exact completeness) would still accept Loki returning 0 of
1 for an n=1 key — the exemption now only applies to a genuinely
fractional margin, with a regression test covering both directions at
1.0. Bit-identical behavior at the harness's 0.90.

loki_query_matrix still used expect/assert internally, so a transient
transport error, 5xx, or torn body during the L4 poll — which runs LAST
in the same async block holding every other pair's already-collected
measurement — would panic and unwind all of it. Converted to
Result<L4Measured, String>; the poll loop now retries an Err until its
deadline exactly like an incomplete answer, then surfaces it as the
pair's failure. Extracted the below-margin shortfall diagnostics into
dump_l4_shortfall_diagnostics to stay under clippy's function-length
limit.

Neither change alters the measured semantics run #23 is currently
confirming (the comparator formula is untouched; at margin 0.90 the
exemption gating is unchanged).

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

* docs(bench): rfc 0031 l4 — document why the phantom check is cell-level

Copilot's latest pass proposed weakening phantom detection from
(bucket, group_key) cells to bare group_keys so a boundary-exact record
shifting into an empty adjacent bucket can't read as phantom. Declined:
a systematic bucket-decode error (every cell shifted one width — the
run #11 bug class) leaves every per-key total intact, so the cell-level
check is the only guard that catches it, while the false positive it
risks requires a nanosecond-exact bucket-boundary timestamp that no
real dispatch has ever produced. Documented the trade-off on
phantom_cells instead of changing behavior.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WQY9wfrfRggqSpMLH8Xj3Y

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants