Lost update on `inbound_client_ips` under PostgreSQL defeats the cluster-wide IP limit
Summary
Two writers read-modify-write the same inbound_client_ips.ips JSON blob. On
PostgreSQL they can interleave and one write is lost. The in-code comment that
calls this benign does not hold for the cross-node path, because
partitionLiveIps counts an address toward the limit purely on the strength of
a timestamp that lives in that blob.
Why
MergeNodeClientIps (internal/web/service/inbound_client_ips.go) reads
current.Ips, merges the node's report into it and writes the result back:
merged := mergeClientIpEntries(oldEntries, incomingEntries, cutoff)
...
// A concurrent check_client_ip_job db.Save on the same row can interleave
// with this update (benign last-writer-wins; any dropped IP reappears on the
// next scan/sync), so only write when the blob actually changed.
if current.Ips != mergedStr {
if err := tx.Model(&model.InboundClientIps{}).Where("id = ?", current.Id).Update("ips", mergedStr).Error; err != nil {check_client_ip_job does its own read-modify-write of the same row
(internal/web/job/check_client_ip_job.go, tx.Save(inboundClientIps)).
The comment's "benign" rests on dropped addresses reappearing on the next scan. That is true for addresses this panel observes itself — they come from the Xray API every scan, not from the blob. It is not true for addresses seen on another node, and the limit depends on those:
// Consider an IP "live" if it was seen locally in this scan, OR if its
// timestamp from the synced database is very recent (e.g. within 2 minutes).
// This ensures cluster-wide limits work even if the IP was seen on another node.
if observedThisScan[ip] || now-ts < 120 {
live = append(live, entry)A remote address reaches this branch only through the blob. If the merge that
carried it is the write that gets lost, the address never enters ipMap, is
never counted as live, and the client keeps a device that should have been
banned. It does not "reappear on the next scan": it reappears only if that node
reports again and the merge is not lost again, and by then the 120-second
liveness window has usually closed.
Why it is invisible on SQLite
internal/database/db.go opens SQLite with _txlock=immediate, so every write
transaction takes a RESERVED lock at BEGIN and write transactions cannot
interleave at all. The lost update is structurally impossible there. PostgreSQL
at READ COMMITTED runs them concurrently, so it is reachable on any Postgres
deployment with more than one node.
Impact
A client exceeds its configured IP limit for as long as the interleave keeps happening. The window is not self-healing, because the same two writers run on independent schedules and can collide repeatedly. Panel-displayed history and cross-node attribution lose entries for the same reason.
Notes
I have a working fix and would rather agree on the shape before opening a PR, because the obvious approaches pull in opposite directions:
- serializing both writers removes the interleave but adds to a path that
ea66aa49has just been deliberately moving work off; - a conditional update (compare-and-set on the previous blob, retry on miss) keeps the writers concurrent but changes the merge into a loop;
- moving the addresses into their own table with one row per address makes the whole read-modify-write disappear, at the cost of a migration.
Happy to prepare whichever you prefer.
Source: MHSanaei/3x-ui