2 Commits

Author	SHA1	Message	Date
mrchatam	6f7a305239	fix(node): stop stale expiry sync from undoing client extensions (#6228) (#6231) ... * fix(node): stop stale expiry sync from undoing client extensions (#6228) After an expired client is extended on the master, a lagging node could overwrite client_traffics with an older absolute expiry and latch enable=false. Reject older absolute expiries on merge, ignore expiry-stale disables when the master is not over quota, lift stale lifecycle fields out of adopted settings, stamp reconcile fingerprints from the pre-lift node blob, and mark the node dirty so the next tick re-pushes. * fix(node): lockstep client_traffics expiry/enable SQL with review fixes (#6228) Make expiry merge keep any master absolute (node only activates when master is unset/duration). Include this tick's up/down deltas in the enable stale-disable quota check so a crossing-tick disable is not dropped. * fix(node): add settings absolute helper for renew/lift guards (#6228) Expose settingsClientAbsoluteExpiry so traffic merge can tell a real node auto-renew (settings+stats later) from lagging ClientStats after a master shorten. Trim lift godoc to the invariant. * fix(node): authority-aware lifecycle merge for multi-node sync (#6228) While config_dirty, accumulate traffic only — do not adopt node expiry/enable/total/reset (and preserve dipped baselines so a false renew cannot fire after clear). On clean ticks, master absolute expiry wins; node auto-renew still goes through nodeClientRenewed when settings also show the later deadline. Defer settings lifecycle lift until after traffic deltas land, align SyncInbound via applyMasterClientLifecycle, and avoid re-MarkNodeDirty when already dirty. * fix(node): clear config_dirty only after the post-reconcile traffic merge (#6228) After a successful ReconcileNode, keep the node dirty through the same tick's SetRemoteTraffic so lagging ClientStats cannot clobber the just-pushed master lifecycle, then ClearNodeDirty. * test(node): cover dirty-gate, master-absolute, and renew false-positives (#6228) Add regressions for extend/shorten while dirty, clean-sibling shorten, settings vs lagging disable, renew recovery after dirty, renew with matching settings, and shorten+Reset lagging stats not treated as renew. * fix(node): address the review findings on the lifecycle merge (#6228) The automated review on #6231 flagged a blocking regression and six smaller issues. All of them are fixed here. Blocking: making the master's absolute expiry always win left nodeClientRenewed as the only channel for a node-side auto-renew, and that required a counter dip. A client that used no traffic in the period never dips, so its renewal was dropped, the master kept the expired deadline and disableInvalidClients removed it with no way back (master-side autoRenewClients skips node inbounds). The node bumps reset_count on every renewal, so that counter is now an independent renewal signal and is persisted with the renewal so it keeps converging. The deferred ClearNodeDirty made every reconcile-success tick merge in dirty mode, which suppressed inbound adoption, new client_traffics rows, the orphan sweeps and the whole SyncInbound record loop -- and left the node dirty forever whenever SetRemoteTraffic errored. The clear goes back to where it was; a separate justPushed flag now freezes only the client lifecycle merge for the tick whose push just landed. staleNodeDisable only recognised a lagging disable by an older expiry, so a quota top-up (raise totalGB, leave the expiry alone) was re-latched to disabled by the next lagging snapshot -- the #6228 symptom on a second axis. The reviewer's suggestion of dropping the expiry precondition outright fails TestNodeQuotaDisable_SameExpiryStillLatches, because the master's own counters legitimately sit below a node's after a seeded-at-zero adoption. nodeDisableIsStale instead compares the limits the node judged the client against with the master's own: matching limits mean a genuine verdict that still latches (#4917), differing limits mean the node has not seen the master's change yet. It also now measures the master deadline against wall-clock now, so an expired master row no longer looks "extended" merely because the node's copy is older still. Also: the settings lift now writes enable in both directions, so a blob fetched before a master disable cannot carry enable=true back into central settings and on to the node; the renewal guard parses the inbound settings once per inbound instead of once per renewing client; the adoption loop only writes settings when they actually changed; and two comments that described mechanisms the code does not use were corrected. The test deadlines are now relative to the run: the merge compares against now, so fixed timestamps would have rotted into the wrong side of it. --------- Co-authored-by: mrchatam <287639636+mrchatam@users.noreply.github.com> Co-authored-by: Sanaei <ho3ein.sanaei@gmail.com>	2026-08-23 23:17:56 +02:00
Sanaei	5bc81dfd1d	fix(node): stop the node sync from deleting clients it never meant to ... A client that hit its quota or expiry was disabled, then destroyed on both panels a few seconds later. Five defects fed the same hard delete. ReconcileNode pushed buildRuntimeInboundForAPI, which strips disabled clients. Every other call site targets an in-memory Xray config, where dropping a user is harmless; a node target is a peer panel's DATABASE, so the node deleted the row, stopped reporting it, and the master mirrored that deletion back. Split the builder in two: buildInboundForNodePush injects fallbacks only, buildInboundForLocalRuntime adds the strip on top. The names now say which targets they are safe for. setRemoteTrafficLocked trusted a config_dirty the caller sampled before the snapshot round-trip. A client added inside that window commits on the same serialized writer and marks the node dirty, but the merge still treated the older snapshot as authoritative and deleted it. Re-read the flag inside the writer. In "selected" sync mode, FilterNodeSnapshot strips a deselected tag, but the sweep loaded every inbound with node_id set, so deselecting a tag read as "the node deleted it" and wiped an inbound the node still serves. Skip tags outside the node's managed set. A failed SyncInbound was logged and swallowed; on SQLite the transaction still commits, and the sweep then deleted the innocent clients whose links that failure had left unbuilt. Skip the sweep for such an inbound, and close the trigger: SyncInbound now stores the trimmed email it looks up by, and email validation rejects every unicode space rather than only U+0020. ClientService.Delete tombstones up front and deliberately keeps the record when an inbound fails, so the next attempt can retry the leftovers. The tombstone did not lift with it, so the next merge dropped the client from the synced settings and finished the deletion this path had refused. Add withdrawClientTombstones on every failure path, in BulkDelete too. Finally, make the sweep itself recoverable. "Ended the merge unattached" is true for a real remote deletion and equally true for a bad merge, so it now stamps sync_orphaned_at instead of deleting; any later merge that sees the client attached clears the mark, and a reaper removes only what stayed orphaned past the grace period. The traffic row survives that window too, or a reclaimed client would come back with its usage, quota and expiry reset. The mark is written by this sweep alone, so orphans from any other cause keep their existing manual-cleanup semantics.	2026-08-01 15:19:08 +02:00