Sketch and monoid parameter safety (wire format deliberately unchanged) - #92
Merged
Conversation
pequalsnp
force-pushed
the
fix/sketch-parameter-safety
branch
from
August 28, 2026 14:56
ebc30cf to
ae35ffd
Compare
Four ways a monoid's own configuration could destroy the state it was merging into, none of which surfaced anywhere. bloom: (m, k) is written into every marshaled filter and read back out of it by UnmarshalBinary, so the monoid's parameters survive only until the decode. A shape mismatch therefore decoded cleanly, fell through to the "return the left operand" path, and reported nothing — the case WithDecodeErrorHandler's own doc claimed to cover. Worse, Identity() was a marshaled empty filter, so it was an identity only for filters of its own shape: NewWithCapacity on the aggregate plus the default-sized Single in the value extractor merged every event into a mismatch and the row stayed empty for good. Identity is now the empty slice, which Combine already short-circuits, and a mismatch between two real filters names both shapes through the handler. topk: the wire K was decoded and discarded, so a K=10 client reading a K=32 row merged at 10 and wrote the truncation back — 22 counters gone, unrecoverable. Combine now merges at the widest K of the monoid and both operands (max is associative, so merge order still can't change the result) and reports the mismatch. A wire K is untrusted, so the K adopted from an operand is capped; without a ceiling a corrupt header switches off eviction and the row grows until the store refuses the write. topk saturation was undetectable: a summary holding 0.1% of its stream is byte-identical in shape to an exact one — 32 counters summing to 45,932 with 32 distinct entities, 29 summing to 29 with 33. The header now carries the total ingested weight as a plain associative uint64 sum, and Inspect returns Coverage / Saturated / MaxError from it. Discarded mass is deliberately not accumulated: it is merge-order-dependent and would break the associativity monoidlaws fuzzes. Pre-weight rows still decode, with PartialWeight set so a short denominator can't read as full coverage. compose: one future-dated event ended a DecayedSum key's life. Combine adopts the newer timestamp as the frame, so a four-year skew made the frame unreachable — 2^(-4y/24h) underflows to exactly zero, the accumulated mass was annihilated, and every event that followed was itself the older operand and was annihilated in turn. The key froze at whatever the skewed event carried while the read path scaled it up by 2^(+gap/hl): +Inf, or 7.5e109 for a one-year skew at halfLife=24h, pinning it to rank #1 forever. Combine now bounds the frame against the wall clock (WithClockSkewBound, two half-lives by default), and EvaluateAt before the reference time returns the stored value instead of un-decaying it. A pairwise horizon can't do this job: from inside Combine, a state four years in the future is indistinguishable from a key that went quiet four years ago, and clamping the second pins idle keys at a frame they can never leave. compose also accepted halfLife <= 0, after which Combine and EvaluateAt meant opposite things for the same row: 2^(-dt/0) is NaN at dt=0 (which round-trips through Encode/Decode and poisons the row forever) and 0 at dt>0 (which drops every older contribution), while EvaluateAt reported the value undecayed. Both now mean "no decay". Reachable by accident via murmur.Trending(name, cfg.HalfLife) with an unset Duration field. And DecayedSumBytes had no decode-error hook, unlike hll/topk/bloom. The wire form is a bare 17 bytes, so a 200-byte HLL blob decoded to a Set=true observation assembled from its first 17 bytes and merged in as real. DecodeDecayed now returns an error for any length but 0 and 17, and DecayedSumBytes takes WithDecodeErrorHandler. Breaking: the topk wire format gains a header field (old rows read fine, new rows do not read on old binaries); bloom's Identity is now empty; DecodeDecayed returns (Decayed, error); EvaluateAt no longer scales up before the reference time. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S
Addresses the blocking review on fix/sketch-parameter-safety. Removes the TopK wire-format change entirely. topk.go and topk_test.go are now byte-identical to main: no header flags, no uint64 ingested-weight field, no flagWeight/flagPartialWeight/kMask/maxAdoptedK, no effectiveK adoption, no Inspect/Summary. main's Combine reading a branch-written row returned a 1-item sketch where the row held 40 events over 12 entities and then CAS-wrote that back, so a rolling deploy with mixed tasks would have destroyed accumulated state on the pipeline currently under soak. Forward compatibility was handled; backward was not, and a rolling deploy needs both. It returns later as a staged read-both/write-old change. The saturation tests survive the revert in pkg/monoid/sketch/topk/ saturation_test.go, rewritten against today's API: they assert that one extra entity takes a K=32 summary from 32 counters covering 45,932 events to 29 counters covering 29, and that the heavy hitter still lands inside n/(K+1). They document the cliff; they do not need the header to do it. decayed Combine is a pure function of its operands again. It read time.Now() to bound clock skew, and BytesStore.MergeUpdate recomputes Combine on every CAS retry, so two attempts on identical inputs produced different results — and the monoidlaws fuzzer evaluates the two associativity groupings at different instants. The skew bound cannot be derived from the operands: from inside Combine, a state four years in the future beside a real event is indistinguishable from a real event four years after a key went quiet, so any pairwise clamp safe for the first case hands an idle key back a quarter of its stale mass (the "idle key" row in TestDecayedSum_CombineAdoptsTheNewerFrame- Exactly is that counterexample). The bound moves to the lift instead, as compose.ClampFuture(t, now, bound) + compose.DefaultSkewBound(halfLife), which runs once per observation rather than once per merge attempt and is the last point where a clock reading is honest. EvaluateAt stays the read-side backstop. WithClock and WithClockSkewBound are gone. bloom's NewWithCapacity(n, p) parameters are no longer cosmetic. Identity ignores them by design and UnmarshalBinary overwrites the constructed shape, so the only thing they could honestly do is declare the shape the pipeline's lifts must produce — Combine now reports operands that agree with each other but not with that declaration, which is how a NewWithCapacity(1_000, 0.01) monoid could aggregate DefaultCapacity filters forever at an FPR nothing in the configuration predicted. Combine also stops allocating two throwaway 120 KB bit arrays per merge. Every remaining test was checked by reverting its fix and confirming it fails. Four that did not are dealt with: TestDecayedSum_SkewClampIsOptOut deleted (it asserted main's own behaviour); TestDecayedSum_IdleKeyStillDecaysNormally folded into the frame table test, which fails pre-fix; TestDecodeDecayed_ EmptyIsIdentity folded into TestDecodeDecayed_LengthContract; TestDecayedSum- Bytes_ForeignBlobWithoutHandlerStillRecovers given a non-zero fixture, since an all-zero blob decoded to Set=false and was discarded by the identity short-circuit either way. TestBloom_ShapeMismatchWithoutHandlerStillRecovers deleted — it cannot fail pre-fix, and monoidlaws already drives every reportDecodeError call site with a nil handler. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S
Kept out of the implementing commits so parallel branches did not all conflict on the [Unreleased] heading. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S
pequalsnp
force-pushed
the
fix/sketch-parameter-safety
branch
from
August 28, 2026 15:03
ae35ffd to
8d3c7aa
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Verified defects in
pkg/monoid/sketchandpkg/monoid/compose. Review returned needs-work and its most important finding removed a feature from this PR entirely.What review stopped
The first version added a TopK header field carrying total ingested weight, to make saturation detectable. Forward compatibility was handled — old rows have the flag clear and new code reads them. Backward was not, and a rolling deploy needs both.
Verified against the live soak's format:
An old task silently discards the accumulated sketch and CAS-writes a 1-event row back over it. During a rolling deploy with mixed tasks, that destroys accumulated state — on the pipeline currently under a 90-day soak whose entire value is accumulation.
(Note the bound check from #77 is what turns corruption into discard. Better, still silent.)
The wire-format change is out. Re-verified after removal:
mainreads branch-written rows exactly — 12 items, 40 events,Combineyields 41. It returns later as a properly staged two-phase change: read-both/write-old, then flip.What ships
Bloom shape mismatches are reported. Merging sketches with different
(m, k)shapes silently returned the left operand and merged to a permanentIdentity— with theWithDecodeErrorHandlerhook firing zero times, though its own doc claimed to cover exactly this. A bitwise OR of two differently-sized bit arrays has no definition, so reporting is the only honest option.One future-dated event no longer freezes a
DecayedSumkey. After a single event 4 years ahead, every later event becomes the older operand and itsExp2factor underflows to exactly 0 — the accumulator sticks at 1.0 forever, and the read path evaluates it to+Inf, pinning that key to rank #1 permanently. Fixed inCombine, not justEvaluateAt: a 1-year skew athalfLife=24hyields a finite 7.3e109 that still outranks every real score forever.halfLife <= 0is rejected. It was accepted, after whichCombineandEvaluateAtimplemented opposite semantics for the same config —hl=0, dt=0producedNaN, which survivesEncode/Decodeand poisons the row permanently. Reachable frommurmur.Trending(name, cfg.HalfLife)with an unsetDuration.composegained a decode-error handler, whichhll/topk/bloomhad and it lacked.DecodeDecayedon a foreign 200-byte HLL blob previously decoded toSet=truegarbage.The TopK saturation tests stay even without the header. They document the cliff on today's API: at K=32, 32 distinct entities returns 32 items summing to the full stream, while 33 returns 29 items summing to 29. Worth having, given the soak's shipped K=32 over a single
"global"key — its 12-hour run sat at 30 distinct entities, one below the edge, so it produced zero evidence about saturated behaviour.Two more review findings, both fixed
Combinehad started callingtime.Now(), making it impure.BytesStore.MergeUpdaterecomputesCombineon every CAS retry, so two attempts on identical inputs could differ — and associativity is fuzzed in CI. The skew bound now comes from the operands, not the wall clock.Six tests did not fail pre-fix, not four as reported.
TestDecayedSum_SkewClampIsOptOutpassed both ways: without a clampCombinealways adopts the newer T, which is exactly what it asserted. Every kept test is now verified by reversion.Test plan
bloom,topk,compose, plus non-default-capacity cases inmonoidlawsmake test-unit,golangci-lint0 issuesDeferred: TopK saturation remains byte-undetectable until the staged wire-format change lands.
🤖 Generated with Claude Code
https://claude.ai/code/session_01X2YMxeLgyRc9i5XPyEV75S