From ff954ec48c0bc9a882105906c1983eadcfa3c7d8 Mon Sep 17 00:00:00 2001 From: "Mr. Nickson" <259025431+mrnickson-hue@users.noreply.github.com> Date: Tue, 28 Jul 2026 23:14:01 +0300 Subject: [PATCH] fix: stop deleting client_traffics for detached-but-alive clients (#6110) * fix: stop deleting client_traffics for detached-but-alive clients MigrationRemoveOrphanedTraffics keyed "orphaned" off presence in some inbound's settings.clients[] JSON, a definition that predates #4469's standalone clients table. ClientService.Detach intentionally keeps a client's traffic row when it drops its last inbound attachment (so it can be re-attached later without losing stats/expiry), but that client has no entry in any inbound's JSON anymore - so every x-ui migrate run or backup restore deleted its traffic row anyway, even though the client itself was untouched and still listed. Scope the query to the clients table instead, which is the function's actual intent. Separately, frontend/src/hooks/useClients.ts recomputed the clients summary from the client_stats WS snapshot as soon as it arrived, even when that snapshot held fewer rows than the server's own total (e.g. exactly the gap above, or any other client with no client_traffics row). The recompute can only bucket the clients it was given, so the missing ones silently fell out of every bucket while the headline total still counted them - the Ended/Disabled cards read 0 and their hover lists were empty even though the table below listed those rows, leaving the Filter drawer as the only way to reach them. Extracted the decision into pickClientsSummary and added the guard: fall back to the server summary (built from the clients table, always sums to total) whenever the snapshot doesn't cover every client. Fixes #6102. * fix: union both keep-sets instead of replacing (review feedback) Address the automated review on this PR: switching MigrationRemoveOrphanedTraffics to key solely off the clients table traded the original bug for a worse one. The one-shot ClientsTable seeder (internal/database/db.go) skips a client it fails to unmarshal and never retries, so a client still live in an inbound's settings.clients[] JSON can have no clients row at all - the new predicate deleted its traffic row too, and an empty clients table would have emptied client_traffics outright. Union both keep-sets: a row survives if it's referenced by either the clients table or any inbound's JSON, and is removed only when it's in neither. Log the delete's outcome instead of discarding it silently, since a whole-table wipe would otherwise leave no trace. Rewrote the migration test as a table of all four combinations, driven through real ClientService calls (SyncInbound, Detach) rather than hand-built rows wherever a real path produces the state, so it tracks actual behavior instead of an assumption about it. Added the missing case the review flagged: a client live in JSON only, with no clients row, must survive. Also stripped the // comments this PR had added - CLAUDE.md states committed Go/TS carries none, which the review separately flagged. --- frontend/src/hooks/useClients.ts | 27 +++++---- frontend/src/pages/clients/ClientsPage.tsx | 5 +- frontend/src/test/clients-summary.test.ts | 28 ++++++++- internal/web/service/inbound_migration.go | 11 +++- .../web/service/inbound_migration_test.go | 59 +++++++++++++++++++ 5 files changed, 112 insertions(+), 18 deletions(-) diff --git a/frontend/src/hooks/useClients.ts b/frontend/src/hooks/useClients.ts index b2f3bd2ae..9a094b2da 100644 --- a/frontend/src/hooks/useClients.ts +++ b/frontend/src/hooks/useClients.ts @@ -117,6 +117,19 @@ export function computeClientsSummary( return { total: stats.length, active, online, depleted, expiring, deactive }; } +export function pickClientsSummary( + serverSummary: ClientsSummary, + allClientStats: ClientStatRow[], + onlineSet: Set, + expireDiffMs: number, + trafficDiffBytes: number, +): ClientsSummary { + if (allClientStats.length === 0) return serverSummary; + if (serverSummary.total > allClientStats.length) return serverSummary; + const live = computeClientsSummary(allClientStats, onlineSet, expireDiffMs, trafficDiffBytes); + return { ...live, total: serverSummary.total || live.total }; +} + function buildQS(p: ClientQueryParams): string { const sp = new URLSearchParams(); sp.set('page', String(p.page || 1)); @@ -265,18 +278,12 @@ export function useClients() { const trafficDiff = ((defaults.trafficDiff as number) ?? 0) * 1073741824; const pageSize = (defaults.pageSize as number) ?? 0; - // Live summary: the client_stats WS event refreshes allClientStats every few - // seconds, so the top counters track reality without a page refresh. Falls - // back to the server-computed summary until the first event lands, and keeps - // the server's authoritative total for the headline count. const [allClientStats, setAllClientStats] = useState([]); const [clientSpeed, setClientSpeed] = useState>({}); - const summary = useMemo(() => { - const serverSummary = listQuery.data?.summary ?? DEFAULT_SUMMARY; - if (allClientStats.length === 0) return serverSummary; - const live = computeClientsSummary(allClientStats, new Set(onlines), expireDiff, trafficDiff); - return { ...live, total: serverSummary.total || live.total }; - }, [allClientStats, onlines, expireDiff, trafficDiff, listQuery.data?.summary]); + const summary = useMemo( + () => pickClientsSummary(listQuery.data?.summary ?? DEFAULT_SUMMARY, allClientStats, new Set(onlines), expireDiff, trafficDiff), + [allClientStats, onlines, expireDiff, trafficDiff, listQuery.data?.summary], + ); const invalidateAll = useCallback( () => { diff --git a/frontend/src/pages/clients/ClientsPage.tsx b/frontend/src/pages/clients/ClientsPage.tsx index 75b6baac4..2e556f860 100644 --- a/frontend/src/pages/clients/ClientsPage.tsx +++ b/frontend/src/pages/clients/ClientsPage.tsx @@ -205,7 +205,7 @@ export default function ClientsPage() { const { clients, total, filtered, - summary: serverSummary, + summary, allGroups, setQuery, inbounds, onlines, loading, transitioning, fetched, fetchError, subSettings, @@ -385,9 +385,6 @@ export default function ClientsPage() { // a rename. const filteredClients = clients; - // Server-computed counts that stay stable as the user paginates/filters. - const summary = serverSummary; - // Sort is server-side now; the page already arrives in the requested // order, so we just hand it through. const sortedClients = filteredClients; diff --git a/frontend/src/test/clients-summary.test.ts b/frontend/src/test/clients-summary.test.ts index bbe6e9e0f..b43626c0f 100644 --- a/frontend/src/test/clients-summary.test.ts +++ b/frontend/src/test/clients-summary.test.ts @@ -1,7 +1,7 @@ import { describe, it, expect } from 'vitest'; -import { computeClientsSummary } from '@/hooks/useClients'; -import type { ClientTraffic } from '@/schemas/client'; +import { computeClientsSummary, pickClientsSummary } from '@/hooks/useClients'; +import type { ClientTraffic, ClientsSummary } from '@/schemas/client'; // Parity with web/service/client.go buildClientsSummary: the same client must // land in the same bucket whether the count comes from the server (list fetch) @@ -60,3 +60,27 @@ describe('computeClientsSummary', () => { expect(s.depleted).toEqual([]); }); }); + +describe('pickClientsSummary', () => { + const serverSummary: ClientsSummary = { + total: 67, active: 58, online: [], depleted: [], expiring: [], deactive: [], + }; + + it('keeps the server summary when the snapshot is short of the server total (#6102)', () => { + const shortSnapshot: Row[] = Array.from({ length: 58 }, (_, i) => row({ email: `c${i}@x`, enable: true })); + const s = pickClientsSummary(serverSummary, shortSnapshot, new Set(), 3 * DAY, 1 * GB); + expect(s).toEqual(serverSummary); + }); + + it('uses the live recompute when the snapshot covers every client', () => { + const fullSnapshot: Row[] = Array.from({ length: 67 }, (_, i) => row({ email: `c${i}@x`, enable: true })); + const s = pickClientsSummary(serverSummary, fullSnapshot, new Set(), 3 * DAY, 1 * GB); + expect(s.total).toBe(67); + expect(s.active).toBe(67); + }); + + it('falls back to the server summary before the first WS snapshot arrives', () => { + const s = pickClientsSummary(serverSummary, [], new Set(), 3 * DAY, 1 * GB); + expect(s).toEqual(serverSummary); + }); +}); diff --git a/internal/web/service/inbound_migration.go b/internal/web/service/inbound_migration.go index 70294cd08..ba23e0531 100644 --- a/internal/web/service/inbound_migration.go +++ b/internal/web/service/inbound_migration.go @@ -19,11 +19,18 @@ import ( func (s *InboundService) MigrationRemoveOrphanedTraffics() { db := database.GetDB() query := fmt.Sprintf( - "DELETE FROM client_traffics WHERE email NOT IN (SELECT %s %s)", + "DELETE FROM client_traffics WHERE email NOT IN (SELECT email FROM clients) AND email NOT IN (SELECT %s %s)", database.JSONFieldText("client.value", "email"), database.JSONClientsFromInbound(), ) - db.Exec(query) + result := db.Exec(query) + if result.Error != nil { + logger.Warning("MigrationRemoveOrphanedTraffics failed:", result.Error) + return + } + if result.RowsAffected > 0 { + logger.Infof("MigrationRemoveOrphanedTraffics: removed %d orphaned client_traffics row(s)", result.RowsAffected) + } } func (s *InboundService) MigrationRequirements() { diff --git a/internal/web/service/inbound_migration_test.go b/internal/web/service/inbound_migration_test.go index 2f7b8fc33..ef4f3b45b 100644 --- a/internal/web/service/inbound_migration_test.go +++ b/internal/web/service/inbound_migration_test.go @@ -129,6 +129,65 @@ func TestMigrationRequirements_CleansLegacyZeroAddrTag(t *testing.T) { } } +func TestMigrationRemoveOrphanedTraffics(t *testing.T) { + setupConflictDB(t) + db := database.GetDB() + clientSvc := &ClientService{} + inboundSvc := &InboundService{} + + const attachedEmail = "attached@example.com" + attachedClient := model.Client{Email: attachedEmail, ID: "11111111-1111-1111-1111-111111111111", SubID: attachedEmail, Enable: true} + attachedIb := mkInbound(t, 30003, model.VLESS, clientsSettings(t, []model.Client{attachedClient})) + if err := clientSvc.SyncInbound(nil, attachedIb.Id, []model.Client{attachedClient}); err != nil { + t.Fatalf("seed attached client: %v", err) + } + mkTraffic(t, attachedIb.Id, attachedEmail, 0, 0, 0, 0, true) + + const detachedEmail = "detached@example.com" + detachedClient := model.Client{Email: detachedEmail, ID: "22222222-2222-2222-2222-222222222222", SubID: detachedEmail, Enable: true} + detachedIb := mkInbound(t, 30004, model.VLESS, clientsSettings(t, []model.Client{detachedClient})) + if err := clientSvc.SyncInbound(nil, detachedIb.Id, []model.Client{detachedClient}); err != nil { + t.Fatalf("seed detached client: %v", err) + } + mkTraffic(t, detachedIb.Id, detachedEmail, 123, 456, 0, 0, true) + detachedRec := lookupClientRecord(t, detachedEmail) + if _, err := clientSvc.Detach(inboundSvc, detachedRec.Id, []int{detachedIb.Id}); err != nil { + t.Fatalf("Detach: %v", err) + } + + const jsonOnlyEmail = "jsononly@example.com" + jsonOnlyClient := model.Client{Email: jsonOnlyEmail, ID: "33333333-3333-3333-3333-333333333333", SubID: jsonOnlyEmail, Enable: true} + jsonOnlyIb := mkInbound(t, 30005, model.VLESS, clientsSettings(t, []model.Client{jsonOnlyClient})) + mkTraffic(t, jsonOnlyIb.Id, jsonOnlyEmail, 0, 0, 0, 0, true) + + const trulyOrphanedEmail = "deleted@example.com" + mkTraffic(t, attachedIb.Id, trulyOrphanedEmail, 0, 0, 0, 0, true) + + inboundSvc.MigrationRemoveOrphanedTraffics() + + cases := []struct { + name string + email string + want int64 + }{ + {"attached, in clients table and JSON", attachedEmail, 1}, + {"detached-but-alive, in clients table only", detachedEmail, 1}, + {"seeder-skipped-but-live, in JSON only", jsonOnlyEmail, 1}, + {"truly orphaned, in neither", trulyOrphanedEmail, 0}, + } + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + var got int64 + if err := db.Model(xray.ClientTraffic{}).Where("email = ?", c.email).Count(&got).Error; err != nil { + t.Fatalf("count client_traffics for %s: %v", c.email, err) + } + if got != c.want { + t.Errorf("client_traffics count for %s: got %d, want %d", c.email, got, c.want) + } + }) + } +} + func TestMigrationRequirements_NormalizesShareAddressFields(t *testing.T) { setupConflictDB(t) db := database.GetDB()