fix(node-sync): do not latch node disable when master allows (#6870) (#6876)

* fix(node-sync): do not latch node disable when master allows (#6870)

nodeDisableIsStale trusted any node-reported enable=false whose limits matched
the master's, treating it as a genuine verdict and latching it on the master.
But matching limits no longer prove the verdict: a single stray false
(a depleted snapshot the master later re-enabled, a stale
disableInvalidClients hit, or a push that lagged a quota reset) leaves the
client stuck disabled on the node, and every master tick flips the client's
state again. This is the loop reported in #6870.

Make nodeDisableIsStale a single rule: stale iff the master's own deadline
and quota still permit the client. A genuine combined-quota or expiry
disable (masterLimitsAllowClient == false) still latches, preserving #4917.
Mirror the same rule in ClientTrafficEnableMergeExpr so the SQL UPDATE and
the Go in-memory replay cannot drift.

* fix(node-sync): keep node usage over quota latching a node disable

Dropping the matching-limits latch (#6870) let the master re-enable a
client the node had genuinely counted past its quota whenever the
master's own counters lagged, e.g. after a seeded-at-zero adoption,
reopening #4917. A node's enable=false now stays latched when the
node's own usage already spends the master's quota; otherwise the
master's limits decide, so a stray disable no longer loops.

Restores TestNodeQuotaDisable_SameExpiryStillLatches as it was on main.

---------

Co-authored-by: Alireza Ahmadi <alireza7@gmail.com>
This commit is contained in:
25f2002654
2026-10-23 22:34:46 +05:30
committed by GitHub
parent e2184777a0
commit f172bc3f10
3 changed files with 72 additions and 32 deletions
@@ -54,12 +54,6 @@ func TestNodeDisableIsStale(t *testing.T) {
wantStale bool
}{
{name: "nil master", node: xray.ClientTraffic{ExpiryTime: earlyAbs}},
{
name: "matching limits are a genuine verdict",
master: &xray.ClientTraffic{ExpiryTime: lateAbs, Total: quota},
node: xray.ClientTraffic{ExpiryTime: lateAbs, Total: quota},
wantStale: false,
},
{
name: "node still holds the pre-extension deadline",
master: &xray.ClientTraffic{ExpiryTime: lateAbs},
@@ -69,7 +63,7 @@ func TestNodeDisableIsStale(t *testing.T) {
{
name: "node still holds the pre-top-up quota",
master: &xray.ClientTraffic{ExpiryTime: lateAbs, Total: 2 * quota, Up: 60, Down: 50},
node: xray.ClientTraffic{ExpiryTime: lateAbs, Total: quota},
node: xray.ClientTraffic{ExpiryTime: lateAbs, Total: quota, Up: 60, Down: 50},
wantStale: true,
},
{
@@ -102,10 +96,21 @@ func TestNodeDisableIsStale(t *testing.T) {
node: xray.ClientTraffic{ExpiryTime: -2592000000},
wantStale: true,
},
{
name: "matching limits, neither master nor node usage spends the quota (#6870)",
master: &xray.ClientTraffic{ExpiryTime: lateAbs, Total: quota, Up: 10, Down: 10},
node: xray.ClientTraffic{ExpiryTime: lateAbs, Total: quota, Up: 10, Down: 10},
wantStale: true,
},
{
name: "node usage spends the quota the master undercounts (#4917)",
master: &xray.ClientTraffic{ExpiryTime: lateAbs, Total: quota, Up: 40, Down: 50},
node: xray.ClientTraffic{ExpiryTime: lateAbs, Total: quota, Up: 60, Down: 50},
},
}
for _, c := range cases {
t.Run(c.name, func(t *testing.T) {
if got := nodeDisableIsStale(c.master, c.node, now, c.deltaUp, c.deltaDown); got != c.wantStale {
if got := nodeDisableIsStale(c.master, c.node.Up+c.node.Down, now, c.deltaUp, c.deltaDown); got != c.wantStale {
t.Fatalf("nodeDisableIsStale(...) = %v, want %v", got, c.wantStale)
}
})
@@ -389,8 +394,8 @@ func TestNodeStaleLift_MarksNodeDirty(t *testing.T) {
}
}
// A node disable decided on the master's own limits is genuine and must still
// one-way-merge enable=false onto the master (#4917).
// A node that counted past the quota latches enable=false even while the master,
// seeded at zero on adoption, still counts below it (#4917).
func TestNodeQuotaDisable_SameExpiryStillLatches(t *testing.T) {
db := initTrafficTestDB(t)
createNodeInbound(t, db, 1, "n1-in", 41001)
@@ -410,6 +415,27 @@ func TestNodeQuotaDisable_SameExpiryStillLatches(t *testing.T) {
}
}
// A stray node enable=false that neither the node's usage nor the master's limits
// justify must not latch onto the master and loop back to the node (#6870).
func TestNodeStrayDisable_DoesNotLatch(t *testing.T) {
db := initTrafficTestDB(t)
createNodeInbound(t, db, 1, "n1-in", 41001)
svc := &InboundService{}
const email = "stray"
const expiry = int64(1893456000000)
syncNode(t, svc, 1, "n1-in", xray.ClientTraffic{
Email: email, Up: 10, Down: 10, Total: 100, ExpiryTime: expiry, Enable: true,
})
syncNode(t, svc, 1, "n1-in", xray.ClientTraffic{
Email: email, Up: 20, Down: 20, Total: 100, ExpiryTime: expiry, Enable: false,
})
if got := readTraffic(t, db, email); !got.Enable {
t.Fatal("stray node disable within quota and expiry latched master enable off (#6870)")
}
}
// Once the master row is itself depleted, a node disable latches even though the
// node's limits lag — otherwise genuine quota cuts would be skipped.
func TestNodeQuotaDisable_OlderExpiryStillLatchesWhenOverQuota(t *testing.T) {
@@ -870,6 +896,7 @@ func TestClientTrafficMergeSQLMatchesHelpers(t *testing.T) {
masterUp, masterDown, tot int64
deltaUp, deltaDown int64
nodeExpiry, nodeTotal int64
nodeUp, nodeDown int64
nodeEnable bool
wantExpiry int64
wantEnable bool
@@ -920,6 +947,20 @@ func TestClientTrafficMergeSQLMatchesHelpers(t *testing.T) {
nodeExpiry: earlyAbs, nodeTotal: 100, nodeEnable: false,
wantExpiry: lateAbs, wantEnable: false,
},
{
name: "matching limits, master and node under quota (#6870)",
masterExpiry: lateAbs, masterEnable: true,
masterUp: 10, masterDown: 10, tot: 100,
nodeExpiry: lateAbs, nodeTotal: 100, nodeUp: 10, nodeDown: 10, nodeEnable: false,
wantExpiry: lateAbs, wantEnable: true,
},
{
name: "node usage spends the quota the master undercounts (#4917)",
masterExpiry: lateAbs, masterEnable: true,
masterUp: 40, masterDown: 50, tot: 100,
nodeExpiry: lateAbs, nodeTotal: 100, nodeUp: 60, nodeDown: 50, nodeEnable: false,
wantExpiry: lateAbs, wantEnable: false,
},
}
enableExpr := database.ClientTrafficEnableMergeExpr()
@@ -939,8 +980,8 @@ func TestClientTrafficMergeSQLMatchesHelpers(t *testing.T) {
}
wantExpiry := mergeActivationExpiry(c.masterExpiry, c.nodeExpiry)
wantEnable := c.masterEnable
node := xray.ClientTraffic{ExpiryTime: c.nodeExpiry, Total: c.nodeTotal}
if !c.nodeEnable && !nodeDisableIsStale(master, node, now, c.deltaUp, c.deltaDown) {
node := xray.ClientTraffic{ExpiryTime: c.nodeExpiry, Total: c.nodeTotal, Up: c.nodeUp, Down: c.nodeDown}
if !c.nodeEnable && !nodeDisableIsStale(master, node.Up+node.Down, now, c.deltaUp, c.deltaDown) {
wantEnable = false
}
if wantExpiry != c.wantExpiry || wantEnable != c.wantEnable {
@@ -953,7 +994,7 @@ func TestClientTrafficMergeSQLMatchesHelpers(t *testing.T) {
`UPDATE client_traffics SET enable = %s, expiry_time = %s WHERE email = ?`,
enableExpr, expiryExpr,
),
c.nodeEnable, c.nodeExpiry, c.nodeTotal, now, c.deltaUp, c.deltaDown,
c.nodeEnable, now, c.deltaUp, c.deltaDown, node.Up, node.Down,
c.nodeExpiry,
rowEmail,
).Error; err != nil {