LogQL: correct the #227 audit's residual, and remove an unreachable arm (#241, three waves) by hshimizu · Pull Request #446 · digitalis-io/pulsusdb

hshimizu · 2026-08-13T02:54:01Z

Three independently revertable waves on #241. No Closes trailer — the issue stays open for its closeout comment and the recorded residuals.

The issue was wrong about itself. The post-aggregation group_key maps it names are already charged, under a Ledger against MAX_POST_AGG_BYTES, and are row 521 of the very audit table the issue says is silent about them. What was wrong is the audit's text.

Wave 1 (2fd153f) removes an unreachable SQL-aggregated range arm and corrects the audit. The arm's guard client.is_none() && step_ns.is_some() is unsatisfiable because || is_range forces client aggregation for every range query — and that line predates #257's filing, so the remedy was removal, not a charge. Discharges #257.

Wave 2 (7376ff0) adds the instant leaf's retained-byte witness and a generated 45-row reachability table: driven=8 uncovered=9 unreachable=28, uncovered rows printed rather than omitted.

Wave 3 (21fe499) measures the | json per-row expansion ladder and records rather than charges — F = 3.583, flat from 16 KiB, F × MAX_DECOMPRESSED_BYTES = 240,451,059 B against a 67,108,864 B threshold. The pre-committed rule selected "charge" and turned out not to determine a coefficient: every candidate is either the measured ratio itself, which must never be pinned, or an under-charge — and any of them would refuse a | json row the reference serves.

Five instruments here were each defeated once before they held — a substring census that could not catch a restatement, a rule that let one pricing helper swap for another three times its size, seven hand-written labels against a three-variant type, a parallel field list renamed rather than removed, and a gate that measured acceptance where it needed derivation. Every one was found by building and running it rather than reading it.

The plan's three-clause enumeration rule ships as a committed ENUMERATION-CHECKLIST with a syn scan asserting set equality in both directions, and its own limit written beside it: it proves equality among marked declarations, not that every relevant enumeration got a marker.

Reviewer VERDICT: PASS on 21fe499: #241 (comment) — nine closed-list checks reproduced; the full gauntlet reconciliation is left to CI.

https://claude.ai/code/session_01B1xu1frZL9oF8MYRhkiMhv

