Repository navigation
fix(anomaly): periodic refinement, mixed-burst labels and censored-episode confidence - #108
Merged
Merged
Conversation
Two deterministic reproductions for the periodic detector in the merged (not yet integrated) internal/anomaly library. Both fail on master: - A 5m train with gaps alternating 294s/306s, MinPeriod=299s, MaxPeriod=301s, tolerance 6s: 300s explains every gap and meets every criterion (asserted as a fixture guard), but every seed g/k lies outside the range and is dropped before chainRefined can refine it into 300s. - A periodic chain whose bursts merge an expected and an unclassified event is labelled unclassified instead of mixed when the newest pulse starts with an unclassified event, or when the only mixed burst sits among unclassified pulses. Controls cover the label cases that are already correct. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Periodic detector of the merged, not yet integrated internal/anomaly library. Narrow period range: estimate() dropped every seed g/k outside [MinPeriod, MaxPeriod] before chainRefined could refine it, so a range narrower than the jitter (gaps 294s/306s, range 299s..301s, tol 6s) found no period although 300s meets every criterion. A seed is now measured while some period in range may explain its gap as k periods (seedInReach: k*MinPeriod - tol(MinPeriod) <= g <= k*MaxPeriod + tol(MaxPeriod), sound for absolute and relative jitter since k*P - tol(P) never decreases in P). An out-of-range seed only feeds its refinement and is never the estimate itself. The seed count per pulse is still at most recentGaps*(MaxMissing+1), so searchCells (Chance) and periodicWorkBound are unchanged; measuredSteps is refreshed (+0..6.8% gaps visited). Mixed bursts: the chain label counted only all-expected pulses, so a chain whose only expected events sat in bursts together with unclassified events came out unclassified. Such a chain is now mixed; monitored still wins, and all-expected chains stay expected. Adds a property test for seedInReach (soundness under absolute, relative and combined jitter, and tight bounds) and an invariant test that an out-of-range seed is measured but never reported. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…exhaustively From the independent review of 74f86e8 (verdict: approve, one should-fix). - With a tolerance below k-1 ns, seedInReach alone rejected a gap in (k*MaxPeriod+tol, k*MaxPeriod+k-1] whose seed floor(g/k) = MaxPeriod is in range, so the fix could drop a seed master measured and lower the estimate. estimate() now measures every in-range seed and, beyond them, the out-of-range seeds seedInReach keeps: the seed pool is a superset of master's and the in-range ranking is unchanged. Regression test: TestPeriodicInRangeSeedIsAlwaysMeasured (fails with seedInReach-only admission). - seedInReach's comment states that the test is necessary, not sufficient, and gives the monotonicity argument including the 1 ns truncation step of tolNanos. - TestSeedInReachIsSoundExhaustively checks every explained gap over small integer periods, with absolute, relative and combined jitter and every k; the sampled test no longer restates the formula as a tightness check. - The narrow-range reproduction also runs with a relative tolerance. Work counters of TestPeriodicSearchWorkIsBounded are unchanged by this commit. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…fidenceLow The two remaining findings from the review of the merged (not yet integrated) anomaly library. Every new test fails on 9899b9f at its intended assertion: - TestPeriodicRelativeJitterRefinesWithTheCandidateTolerance: JitterRel 2%, gaps repeating 305.95s, 305.95s, 294.05s. 300s explains every gap within its own 6s tolerance and meets every criterion (asserted as a fixture guard), but chainRefined builds the common range with the seed's tolerance, so no seed refines to a period that explains the train and it is never detected. - TestCensoredNewStreamKeepsLowConfidenceOn{QuietEnd,EvictionEnd,Reopen}: a new-stream episode opened from censored evidence is reported low, but its quiet end, its eviction end and a reopen within the cooldown (by an uncensored reactivation) come out as route_observed. Controls (pass before and after): an uncensored stream is never lowered, and the low confidence does not carry into a new episode after the cooldown. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…eLow per episode
Relative jitter (P2): chainRefined intersected the ranges
[(g-tol)/k, (g+tol)/k] with the seed's tolerance, although a period P
explains gap g with its own, tol(P) = max(JitterAbs, JitterRel*P). The
new periodRange returns exactly {P : |g - k*P| <= tol(P)}: the integer
ends as before when JitterRel is 0 (proven against the old formula as an
oracle), and otherwise float estimates of g/(k+JitterRel) and
g/(k-JitterRel) stepped to the exact ends (k*P -+ tol(P) never decrease
in P). Still at most one refined period per seed, so searchCells and the
Chance union bound are unchanged.
The refined period is now clamped into the common range intersected with
[MinPeriod, MaxPeriod]. With exact ranges a common range can straddle
MaxPeriod; unclamped, the phase estimate above it was rejected although
periods in range explain every gap (15 of 10,000 narrow-range edge
trains found by 9899b9f were lost without it). Under absolute jitter
this only adds detections at the range ends.
periodRange results are memoized in the Detector-owned scratch, one slot
per (pulse mod recentGaps, k), at most recentGaps*(maxMissingLimit+1)
entries; an entry is used only when g, JitterAbs and JitterRel match.
Lost ConfidenceLow (P3): the episode now records that one of its hot
signals came from censored new-stream evidence (like its traffic label
and coverage), every transition carries it, and emit lowers the
confidence for any transition of such an episode: quiet end, eviction end
and a reopen within the cooldown. A new episode starts without it.
Golden: exactly one field changes, the quiet end of censored episode
7771b168 from route_observed to low (sha256 3d6fbf08...). The golden
guard keeps its pinned hash and maps that one documented field back.
The pinned signals of Monte Carlo train 1240 become 57/2/69: the train,
broken at gap 58, is found again as 172.48s (residual 2.561s <= 2.587s,
Chance 1.1e-11), which the seed-tolerance range could not reach.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Owner
Author
|
Review feedback addressed in commit
|
dborup
changed the title
dborup
marked this pull request as ready for review
September 28, 2026 15:18
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.
Summary
This PR fixes all four findings from the review of the anomaly library merged in PR #94. That library is not yet wired into production: no server or ingestor code imports it, and CI only vets and tests it as its own module. This PR adds no runtime integration, schema change or deploy, and every change is inside
internal/anomaly/.[MinPeriod, MaxPeriod]is narrower than the jitterestimate()dropped every seedg/koutside the range beforechainRefinedcould refine itseedInReachholds, but used only for their refinement; in-range seeds are always measuredTestPeriodicNarrowRangeFindsPeriodBehindOutOfRangeSeeds(absolute and relative)unclassifiedmixed;monitoredstill winsTestPeriodicBurstTrafficLabelJitterRel > 0a valid period is never foundchainRefinedbuilt the common range with the seed's tolerance, not each candidate period'speriodRangereturns exactly{P : |g − k·P| ≤ tol(P)}, and the refined period is clamped into common range ∩[MinPeriod, MaxPeriod]TestPeriodicRelativeJitterRefinesWithTheCandidateTolerancelow, but later transitions come outroute_observedlow: quiet end, eviction end and reopen within cooldownTestCensoredNewStreamKeepsLowConfidenceOn{QuietEnd,EvictionEnd,Reopen}Commits
827cf50etest: reproduces findings 1 and 2.74f86e81fix: findings 1 and 2.9899b9f6reviewfix: always measure in-range seeds, and proveseedInReachexhaustively.0a30e8b4test only: reproduces findings 3 and 4. All four new tests fail on9899b9f6at their intended assertions; the controls pass.cfbc7c84fix: findings 3 and 4, the clamp, the memo, the golden update and the train-1240 expectation.Files:
periodic.go,detector.goandstate.go, plus tests inperiodic_followup_test.go,confidence_followup_test.go,review_fixes_test.go,chance_test.goandperiodic_bench_test.go, andtestdata/replay_golden.json.Details of findings 1 and 2 (from the earlier commits)
Finding 1.
seedInReach(g, k)keeps a seed whilek·MinPeriod − tol(MinPeriod) ≤ g ≤ k·MaxPeriod + tol(MaxPeriod).k·P ± tol(P)never decreases in P.recentGaps·(MaxMissing+1), sosearchCells(the Chance union bound) andperiodicWorkBoundstill hold.Finding 2. The chain is evaluated when its newest pulse starts, which is causal, so that pulse holds only its first event. The repro cases therefore do not rely on the newest pulse.
Details of finding 3
periodRange(g, k)returns the exact integer range of periods P that explain gap g as k periods, each with its own tolerancetol(P) = max(JitterAbs, ⌊JitterRel·P⌋).JitterRel = 0: the two integer divisions of the old code.TestPeriodRangeWithAbsoluteJitterIsTheOldRangecompares them with the old formula as an oracle: exhaustive for g ≤ 2000, k ≤ 9 and JitterAbs ≤ 40, plus 200,000 samples at nanosecond scale.JitterRel > 0: floating-point estimates ofg/(k+JitterRel)andg/(k−JitterRel)are stepped to the exact ends.k·P ± tol(P)never decreasing in P.TestPeriodRangeIsExactlyThePeriodsThatExplainTheGapchecks it by brute force over small integers for absolute, relative and combined jitter, and checks the defining ends on 200,000 samples at nanosecond scale.searchCellsargument, that the highest lower endL_jof the common range explains every other gap, carries over withL_jas the lowest period explaining gap j. Chance andsearchCellsare unchanged.Clamp into the search range (approved). With exact ranges, a common range can straddle
MaxPeriod. The phase estimate (the mean gap) then lies beyond it and used to be rejected, although periods in range explain every gap. Without the clamp, 15 of 10,000 narrow-range edge trains that9899b9f6found were lost (McNemar p = 6e-5). With the clamp none are lost anywhere, and under absolute jitter it only adds detections at the range ends: narrow-abs6 edge trains go from 9,991 to 10,000.TestPeriodicRefinementIsClampedIntoTheSearchRangecovers both jitter types, with fixture guards that the true period meets every criterion and that the mean gap exceedsMaxPeriod.Memo.
periodRangeresults are cached in the scratch that the Detector owns.(pulse mod recentGaps, k), so at most 144 entries, allocated once.TestRangeMemoIsNeverStalemakes 200,000 interleaved queries across rules, gaps, multiples and shared slots.Details of finding 4
episode.notenow also records whether the hot signal came from censoredNewStreamEvidence.move()copies it onto each transition, andemit()lowers the confidence of every transition of such an episode, whether or not it has a trigger.lowdoes not leak into the next episode.Golden and changed expectations (approved)
testdata/replay_golden.json: exactly one field changes. The quiet end of censored episode7771b168goes fromroute_observedtolow; that episode opens aslowin the same golden. The new sha256 is3d6fbf08….TestGoldenChangedOnlyInTheDocumentedChanceValueskeeps its pinned hash unchanged. It now asserts that this one quiet end islowand maps it back before hashing, so any other change to the golden still fails.TestRefinedCandidateReplacesOnlyWhenItWouldSignal, train 1240: the expectation changes from 57/1/57 to 57/2/69, with the reason in the test comment.measuredSteps: not updated. The total gaps visited change by +3.1 % at most (jitter/1: 6,242,089 → 6,434,483), well inside the 25 % guard. The peak per pulse is 114 chains and 11,765 steps, against a bound of 302 and 79,314.Mutation evidence
All 22 mutants are killed, each on a scratch copy of the final tree with the full package test suite.
…RelativeJitterRefinesWithTheCandidateTolerance, the train-1240 testperiodRangeoff by one (lo, hi)TestPeriodRangeWithAbsoluteJitterIsTheOldRange, and 7 more…IsExactlyThePeriodsThatExplainTheGap, clamp test, and 4 moreTestPeriodRangeIsExactlyThePeriodsThatExplainTheGap…OnQuietEnd,…OnReopen, golden replay…OnEvictionEnd…OnReopennotenever records censoringemitignores the episode flag (master behaviour)lowleaks into the next episodeTestNewStreamConfidenceIsNotLoweredWithoutCensoringTestPeriodicRefinementIsClampedIntoTheSearchRangeTestRangeMemoIsNeverStaleFalse alarms and detection
Paired probe: identical trains fed to master
b3e44761,9899b9f6and this PR, with 10,000 trains of 200 pulses per cell, each train using a fixed seed. A train counts as a false alarm if it signals at least once, and as a detection if it signals within tol of its true period.Configs:
fixture(the test rule: JitterAbs 5 s, JitterRel 2 %),rel2(JitterRel 2 %),rel3abs1-tight(3 % + 1 s, MaxChance 1e-9),rel25(JitterRel 0.25, the maximum),narrow-rel2andnarrow-abs6(range [299 s, 301 s]),abs6,rel2-miss8(MaxMissing 8) andrel2-loose(MaxChance 1).False alarms (null trains; Poisson with a mean gap of 300 s, and uniform gaps in [30 s, 570 s])
Master and
9899b9f6are identical in every null cell.Confirmed on 100,000 further independent trains for
rel25:periodRange. A variant without the clamp gives identical results.rel2,rel2-loose,rel3abs1-tight), uniform null, 100,000 trains each: 1 vs 1 and 0 vs 0 (95 % upper bound 0.003 %).This is accepted as a documented limitation: see "Known limitations".
Detection (no train detected before is lost)
"Jittered" trains have residuals up to ±0.95·tol, 10 % missing pulses and 2 % stray pulses. "Edge" trains have residuals of 0.90–0.99·tol, with two of three gaps long, as in the finding 3 reproduction. Master detects no narrow-range edge train at all; that is finding 1.
Performance
BenchmarkPeriodicSearchwas run interleaved: prebuilt test binaries for master,9899b9f6and this PR, 3 rounds × 2 runs each (n = 6 per version), with the machine's load average between 3.0 and 4.4 on 12 cores. The table shows medians.n = 6 per version (3 interleaved rounds x 2). Delta vs 9899b9f: -0.2% .. +35.4%; vs master: +0.6% .. +42.5%.
Costs:
fixture/alternatingis +35 %, and 41 % more gaps are visited per pulse (73 → 103), because the exact ranges let the range walk go further. Bursty and noise shapes are about +13–18 %.Test results
All of the following were run locally on the final tree
cfbc7c84:internal/anomaly:gofmt -l .is empty,go vet ./...is clean, andgo test -count=1 ./...andgo test -race -count=1 ./...pass.TestIndependent*run with-count=10: 60/60 pass.git diff --checkis clean.3d6fbf08….TestOfflineReplayskips, as on master: it needs explicit replay flags.Review
74f86e81and9899b9f6in a pre-review, with no blockers.Known limitations
searchCellscomment already notes it is not a bound for traffic more regular than Poisson.MinPeriodorMaxPeriodwhen the train's own estimate lies just beyond the range, as long as an in-range period explains every gap. A steady train whose gaps are all outside the range, such as constant 294 s gaps with range [299 s, 301 s], is still not reported, because the seed's own chain already explains it (TestPeriodicOutOfRangeSeedIsNeverReported).seedInReachis necessary, not sufficient. Some admitted out-of-range seeds cannot be refined. They cost work but stay within the documented bound.🤖 Generated with Claude Code