From 74145ee8f3ceda3785c3591e43ff26b9a616dfa2 Mon Sep 17 00:00:00 2001 From: Vitor Pamplona Date: Wed, 5 Aug 2026 16:08:56 +0000 Subject: [PATCH] Code-review fixes: two ways per-kind spans could be recorded and not used MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both found in the review pass over the previous commit, both the same shape — a band written that no lookup can reach, or reaches wrongly. - Spans for kinds the filter never named were stored as given. Inert for legs(), which only looks up the filter's own kinds, but NOT for Band.minCreatedAt — and that is what SyncCoverageFile writes as its rollback-compat `min`/`max`. A relay answering with more than it was asked for (or a caller whose containment check runs against a different filter than the band is keyed by) would push that floor below anything the filter's kinds support, so a binary from before per-kind spans would read the file and over-claim. The fix, undone through the compatibility path it added. - observedByKind on a filter that names NO kinds was stored per kind, while legs() for such a filter reads only ALL_KINDS. The band was recorded, persisted, and never consulted: a resume that silently did not resume. Collapsed to the union, which is the only claim a kind-less filter can make. Why these were not in the initial diff: both live where the new per-kind path meets an OLD assumption — that record()'s input is already scoped to the filter, and that a band's keys are always the filter's kinds. Neither held once callers began supplying the map themselves. Co-Authored-By: Claude Opus 5 --- .../relay/client/accessories/SyncCoverage.kt | 26 ++++++++- .../client/accessories/SyncCoverageTest.kt | 57 +++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/SyncCoverage.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/SyncCoverage.kt index 4e6de68696..74bcddbb3d 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/SyncCoverage.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/SyncCoverage.kt @@ -251,7 +251,31 @@ class SyncCoverage( isPlausible(it.min, now()) && isPlausible(it.max, now()) } if (plausible.isEmpty()) return - put(url, filter, plausible, complete = false) + val named = filter.kinds + val spans = + if (named.isNullOrEmpty()) { + // A filter naming no kinds cannot be split, so [legs] reads + // ALL_KINDS and nothing else. Storing what the walk saw per + // kind would record a band no lookup can ever reach — it + // would exist and do nothing. Collapse to the union, which + // is the only claim such a filter can make. + mapOf(ALL_KINDS to plausible.values.reduce { a, b -> a.widen(b) }) + } else { + // Only kinds the filter NAMES. A relay may answer with more + // than it was asked for, and a caller whose containment + // check runs against a different filter than the band is + // keyed by passes those straight through. Keeping them + // would be inert for [legs] — which looks up the filter's + // own kinds — but NOT for [Band.minCreatedAt], which the + // state file writes as its rollback-compat `min`/`max`. An + // off-filter kind seen further back would widen those past + // anything the filter's kinds support, so a binary from + // before per-kind spans would read that file and + // over-claim: this fix undone through the compat path. + plausible.filterKeys { it in named } + } + if (spans.isEmpty()) return + put(url, filter, spans, complete = false) return } diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/SyncCoverageTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/SyncCoverageTest.kt index 87cac2c977..3c26e1891e 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/SyncCoverageTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip01Core/relay/client/accessories/SyncCoverageTest.kt @@ -502,4 +502,61 @@ class SyncCoverageTest { assertEquals(listOf(0, 30382), legs[0].kinds) assertEquals(1_690_000_000L, legs[0].until) } + + @Test + fun `a kind the filter never asked for cannot widen the band`() { + // A relay may answer with more than it was asked for. Those spans are + // inert for legs(), which only looks up the filter's own kinds — but + // NOT for Band.minCreatedAt, which the state file writes as its + // rollback-compat min/max. Left in, a stray kind seen further back + // would widen that past anything the filter's kinds support, and a + // binary from before per-kind spans would read the file and over-claim. + val c = SyncCoverage() + c.record( + relay, + profiles, + null, + null, + paged = true, + observedByKind = + mapOf( + 0 to SyncCoverage.Span(1_690_000_000L, 1_700_000_000L), + // never asked for, and much older + 1 to SyncCoverage.Span(1_600_000_000L, 1_610_000_000L), + ), + ) + + val band = c.band(relay, profiles)!! + assertEquals(setOf(0), band.spans.keys, "only the kind the filter names") + assertEquals(1_690_000_000L, band.minCreatedAt, "…so the compat floor stays honest") + } + + @Test + fun `per-kind evidence on a filter naming no kinds collapses to one span`() { + // Such a filter cannot be split, so legs() reads ALL_KINDS and nothing + // else. Storing per-kind spans here would record a band no lookup can + // reach — present in the file, doing nothing. + val anyKind = Filter(authors = listOf("a".repeat(64))) + val c = SyncCoverage() + c.record( + relay, + anyKind, + null, + null, + paged = true, + observedByKind = + mapOf( + 0 to SyncCoverage.Span(1_690_000_000L, 1_695_000_000L), + 30382 to SyncCoverage.Span(1_697_000_000L, 1_700_000_000L), + ), + ) + + val band = c.band(relay, anyKind)!! + assertEquals(setOf(SyncCoverage.ALL_KINDS), band.spans.keys) + assertEquals(1_690_000_000L, band.spans.getValue(SyncCoverage.ALL_KINDS).min, "the union, not one of them") + assertEquals(1_700_000_000L, band.spans.getValue(SyncCoverage.ALL_KINDS).max) + // …and it is actually USED, which is the half that was silently missing. + assertEquals(2, c.legs(relay, anyKind).size) + assertEquals(1_690_000_000L, c.legs(relay, anyKind)[0].until) + } }