Skip to content

Commit 2ca2695

Browse files
Ryanmello07claude
andcommitted
review fixes: migration must not feed the recovery tracker
The tracker's entries close on the next provider-originated ingress for the destination -- and after a migration the OLD exit is alive and delivering until the server validates the new path, so every entry closed milliseconds later as a fake ~0s recovery and a fake rebindsAccepted: the two headline metrics this program is judged by, corrupted by the mechanism built to improve them. Migration arms nothing now; the [rel] migrate line is its field signal. Also: remaining counts flows genuinely still riding the exit (the pre-sweep subtraction counted stale book entries as stayers), and the seam wiring is pinned by source anchors -- deleting the two wiring lines previously failed no test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 parent 703a607 commit 2ca2695

2 files changed

Lines changed: 33 additions & 12 deletions

File tree

ip_remote_multi_client.go

Lines changed: 21 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2966,10 +2966,15 @@ func (self *RemoteUserNatMultiClient) migrateClientFlows(client *multiClientChan
29662966
}
29672967

29682968
rebindable := []*multiClientChannelUpdate{}
2969+
// pointing counts entries whose flow genuinely still rides this exit;
2970+
// stale entries (already re-raced away, or mid-re-race nil) belong to
2971+
// nobody's remaining count and are cleaned up by the send path
2972+
pointing := 0
29692973
for update := range updates {
29702974
if update.client.Load() != client {
29712975
continue
29722976
}
2977+
pointing += 1
29732978
// removeClient's partition: established quic moves; everything
29742979
// else stays -- here, stays ALIVE on the draining exit
29752980
if 0 < len(candidates) &&
@@ -2979,39 +2984,45 @@ func (self *RemoteUserNatMultiClient) migrateClientFlows(client *multiClientChan
29792984
rebindable = append(rebindable, update)
29802985
}
29812986
}
2982-
remaining = len(updates) - len(rebindable)
29832987
if len(rebindable) == 0 || len(candidates) == 0 {
2984-
remaining = len(updates)
2988+
remaining = pointing
29852989
return
29862990
}
29872991

2988-
var unplaced []*multiClientChannelUpdate
2989-
reboundFlows, replacements, unplaced = self.rebindFlowsWithLock(client, rebindable, candidates)
2992+
reboundFlows, replacements, _ = self.rebindFlowsWithLock(client, rebindable, candidates)
29902993
// unlike the removal, unplaced flows are NOT torn down: they stay
29912994
// registered on the alive exit and keep working
2992-
remaining += len(unplaced)
29932995

29942996
// the moved flows leave the draining exit's book, so the cap, the
29952997
// flowless check, and the eventual close's teardown all see the
29962998
// truth. rebindFlowsWithLock stored the replacement into each moved
29972999
// update's client, so "moved" is exactly "no longer points here" --
29983000
// the same catch-up bindClientFlow performs on the send path,
2999-
// inlined because bindClientFlow takes this lock.
3001+
// inlined because bindClientFlow takes this lock. After the sweep,
3002+
// the book holds exactly the flows still riding this exit, which is
3003+
// the honest remaining count (the pre-sweep subtraction counted
3004+
// stale entries as stayers; review finding, 2026-08-03).
30003005
for update := range updates {
30013006
if update.client.Load() != client {
30023007
delete(updates, update)
30033008
}
30043009
}
3010+
remaining = len(updates)
30053011
if len(updates) == 0 {
30063012
delete(self.clientUpdates, client)
30073013
}
30083014
}()
30093015
rebound = len(reboundFlows)
30103016

3011-
// recorded outside stateLock, same as the removal path: the tracker
3012-
// classifies each move by whether the destination keeps answering the
3013-
// same local port (migration accepted) or a new one (the app re-dialed)
3014-
self.reliabilityMetrics.exitLostRebound(reboundFlows)
3017+
// The recovery tracker is deliberately NOT armed here, unlike the
3018+
// removal path. Its entries close on the next provider-originated
3019+
// ingress for the destination -- and after a migration the OLD exit is
3020+
// still alive and still delivering until the server validates the new
3021+
// path, so every entry would close milliseconds later as a fake ~0s
3022+
// "recovery" and a fake rebindsAccepted, systematically corrupting the
3023+
// two headline metrics this program is judged by (review finding,
3024+
// 2026-08-03 -- the shipped-metric-measures-the-wrong-thing class). The
3025+
// [rel] migrate line below is this mechanism's field signal.
30153026

30163027
if 0 < rebound {
30173028
loggerOrDefault(self.log).Infof("%s\n", relEvent(

ip_remote_multi_client_rotation_test.go

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -312,8 +312,18 @@ func TestDrainBranchWarnsUnconditionally(t *testing.T) {
312312
// retirement stays a deadline teardown
313313
drainAt := strings.Index(body, `printStats("client drain")`)
314314
rest := body[drainAt:]
315-
if !strings.Contains(rest[:strings.Index(rest, "} else")], "markDrainMigrateOnce()") {
316-
t.Error("the drain branch does not run the once-latched migration: retirement tears down movable flows at the deadline instead of handing them off")
315+
drainBranch := rest[:strings.Index(rest, "} else")]
316+
if !strings.Contains(drainBranch, "markDrainMigrateOnce()") {
317+
t.Error("the drain branch does not latch the migration: retirement would migrate every resize pass, or never")
318+
}
319+
if !strings.Contains(drainBranch, "self.clientMigrateFunc(client)") {
320+
t.Error("the drain branch does not call the migration seam: retirement tears down movable flows at the deadline instead of handing them off")
321+
}
322+
// and the seam is actually WIRED by the parent for both windows -- a
323+
// correct-but-unwired seam is the failure mode this suite pins against,
324+
// and deleting the two wiring lines would otherwise fail no test
325+
if got := strings.Count(source, "clientMigrateFunc = multiClient.migrateClientFlows"); got < 2 {
326+
t.Errorf("the migration seam is wired %d time(s), want 2 (quality and speed windows): an unwired seam makes every drain a deadline teardown again", got)
317327
}
318328
if got := nextWarningCall(`printStats("client health warning")`); got != "setWarning(remove, warnUnhealthy)" {
319329
t.Errorf("the health-warning branch warns with %q, want the rank-derived setWarning(remove, warnUnhealthy)", got)

0 commit comments

Comments
 (0)