fix(routing): decay quarantine reconviction count over quiet time

Author: Ryanmello07

ip_remote_multi_client.go

@@ -9955,18 +9955,18 @@ type multiClientChannel struct {
 	// as quarantineMigrated and the rest of the episode bookkeeping beside
 	// it, not new durable state. Guarded by stateLock.
 	//
-	// KNOWN LIMITATION, tracked separately, not yet fixed: this counter never
-	// DECAYS within a session -- a provider that flaps early and then runs
-	// clean for hours still escalates straight to the 240s cap on its next
-	// bench rather than re-starting at 60s. The blast radius is bounded and
-	// self-healing even so: the worst case is a stale-bad exit taking up to
-	// 240s instead of 60s to be force-evicted by the expiry escape, and
-	// release-on-receive-progress (addReceiveAck's clear, unrelated to this
-	// counter) remains a fully independent per-poll acquittal path this
-	// cannot delay -- a genuinely recovered exit is never held past the
-	// evidence that acquits it. Clean-interval decay analogous to
-	// quarantineMemoryDuration should land before QuarantineDampening is
-	// ever turned on for real traffic.
+	// This raw field is the historical high-water mark and is NEVER
+	// decremented in storage -- decay is applied lazily at read time by
+	// quarantineReconvictionCount(), which removes one step per whole
+	// quarantineReconvictionDecayInterval elapsed since quarantineLiftTime
+	// (the same stamp this field advances beside above, so a fresh
+	// conviction resets the decay clock for free -- no separate field to
+	// keep in sync). See quarantineReconvictionDecay for the pure math.
+	// QuarantineDampening off short-circuits the whole computation --
+	// quarantineReconvictionCount() returns this field verbatim, no clock
+	// read at all -- so a default build's benchDuration, and the
+	// StallEvents telemetry this field also feeds (see exitMetricsSnapshot),
+	// stay byte-for-byte what they were before decay existed.
 	quarantineReconvictions int
 
 	// pendingSendTime is when the current run of unacked sends began, reset on
@@ -10623,12 +10623,66 @@ func (self *multiClientChannel) quarantineState() (blackholeReason, time.Time) {
 	return self.quarantineReason, self.quarantineStart
 }
 
+// quarantineReconvictionDecayInterval is the clean interval one reconviction
+// step decays over: quarantineReconvictionCount() removes one step from the
+// raw count for every whole interval elapsed since quarantineLiftTime, the
+// stamp of the LAST completed bench-then-lift cycle (the same instant
+// quarantineReconvictions itself advances on -- see clearQuarantineWithLock
+// -- so a fresh conviction resets this clock for free). Set equal to
+// quarantineMemoryDuration: the existing answer this file already gives for
+// "how long counts as a clean interval" for the sibling survived-quarantine
+// memory, reused rather than inventing a second opinion on the same
+// question -- though the two remain independent constants should they ever
+// need to diverge.
+const quarantineReconvictionDecayInterval = quarantineMemoryDuration
+
+// quarantineReconvictionDecay returns reconvictions with one step removed
+// for every whole quarantineReconvictionDecayInterval elapsed is worth,
+// floored at 0. A non-positive reconvictions or a non-positive elapsed (a
+// clock problem, not evidence of a longer clean interval -- the same
+// convention reentryScorePenalty's elapsed<0 clamp uses) both read as "no
+// decay to apply" rather than propagating toward a negative result: a
+// negative count feeding benchDuration or exitScore's StallEvents term is
+// exactly the defect class this branch has already shipped twice (negative
+// StallEvents scoring as a bonus, a zero RTT scoring best), so this must
+// never produce one. Pure: no clock reads, no locks -- mirrors benchDuration
+// and reentryScorePenalty.
+func quarantineReconvictionDecay(reconvictions int, elapsed time.Duration) int {
+	if reconvictions <= 0 {
+		return 0
+	}
+	if elapsed <= 0 {
+		return reconvictions
+	}
+	decayed := reconvictions - int(elapsed/quarantineReconvictionDecayInterval)
+	if decayed < 0 {
+		return 0
+	}
+	return decayed
+}
+
 // quarantineReconvictionCount reads the completed bench-then-lift cycle
-// count benchDuration escalates on; see the field comment.
+// count benchDuration escalates on; see the field comment. With
+// QuarantineDampening on, the raw count is decayed by elapsed quiet time
+// since the last completed cycle (quarantineReconvictionDecay, above) before
+// being returned, so a channel that reconvicted long ago and has been clean
+// since reads a low count again rather than the historical peak. With the
+// knob at its zero value (off) this returns the raw field verbatim and does
+// not even read the clock -- so a default build's reading, and everything
+// downstream of it (benchDuration, and exitMetricsSnapshot's StallEvents),
+// stays byte-for-byte what it was before this decay existed.
 func (self *multiClientChannel) quarantineReconvictionCount() int {
+	// reliabilitySettings() is a bare atomic load (see its own comment),
+	// never blocking, so reading it before the lock adds no new lock
+	// ordering and keeps the locked section to the two fields below.
+	dampening := self.reliabilitySettings().QuarantineDampening
+
 	self.stateLock.Lock()
 	defer self.stateLock.Unlock()
-	return self.quarantineReconvictions
+	if !dampening || self.quarantineLiftTime.IsZero() {
+		return self.quarantineReconvictions
+	}
+	return quarantineReconvictionDecay(self.quarantineReconvictions, time.Now().Sub(self.quarantineLiftTime))
 }
 
 // quarantineReentryElapsed reports how long it has been since this channel's

routing_quarantine_test.go

@@ -155,6 +155,117 @@ func TestQuarantineReconvictionsSurviveReceiveProgressRelease(t *testing.T) {
 	}
 }
 
