fix(nodes): sync "start after first connect" expiry so un-activated nodes do not reset it (#5319)

* fix(nodes): stop un-activated nodes from resetting "start after first connect" expiry

In a multi-node setup a client is attached to inbounds on several nodes, but
its `client_traffics` row is shared per-email (the column is `gorm:"unique"`).
With "start expiry after first connect", the expiry is stored as a negative
duration and each node converts it to an absolute deadline (now+duration) the
first time the client connects *there*.

The master's per-node traffic merge wrote `expiry_time = ?` unconditionally for
every node sync. So a node where the client never connected keeps reporting the
un-activated negative duration and clobbers the absolute deadline that the node
where the client *did* connect had already activated — last writer wins. The
shared row flip-flops and usually lands back on the negative value, so the main
panel shows the timer "not started" while the active node counts down, and the
subscription (which reads this row and recomputes negative as now+duration on
every fetch) reports a perpetually-resetting, wrong expiry and usage.

Guard the merge so an un-activated (<= 0) value reported by a node can never
reset an already-activated absolute deadline. A positive node value is still
adopted, so a node that legitimately moves the deadline forward (traffic reset /
auto-renew) still propagates. The rule lives in both the SQL CASE used by the
merge and a small `mergeActivationExpiry` helper (kept in lockstep) that the
structural-change check reuses so the guard does not trigger spurious config
re-pushes.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

* fix(nodes): cast expiry merge params to BIGINT for Postgres

The "start after first connect" merge guard introduced the comparison
`? <= 0` in the client_traffics expiry_time CASE. There Postgres infers
the parameter type as int4 from the literal 0, so binding a real expiry
value — a negative start-after-connect duration or a positive absolute
deadline (~1.7e12 ms) — overflows int4 and the whole setRemoteTrafficLocked
transaction fails, breaking node traffic and expiry sync on Postgres.
SQLite (dynamic typing) was unaffected.

Wrap both params in CAST(? AS BIGINT) (portable across SQLite and
Postgres) so the parameter is typed bigint, matching the explicit casts
the sibling GreatestExpr/ClientTrafficEnableMergeExpr helpers already use.

Verified against Postgres 16: TestNodeFirstConnectExpiry_NotClobbered
failed before this change and passes after; SQLite suite unchanged.

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Sanaei <ho3ein.sanaei@gmail.com>
This commit is contained in:
Rouzbeh†
2026-06-15 15:46:19 +02:00
committed by GitHub
parent 7605902324
commit 628406117e
2 changed files with 173 additions and 4 deletions
+32 -4
View File
@@ -153,6 +153,25 @@ func (s *InboundService) upsertNodeBaseline(tx *gorm.DB, nodeID int, email strin
}).Create(&model.NodeClientTraffic{NodeId: nodeID, Email: email, Up: up, Down: down}).Error
}
// mergeActivationExpiry reconciles a node-reported client expiry with the value
// already stored on the master. "Start after first connect" persists a negative
// duration that each node converts to an absolute deadline (now+duration) the
// first time the client connects there. The per-email client_traffics row is
// shared across every node, so a node that has not yet seen a first connection
// keeps reporting the negative duration — which must never reset a deadline
// another node already activated.
//
// A node may legitimately move an already-activated deadline forward (traffic
// reset / auto-renew extends it), so any positive node value is still adopted —
// only an un-activated (<= 0) value is rejected once an absolute deadline
// exists. Kept in lockstep with the SQL CASE in setRemoteTrafficLocked.
func mergeActivationExpiry(existing, node int64) int64 {
if existing > 0 && node <= 0 {
return existing
}
return node
}
func (s *InboundService) SetRemoteTraffic(nodeID int, snap *runtime.TrafficSnapshot, dirty bool) (bool, error) {
var structuralChange bool
err := submitTrafficWrite(func() error {
@@ -544,22 +563,31 @@ func (s *InboundService) setRemoteTrafficLocked(nodeID int, snap *runtime.Traffi
if existing := centralCSByEmail[cs.Email]; existing != nil &&
(existing.Enable != cs.Enable ||
existing.Total != cs.Total ||
existing.ExpiryTime != cs.ExpiryTime ||
existing.ExpiryTime != mergeActivationExpiry(existing.ExpiryTime, cs.ExpiryTime) ||
existing.Reset != cs.Reset) {
structuralChange = true
}
enableExpr := database.ClientTrafficEnableMergeExpr()
// expiry_time merge mirrors mergeActivationExpiry: a node that has not
// yet seen the client's first connection keeps reporting the negative
// "start after first connect" duration, which must never reset the
// absolute deadline another node already activated. A positive node
// value is still adopted (e.g. auto-renew moves the deadline forward).
// CAST(? AS BIGINT): in the `<= 0` comparison Postgres would otherwise
// infer int4 from the literal and overflow on real expiry values.
if err := tx.Exec(
fmt.Sprintf(
`UPDATE client_traffics
SET up = up + ?, down = down + ?, enable = %s, total = ?, expiry_time = ?, reset = ?,
last_online = %s
SET up = up + ?, down = down + ?, enable = %s, total = ?,
expiry_time = CASE WHEN expiry_time > 0 AND CAST(? AS BIGINT) <= 0 THEN expiry_time ELSE CAST(? AS BIGINT) END,
reset = ?, last_online = %s
WHERE email = ?`,
enableExpr,
database.GreatestExpr("last_online", "?"),
),
deltaUp, deltaDown, cs.Enable, cs.Total, cs.ExpiryTime, cs.Reset,
deltaUp, deltaDown, cs.Enable, cs.Total,
cs.ExpiryTime, cs.ExpiryTime, cs.Reset,
cs.LastOnline, cs.Email,
).Error; err != nil {
return false, err