…udit (#241 wave 1) Discharges #257. The site it named — `run_metric_inner`'s SQL-aggregated RANGE arm, `client.is_none() && step_ns.is_some()` — is structurally unreachable and already was on the day #257 was filed: `metric_plan` forces `client = Some(..)` for every `QuerySpec::Range` (`plan.rs:1801`, the `|| is_range` disjunct, present at `8d1f4519`), and `step_ns` is `None` for exactly `QuerySpec::Instant`. There is nothing to charge because the code cannot run, so the remedy is removal, not a bound. The arm and its `explain_metric_into` twin are replaced by the crate's internal-refusal shape rather than deleted outright, so a planner change that reintroduces the state surfaces as a named error instead of a wrong answer. `tests/logql_plan_build_differential.rs:: every_planned_range_leaf_is_client_aggregated` is the tripwire that catches it before a request does; its doc says plainly that the PROOF is a total read of the only `MetricPlan` constructor and the test is a regression detector over a corpus. `sql::metric_range` is untouched — it is `pub`, documented by `docs/schemas.md` §3.2, and driven live by `tests/rollup_differential.rs`. `post_agg::charged_range_chain`/`measure_range` lose their only caller with the arm and go with it; `charged_instant_chain` records why there is no range twin. Audit corrections in `charge.rs`, which is what this issue turned out to be about: * the `MAX_LEAF_RETAINED_BYTES` residual no longer charges #257 to the SQL path, and now states the post-aggregation cap as a PER-NODE bound on BYTES with NO term-count bound — an earlier revision implied a global admission ceiling, and two term counts quoted in support of it were both stale; * a new audit row records the removed SQL-aggregated range arm as REMOVED, not bounded, so a later reader finds the disposition instead of re-deriving it as an uncharged container; * the post-aggregation row now NAMES the shipped gate that establishes its property, and a test asserts a function of that exact name exists in the witness file — a relation between two artifacts, not more prose; * `MAX_CLIENT_AGG_GROUP_BYTES` records the settled answer to an item left "unverified in either direction": #374's write-time label bounds DO NOT REACH the query-time label set. `pipeline.rs`'s `| json` residual note gains its owning issue and the settled shape of that residual: per-ROW, not query-lifetime, with the charged half already charged. No golden is regenerated (`git diff --stat crates/pulsus-read/tests/golden/` is empty). Two census pins move with the code: the streams retention inventory loses the three `by_fp` rows that went with the arm, and the `PipelineInvalid` per-file census goes 12 -> 14 for the two refusals. The planner citations here name SYMBOLS, not `plan.rs:NNNN`. Wave 2 adds a macro above `metric_plan` and shifts every line in it by 25, so a line number written here would be correct in this commit and wrong in the tree anyone reads — and correcting it from wave 2 would make this commit un-revertable on top of that one. `metric_plan`, `|| is_range` and `requires_unwrap`/`forbids_unwrap` are each a single production hit under `git grep`, so nothing is lost. Claude-Session: https://claude.ai/code/session_01B1xu1frZL9oF8MYRhkiMhv

The censuses in `charge.rs` enumerate CHARGES, and `MAX_LEAF_RETAINED_BYTES`' own doc says what that costs: "A retained structure governed by NO cap is outside it by construction." A registry of charges cannot see a retention nobody charged. So this adds no list. It turns the audit table into an equation with a residual that must be zero, measured on the shipped instant-leaf seam: measured_peak <= instant_leaf_charged_bytes + instant_leaf_bounded_bytes Every addend is either a CHARGED counter of `ClientAggState` — priced by ONE exhaustive destructure, with no second field list to keep in step — or a row of the query-lifetime audit table. `run_client_agg_rows_folded_measured` is the seam; `run_client_agg_rows_folded` delegates to it byte-for-byte and drops the second member, so the witness measures the shipped path rather than a copy. The second member is `Option<u64>` and is `None` for a RANGE window: `RangeSlideState` discharges as it slides, so a finish-time snapshot is below its own peak and would be unsound as the priced side of the identity. Reporting `0` there would be a measurement that reads as a fact. `RangeAggOp` and `ClientValue` gain a macro-emitted `ALL` in the `bin_ops!`/`vector_agg_ops!` shape, which is what lets the coverage table be GENERATED from `RangeAggOp::ALL x ClientValue::ALL` rather than typed: 45 rows, each row's `ReducerClass` computed by calling `reducer_class` and each row's status answered by a wildcard-free `match`. The gate on it is a row-level iff against the REAL planner — reachable(op, value) <=> (plan() succeeded AND planned client.value == value) — because syntax acceptance alone is satisfiable by a text that planned a DIFFERENT value, and a gate resting on it cannot go red on derivation drift. Measured: 22 texts accepted, 17 semantic matches, 45/45 rows agreeing, 8 pairs driven by a fixture and 9 reachable-but-uncovered. The `ENUMERATION-CHECKLIST` block at the head of the witness classifies every enumeration the file asserts over against all three clauses of the rule this issue produced, and a `syn`-based marker scan asserts set equality with the block in BOTH directions. Disclosed, because it is a deviation: AC 8's per-term necessity check cannot pass for any term of this model — every coefficient is a deliberate over-approximation, so each term alone has slack enough to cover its neighbours, and AC 8's remedy (delete the term) would leave a `ClientAggState` field undispositioned in the exhaustive destructure the same plan requires. The sweep is still RUN and its per-term result printed; the committed gate is the weaker but true one — dropping the whole bounded side must redden at least one fixture — and both tests say so in their own docs. The `client_values!` macro added here shifts `metric_plan` down by 25 lines, so every `plan.rs:NNNN` this commit's own witness cites is derived against THIS tree — the unwrap-arity block at 1760-1776, the `ClientValue` ladder at 1829-1835, and the line-targeted break site at 1831 (that expression occurs five times; 1831 is the ordinary planner's ladder and 2778 is the variant-local one, which AC 16's bare-range probe never reaches). Every boundary re-read here with `sed -n`. Wave 1's citations name symbols precisely so this shift cannot invalidate them. Claude-Session: https://claude.ai/code/session_01B1xu1frZL9oF8MYRhkiMhv

#241 wave 3) `pipeline.rs`'s `JsonPaths` doc names, in the code's own words, what `MAX_JSON_FLATTEN_KEY_BYTES` does not cover: "the row's label vector spine and the parsed `serde_json::Value`". That residual is per-ROW and half-charged already, so the only open question was how far one row's live heap can exceed the body that produced it. Reading cannot answer that; it is measured here, against a rule fixed before the first measurement was taken and reproduced verbatim in the module doc. Measured (`cargo test -p pulsus-read --test logql_row_expansion_ladder -- --ignored --nocapture --test-threads=1`, this machine): B= 977 median f = 2.718 B= 16 105 median f = 3.583 B= 257 909 median f = 3.582 B=4 126 651 median f = 3.581 F = 3.583; F * MAX_DECOMPRESSED_BYTES = 240 451 059 B > MAX_JSON_FLATTEN_KEY_BYTES = 67 108 864 B So the rule's record-nothing branch does NOT fire and the rule selects "charge the residual". The charge is NOT implemented, and the reason is a gap in the rule rather than a judgement about the answer: the rule fixes the ladder, the statistic and the branch but not the COEFFICIENT, and every candidate is the measured ratio itself — the machine- and profile-dependent kind of figure `plan_recursive_control.rs:21-24` says must never be pinned as a threshold. `alloc_block_bytes` (2x) and `grown_alloc_bytes` (3x) both sit below the measured 3.583x and would under-charge. Separately, any of them tightens the existing `RowBudget::JsonFlattenKeys` 422 to refuse a `| json` row the reference serves, which is a divergence needing a ledger entry and docs that this plan does not provision. Referred with the measurement in hand. Two disclosed deviations, both forced by measurement: * the plan chose 4 MiB as the top rung "so the whole file stays inside the `ci` job". One 4 MiB rung costs 231.6 s. The full pre-committed ladder is therefore `#[ignore]`d in the `zz_regenerate_golden` shape and its transcript recorded; the always-on test walks the 1 KiB/16 KiB/ 256 KiB prefix, which still spans 256x and yields the same F because the maximum median sits at the 16 KiB rung. * the wall time is quadratic and the ladder proves it is not the instrument: an UNINSTRUMENTED `run_into` costs 129.8 us / 1.21 ms / 138.7 ms / 40.52 s across the four rungs. `set_label_at` (`pipeline.rs:3411`) resolves each emitted key with a linear `labels.iter().position(..)` and the flatten calls it once per leaf, so a row emitting m labels costs O(m^2). Flagged, not fixed — it is a latency property and this issue is about bytes, and the byte ratio is flat across the same ladder. Claude-Session: https://claude.ai/code/session_01B1xu1frZL9oF8MYRhkiMhv

`assert_gzip_identity_metrics` scraped `/metrics` twice and required the two decoded bodies to be equal after filtering out exactly one metric name. `ops::metrics_handler` bridges live counters and gauges into every scrape, so any second value that starts moving fails it; it flaked on PR #446 on a change touching no metrics code. The race was only ever in the body comparison, so only that changes: the body equality now comes from ONE response gzipped locally. The filter is deleted rather than left dead. Both legs keep their own header cells, each asserted against its own response and never against the other's — the identity leg's Content-Encoding ABSENCE is a per-route cell, and the matrix exists to catch a route that stops going through the global CompressionLayer, which the other routes' passing legs cannot show. Headers of two scrapes do not race. Claude-Session: https://claude.ai/code/session_01B1xu1frZL9oF8MYRhkiMhv

* LogQL: index the label vector inside the | json flatten (#447) `insert_flattened` resolved every emitted key with `label_position`'s `labels.iter().position(..)`, so a row emitting m labels cost m(m-1)/2 string comparisons. One uninstrumented `run_into` over a 4 126 651 B flat object (m = 67 650) cost 34-42 s before this and 0.11-0.23 s after — the `LADDER-WALL ... B=4126651` line of `CARGO_INCREMENTAL=0 cargo test -p pulsus-read --test logql_row_expansion_ladder -- --include-ignored --nocapture --test-threads=1`, 4 runs before and 60 after, one process per run. Both figures are RANGES because the measuring machine is shared: several builds run on it concurrently, the 4 MiB rung's spread is about 2x, and no single value inside either range is reproducible on demand. The gap between the ranges is the finding — every point in one is two orders of magnitude from every point in the other — and the file's docs now say so at each place a wall time appears. Adds `LabelIndex`: an open-addressed `Vec<u32>` holding positions, not names, so every probe compares against `labels[i].0` — the same comparison `label_position` makes, which is why the answers coincide by construction — and the table allocates nothing per key. Built once per flatten and dropped with it, because positions are stable only while the vector is append-or-overwrite-in-place, which is exactly that walk's shape. First occurrence wins, matching `position`. Behaviour is unchanged; the differential test pins the equivalence over duplicates, `_extracted` collisions, empty and non-ASCII names, and the load-factor test pins the invariant that makes the probe loop terminate. Two knock-on records in `tests/logql_row_expansion_ladder.rs`: `F` moves 3.583 -> 3.934 (worst case 240 451 059 B -> 264 006 270 B) on the index table's doubling transient, with the pre-committed rule still selecting the same branch; and `zz_the_full_precommitted_ladder` loses its `#[ignore]`, which the file itself records as wall-time-only, because the test now costs 1.2-2.0 s. Those two byte figures are the only numbers in that file still written as single values, and they earn it by being gated: `apply_the_rule` now asserts them by EQUALITY against new `F_MILLI` and `WORST_CASE_BYTES` constants, alongside the pre-existing bounds and branch-selection assertions, which pass over a wide band and would not have caught a stale figure. They are deterministic allocation counts under a pinned toolchain, so an exact gate is available for them in a way it is not for anything timed. The prose repeating those two figures is gated too, by a count rather than by a list. `the_quoted_figures_are_counted_so_a_new_quotation_cannot _go_unchecked` reads the file's own source through `include_str!` and counts occurrences of each figure's rendering, against `F_QUOTATIONS` and `WORST_CASE_QUOTATIONS`; the needles are built from the two constants at run time, so neither literal appears in the searcher and the count is of quotations elsewhere. A hand-written inventory of sites was tried first and was wrong on the day it was written — it has no failure mode, and the edit that adds a mention silently falsifies it. Issue #241's closeout comment is the one quotation outside any test's reach; it stays named in prose because nothing in the tree can check it. Claude-Session: https://claude.ai/code/session_01B1xu1frZL9oF8MYRhkiMhv * Tests: /metrics gzip conformance stops comparing two scrapes (#447) `assert_gzip_identity_metrics` scraped `/metrics` twice and required the two decoded bodies to be equal after filtering out exactly one metric name. `ops::metrics_handler` bridges live counters and gauges into every scrape, so any second value that starts moving fails it; it flaked on PR #446 on a change touching no metrics code. The race was only ever in the body comparison, so only that changes: the body equality now comes from ONE response gzipped locally. The filter is deleted rather than left dead. Both legs keep their own header cells, each asserted against its own response and never against the other's — the identity leg's Content-Encoding ABSENCE is a per-route cell, and the matrix exists to catch a route that stops going through the global CompressionLayer, which the other routes' passing legs cannot show. Headers of two scrapes do not race. Claude-Session: https://claude.ai/code/session_01B1xu1frZL9oF8MYRhkiMhv

hshimizu added 3 commits August 13, 2026 03:16

hshimizu merged commit 5d91ef1 into main Aug 13, 2026
19 of 20 checks passed

hshimizu deleted the issue-241-post-agg-audit branch August 13, 2026 03:29

This was referenced Aug 13, 2026

Post-aggregation group maps hold unbounded owned labels (gap in the #227 audit) #241

Closed

LogQL: | json is O(m²) per row — 41 s at 4 MB, and a fragile /metrics conformance test #447

Closed

Navigation Menu

Uh oh!

LogQL: correct the #227 audit's residual, and remove an unreachable arm (#241, three waves) - #446

LogQL: correct the #227 audit's residual, and remove an unreachable arm (#241, three waves)#446
hshimizu merged 3 commits into
mainfrom
issue-241-post-agg-audit

hshimizu commented Aug 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

hshimizu commented Aug 13, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant