Skip to content

docs(rfc-0001): fill in drafted-bar content for the template miner - #5

Merged
jensholdgaard merged 4 commits into
mainfrom
docs/rfc-0001-drafted-content
May 2, 2026
Merged

docs(rfc-0001): fill in drafted-bar content for the template miner#5
jensholdgaard merged 4 commits into
mainfrom
docs/rfc-0001-drafted-content

Conversation

@jensholdgaard

Copy link
Copy Markdown
Owner

Summary

Replaces the scaffold prompts in §§1–4, §6, and §7 of RFC 0001 with committed prose, so the RFC actually meets the Drafted gate from docs/verification.md §3 ("§§1–4 and §§7–8 filled enough that two engineers reading would produce roughly the same implementation"). Status stays drafted; the follow-up PR adds §5 Acceptance criteria and moves it to specified.

§5 gap table and §8 testing strategy are unchanged in substance. §9 is restructured around questions that genuinely remain (persistence parameters, corpus-dependent tuning, edge cases, deferred follow-ups). §10 picks up the hazards / benchmarks / RFC-0002 cross-refs the body now asserts.

Five load-bearing commitments

Each is a real architectural decision; rationale lives in the body.

§ Commitment Why
6.1 Template identity is a per-tenant monotonic u64 Preserves CLAUDE.md §3.7 by construction; keeps (template_id, template_version) as a meaningful compound key. Content hash would make identity global, which is a tenant-isolation leak dressed up as a feature.
6.3 Confidence is simSeq / threshold Decision boundary lands at 1.0 across tenants regardless of configured threshold, so confidence_p50 / confidence_p01 mean what their names say.
4.2 Drain3 pre-tree-walk masking adopted, with masked tokens becoming typed parameters Without the typed-param modification, masking discards the original bytes and §3.3 reconstruction is impossible.
6.6 Always capture every inter-token separator into a parallel array; lossy_flag set only on tokenizer failure Removes the H7 failure mode where a "is this whitespace trivial" heuristic decides whether to lie to the user.
6.9 Persistence: in-memory + WAL replay + periodic snapshots (direction only; target store, cadence, scope deferred to §9) Direction is committed; the parameters are explicitly open.

