From 2c18cbe1cd2ee3954b36053e1a1f29b074cc8145 Mon Sep 17 00:00:00 2001 From: MHSanaei Date: Sat, 24 Oct 2026 20:07:55 +0200 Subject: [PATCH] fix(sub): close a deleted host binding only on the inbound it served Deleting the only host a client was bound to blanked that client on every inbound. Once the host row was gone, the binding could no longer say which inbound it guarded, so it failed closed everywhere: a client on inbounds A and B, bound to one host on A, lost B's link the moment that host was deleted, and nothing in the panel hinted at it. Every host delete (host groups, the externalProxy sync, inbound deletion, the depleted-inbound sweep and the orphan prune) now goes through database.DeleteHosts, which keeps each deleted host's inbound in host_tombstones in the same transaction. A binding to a deleted host closes only that inbound; ids that resolve to no host at all still fail closed everywhere. The Postgres Host step now runs the deletion cases as well. --- .github/workflows/ci.yml | 8 +-- internal/database/db.go | 42 ++++++++++++--- internal/database/migrate_data.go | 1 + internal/database/model/model.go | 9 ++++ internal/sub/host_order_test.go | 3 +- internal/sub/host_sub.go | 39 ++++++++++---- internal/sub/host_sub_test.go | 69 +++++++++++++++++++++++-- internal/web/service/host.go | 12 +++-- internal/web/service/inbound.go | 2 +- internal/web/service/inbound_traffic.go | 2 +- 10 files changed, 155 insertions(+), 32 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 58ca62213..341064b44 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -102,13 +102,13 @@ jobs: exit 1 fi - - name: PostgreSQL subscription Host ordering tests + - name: PostgreSQL subscription Host tests run: | set -o pipefail - go test ./internal/sub -run '^TestSubHostOrder_Postgres$' -count=1 -v | tee /tmp/postgres-host-order.log - passed=$(grep -c -- '^--- PASS' /tmp/postgres-host-order.log || true) + go test ./internal/sub -run '^TestSubHosts_Postgres$' -count=1 -v | tee /tmp/postgres-hosts.log + passed=$(grep -c -- '^--- PASS' /tmp/postgres-hosts.log || true) if [ "$passed" -ne 1 ]; then - echo "expected passing PostgreSQL Host ordering coverage, got $passed" >&2 + echo "expected passing PostgreSQL Host coverage, got $passed" >&2 exit 1 fi diff --git a/internal/database/db.go b/internal/database/db.go index 705469c70..924953a2f 100644 --- a/internal/database/db.go +++ b/internal/database/db.go @@ -32,6 +32,7 @@ import ( "gorm.io/driver/postgres" "gorm.io/driver/sqlite" "gorm.io/gorm" + "gorm.io/gorm/clause" "gorm.io/gorm/logger" ) @@ -78,6 +79,7 @@ func allModels() []any { &model.ClientGroup{}, &model.InboundFallback{}, &model.Host{}, + &model.HostTombstone{}, &model.NodeClientTraffic{}, &model.NodeClientIp{}, &model.ClientGlobalTraffic{}, @@ -873,7 +875,7 @@ func SyncHostsFromExternalProxy(tx *gorm.DB, inboundId int, oldStreamSettings, n continue } if i >= len(added) { - if err := tx.Delete(host).Error; err != nil { + if _, err := DeleteHosts(tx, "id = ?", host.Id); err != nil { return err } continue @@ -1048,17 +1050,43 @@ func anyToNonEmptyStrings(v any) []string { } func pruneOrphanedHosts() error { - res := db.Exec("DELETE FROM hosts WHERE inbound_id NOT IN (SELECT id FROM inbounds)") - if res.Error != nil { - log.Printf("Error pruning orphaned hosts rows: %v", res.Error) - return res.Error + var pruned int64 + err := db.Transaction(func(tx *gorm.DB) error { + var err error + pruned, err = DeleteHosts(tx, "inbound_id NOT IN (SELECT id FROM inbounds)") + return err + }) + if err != nil { + log.Printf("Error pruning orphaned hosts rows: %v", err) + return err } - if res.RowsAffected > 0 { - log.Printf("Pruned %d orphaned hosts row(s)", res.RowsAffected) + if pruned > 0 { + log.Printf("Pruned %d orphaned hosts row(s)", pruned) } return nil } +// DeleteHosts deletes the hosts matching query and keeps each one's inbound in host_tombstones, in +// the caller's transaction, so a client still bound to one fails closed on that inbound alone. +func DeleteHosts(tx *gorm.DB, query any, args ...any) (int64, error) { + var tombstones []model.HostTombstone + if err := tx.Model(&model.Host{}).Select("id AS host_id, inbound_id").Where(query, args...).Scan(&tombstones).Error; err != nil { + return 0, err + } + if len(tombstones) == 0 { + return 0, nil + } + // migrate-db resyncs the hosts sequence to MAX(id), so a deleted id can return; keep its latest inbound. + if err := tx.Clauses(clause.OnConflict{ + Columns: []clause.Column{{Name: "host_id"}}, + DoUpdates: clause.AssignmentColumns([]string{"inbound_id"}), + }).CreateInBatches(&tombstones, 500).Error; err != nil { + return 0, err + } + res := tx.Where(query, args...).Delete(&model.Host{}) + return res.RowsAffected, res.Error +} + func pruneOrphanedClientInbounds() error { res := db.Exec("DELETE FROM client_inbounds WHERE inbound_id NOT IN (SELECT id FROM inbounds)") if res.Error != nil { diff --git a/internal/database/migrate_data.go b/internal/database/migrate_data.go index a6eaf0409..a93e004a4 100644 --- a/internal/database/migrate_data.go +++ b/internal/database/migrate_data.go @@ -53,6 +53,7 @@ func migrationModels() []any { &model.ClientGroup{}, &model.InboundFallback{}, &model.Host{}, + &model.HostTombstone{}, &model.NodeClientTraffic{}, &model.NodeClientIp{}, &model.ClientGlobalTraffic{}, diff --git a/internal/database/model/model.go b/internal/database/model/model.go index 42ac2cc37..ccb1b8afd 100644 --- a/internal/database/model/model.go +++ b/internal/database/model/model.go @@ -1221,6 +1221,15 @@ type Host struct { func (Host) TableName() string { return "hosts" } +// HostTombstone keeps the inbound a deleted host served, so a client still bound to that host +// fails closed on that inbound alone. +type HostTombstone struct { + HostId int `json:"hostId" gorm:"primaryKey;autoIncrement:false;column:host_id"` + InboundId int `json:"inboundId" gorm:"not null;column:inbound_id"` +} + +func (HostTombstone) TableName() string { return "host_tombstones" } + // KeepAliveSeconds returns a scalar value; ranges are emitted by native AWG config. func (c Client) KeepAliveSeconds() int { if c.KeepAlive == nil { diff --git a/internal/sub/host_order_test.go b/internal/sub/host_order_test.go index 2e804b8bb..16e3fe7b0 100644 --- a/internal/sub/host_order_test.go +++ b/internal/sub/host_order_test.go @@ -22,7 +22,7 @@ import ( "github.com/mhsanaei/3x-ui/v3/internal/web/service" ) -func TestSubHostOrder_Postgres(t *testing.T) { +func TestSubHosts_Postgres(t *testing.T) { if os.Getenv("XUI_DB_TYPE") != "postgres" || os.Getenv("XUI_DB_DSN") == "" { t.Skip("requires a disposable PostgreSQL database") } @@ -34,6 +34,7 @@ func TestSubHostOrder_Postgres(t *testing.T) { {"filters", TestSub_HostOrderFiltersAndFallbacks}, {"base64", TestSub_HostOrderBase64AndInfoNode}, {"balancer", TestSub_HostOrderBalancerSlots}, + {"deleted binding", TestSub_ClientHostRule_DeletedHostClosesOnlyItsInbound}, } { t.Run(scenario.name, func(t *testing.T) { cleanup, err := testpg.IsolatePackage("host_order") diff --git a/internal/sub/host_sub.go b/internal/sub/host_sub.go index a8ee17b8b..3a27936c8 100644 --- a/internal/sub/host_sub.go +++ b/internal/sub/host_sub.go @@ -20,35 +20,54 @@ const ( hostBindingBound // render only the returned bound hosts of this inbound ) -// clientHostBinding resolves the client's bound host rules against one inbound. A binding whose -// every id is gone fails closed; a missing id beside a live one never silences other inbounds. +// clientHostBinding resolves the client's bound host rules against one inbound. A deleted bound host +// closes only the inbound it served; ids that resolve to no inbound at all fail closed everywhere. func (s *SubService) clientHostBinding(inbound *model.Inbound, client model.Client) (hostBinding, []*model.Host) { ids := model.EffectiveClientHostRuleIds(client) - if len(ids) == 0 || database.GetDB() == nil { + db := database.GetDB() + if len(ids) == 0 || db == nil { return hostBindingNone, nil } var rows []model.Host - if err := database.GetDB().Where("id IN ?", ids).Find(&rows).Error; err != nil { + if err := db.Where("id IN ?", ids).Find(&rows).Error; err != nil { logger.Warning("SubService - clientHostBinding:", err) return hostBindingClosed, nil } - if len(rows) == 0 { - return hostBindingClosed, nil - } byId := make(map[int]*model.Host, len(rows)) for i := range rows { byId[rows[i].Id] = &rows[i] } var here []*model.Host + var gone []int for _, id := range ids { - if h, ok := byId[id]; ok && h.InboundId == inbound.Id { + h, ok := byId[id] + switch { + case !ok: + gone = append(gone, id) + case h.InboundId == inbound.Id: here = append(here, h) } } - if len(here) == 0 { + if len(here) > 0 { + return hostBindingBound, here + } + if len(gone) == 0 { return hostBindingNone, nil } - return hostBindingBound, here + var tombstones []model.HostTombstone + if err := db.Where("host_id IN ?", gone).Find(&tombstones).Error; err != nil { + logger.Warning("SubService - clientHostBinding:", err) + return hostBindingClosed, nil + } + if len(rows) == 0 && len(tombstones) == 0 { + return hostBindingClosed, nil + } + for _, tomb := range tombstones { + if tomb.InboundId == inbound.Id { + return hostBindingClosed, nil + } + } + return hostBindingNone, nil } // clientHostEndpoints returns the endpoints of the client's usable bound hosts on this inbound, or diff --git a/internal/sub/host_sub_test.go b/internal/sub/host_sub_test.go index 1be9c36f5..90fd84d73 100644 --- a/internal/sub/host_sub_test.go +++ b/internal/sub/host_sub_test.go @@ -1160,8 +1160,8 @@ func TestSub_ClientHostRule_MultipleBindings(t *testing.T) { } } -// A deleted binding is skipped; with no valid binding left the client fails closed with zero -// links (no fallback, no dummy link) but stays in traffic accounting. +// A deleted binding is skipped; a binding that resolves to no inbound fails closed with zero links +// (no fallback, no dummy link) but stays in traffic accounting. func TestSub_ClientHostRule_DeletedFallsBack(t *testing.T) { t.Run("deleted and valid binding renders only the valid host", func(t *testing.T) { seedSubDB(t) @@ -1191,7 +1191,7 @@ func TestSub_ClientHostRule_DeletedFallsBack(t *testing.T) { } }) - t.Run("deleted-only binding fails closed with zero links", func(t *testing.T) { + t.Run("binding that resolves to no inbound fails closed with zero links", func(t *testing.T) { seedSubDB(t) ib := seedSubInbound(t, "s1", "fb", 445, 1, tcpTLSStream) seedHost(t, &model.Host{InboundId: ib.Id, SortOrder: 1, Remark: "A", Address: "a.cdn.com", Port: 443, Security: "tls", Sni: "a.sni"}) @@ -1228,8 +1228,8 @@ func TestSub_ClientHostRule_DeletedFallsBack(t *testing.T) { } }) - // A deleted host id must not silence an inbound without the client's other bound hosts: - // relevance comes from rows that still exist, fail-closed only when every bound id is gone. + // An unknown host id must not silence an inbound without the client's other bound hosts: the + // live and deleted hosts the ids resolve to decide which inbounds the binding guards. t.Run("deleted id on one inbound does not silence an unrelated inbound", func(t *testing.T) { seedSubDB(t) ibA := seedSubInbound(t, "s1", "xa", 4471, 1, tcpTLSStream) @@ -1267,6 +1267,65 @@ func TestSub_ClientHostRule_DeletedFallsBack(t *testing.T) { }) } +// Deleting a client's only bound host blanked it on every inbound, since the gone rows no longer +// said which inbound the binding guarded; only the inbound the host served may close. +func TestSub_ClientHostRule_DeletedHostClosesOnlyItsInbound(t *testing.T) { + for _, tc := range []struct { + name string + subId string + portA int + remove func(t *testing.T, ibA *model.Inbound, groupId string) error + }{ + {"by its host group", "dg", 4481, func(_ *testing.T, _ *model.Inbound, groupId string) error { + return (&service.HostService{}).DeleteHostGroup(groupId) + }}, + {"with its inbound", "di", 4483, func(t *testing.T, ibA *model.Inbound, _ string) error { + if err := database.GetDB().Model(ibA).Update("enable", false).Error; err != nil { + t.Fatalf("disable inbound A: %v", err) + } + _, err := (&service.InboundService{}).DelInbound(ibA.Id) + return err + }}, + } { + t.Run(tc.name, func(t *testing.T) { + seedSubDB(t) + ibA := seedSubInbound(t, tc.subId, tc.subId+"a", tc.portA, 1, tcpTLSStream) + ibB := seedSubInbound(t, tc.subId, tc.subId+"b", tc.portA+1, 2, tcpTLSStream) + email, groupId := tc.subId+"b@e", tc.subId+"-grp" + + db := database.GetDB() + var client model.ClientRecord + if err := db.Where("email = ?", email).First(&client).Error; err != nil { + t.Fatalf("load client %s: %v", email, err) + } + if err := db.Create(&model.ClientInbound{ClientId: client.Id, InboundId: ibA.Id}).Error; err != nil { + t.Fatalf("link client to inbound A: %v", err) + } + hostA := seedHost(t, &model.Host{GroupId: groupId, InboundId: ibA.Id, SortOrder: 1, Remark: "A", Address: "a.cdn.com", Port: 443, Security: "tls", Sni: "a.sni"}) + if err := db.Model(&model.ClientRecord{}).Where("email = ?", email). + Update("client_host_rule_ids", model.EncodeClientHostRuleIds([]int{hostA.Id})).Error; err != nil { + t.Fatalf("bind host rule: %v", err) + } + if err := tc.remove(t, ibA, groupId); err != nil { + t.Fatalf("delete the bound host: %v", err) + } + + links, _, _, _, err := NewSubService("").GetSubs(tc.subId, "req.example.com") + if err != nil { + t.Fatalf("GetSubs: %v", err) + } + joined := strings.Join(links, "\n") + uuid := "11111111-2222-4333-8444-" + fmt.Sprintf("%012d", ibB.Port) + if strings.Contains(joined, "a.cdn.com") || strings.Contains(joined, fmt.Sprintf("%s@203.0.113.5:%d", uuid, ibA.Port)) { + t.Fatalf("inbound A lost the client's bound host and must render nothing for it: %s", joined) + } + if !strings.Contains(joined, fmt.Sprintf("%s@203.0.113.5:%d", uuid, ibB.Port)) { + t.Fatalf("inbound B never had a binding and must keep the client's link: %s", joined) + } + }) + } +} + // A client bound to a disabled host fails closed on every format (no links, outbounds or proxies) // but stays in traffic accounting, so quota and expiry headers stay correct. func TestSub_ClientHostRule_DisabledFailsClosed(t *testing.T) { diff --git a/internal/web/service/host.go b/internal/web/service/host.go index 1ad65bc60..c4d2c88a1 100644 --- a/internal/web/service/host.go +++ b/internal/web/service/host.go @@ -337,7 +337,7 @@ func (s *HostService) UpdateHostGroup(groupId string, req *entity.HostGroup) ([] } } if len(toDelete) > 0 { - if err := tx.Where("id IN ?", toDelete).Delete(&model.Host{}).Error; err != nil { + if _, err := database.DeleteHosts(tx, "id IN ?", toDelete); err != nil { return err } } @@ -360,7 +360,10 @@ func (s *HostService) UpdateHostGroup(groupId string, req *entity.HostGroup) ([] } func (s *HostService) DeleteHostGroup(groupId string) error { - return database.GetDB().Where("group_id = ?", groupId).Delete(&model.Host{}).Error + return database.GetDB().Transaction(func(tx *gorm.DB) error { + _, err := database.DeleteHosts(tx, "group_id = ?", groupId) + return err + }) } func (s *HostService) SetHostGroupEnable(groupId string, enable bool) error { @@ -378,7 +381,10 @@ func (s *HostService) DeleteHostsGroup(groupIds []string) error { if len(groupIds) == 0 { return nil } - return database.GetDB().Where("group_id IN ?", groupIds).Delete(&model.Host{}).Error + return database.GetDB().Transaction(func(tx *gorm.DB) error { + _, err := database.DeleteHosts(tx, "group_id IN ?", groupIds) + return err + }) } func (s *HostService) ReorderHostGroups(groupIds []string) error { diff --git a/internal/web/service/inbound.go b/internal/web/service/inbound.go index 872a41669..31d9dd4d6 100644 --- a/internal/web/service/inbound.go +++ b/internal/web/service/inbound.go @@ -1665,7 +1665,7 @@ func (s *InboundService) delInbound(id int) (bool, func(), error) { if err := tx.Delete(model.Inbound{}, id).Error; err != nil { return err } - if err := tx.Where("inbound_id = ?", id).Delete(&model.Host{}).Error; err != nil { + if _, err := database.DeleteHosts(tx, "inbound_id = ?", id); err != nil { return err } // Drop the deleted inbound from any sub-balancer that selects it; a diff --git a/internal/web/service/inbound_traffic.go b/internal/web/service/inbound_traffic.go index c0f76f772..fb8129377 100644 --- a/internal/web/service/inbound_traffic.go +++ b/internal/web/service/inbound_traffic.go @@ -964,7 +964,7 @@ func (s *InboundService) DelDepletedClients(id int) (err error) { if err := s.clientService.DetachInbound(tx, inbound.Id); err != nil { return err } - if err := tx.Where("inbound_id = ?", inbound.Id).Delete(&model.Host{}).Error; err != nil { + if _, err := database.DeleteHosts(tx, "inbound_id = ?", inbound.Id); err != nil { return err } if err := tx.Delete(model.Inbound{}, inbound.Id).Error; err != nil {