mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-08-09 08:04:45 +00:00
Code-review fixes: two ways per-kind spans could be recorded and not used
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 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 5
parent
42a91ffb79
commit
74145ee8f3
+25
-1
@@ -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
|
||||
}
|
||||
|
||||
|
||||
+57
@@ -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)
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user