fix/cov-merge
safeviewing older commit0ae4db9 · incrementalPR #311reviewed 2026-07-14 22:48 UTC0H · 0M · 1L · 1I- Purpose
- Fix a silent read-blackout caused by order-dependent coverage bookkeeping: collecting an older window after a newer one produced an inverted coverage row (endedAt < startedAt), which clampWindowToCoverage read as 'guaranteed empty'. Found live: 168 sites billed ~1/N of legacy because each read only its last chunk.
- Goal
- Make coverage windows order-independent (interval union) and decouple the billing read path from the coverage clamp so billing always reflects the metrics actually present in Tinybird.
- Sub-goals
- SG-1 (prior commits): Replace extend-open mutation with pure mergeCoverageWindows interval union; real gaps preserved; at most one open window stays open.
- SG-2 (prior commit): DB integration test for the coverage-window merge — records newer then older, asserts no inversion, clamp doesn't blackout the earlier read.
- SG-3 (this commit): Add ignoreCoverage flag to ResolveMetricStreamWindowParams + ResolveSiteEnergyMetricsInput; propagate through derivation recursion; set it unconditionally on both billing callers (bill-compute.shells.ts and bill-for-contract.shells.ts).
- What
- ignoreCoverage?: boolean added to ResolveMetricStreamWindowParams and ResolveSiteEnergyMetricsInput. When set, the clampWindowToCoverage call is bypassed and the raw [fromUtc, toUtc) window goes straight to Tinybird. Propagated through resolveDerivationWindow so derivation input reads also bypass coverage. Both billing callers set ignoreCoverage: true. Two unit tests added via resolveWindowWithReader port verifying the default-clamp vs. bypass behavior.
- Why
- Coverage windows are a collection-planning guard and dashboard filter; they should not trim billing reads. A stale or partial coverage row (e.g. the inverted-window case this branch fixes) would silently zero out a month's worth of billing data. Billing reads raw Tinybird — missing slots fill with 0 in the integral, which is correct behavior.
- Areas
- domains/metrics/src/metric+54−4domains/cross-domain/src+12−1
- Blast
- 5 files, +66/-5 lines. Affects every billing read path (computeBillShell and persistBillForContract PPA path). Coverage clamp is unchanged for dashboards/generic reads (ignoreCoverage defaults undefined/false). Derivation reads within billing also bypass coverage — correct because derivation streams have no coverage rows of their own anyway.
Findings · 2
conventions1
bill-for-contract.shells.ts sets ignoreCoverage: true without a comment
domains/cross-domain/src/bill-for-contract.shells.ts:177
bill-compute.shells.ts has a 3-line comment explaining why ignoreCoverage is set (the WHY is non-obvious per CLAUDE.md rule). The analogous change in bill-for-contract.shells.ts is a bare field addition with no comment. A one-liner referencing the JSDoc on ResolveSiteEnergyMetricsInput.ignoreCoverage would make the intent discoverable without a code-side search.
tests1
No test for ignoreCoverage propagation through a derivation stream
domains/metrics/src/metric/metric.queries.test.ts
The two new tests cover the physical-source path (coverage start is used vs. bypassed). The derivation propagation (metric.queries.ts line 305 in the diff) is correct by construction, but there is no test asserting that when a billing call hits a derivation stream, all its input streams also bypass coverage. A test using a two-level reader mock would guard against a future accidental omission of `ignoreCoverage: params.ignoreCoverage` in resolveDerivationWindow.