+// TestQuarantineReconvictionDecaySteps is the pure decay math Task 6 adds:
+// one step removed per whole quarantineReconvictionDecayInterval of elapsed
+// time, floored at 0, with both a negative count and a negative elapsed
+// (clock trouble, not evidence of a longer clean interval -- the same
+// convention reentryScorePenalty's elapsed<0 clamp already uses) reading as
+// "no decay to apply" rather than propagating a negative result. A negative
+// reconviction count feeding benchDuration or exitScore's StallEvents term
+// is exactly the defect class this branch has already shipped twice
+// (negative StallEvents scoring as a bonus, a zero RTT scoring best), so
+// this must never produce one.
+func TestQuarantineReconvictionDecaySteps(t *testing.T) {
+	interval := quarantineReconvictionDecayInterval
+
+	if got := quarantineReconvictionDecay(5, 0); got != 5 {
+		t.Fatalf("zero elapsed must not decay: got %d, want 5", got)
+	}
+	if got := quarantineReconvictionDecay(5, interval-time.Second); got != 5 {
+		t.Fatalf("just under one elapsed interval must not decay yet: got %d, want 5", got)
+	}
+	if got := quarantineReconvictionDecay(5, interval); got != 4 {
+		t.Fatalf("exactly one elapsed interval must remove exactly one step: got %d, want 4", got)
+	}
+	if got := quarantineReconvictionDecay(5, 2*interval); got != 3 {
+		t.Fatalf("two elapsed intervals must remove exactly two steps: got %d, want 3", got)
+	}
+	if got := quarantineReconvictionDecay(5, 100*interval); got != 0 {
+		t.Fatalf("many elapsed intervals must floor at 0, not go negative: got %d, want 0", got)
+	}
+	if got := quarantineReconvictionDecay(0, 100*interval); got != 0 {
+		t.Fatalf("a count already at 0 must stay 0: got %d, want 0", got)
+	}
+	if got := quarantineReconvictionDecay(-3, interval); got != 0 {
+		t.Fatalf("a negative count must clamp to 0, not go more negative: got %d, want 0", got)
+	}
+	if got := quarantineReconvictionDecay(5, -time.Second); got != 5 {
+		t.Fatalf("negative elapsed must clamp to no-decay, got %d, want 5", got)
+	}
+}
+
+// TestQuarantineReconvictionCountDecaysOverQuietTime is Task 6's channel-level
+// proof: with QuarantineDampening on, quarantineReconvictionCount() decays
+// the count by whole quarantineReconvictionDecayInterval steps measured from
+// quarantineLiftTime (the last completed lift), floors at 0 once enough
+// quiet time has passed, and a fresh conviction is visible immediately
+// afterward rather than reading as still-decayed. Deliberately picks
+// intervals where the decayed and un-decayed readings differ (1 and 0
+// against a raw count of 2) so this cannot pass against the un-fixed
+// always-climbing counter.
+func TestQuarantineReconvictionCountDecaysOverQuietTime(t *testing.T) {
+	client := stallTestChannel()
+	client.settings.QuarantineDampening = true
+
+	// two completed bench-then-lift cycles -> raw reconvictions = 2
+	client.setQuarantined(blackholeNoReceiveAck)
+	client.clearQuarantine()
+	client.setQuarantined(blackholeNoReceiveSyn)
+	client.clearQuarantine()
+	if got := client.quarantineReconvictionCount(); got != 2 {
+		t.Fatalf("expected 2 reconvictions right after the second lift, got %d", got)
+	}
+
+	// age the last lift back by just over one decay interval: exactly one
+	// step must be removed
+	client.stateLock.Lock()
+	client.quarantineLiftTime = time.Now().Add(-quarantineReconvictionDecayInterval - time.Second)
+	client.stateLock.Unlock()
+	if got := client.quarantineReconvictionCount(); got != 1 {
+		t.Fatalf("one elapsed decay interval must remove exactly one step, got %d, want 1", got)
+	}
+
+	// age it back far enough for well past both steps: must floor at 0, not
+	// go negative
+	client.stateLock.Lock()
+	client.quarantineLiftTime = time.Now().Add(-5*quarantineReconvictionDecayInterval - time.Second)
+	client.stateLock.Unlock()
+	if got := client.quarantineReconvictionCount(); got != 0 {
+		t.Fatalf("enough quiet time must decay reconvictions to exactly 0, got %d, want 0", got)
+	}
+
+	// a fresh conviction resets the decay clock: the new lift's elapsed time
+	// is ~0, so the count must be visible right away, not read as decayed
+	client.setQuarantined(blackholeNoReceiveAck)
+	client.clearQuarantine()
+	if got := client.quarantineReconvictionCount(); got == 0 {
+		t.Fatal("a fresh conviction must reset the decay clock, not read as still-decayed")
+	}
+}
+
+// TestQuarantineReconvictionCountInertWhenDampeningOff pins the zero-value-off
+// contract: with QuarantineDampening at its zero value (false), an aged
+// quarantineLiftTime must not decay the count at all -- quarantineReconvictionCount()
+// must keep returning the raw, ever-climbing reading a default build has
+// always returned, with no clock read and no decay computation performed.
+func TestQuarantineReconvictionCountInertWhenDampeningOff(t *testing.T) {
+	client := stallTestChannel()
+	// QuarantineDampening left at its zero value (false)
+
+	client.setQuarantined(blackholeNoReceiveAck)
+	client.clearQuarantine()
+	client.setQuarantined(blackholeNoReceiveSyn)
+	client.clearQuarantine()
+
+	client.stateLock.Lock()
+	client.quarantineLiftTime = time.Now().Add(-10 * quarantineReconvictionDecayInterval)
+	client.stateLock.Unlock()
+
+	if got := client.quarantineReconvictionCount(); got != 2 {
+		t.Fatalf("QuarantineDampening off must never decay the count, got %d, want 2", got)
+	}
+}
+
 // TestReentryScorePenaltyDecaysToZero is the pure decay curve: full weight
 // at the instant of release (elapsed==0), zero once ramp has fully elapsed,
 // and strictly decreasing in between. ramp<=0 is the zero-value-off legacy