fix(traffic): prevent phantom quota consumption from stale node data (#5412)

Three related bugs caused inflated traffic counters and spurious quota
hits on multi-node setups, most visibly when a client email was renamed
while a node was offline or its PostgreSQL deadlocked.

**Fix 1 — phantom quota (root cause)** `setRemoteTrafficLocked`
new-row path: when master had no `client_traffics` row for an email
that a node reported, it seeded the row with `Up: cs.Up` — importing
the node's full accumulated counter as if it were fresh quota usage.
If the node retained stale data from a previously-deleted account (e.g.
a failed deletion during an outage), the ghost 50 GB appeared on the
new client immediately and triggered `disableInvalidClients` the same
tick. Fixed by seeding at `Up: 0`; the current node value still becomes
the baseline so only future increments count.

**Fix 2 — PostgreSQL deadlock** `addClientTraffic` did a
read-modify-write via `tx.Save(slice)`, issuing UPDATEs in slice order.
Two concurrent goroutines locking the same rows in opposite order
deadlock on PostgreSQL (SQLite avoids this with file-level
serialisation). Replaced with atomic per-email
`UPDATE SET up=up+?, down=down+?` statements. Also preserves the
delayed-start ExpiryTime conversion that `adjustTraffics` computes
in-memory but the old Save path persisted to the DB.

**Fix 3 & 4 — stale `inbound_id` filters** `autoRenewClients` used
`WHERE inbound_id NOT IN (node inbounds)` to skip node clients, but
`client_traffics.inbound_id` is set once on INSERT and never refreshed.
Replaced with an email-based subquery through `client_inbounds` (the
authoritative source). Also added a safe type assertion for
`settings["clients"].([]any)` that previously panicked on nil.

**Fix 5 — stale `inbound_id` in reset** `resetAllClientTrafficsLocked`
used `WHERE inbound_id = ?` to find which emails to reset; same staleness
problem. Replaced with the `client_inbounds` join for email lookup;
the `inbounds.last_traffic_reset_time` update still correctly uses the
inbound ID directly on the `inbounds` table.

Tests updated to reflect the new seeding-at-zero semantics and a new
`TestGhostData_NoPhantomTraffic` test reproduces the exact 50 GB
phantom scenario.
This commit is contained in:
Younes
2026-06-20 00:36:35 +02:00
committed by GitHub
parent 4f99e48ab7
commit fb03b0e9f1
4 changed files with 124 additions and 45 deletions
+43 -15
View File
@@ -133,9 +133,7 @@ func (s *InboundService) addClientTraffic(tx *gorm.DB, traffics []*xray.ClientTr
return err
}
// Index by email for O(N) merge — the previous nested loop was O(N²)
// and dominated each cron tick on inbounds with thousands of active
// clients (7500 × 7500 = 56M string comparisons every 10 seconds).
// Index by email for O(N) merge.
trafficByEmail := make(map[string]*xray.ClientTraffic, len(traffics))
for i := range traffics {
if traffics[i] != nil {
@@ -143,21 +141,39 @@ func (s *InboundService) addClientTraffic(tx *gorm.DB, traffics []*xray.ClientTr
}
}
now := time.Now().UnixMilli()
for dbTraffic_index := range dbClientTraffics {
t, ok := trafficByEmail[dbClientTraffics[dbTraffic_index].Email]
if !ok {
// Use atomic per-row UPDATE instead of read-modify-write Save. tx.Save
// issues UPDATEs in slice order, which varies between concurrent callers;
// on PostgreSQL two transactions locking the same rows in opposite order
// deadlock. An atomic "SET up = up + ?" never holds a row lock across a
// subsequent lock acquisition, so concurrent writers cannot deadlock.
for _, ct := range dbClientTraffics {
t, ok := trafficByEmail[ct.Email]
if !ok || (t.Up == 0 && t.Down == 0) {
continue
}
dbClientTraffics[dbTraffic_index].Up += t.Up
dbClientTraffics[dbTraffic_index].Down += t.Down
if t.Up+t.Down > 0 {
dbClientTraffics[dbTraffic_index].LastOnline = now
if err = tx.Exec(
fmt.Sprintf(
`UPDATE client_traffics SET up = up + ?, down = down + ?, last_online = %s WHERE email = ?`,
database.GreatestExpr("last_online", "?"),
),
t.Up, t.Down, now, ct.Email,
).Error; err != nil {
logger.Warning("AddClientTraffic update data ", err)
}
}
err = tx.Save(dbClientTraffics).Error
if err != nil {
logger.Warning("AddClientTraffic update data ", err)
// adjustTraffics converts delayed-start rows (negative ExpiryTime → absolute
// deadline) in-memory. Persist that conversion now since the traffic UPDATE
// above only touches up/down/last_online.
for _, ct := range dbClientTraffics {
if ct.ExpiryTime > 0 {
if err = tx.Exec(
`UPDATE client_traffics SET expiry_time = ? WHERE email = ? AND expiry_time < 0`,
ct.ExpiryTime, ct.Email,
).Error; err != nil {
logger.Warning("AddClientTraffic update expiry_time ", err)
}
}
}
return nil
@@ -272,9 +288,18 @@ func (s *InboundService) autoRenewClients(tx *gorm.DB) (bool, int64, error) {
now := time.Now().Unix() * 1000
var err, err1 error
// Filter to clients that have at least one local inbound. Using
// client_traffics.inbound_id is wrong: it goes stale after an inbound is
// deleted/recreated and always points to the first inbound the client was
// attached to, so it could be a node inbound even when the client also has
// local inbounds. The email-based join through client_inbounds is authoritative.
err = tx.Model(xray.ClientTraffic{}).
Where("reset > 0 and expiry_time > 0 and expiry_time <= ?", now).
Where("inbound_id NOT IN (?)", tx.Model(&model.Inbound{}).Select("id").Where("node_id IS NOT NULL")).
Where("email IN (?)", tx.Table("client_inbounds ci").
Select("c.email").
Joins("JOIN clients c ON c.id = ci.client_id").
Joins("JOIN inbounds i ON i.id = ci.inbound_id").
Where("i.node_id IS NULL")).
Find(&traffics).Error
if err != nil {
return false, 0, err
@@ -326,7 +351,10 @@ func (s *InboundService) autoRenewClients(tx *gorm.DB) (bool, int64, error) {
for inbound_index := range inbounds {
settings := map[string]any{}
json.Unmarshal([]byte(inbounds[inbound_index].Settings), &settings)
clients := settings["clients"].([]any)
clients, _ := settings["clients"].([]any)
if len(clients) == 0 {
continue
}
for client_index := range clients {
c := clients[client_index].(map[string]any)
for traffic_index, traffic := range traffics {