From e97f6b1687e431e7605f4d6c3ac49438eec83fdf Mon Sep 17 00:00:00 2001 From: "lei.chen" Date: Tue, 20 Oct 2026 02:21:42 +0000 Subject: [PATCH] fix(advisor): disarm dedup key on full resolution so recurrences report MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Live-cluster gap found 2026-10-20 (t1/t2/t3): t3 went down, recovered, went down again — and the second outage never reported. The dedup key survived the healthy rounds, so the recurring situation looked "unchanged" forever. Same hazard for pend_stall: a stall that empties and later re-forms identically stays silent. Fix: fire/clear pairing. When a detector's condition is fully absent (all nodes healthy, no stalled jobs, no skew), it clears its scenario's dedup key; the next occurrence is a fresh situation and fires. Unit tests pin the outage→recovery→identical-outage cycle for worker_health and the stall→resolve→re-form cycle for pend_stall. exit_burst deliberately does NOT clear: RecentExits is a per-scan window (consumed once), so an empty window is the steady state, not a resolution — clearing there would re-fire bursts whose situation never changed. Cluster-verified cycle: t3 close → fired (glm, 11.1s); open → silent; close again → fired again (22.1s, 2 findings). All detect tests green; advisor restarted on the fix. Co-Authored-By: Claude Code --- .../advisor/internal/detect/detect.go | 32 +++++++++-- .../advisor/internal/detect/detect_test.go | 57 +++++++++++++++++++ 2 files changed, 84 insertions(+), 5 deletions(-) diff --git a/src/frontend/advisor/internal/detect/detect.go b/src/frontend/advisor/internal/detect/detect.go index 1741cde4..e88a8841 100644 --- a/src/frontend/advisor/internal/detect/detect.go +++ b/src/frontend/advisor/internal/detect/detect.go @@ -1,8 +1,9 @@ // Package detect holds the advisor's deterministic triggers — cheap, no // LLM. A detector looks at one snapshot and decides whether a scenario is -// worth a provider round: threshold, then dedup (never re-evaluate an -// identical situation), then cooldown (dampen A→B→A alternations that -// last-key dedup cannot catch). +// worth a provider round: threshold, then dedup. The dedup key hashes the +// situation: an unchanged situation never re-fires, any change fires +// immediately, and a situation that resolves fully disarms its key so a +// later recurrence reports again (fire/clear pair). package detect import ( @@ -26,7 +27,8 @@ const ( type Trigger struct { Scenario string // DedupKey hashes the situation (job set + reason set / node set). The - // same key never fires twice in a row; any change fires immediately. + // same key never fires twice in a row; any change fires immediately; + // a full resolution disarms the key (clear) so a recurrence fires again. DedupKey string // Fingerprint is a human-readable summary of what matched (logged, and // what the echo provider re-derives from the state). @@ -71,7 +73,9 @@ func NewDetector(lim Limits) *Detector { } // Detect runs all detectors over the snapshot and returns the scenarios to -// evaluate this round, in stable order. +// evaluate this round, in stable order. Each detector arms its dedup key +// when it fires and disarms it (clear) when its condition is absent, so a +// resolved-then-recurred situation reports again. func (d *Detector) Detect(st *snapshot.State) []Trigger { var out []Trigger if t, ok := d.pendStall(st); ok { @@ -107,8 +111,20 @@ func (d *Detector) fire(scenario, key, fingerprint string) (Trigger, bool) { return Trigger{Scenario: scenario, DedupKey: key, Fingerprint: fingerprint}, true } +// clear forgets the scenario's dedup key when its condition is absent this +// round. Without this, a situation that resolves and then recurs identically +// (same node goes down twice, the same stall re-forms) is swallowed as +// "unchanged" forever — observed live (2026-10-20): t3 went down, recovered, +// went down again, and the second outage never reported. The key is armed by +// fire() and disarmed by clear(); each detector's healthy/empty early-return +// is the disarm point. +func (d *Detector) clear(scenario string) { + delete(d.lastKey, scenario) +} + func (d *Detector) pendStall(st *snapshot.State) (Trigger, bool) { if len(st.PendTop) == 0 && st.Cluster.PendingJobs < d.lim.PendSpikeMin { + d.clear(ScenarioPendStall) return Trigger{}, false } // Key: the stalled job set and their reasons. @@ -126,6 +142,7 @@ func (d *Detector) workerHealth(st *snapshot.State) (Trigger, bool) { } } if len(bad) == 0 { + d.clear(ScenarioWorkerHealth) return Trigger{}, false } key := hashOf(join(bad, ",")) @@ -146,6 +163,7 @@ func (d *Detector) utilSkew(st *snapshot.State) (Trigger, bool) { } } if len(hot) == 0 || len(idle) == 0 { + d.clear(ScenarioUtilSkew) return Trigger{}, false } key := hashOf("hot=" + join(hot, ",") + ";idle=" + join(idle, ",")) @@ -161,6 +179,10 @@ func (d *Detector) exitBurst(st *snapshot.State) (Trigger, bool) { } } if len(ids) < d.lim.ExitBurstMin { + // Note: NOT cleared. RecentExits is a per-scan window (exits are + // consumed once, see snapshot Build); an empty window is the steady + // state, not "the burst resolved" — arming/disarming here would + // re-fire a burst whose situation never actually changed. return Trigger{}, false } key := hashOf(join(ids, ",")) diff --git a/src/frontend/advisor/internal/detect/detect_test.go b/src/frontend/advisor/internal/detect/detect_test.go index 3d26a9f9..da018c70 100644 --- a/src/frontend/advisor/internal/detect/detect_test.go +++ b/src/frontend/advisor/internal/detect/detect_test.go @@ -98,6 +98,63 @@ func TestDetect_WorkerHealth(t *testing.T) { } } +// TestDetect_RecoveredThenRecurred fires again covers the live-cluster gap +// found 2026-10-20 (t1/t2/t3): t3 went down, recovered, went down again — +// and the second outage was silently deduped because the key survived the +// healthy rounds. Resolution must disarm the scenario's key. +func TestDetect_RecoveredThenRecurredFiresAgain(t *testing.T) { + st := healthyState() + st.Nodes[1].Status = "down" + d := NewDetector(DefaultLimits()) + if trigs := d.Detect(st); len(trigs) != 1 { + t.Fatalf("first outage must fire: %+v", trigs) + } + + // Recovery: all nodes healthy → no trigger, key disarmed. + st.Nodes[1].Status = "OK" + if got := d.Detect(st); len(got) != 0 { + t.Fatalf("recovery must not fire: %+v", got) + } + + // The identical outage recurs → must fire again (was silently deduped). + st.Nodes[1].Status = "down" + trigs := d.Detect(st) + if len(trigs) != 1 || trigs[0].Scenario != ScenarioWorkerHealth { + t.Fatalf("recurred identical outage must fire again: %+v", trigs) + } + + // And the fire/clear cycle is repeatable. + st.Nodes[1].Status = "OK" + if got := d.Detect(st); len(got) != 0 { + t.Fatalf("second recovery must not fire: %+v", got) + } + st.Nodes[1].Status = "down" + if trigs := d.Detect(st); len(trigs) != 1 { + t.Fatalf("third outage must fire: %+v", trigs) + } +} + +// Same contract for pend_stall: the stalled set emptying disarms the key, so +// a stall re-forming from scratch reports again. +func TestDetect_PendStall_ResolvedThenReformed(t *testing.T) { + st := stalledState() + d := NewDetector(DefaultLimits()) + if trigs := d.Detect(st); len(trigs) != 1 || trigs[0].Scenario != ScenarioPendStall { + t.Fatalf("stall must fire: %+v", trigs) + } + + // Stall resolves (no pending at all) → silent, key disarmed. + empty := healthyState() + if got := d.Detect(empty); len(got) != 0 { + t.Fatalf("resolution must not fire: %+v", got) + } + + // The same stall re-forms → fires again. + if trigs := d.Detect(st); len(trigs) != 1 || trigs[0].Scenario != ScenarioPendStall { + t.Fatalf("re-formed stall must fire again: %+v", trigs) + } +} + func TestDetect_UtilSkew(t *testing.T) { st := healthyState() st.Nodes[0].Utilization = 0.9