Invariants and hazards this RFC commits to defend

  • Invariants: [§3.1], [§3.2], [§3.3], [§3.5], [§3.7]. ([§3.4] WAL durability is referenced but owned by a future RFC; [§3.6] object-storage-as-truth is referenced but doesn't change here.)
  • Hazards: H1 (template miner correctness), H2 (parameter cardinality blowup), H5 (template schema evolution), H7 (bit-identical body reconstruction).

These are the scenario sources the next PR will turn into §5 Acceptance criteria (H1.x, H2.x, H5.x, H7.x, plus invariant-rooted §3.x.y and RFC0001.x for design-internal scenarios).

Out of scope for this PR

  • §5 Acceptance criteria (next PR — moves status draftedspecified).
  • Test stubs in crates/ourios-miner/ (the PR after that — moves to red).
  • Snapshot target/cadence/scope decisions (deferred in §9; concrete numbers come with the snapshot RFC or with ourios-wal's landing).

Related

  • RFC: docs/rfcs/0001-template-miner.md
  • Process: docs/verification.md §3 (Drafted gate), §6 (the worked example this RFC is the substrate for)
  • Hazards: docs/hazards.md H1, H2, H5, H7
  • Benchmarks: docs/benchmarks.md C1, C2, C3, C4, D1, E1, E2
  • Cross-RFC: docs/rfcs/0002-query-dsl.md §5.4 (template primitives in the DSL surface)

Checklist

  • cargo fmt --all --check clean
  • cargo clippy --all-targets --all-features -- -D warnings clean
  • cargo test --all-features clean (no tests yet)
  • mdbook build clean
  • Docs updated (this PR is docs-only)
  • RFC linked
  • CHANGELOG.md — intentionally not updated; per the project's conventional-commits + git-cliff setup, the changelog is regenerated at release time, not per-PR

🤖 Generated with Claude Code

Replaces the scaffold prompts in §§1-4, §6, and §7 with committed
prose so RFC 0001 actually meets the Drafted gate from
verification.md §3 ("two engineers reading would produce roughly
the same implementation"). §5 gap table and §8 testing strategy
are unchanged in substance; §9 is restructured around questions
that genuinely remain (persistence parameters, corpus-dependent
tuning, edge cases, deferred follow-ups); §10 picks up the
hazards / benchmarks / RFC-0002 cross-refs the body now asserts.

Five load-bearing commitments, with rationale in the body:

- Template identity (§6.1): per-tenant monotonic u64. Preserves
  CLAUDE.md §3.7 by construction and keeps
  (template_id, template_version) as a meaningful compound key.
- Confidence metric (§6.3): simSeq / threshold ratio. Decision
  boundary lands at 1.0 across tenants regardless of configured
  threshold, so confidence_p50 / confidence_p01 mean what the
  names say.
- Drain3 inheritance (§4.2): pre-tree-walk masking adopted, with
  the modification that masked tokens become typed parameters.
  This is what makes §3.3 reconstruction possible at all.
- Body reconstruction (§6.6): always capture every inter-token
  separator into a parallel array; lossy_flag is set only on
  tokenizer failure, never on a fuzzy "is this whitespace
  trivial" heuristic. Removes the H7 failure mode.
- Persistence (§6.9): in-memory + WAL replay + periodic snapshots
  (direction only; target store, cadence, and scope deferred to
  §9).

Status remains drafted. The follow-up PR adds §5 Acceptance
criteria and moves status to specified per docs/verification.md §6.

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

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

This PR updates RFC 0001 (“Template miner”) to replace scaffold prompts with committed prose so the RFC meets the “Drafted” maturity gate requirements described in docs/verification.md.

Changes:

  • Rewrites RFC §§1–4 with concrete motivation/background and a worked example of Drain/Drain3 behavior.
  • Expands §6 into a detailed proposed design/spec (data model, algorithm sketch, confidence model, reconstruction, telemetry, persistence).
  • Updates alternatives, open questions, and references to reflect the now-committed direction and cross-doc links.

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

Comment thread docs/rfcs/0001-template-miner.md Outdated
Comment thread docs/rfcs/0001-template-miner.md Outdated
merges_total.inc()
attach(L_masked, typed_params, separators, candidate,
confidence,
lossy_flag = (confidence < 1.0 ? <see §6.6> : false))
Comment thread docs/rfcs/0001-template-miner.md Outdated
# no leaves under parent yet; create one
leaf = new Leaf(template = L_masked)
parent.leaves.push(leaf)
emit_audit(template_created, ...)
Comment thread docs/rfcs/0001-template-miner.md Outdated
Comment thread docs/rfcs/0001-template-miner.md
Comment thread docs/rfcs/0001-template-miner.md Outdated
Comment thread docs/rfcs/0001-template-miner.md
Comment thread docs/rfcs/0001-template-miner.md
Comment thread docs/rfcs/0001-template-miner.md Outdated
Comment thread docs/rfcs/0001-template-miner.md Outdated
jensholdgaard and others added 2 commits May 2, 2026 13:19
Eight inline review fixes from PR #5:

- §1 intro: stop pointing at §5 as the gap list. §5 is now the
  Acceptance criteria placeholder per docs/rfcs/README.md required
  sections; the gap table moves to §6's intro paragraphs (keeps
  every §6.x cross-reference valid).
- §3.5 worked example: "position 3" was 0-indexed against a
  1-indexed surrounding example; clarify as "token position 4
  (1-indexed)".
- §4.2: "§3.3 reconstruction" -> "[§3.3] reconstruction" to match
  the bracketed-invariant convention used elsewhere.
- §6.1: add `slot_types: Vec<HashSet<ParamType>>` to the leaf so
  the type-expansion path in §6.2 has somewhere to read from.
- §6.2: drop the undefined `emit_audit(template_created, ...)`
  call (template_count already covers creation; §6.4 reserves the
  audit stream for widening events). Make the type-expansion path
  explicit in the algorithm and in the Branching invariants list.
  Enumerate "known separators" as Unicode whitespace only;
  punctuation stays inside tokens for the masking layer.
- §6.2: align the lossy_flag pseudocode comment with §6.6 — set
  only on tokenizer/preprocessing failure, never on confidence
  alone.
- §6.4: add `event_type: AuditEventType` to the schema so the
  §6.7 drift query has a typed field to filter on. Enumerate
  template_widened, template_type_expanded, and
  template_widening_rejected_degenerate. Re-label `merges_total`
  by event_type and explain which event types increment it.

Two open items deferred to the user / team:

- Copilot flagged that docs/hazards.md H1 says "lossy zone -> set
  lossy_flag" while this RFC says lossy_flag is reserved for
  reconstruction failure. Pick which side moves.
- Copilot flagged that CLAUDE.md §3.1 lists confidence_p50 /
  confidence_p01 as mandatory metrics, while §6.8 derives them
  from the confidence histogram. Pick whether to emit them as
  separate series or amend the §3.1 wording.

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

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

Two team decisions from the PR #5 review thread:

A. lossy_flag semantics — RFC §6.6 wording stands; docs/hazards.md
   H1 is the side that moves. The lossy zone retains the body but
   reconstruction still succeeds, so lossy_flag is NOT set. The
   flag is reserved for H7 (genuine tokenizer / preprocessing
   failure where round-trip is impossible). Updated H1's three-zone
   description to reflect this and cross-link RFC §6.6 as the
   precise definition.

B. confidence_p50 / confidence_p01 — RFC §6.8 now emits both as
   explicit gauges in addition to the confidence histogram. The
   histogram remains the source of truth; the gauges are
   convenient named views recomputed on a short ticker (default
   10 s, configurable) and cached between ticks so scrapes never
   block. This matches CLAUDE.md §3.1's literal wording and lets
   alerting rules name the metric directly instead of writing
   histogram_quantile() at every reference.

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

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

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

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


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

# any token matching a rule is replaced with its type
# tag (e.g. <IP>) and the original bytes are pushed
# into typed_params with that tag. Unmasked tokens
# remain literal.
Comment thread docs/hazards.md
(< floor, retain body, increment counter). `lossy_flag` is
reserved for the H7 case (genuine tokenizer / preprocessing
failure where `reconstruct(record) != ingested_bytes` is
possible); it is not a low-confidence signal. See
Comment thread docs/rfcs/0001-template-miner.md Outdated
Comment on lines +660 to +672
**Alias semantics — by-default-broad, version-pinned-on-request.**
A query that says `template_id = X` is interpreted as "rows whose
`template_id == X`, any version" — i.e. follow the alias chain
across all versions. This is the common case (operators usually
mean "show me this template's events," not "show me this exact
structural snapshot of the template"). A query that says
`(template_id, template_version) = (X, V)` returns only rows from
that exact state.

The DSL surface for this is RFC 0002 §5.4 (`template_id.resolves_to(X)`
is the explicit cross-version form; bare `template_id = X` is the
implicit form). The data model in §6.1 supports both shapes
unambiguously.
Three new findings from Copilot's second review pass on the
previous round of fixes:

- RFC §6.2: spell out that widening a literal token into a fresh
  <*> slot has to capture that literal as a STR-typed param,
  otherwise the row breaks §6.1's "one entry per <*> slot"
  invariant and §6.6's reconstruct() has no value to insert.
  Introduce build_params() in the algorithm; document at the
  fresh-leaf branches that params == typed_params there.
  §6.1 picks up a one-line restatement of params.len() ==
  count(<*> in template).

- RFC §6.7: the previous "alias by default broad" wording
  contradicted RFC 0002 §5.4, which keeps `where template_id = X`
  as a literal predicate and exposes alias resolution only via
  `template_id.resolves_to(X)`. Rewrite the section to separate
  the two cross-cutting questions explicitly: cross-version is
  free (template_id is stable across widenings of one leaf;
  template_version distinguishes snapshots), cross-alias requires
  the explicit `.resolves_to()` form. Both RFCs now agree on the
  DSL contract.

- docs/verification.md: H1.2 still asserted lossy_flag = true on
  a lossy-zone match; that contradicts the contract this PR just
  put in place across §6.6 and hazards H1. Update the worked
  example so the next Specified/Red PR derives the right test.

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

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@jensholdgaard
jensholdgaard merged commit dde4864 into main May 2, 2026
7 checks passed
@jensholdgaard
jensholdgaard deleted the docs/rfc-0001-drafted-content branch May 2, 2026 18:54
jensholdgaard added a commit that referenced this pull request Jun 12, 2026
…ry driver (#185)

* docs(rfc-0008,rfc-0001): specify snapshot restore v2 — offset sink, retain floor, driver

RFC 0008 §6.1/§6.6/§6.7 amendment: FrameSink::consume carries the
frame's WalOffset (per-consumer replay horizons — Parquet consumes
frames above the checkpoint X, the miner above its snapshot's
high-water mark S); housekeeping becomes an explicit API taking an
optional retain floor so truncation reclaims only segments wholly
below min(X, S) — the CHECKPOINT sidecar always records the true
Parquet horizon, since capping it would re-feed published records.
New RFC0008.7 retain-floor arm + scenario RFC0008.10 (startup
recovery driver: replay before listeners, routing, snapshot at
rotation). The §9 miner-snapshot open question is RESOLVED.

RFC 0001 §6.9 v2 amendment: the known-version branch now restores
the tree and replays only the WAL tail above S (the v1 full-replay
carve-out existed solely because Wal::checkpoint was a stub); the
v1 double-apply hazard is resolved by offset routing rather than by
refusing to restore. Stale-snapshot fallback: a WAL externally
truncated past S restores + replays survivors + emits a structured
warning naming the gap (hazard #5 surfaced via the RFC 0010 drift
query, never silent). New criteria §3.5.3 (restore-equivalence,
field-by-field via the snapshot payload of both trees) and §3.5.4
(loud degradation); §9 resume entry RESOLVED. No snapshot format
change — the high-water mark has been in the payload since v1.

Design decisions (maintainer, 2026-06-12): retain floor at
housekeeping over snapshot-before-checkpoint coupling; full
production driver in scope (serve() replays before listening).

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

* docs(rfc-0008,rfc-0001): replay delivers everything; suppression is per-consumer in the driver

Review round 1 — Copilot caught a real inconsistency: the in-replay
checkpoint skip made the retain floor useless (a lagging snapshot's
(S, X] frames were retained on disk but never deliverable, so the
miner could not close its state gap). Suppression moves out of
Wal::replay into the recovery driver: replay delivers every
well-formed surviving frame with its offset; the driver suppresses
per consumer (Parquet > X via the new last_checkpoint accessor,
miner > S), handling both orderings of S and X. RFC0008.7 arm 2
reworded accordingly (sidecar survives; the driver's Parquet-side
suppression uses it); RFC0008.10 gains a lagging-snapshot catch-up
arm. Also: the stale-fallback example is corrected to manual WAL
segment deletion (deleting snapshots triggers full-replay fallback,
not a truncated WAL), and §3.5.4 introduces X explicitly, scoping
'data side complete' to truncation that never exceeded the
checkpoint.

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

* docs(rfc-0008): invalid sidecar aborts recovery; driver-suppression wording in §6.7

Copilot round 2: a present-but-invalid CHECKPOINT (bad magic, unknown
version, non-zero flags, wrong size) is a structured corruption error
that aborts recovery — silently treating it as None would drop the
Parquet suppression horizon and duplicate every already-published
record; removal by the operator is an explicit acceptance of
at-least-once re-publish. The §6.7 durability paragraph now speaks in
driver-suppression terms instead of the retired skip-inside-replay
model.

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

* docs(rfc-0008): rfc0008.7 test-plan arm count three -> four

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

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
jensholdgaard added a commit that referenced this pull request Jun 12, 2026
…fset sink (#186)

* feat(wal): checkpoint sidecar, retain-floor housekeeping, offset-carrying sink (RFC 0008 §6.7)

Implements the §6.7 contract as amended 2026-06-12:

- Wal::checkpoint persists the 32 B OWCK v1 sidecar atomically
  (CHECKPOINT.tmp -> fsync -> rename -> parent-dir fsync); advance is
  monotonic, re-asserting the current value is an idempotent no-op,
  and the in-memory offset is not advanced on error.
- Wal::open reads the sidecar; a present-but-invalid one (wrong
  magic / version / flags / size) is OpenError::Corrupt and aborts
  before any recovery — silently treating it as None would drop the
  Parquet suppression horizon and duplicate published records.
- Wal::last_checkpoint exposes the offset as the recovery driver's
  Parquet-side suppression horizon.
- Wal::housekeeping(retain_floor) unlinks whole segments wholly
  below min(checkpoint, floor) — never the current append segment;
  segment identity comes from the in-file header, not the filename.
  The floor is the lagging-miner-snapshot guard (hazard #5).
- FrameSink::consume now receives the frame's append-offset and
  replay delivers every well-formed surviving frame — suppression
  moved out of replay into the driver, per consumer (an in-replay
  skip would make floor-retained frames undeliverable).
- metrics() implemented: exact counters (appends, syncs, unflushed
  bytes, corrupt frames) + best-effort disk_bytes / segment_count
  directory walk + checkpoint fields.
- wal_crash_fixture gains a CHECKPOINT op (checkpoint at the last
  append's offset, echoed to stdout) for the RFC0008.7 SIGKILL arm.

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

* test: flip the four RFC0008.7 checkpoint arms

Normal-flow truncation (wholly-below unlinked, straddler kept,
wal_disk_bytes drops by the unlinked bytes); SIGKILL between
checkpoint(X) and housekeeping (sidecar survives, replay delivers
everything, partitioning on X yields exactly the published prefix);
surviving-segments offsets (fresh open + checkpoint(Y > X) with no
global counter); retain floor (S < X holds the (S, X] segment back
until the floor advances). Multi-segment roots are minted in scratch
roots and moved in — rotation (RFC0008.6) is still red.

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

* fix(wal): housekeeping skips the live segment by header uuid, not path

Both reviewers caught it: a renamed live append segment slipped the
path-based guard, and unlinking it leaves the writer appending into
an unlinked inode no later open would see. The identity check now
reads the candidate's header first and skips on uuid equality,
consistent with the pass's rename-resilient identity rule. Pinned by
a test that renames the live segment, checkpoints past its every
frame, and asserts housekeeping leaves it alone.

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

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
jensholdgaard added a commit that referenced this pull request Jun 12, 2026
…ver (#187)

* feat(miner): snapshot restore v2 — recover() returns the state, restore_tenant rebuilds the tree

RFC 0001 §6.9 v2 (amended 2026-06-12, PR #185): the known-version
branch now restores instead of discarding. recover() returns
(Option<SnapshotState>, RecoveryOutcome) — Restored replaces
KnownVersionDiscarded; the v1 discard-contract test is retired per
the RFC-gated amendment and replaced by the restore contract.

MinerCluster::restore_tenant rebuilds a tenant tree from a
SnapshotState: leaves re-descend by their creation-time masked path
(a path-position wildcard resolves to its singleton mask tag —
widening/type-expansion are impossible at path positions because
candidates share their first walk_depth masked tokens by
construction); structured-template map and template_count rebuilt;
the cluster-wide template_id allocator bumps past every restored id.
Semantically inconsistent snapshots (empty template, slot-count
mismatch, non-mask path slot) are RestoreError::Inconsistent — the
driver treats them as corrupt (discard, full replay) per §6.9.

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

* feat(ingester): startup recovery driver + per-tenant snapshot store (RFC 0008 §6.6 / RFC0008.10)

recovery::recover restores each tenant's snapshot into the miner
(uuid-parse failures and restore_tenant rejections discard the
artefact as corrupt — §6.9), then replays the WAL through a sink
that decodes, fans out, and feeds the miner only frames above that
tenant's high-water mark; the Parquet horizon (last_checkpoint) is
read and reported — its consumer joins when the Parquet write path
does. Stale-gap detection per §3.5.4: S below the checkpoint with
S's segment absent from the replayed set (internally unreachable
under the §6.7 retain floor; a hit means external mutation and is
surfaced, never silent — hazard #5). A frame that fails decode or
fan-out stops replay loudly: it was valid when acked, so this is
corruption-adjacent.

snapshot_store: <wal_root>/snapshots/<tenant>.snap artefacts, atomic
tmp -> fsync -> rename -> parent fsync; load_all returns raw bytes
(version dispatch stays in recover). Journal::sync now returns the
durable offset so the pipeline can track last_durable for the
shutdown-cadence snapshot.

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

* feat(server): recovery before listeners; snapshot cadence at post-recovery + shutdown

serve() now runs the RFC0008.10 sequence: open WAL, restore + replay
through the recovery driver, warn per stale-gap tenant (stderr — the
documented stopgap until structured logging lands), write fresh
snapshots at the replayed high-water, and only then construct the
pipeline and bind both listeners. ReceiverHandle::shutdown writes
snapshots again after both listeners stop; a write failure degrades
to a warning — the snapshot is a rebuildable cache (§6.9), never
durable state. Per-rotation cadence stays blocked on RFC0008.6.

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

* test: land RFC0001 §3.5.3/§3.5.4 and the RFC0008.10 driver scenario

Restore-equivalence (restored + tail-replayed miner equals a
from-scratch control, with the suppression counter proving no frame
at or below S reached it), corrupt-version full-replay fallback,
the stale-gap arm (external segment deletion past S with a
checkpoint above it -> loud report, survivors still fold), cold
start, and the served-binary end-to-end (pre-populated WAL +
snapshot -> serve -> live export -> shutdown artefacts equal a
control).

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

* fix(ingester,miner,server): restore requires a concrete horizon; seed last_durable from recovery

Review round 1:
- a known-version snapshot whose wal_high_water is absent or
  unparseable is discarded (full replay) instead of restored —
  restoring without a horizon cannot suppress, which is exactly the
  v1 double-apply hazard (§6.9 maps it to the discard class)
- IngestPipeline::with_last_durable seeds the recovered high-water,
  so a zero-traffic shutdown no longer overwrites the post-recovery
  snapshots with a horizonless artefact that would force full replay
- restore_tenant rejects duplicate template_ids (across leaves +
  structured) and duplicate structured (severity, scope) keys as
  Inconsistent
- load_all skips non-file *.snap entries instead of aborting recovery
- colocated unit tests for recovery.rs (parse_high_water, the
  extracted stale_gap classification helper, sink rejection)

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

* docs(server): shutdown doc reflects the handle retaining the pipeline

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

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
jensholdgaard added a commit that referenced this pull request Jul 11, 2026
Copilot caught that the flags are -server.grpc-max-recv/send-msg-size-
bytes (dskit's server registry, defaults exactly the 4 MiB we hit), not
-server.grpc-server-max-*. The wrong names would have failed Loki's
startup and burned run #5. Verified against dskit source. Also backtick
the kafka service name in the comment.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jensholdgaard added a commit that referenced this pull request Jul 11, 2026
…flation) (#478)

* fix(bench): raise Loki's internal gRPC cap — single-line inflation (run #4)

Run #4 (29165198664) failed on the SAME ~5.27 MB internal message as
runs #2/#3 despite the outer cap halving (3 MiB → 1.5 MB), and the
fail-fast stayed silent — decisive: a single kafka LogsData line's
content alone inflates past Loki's stock 4 MiB internal gRPC cap. No
outer batching can split an indivisible unit.

Add -server.grpc-server-max-recv/send-msg-size=16 MiB to the indicative
run's documented ingest-side flags (standard operator tuning, in Loki's
favour — it lets Loki accept the data at all). This preserves the
identical-ingest precondition the equivalence check requires; skipping
the line would silently unequalize the two corpora.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(bench): correct dskit flag names for the gRPC msg-size cap

Copilot caught that the flags are -server.grpc-max-recv/send-msg-size-
bytes (dskit's server registry, defaults exactly the 4 MiB we hit), not
-server.grpc-server-max-*. The wrong names would have failed Loki's
startup and burned run #5. Verified against dskit source. Also backtick
the kafka service name in the comment.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jensholdgaard added a commit that referenced this pull request Jul 11, 2026
…#479)

* feat(bench): rfc 0031 — storage-side Loki bytes (conservative metric)

Run #5's 146.9x carried a metric asymmetry: Loki's
summary.totalBytesProcessed counts DECOMPRESSED engine-side bytes, while
Ourios's bytes_read counts COMPRESSED bytes fetched from storage — so
the ratio overstated Loki's storage reads by the chunk compression
ratio.

- parse_loki_fetched_bytes: recursively sums compressedBytes /
  headChunkBytes across the stats tree (querier + ingester, store +
  head) — resilient to section layout across Loki versions. Zeros are
  legitimate (head-chunk-served queries); a missing stats block errors
  (same honesty rule as the processed parser).
- The indicative report now prints BOTH figures and BOTH gate ratios,
  with the storage-side one (compressed + head-chunk) marked PRIMARY —
  the conservative, apples-to-apples number the §9 entry should carry.
  head_chunk bytes (memory-served, uncompressed) are reported in the sum
  rather than ignored: dropping them would understate Loki's data
  touched on head-served queries.
- Report block extracted to print_indicative_report (too_many_lines).

Unit-tested (section summing, empty-stats zeros, missing-stats error);
report validated via the next dispatch run.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(bench): reject a non-object data.stats (Copilot review)

'stats': null passed the existence check and silently yielded zeros —
now filtered to objects only, so it errors per the doc. Tested.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
jensholdgaard added a commit that referenced this pull request Jul 11, 2026
…hmetic

Copilot caught a real latent hazard: the severity pair queries the full
corpus window, but the picker only EXCLUDED zero-time_unix_nano rows
from its bands — it never checked whether the picked predicate could
select one. A zero-time row with severity >= threshold (DSL side) or
carrying the pair's text at any severity (LogQL side) is returned by
both systems with different answer timestamps: a deterministic
equivalence failure. Runs #5/#6 passed by corpus luck, not by
construction.

Zero-time rows are now tallied into per-service POISON bands and any
candidate whose predicate could select one is disqualified, on both
directions of the predicate. Unit-tested directly on hand-built bands.

Also: checked_add on the two flagged window-end computations so a
corrupted corpus fails loudly instead of wrapping in release mode.

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 11, 2026
…ve run (#480)

* feat(bench): rfc 0031 — three-point selectivity curve in the indicative run

Run #6's storage-side result (5.95x on a 1-row answer) is a single point
at the extreme-selectivity end, where Ourios's fixed per-query footer/
metadata reads dominate. This extends the indicative run to a curve:
the severity pair (L2 family) plus two time-window slices (~100 and
~2000 rows on the picked service, L6 family, reported under f_l6), all
measured against ONE container and ONE corpus replay.

Window pairs are chosen with clean edges (no shared timestamp at the
start, >=2 ns gap past the end) so [a, b) selects identical rows on both
systems regardless of range-end inclusivity, and windows containing any
zero-time_unix_nano row's observed-fallback timestamp are rejected —
both systems would return such a row with DIFFERENT answer timestamps,
a guaranteed equivalence mismatch.

Locally proven before spending a run: picker edge/poison/centring unit
tests, a fixture pass for the timestamp collector, and a fixture-store
test that the bare-service DSL + now/window mapping slices exactly
[start, end). Equivalence stays asserted per pair; gates stay REPORTED
under the provisional §7 margins.

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

* docs(bench): rfc 0031 — correct PairSpec now/window field doc

The [start, end) mapping only holds for the time-window slices; the
severity pair windows the full corpus and lets the predicate select.

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

* refactor(bench): rfc 0031 — checked_add on pick_window_pair's return

The +1 was provably safe (valid() already gated it), but the checked
form keeps that safety local if the two sites ever decouple.

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

* fix(bench): rfc 0031 — poison-aware pair picker + checked window arithmetic

Copilot caught a real latent hazard: the severity pair queries the full
corpus window, but the picker only EXCLUDED zero-time_unix_nano rows
from its bands — it never checked whether the picked predicate could
select one. A zero-time row with severity >= threshold (DSL side) or
carrying the pair's text at any severity (LogQL side) is returned by
both systems with different answer timestamps: a deterministic
equivalence failure. Runs #5/#6 passed by corpus luck, not by
construction.

Zero-time rows are now tallied into per-service POISON bands and any
candidate whose predicate could select one is disqualified, on both
directions of the predicate. Unit-tested directly on hand-built bands.

Also: checked_add on the two flagged window-end computations so a
corrupted corpus fails loudly instead of wrapping in release mode.

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 15, 2026
…m regressions

PR review findings #5, #6, #7.

#5: the RFC0002.7 round-trip generator's `bare_field()` — shared by
`path_field()`, `group_term()`, and the `project` field list — omitted
`Field::EventName`, so grouped-query round-trips never covered
`count by event_name`. It's a valid bare field everywhere the real
grammar's `bare_field` (parse.rs) allows it, so fixed in place.

#6: `rfc0002_12_count_by_matches_naive_oracle`'s foreign-tenant fixture
is written via `write_all`, which partitions by the record's own
`tenant_id` — so the "b" row lands under tenant "b"'s own directory and
the row-level `tenant_id == tenant` backstop in `execute_aggregate`
(CLAUDE.md §3.7) is never exercised, only directory-level scoping. Adds
`rfc0002_12_aggregation_tenant_backstop_excludes_misplaced_row`, which
plants a tenant "b" row *inside* tenant "a"'s partition directory (the
shape a partitioning bug or on-disk corruption would produce — the
`ourios-parquet` writer's RFC 0005 §3.9 row-vs-path contract refuses a
mismatched tenant_id at write time, so the row is written honestly then
relocated) and asserts the backstop filter, not partitioning, keeps it
out of both the count and the group map. Manually verified this test
fails without the backstop filter, confirming it exercises the guard.

#7: RFC0002.15 covered a `params` list shorter than `n + 1`, but not the
distinct case of a list that HAS slot n whose own `value` decodes as
Parquet-level NULL (the field is nullable — RFC 0005 §3.2 — even though
`Param.value` is a non-`Option` Rust `String`, so only a raw/corrupted
writer can produce it). Adds
`rfc0002_15_present_but_null_param_slot_excluded_and_tallied`, built
with a raw arrow-array batch (mirroring `forward_compat.rs`'s
schema-drift fixtures) so the disposition is proven on the actual
`decode_aggregate` code path rather than assumed from the short-list
case.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
jensholdgaard added a commit that referenced this pull request Jul 15, 2026
…bucket(w) (#533)

* feat(querier): rfc 0002 green — count-by execution with param(n) and bucket(w)

The aggregation-execution slice of the RFC 0002 amendment 2026-07-15
(RFC 0031 L4): `count [by …]` now executes end-to-end, discharging §5
scenarios RFC0002.12/.13/.15/.16. RFC0002.14 (the grammar/compile error
contracts) stays an ignored red stub for its own slice.

Surface (§7 v1.1 / §6.4 amendment):
- IR: the aggregation `by`-list widens from `Vec<Field>` to
  `Vec<GroupTerm>` (field | `param(n)` | `bucket(duration)`).
- Parser: `group_list`/`group_term` productions, confined to `by`-lists;
  positive + negative parse tests per production.
- Structured surface: `{"param": n}` / `{"bucket": "<duration>"}`
  by-elements (widths validated by the string-DSL lexer, RFC0002.2);
  `structured_query.schema.json` gains the additive `group_term` def
  (snapshot-gated by RFC0002.11, which also gains instances).
- Serializer: group terms round-trip (corpus + proptest generator).

Compile (§6.3/§6.5 amendment):
- `compile::validate` lifts the `count` rejection ONLY — sum/min/max/avg,
  sort, project, render keep the explicit rejection. Enforces the
  single-template pinning rule for `param(n)` (top-conjunctive
  `template_id == N`, all naming one N; `resolves_to` does not pin),
  positive bucket widths, and the duplicate-term rules.
- Group terms lower as expressions inside the existing Aggregate row:
  `param(n)` = `array_element(params, n+1).value` (stored string form,
  no type promotion); `bucket(w)` = floor division of the effective
  timestamp (with the §3.9 `time_unix_nano` fallback) into half-open
  epoch-aligned UTC windows; `service` = the RFC 0022 promoted column.

Execute:
- One grouped-count scan per aggregation query (Filter → Aggregate,
  the drift precedent) with a row-level `tenant_id` guard mirroring
  drift's (§3.7 — group values are row contents). `rows` stays the
  total matching count, derived from the same scan.
- Short/NULL `param(n)` rows are EXCLUDED from every group (no synthetic
  absent key) and tallied on the new `QueryStats.rows_excluded`,
  surfaced on the RFC 0016 stats DTO (RFC0002.15).
- Result carrier: `QueryResult.aggregate: Option<Vec<AggregateGroup>>`
  (`key: Vec<String>` per by-term in query order — bucket keys RFC 3339
  UTC window starts — sorted, engine-free per hazard §4.6); the RFC 0016
  response gains the additive `aggregate` field so the HTTP surface
  cannot silently drop the map.
- RFC0002.16 honest bytes: the total is the group-column scan alone —
  zero row materialization, zero template-map acquisition (the RFC 0033
  acquisition was already lazy; the aggregation path never renders).

Invariants: hazard §4.6 (no DataFusion/arrow/SQL crosses the surface —
plain strings/ints only); §3.7 multi-tenancy (partition scope + the new
row-level tenant filter on the aggregation plan). Contract changes
sanctioned by the maintainer-merged amendment (#531): the `count` case
moves out of rfc0002_6_unsupported_stage_rejected (RFC0002.12 names the
lift), and rfc0005_14's error-precedence probe switches from `count` to
the still-rejected `render`.

Verified: cargo fmt --check; workspace clippy --all-targets
--all-features -D warnings; strict rustdoc (ourios-querier); full
cargo nextest run (1107 passed); .12/.13/.15/.16 force-run green,
.14 still ignored-failing.

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

* fix(querier): structured by-list rejects resource/attr paths + param u32::MAX bound

PR review findings #1 and #3. The structured JSON surface's group_term
accepted {resource|attr} field objects in a by-list, but the string DSL's
group_term = field production (§7 v1.1) is bare-field-only — so the
structured surface could express count/aggregate-by queries the string
grammar cannot, which then failed in planning instead of at validation.

RawGroupTerm::into_ir now rejects a {resource|attr} object with a clean
DslError, and the schema gains a bare_field $defs entry so schema
validation itself rejects the shape instead of only the runtime
converter. The schema's param integer also gets an explicit maximum
(u32::MAX) so an out-of-range param slot fails schema validation
cleanly rather than succeeding the schema and then failing Rust
deserialization.

Adds schema instance-list cases (resource/attr group term, param past
u32::MAX) and a structured.rs unit test for the runtime rejection.

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

* fix(querier): reject count+limit; property-test the §6.3 planner invariants

PR review findings #2 and #4.

#2: `count [by …] | limit n` silently dropped the `limit` — execution
terminates in `Terminal::Aggregate`, which never consults `plan.limit`
(the aggregation map is the whole result; group-limiting semantics
aren't implemented). `validate()` now rejects the combination with a
clear QueryError::InvalidQuery instead of quietly returning the wrong
thing.

#4: pin detection (top-conjunctive `template_id == N`; `or`/`not`/
`resolves_to` don't pin), param-position duplication (at most one
`param(n)` per distinct n), and bucket constraints (positive width, at
most one `bucket(...)`) were covered only by hand-picked examples.
Adds a proptest generating arbitrary predicates and by-lists, checked
against an independently tracked ground truth (ground truth recorded
alongside generation, not derived from the code under test), covering
both `pinned_template_id` and `validate()`'s accept/reject decision.
The hand-picked examples stay as-is (CLAUDE.md §6.2).

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

* test(querier): event_name in group-term generator; tenant + NULL-param regressions

PR review findings #5, #6, #7.

#5: the RFC0002.7 round-trip generator's `bare_field()` — shared by
`path_field()`, `group_term()`, and the `project` field list — omitted
`Field::EventName`, so grouped-query round-trips never covered
`count by event_name`. It's a valid bare field everywhere the real
grammar's `bare_field` (parse.rs) allows it, so fixed in place.

#6: `rfc0002_12_count_by_matches_naive_oracle`'s foreign-tenant fixture
is written via `write_all`, which partitions by the record's own
`tenant_id` — so the "b" row lands under tenant "b"'s own directory and
the row-level `tenant_id == tenant` backstop in `execute_aggregate`
(CLAUDE.md §3.7) is never exercised, only directory-level scoping. Adds
`rfc0002_12_aggregation_tenant_backstop_excludes_misplaced_row`, which
plants a tenant "b" row *inside* tenant "a"'s partition directory (the
shape a partitioning bug or on-disk corruption would produce — the
`ourios-parquet` writer's RFC 0005 §3.9 row-vs-path contract refuses a
mismatched tenant_id at write time, so the row is written honestly then
relocated) and asserts the backstop filter, not partitioning, keeps it
out of both the count and the group map. Manually verified this test
fails without the backstop filter, confirming it exercises the guard.

#7: RFC0002.15 covered a `params` list shorter than `n + 1`, but not the
distinct case of a list that HAS slot n whose own `value` decodes as
Parquet-level NULL (the field is nullable — RFC 0005 §3.2 — even though
`Param.value` is a non-`Option` Rust `String`, so only a raw/corrupted
writer can produce it). Adds
`rfc0002_15_present_but_null_param_slot_excluded_and_tallied`, built
with a raw arrow-array batch (mirroring `forward_compat.rs`'s
schema-drift fixtures) so the disposition is proven on the actual
`decode_aggregate` code path rather than assumed from the short-list
case.

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

* fix(querier): rfc 0002 — non_exhaustive QueryStats, typed group-null literal

QueryStats gains #[non_exhaustive] matching QueryResult's convention.
The rows_excluded doc comments now scope to any NULL group key, not
just param(n). The absent-OPTIONAL-column NULL substitute in the
aggregate group-term compiler now carries the field's real Arrow
type (Binary/Timestamp/FixedSizeBinary/Utf8) instead of always Utf8,
so the plan's output schema does not depend on which columns happen
to be present. Regression test covers grouping by an entirely-absent
FixedSizeBinary column (trace_id).

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

* fix(server): rfc 0016 — skip the §7 default-limit injection for count-by queries

apply_limit ran unconditionally, but compile::validate now rejects
count+limit combined (RFC 0002 amendment 2026-07-15). Every
aggregation query sent to the HTTP endpoint was therefore a clean
400. Skip the injection when a Stage::Count is present. Regression
test confirmed via revert: fails without the fix, passes with it.

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

* docs(querier): rfc 0002 — execute_aggregate doc names the tenant backstop scan input

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

* fix(querier): rfc 0002 — checked_add for the excluded-row tally

Matches the existing pattern on rows: an overflow surfaces as an
error rather than silently wrapping in release builds.

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

* fix(querier): rfc 0002 — mark AggregateGroup non_exhaustive

Matches QueryResult/QueryStats' convention for public response types.

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

* fix(querier): rfc 0002 — reject i64-overflowing bucket widths at validate time

bucket_expr's execution lowering casts the width to i64, but
validate_group_terms only checked positivity — a width between
i64::MAX and u64::MAX ns passed validation and failed later during
planning with a different error path. Moved into validate() for one
compile-time contract. Regression test confirmed via revert.

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 15, 2026
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
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