fix/cov-merge
needs attentionc179d41 · incrementalPR #311reviewed 2026-07-15 00:45 UTC0H · 4M · 7L · 3I- Purpose
- Fix a TOCTOU race in the metrics coverage bookkeeping that caused silent read-blackouts when collections arrived out of order (e.g. back-billing an older period after a newer one was already collected).
- Goal
- Make coverage merge order-independent and atomic against concurrent same-source collections by moving the union into the write transaction under a per-source advisory lock.
- Sub-goals
- SG-1: Remove existingCoverage state from decideRecordMetrics — decision returns only batchWindow
- SG-2: Move mergeCoverageWindows call into shell tx, under pg_advisory_xact_lock per source
- SG-3: Add toleranceMs to mergeCoverageWindows so gapless-but-adjacent forward-fill windows coalesce
- SG-4: Add CADENCE_MS lookup for cadence-appropriate tolerance
- SG-5: Add concurrent integration test for advisory lock correctness
- What
- decideRecordMetrics no longer takes state; returns batchWindow. Shell reads coverage inside the tx under pg_advisory_xact_lock, calls mergeCoverageWindows with cadence-based tolerance, then deletes+reinserts. Tests updated to new API + new tolerance + concurrent scenario.
- Why
- Live incident: 168 sites back-billed at ~1/N because collection order triggered coverage window inversion (endedAt < startedAt), causing clampWindowToCoverage to treat all data before the newer window as guaranteed empty.
- Areas
- domains/metrics/src/metric+491−101domains/cross-domain/src+11−1
- Blast
- 10 files, +553/-106 lines. Touches every integration's coverage-write path. Cross-domain billing shells also updated (minor). Read path (clampWindowToCoverage) unchanged.
Findings · 14
correctness3
1M sources silently get wrong tolerance (5 min instead of ~30 days)
domains/metrics/src/metric/metric.shells.ts:127
SOURCE_CADENCES excludes '1M'. Fallback assigns '5m' cadence → 300_000 ms tolerance. Monthly windows (~28+ days apart) never fuse — forward-fill accumulation problem persists for 1M sources. Add '1M' to SOURCE_CADENCES and CADENCE_MS.
Advisory lock has ~1-in-4B collision probability across all sources
domains/metrics/src/metric/metric.shells.ts:216
hashtext() returns int4 (32-bit). Two distinct source UUIDs colliding causes false serialization. Negligible at current scale.
Concurrent integration test doesn't guarantee truly simultaneous transactions
domains/metrics/src/metric/__tests__/metric.record.integration.test.ts:199
Promise.all interleaves await points but doesn't guarantee two DB transactions are in-flight simultaneously. Test validates outcome, not the lock mechanism.
conventions4
Stale file-level comment: step 1 still says 'Loads current coverage windows'
domains/metrics/src/metric/metric.shells.ts:4
Coverage read was moved to inside the tx (step 5). Step numbering in header comment now mismatches the actual flow.
mergeCoverageWindows belongs in lib/coverage, not metric.decisions.ts
domains/metrics/src/metric/metric.decisions.ts:92
canonical-form.md reserves decisions files for Result<Decision,Error> logic. mergeCoverageWindows is a pure interval-union utility invoked exclusively by the shell now.
FCIS boundary shift is architecturally justified
domains/metrics/src/metric/metric.decisions.ts:127
Removing RecordMetricsState is sound: the coverage merge must be atomic under a lock (shell concern). batchWindow is the correct decision boundary. No ADR-016 violation.
CoverageMutation type is shell-only but lives in decisions file
domains/metrics/src/metric/metric.decisions.ts:61
No convention violated (pure/inert type), but co-locating with mergeCoverageWindows in lib/coverage would cluster all coverage-union concerns.
tests4
Concurrent test asserts row count but not window boundary correctness
domains/metrics/src/metric/__tests__/metric.record.integration.test.ts:212
rows.length === 5 passes even if two collections wrote the same year and one was wiped differently. Add: expect(rows.map(r => r.startedAt.getUTCFullYear()).sort()).toEqual([2018,2019,2020,2021,2022]).
toleranceMs + OPEN window adjacency path is untested
domains/metrics/src/metric/__tests__/metric.decisions.test.ts:179
Tolerance tests only exercise closed→closed adjacency. OPEN_END short-circuit makes this safe, but a test documenting the invariant prevents future regressions.
Older-batch-before-open-window test doesn't exercise toleranceMs path
domains/metrics/src/metric/__tests__/metric.decisions.test.ts:167
Regression test uses toleranceMs=0 only. Tolerance interaction for older-batch-before-open-window is untested.
Integration test has no skip guard for missing catalog seeds
domains/metrics/src/metric/__tests__/metric.record.integration.test.ts:75
beforeAll throws on missing egauge/solar_generation seeds. A skipIf guard would emit a descriptive skip rather than an unhelpful hard throw.
improvement3
CADENCE_MS duplicates CADENCE_MINUTES from metric.fingerprint.ts
domains/metrics/src/metric/metric.shells.ts:64
metric.fingerprint.ts already defines CADENCE_MINUTES. CADENCE_MS is the same table × 60_000. Derive one from the other to avoid silent drift when cadences are added.
Silent fallback to '5m' on unknown granularity — same root as 1M correctness gap
domains/metrics/src/metric/metric.shells.ts:127
Should log a warning or return a distinct error rather than silently degrading tolerance for any unrecognized granularity.
writtenMutation default init of empty replace is misleading
domains/metrics/src/metric/metric.shells.ts:74
Initialized to { kind:'replace', segments:[] } — any path skipping the tx would expose a non-null mutation implying coverage was wiped. Initialize to undefined instead.