diff --git a/docs/content/docs/en/reference/api/clients.mdx b/docs/content/docs/en/reference/api/clients.mdx index bcdc48019..14568fbde 100644 --- a/docs/content/docs/en/reference/api/clients.mdx +++ b/docs/content/docs/en/reference/api/clients.mdx @@ -550,21 +550,34 @@ _openapi: WireGuard is the only one of these that can fail. Allocation widens - the search to the containing /16 before giving up with `wireguard: no - free address available in `, and an `allowedIPs` supplied by - the caller is validated instead of allocated: `wireguard: allowedIPs - entry already used by another client:
` when a different - client of that same inbound already holds it. The check is per - inbound, so the same address on two different inbounds is accepted. - The same validation runs on POST /panel/api/clients/{email}/attach, - where a client that already carries an address brings it along. + the search to the containing /16 before giving up with `inbound : + wireguard: no free address available in `, and an `allowedIPs` + supplied by the caller is validated instead of allocated: `inbound + : wireguard: allowedIPs entry already used by another client: +
` when a different client of that same inbound already holds + it. The check is per inbound, so the same address on two different + inbounds is accepted. The same validation runs on POST + /panel/api/clients/{email}/attach, where a client that already carries + an address brings it along. + + + An `inboundIds` entry that names no existing inbound rejects the whole + call before anything is written. Past that, the inbounds are applied + concurrently and independently: one that fails no longer stops the + others, so a `success:false` response can still have created the + client on the rest. Every error names the inbound it came from + (`inbound 7: `), and several failures are reported together, + one per line. `limitHwid` is applied only when every inbound + succeeded, so re-run the call after fixing the failure. heading: create-a-new-client-and-attach-it-to-one-or-more-inbounds-in-a-single-call-body-is-json-per-protocol-secrets-are-generated-server-side-when-omitted-so-callers-can-send-only-the-universal-fields - content: 'A WireGuard client brings its stored `allowedIPs` into the new inbound instead of being given a fresh address, so the call fails with - `wireguard: allowedIPs entry already used by another client: -
` when a different client of the target inbound already holds - it. Free the address on that inbound first — see POST - /panel/api/clients/add for the full rule.' + `inbound : wireguard: allowedIPs entry already used by another + client:
` when a different client of the target inbound + already holds it. Free the address on that inbound first — see POST + /panel/api/clients/add for the full rule. Inbounds are applied + independently, so the remaining ones are still attached and a + `success:false` response can be partial.' heading: attach-an-existing-client-to-one-or-more-additional-inbounds-body-is-json --- diff --git a/docs/public/openapi.json b/docs/public/openapi.json index fda39b8b4..f80ea544b 100644 --- a/docs/public/openapi.json +++ b/docs/public/openapi.json @@ -6954,7 +6954,7 @@ ], "summary": "Create a new client and attach it to one or more inbounds in a single call. Body is JSON. Per-protocol secrets are generated server-side when omitted, so callers can send only the universal fields.", "operationId": "post_panel_api_clients_add", - "description": "Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `wireguard: allowedIPs entry already used by another client:
` when a different client of that same inbound already holds it. The check is per inbound, so the same address on two different inbounds is accepted. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.", + "description": "Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `inbound : wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound : wireguard: allowedIPs entry already used by another client:
` when a different client of that same inbound already holds it. The check is per inbound, so the same address on two different inbounds is accepted. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.\n\nAn `inboundIds` entry that names no existing inbound rejects the whole call before anything is written. Past that, the inbounds are applied concurrently and independently: one that fails no longer stops the others, so a `success:false` response can still have created the client on the rest. Every error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. `limitHwid` is applied only when every inbound succeeded, so re-run the call after fixing the failure.", "requestBody": { "required": true, "content": { @@ -7148,7 +7148,7 @@ ], "summary": "Attach an existing client to one or more additional inbounds. Body is JSON.", "operationId": "post_panel_api_clients_email_attach", - "description": "A WireGuard client brings its stored `allowedIPs` into the new inbound instead of being given a fresh address, so the call fails with `wireguard: allowedIPs entry already used by another client:
` when a different client of the target inbound already holds it. Free the address on that inbound first — see POST /panel/api/clients/add for the full rule.", + "description": "A WireGuard client brings its stored `allowedIPs` into the new inbound instead of being given a fresh address, so the call fails with `inbound : wireguard: allowedIPs entry already used by another client:
` when a different client of the target inbound already holds it. Free the address on that inbound first — see POST /panel/api/clients/add for the full rule. Inbounds are applied independently, so the remaining ones are still attached and a `success:false` response can be partial.", "parameters": [ { "name": "email", diff --git a/frontend/public/openapi.json b/frontend/public/openapi.json index fda39b8b4..f80ea544b 100644 --- a/frontend/public/openapi.json +++ b/frontend/public/openapi.json @@ -6954,7 +6954,7 @@ ], "summary": "Create a new client and attach it to one or more inbounds in a single call. Body is JSON. Per-protocol secrets are generated server-side when omitted, so callers can send only the universal fields.", "operationId": "post_panel_api_clients_add", - "description": "Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `wireguard: allowedIPs entry already used by another client:
` when a different client of that same inbound already holds it. The check is per inbound, so the same address on two different inbounds is accepted. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.", + "description": "Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `inbound : wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound : wireguard: allowedIPs entry already used by another client:
` when a different client of that same inbound already holds it. The check is per inbound, so the same address on two different inbounds is accepted. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.\n\nAn `inboundIds` entry that names no existing inbound rejects the whole call before anything is written. Past that, the inbounds are applied concurrently and independently: one that fails no longer stops the others, so a `success:false` response can still have created the client on the rest. Every error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. `limitHwid` is applied only when every inbound succeeded, so re-run the call after fixing the failure.", "requestBody": { "required": true, "content": { @@ -7148,7 +7148,7 @@ ], "summary": "Attach an existing client to one or more additional inbounds. Body is JSON.", "operationId": "post_panel_api_clients_email_attach", - "description": "A WireGuard client brings its stored `allowedIPs` into the new inbound instead of being given a fresh address, so the call fails with `wireguard: allowedIPs entry already used by another client:
` when a different client of the target inbound already holds it. Free the address on that inbound first — see POST /panel/api/clients/add for the full rule.", + "description": "A WireGuard client brings its stored `allowedIPs` into the new inbound instead of being given a fresh address, so the call fails with `inbound : wireguard: allowedIPs entry already used by another client:
` when a different client of the target inbound already holds it. Free the address on that inbound first — see POST /panel/api/clients/add for the full rule. Inbounds are applied independently, so the remaining ones are still attached and a `success:false` response can be partial.", "parameters": [ { "name": "email", diff --git a/frontend/src/pages/api-docs/endpoints.ts b/frontend/src/pages/api-docs/endpoints.ts index cd79e8428..77a8a8140 100644 --- a/frontend/src/pages/api-docs/endpoints.ts +++ b/frontend/src/pages/api-docs/endpoints.ts @@ -1030,7 +1030,7 @@ export const sections: readonly Section[] = [ summary: 'Create a new client and attach it to one or more inbounds in a single call. Body is JSON. Per-protocol secrets are generated server-side when omitted, so callers can send only the universal fields.', description: - 'Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `wireguard: allowedIPs entry already used by another client:
` when a different client of that same inbound already holds it. The check is per inbound, so the same address on two different inbounds is accepted. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.', + 'Fields the server fills in when they are omitted — a valid value sent by the caller is never overwritten. Re-adding an email that already exists, with its stored `subId`, reuses the stored `id`, `password`, `auth` and `secret` instead of minting new ones, so the identity stays in sync across its inbounds.\n\n- **VLESS / VMess** — `id`, a fresh UUID\n- **Trojan** — `password`\n- **Shadowsocks** — `password`. On a `2022-blake3-*` inbound a supplied password that does not base64-decode to the key length of the cipher (16 or 32 bytes) is replaced by a generated key and the call still succeeds, so read the client back if you did not let the server pick. Legacy ciphers keep any non-empty password\n- **Hysteria** — `auth`\n- **mtproto** — `secret`, a FakeTLS secret derived from the fronting domain of the inbound, or from `www.cloudflare.com` when it has none\n- **WireGuard** — `privateKey` and `publicKey` when both are blank, or `publicKey` alone when only a `privateKey` was sent, plus `allowedIPs`: one free `/32` taken from the /24 the existing peers of that inbound already sit in, or from `10.0.0.0/24` when it has none\n\nAccepted on the same body but never generated: `preSharedKey` and `keepAlive` (WireGuard), `adTag` (mtproto).\n\nWireGuard is the only one of these that can fail. Allocation widens the search to the containing /16 before giving up with `inbound : wireguard: no free address available in `, and an `allowedIPs` supplied by the caller is validated instead of allocated: `inbound : wireguard: allowedIPs entry already used by another client:
` when a different client of that same inbound already holds it. The check is per inbound, so the same address on two different inbounds is accepted. The same validation runs on POST /panel/api/clients/{email}/attach, where a client that already carries an address brings it along.\n\nAn `inboundIds` entry that names no existing inbound rejects the whole call before anything is written. Past that, the inbounds are applied concurrently and independently: one that fails no longer stops the others, so a `success:false` response can still have created the client on the rest. Every error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. `limitHwid` is applied only when every inbound succeeded, so re-run the call after fixing the failure.', params: [ { name: 'client', @@ -1085,7 +1085,7 @@ export const sections: readonly Section[] = [ path: '/panel/api/clients/:email/attach', summary: 'Attach an existing client to one or more additional inbounds. Body is JSON.', description: - 'A WireGuard client brings its stored `allowedIPs` into the new inbound instead of being given a fresh address, so the call fails with `wireguard: allowedIPs entry already used by another client:
` when a different client of the target inbound already holds it. Free the address on that inbound first — see POST /panel/api/clients/add for the full rule.', + 'A WireGuard client brings its stored `allowedIPs` into the new inbound instead of being given a fresh address, so the call fails with `inbound : wireguard: allowedIPs entry already used by another client:
` when a different client of the target inbound already holds it. Free the address on that inbound first — see POST /panel/api/clients/add for the full rule. Inbounds are applied independently, so the remaining ones are still attached and a `success:false` response can be partial.', params: [ { name: 'email', in: 'path', type: 'string', desc: 'Client email (unique identifier).' }, { diff --git a/internal/web/controller/client.go b/internal/web/controller/client.go index 9e9360a0c..d6224ed99 100644 --- a/internal/web/controller/client.go +++ b/internal/web/controller/client.go @@ -186,15 +186,21 @@ func (a *ClientController) create(c *gin.Context) { return } needRestart, err := a.clientService.Create(&a.inboundService, &payload) + // Flagged before the error check: a partly-applied create leaves clients + // committed on the inbounds that succeeded, and those still need the restart. + if needRestart { + a.xrayService.SetToNeedRestart() + } + // A partly-applied call committed real clients; a rejected one touched + // nothing, and broadcasting those would refetch every panel for nothing. + if needRestart || err == nil { + notifyClientsChanged() + } if err != nil { jsonMsg(c, I18nWeb(c, "somethingWentWrong"), err) return } jsonMsgObj(c, I18nWeb(c, "pages.inbounds.toasts.inboundClientAddSuccess"), pendingNodeObj(a.inboundService.AnyNodePending(payload.InboundIds)), nil) - if needRestart { - a.xrayService.SetToNeedRestart() - } - notifyClientsChanged() } func (a *ClientController) update(c *gin.Context) { @@ -251,15 +257,19 @@ func (a *ClientController) attach(c *gin.Context) { return } needRestart, err := a.clientService.AttachByEmail(&a.inboundService, email, body.InboundIds) + if needRestart { + a.xrayService.SetToNeedRestart() + } + // A partly-applied call committed real clients; a rejected one touched + // nothing, and broadcasting those would refetch every panel for nothing. + if needRestart || err == nil { + notifyClientsChanged() + } if err != nil { jsonMsg(c, I18nWeb(c, "somethingWentWrong"), err) return } jsonMsgObj(c, I18nWeb(c, "pages.inbounds.toasts.inboundClientAddSuccess"), pendingNodeObj(a.inboundService.AnyNodePending(body.InboundIds)), nil) - if needRestart { - a.xrayService.SetToNeedRestart() - } - notifyClientsChanged() } func (a *ClientController) setExternalLinks(c *gin.Context) { diff --git a/internal/web/job/ldap_sync_job.go b/internal/web/job/ldap_sync_job.go index b5ad7eec3..6a3561ed0 100644 --- a/internal/web/job/ldap_sync_job.go +++ b/internal/web/job/ldap_sync_job.go @@ -237,22 +237,24 @@ func (j *LdapSyncJob) createClients(newClients []model.Client, inboundIds []int, restartNeeded := false for _, c := range newClients { nr, err := j.clientService.Create(&j.inboundService, &service.ClientCreatePayload{Client: c, InboundIds: inboundIds}) + // Read before the error check: a partly-applied create still committed + // clients on the inbounds that succeeded, and those need the restart. + if nr { + restartNeeded = true + } if err != nil { logger.Warningf("Failed to add client %s for tags %s: %v", c.Email, tagList, err) continue } created++ - if nr { - restartNeeded = true - } + } + if restartNeeded { + j.xrayService.SetToNeedRestart() } if created == 0 { return } logger.Infof("LDAP auto-create: %d clients for %s", created, tagList) - if restartNeeded { - j.xrayService.SetToNeedRestart() - } } func (j *LdapSyncJob) batchSetEnable(ib *model.Inbound, emails []string, enable bool) { diff --git a/internal/web/job/ldap_sync_job_test.go b/internal/web/job/ldap_sync_job_test.go index 1df9dba3e..9f2463ea0 100644 --- a/internal/web/job/ldap_sync_job_test.go +++ b/internal/web/job/ldap_sync_job_test.go @@ -91,3 +91,42 @@ func TestLdapCreateClients_AttachesToAllConfiguredInbounds(t *testing.T) { t.Error("vless inbound client must get a generated uuid") } } + +// TestLdapCreateClients_FlagsRestartWhenEveryClientPartlyApplies pins that the +// restart survives created == 0, the case a partly-applied batch always hits. +func TestLdapCreateClients_FlagsRestartWhenEveryClientPartlyApplies(t *testing.T) { + initLdapJobDB(t) + db := database.GetDB() + + healthy := &model.Inbound{ + UserId: 1, Tag: "in-42180-tcp", Enable: true, Port: 42180, + Protocol: model.VLESS, Settings: `{"clients": []}`, + StreamSettings: `{"network":"tcp","security":"none"}`, + } + broken := &model.Inbound{ + UserId: 1, Tag: "in-42181-tcp", Enable: true, Port: 42181, + Protocol: model.VLESS, Settings: `{"clients":`, + StreamSettings: `{"network":"tcp","security":"none"}`, + } + for _, ib := range []*model.Inbound{healthy, broken} { + if err := db.Create(ib).Error; err != nil { + t.Fatalf("create inbound %s: %v", ib.Tag, err) + } + } + + j := NewLdapSyncJob() + j.xrayService.IsNeedRestartAndSetFalse() + j.createClients([]model.Client{j.buildClient("partial@example.com", 0, 0, 0)}, + []int{healthy.Id, broken.Id}, []string{healthy.Tag, broken.Tag}) + + clients, err := (&service.ClientService{}).ListForInbound(nil, healthy.Id) + if err != nil { + t.Fatalf("ListForInbound(%s): %v", healthy.Tag, err) + } + if len(clients) != 1 { + t.Fatalf("healthy inbound holds %d clients, want the partly-applied 1", len(clients)) + } + if !j.xrayService.IsNeedRestartAndSetFalse() { + t.Fatal("a partly-applied LDAP batch left Xray unflagged for restart") + } +} diff --git a/internal/web/service/client_create_fanout_test.go b/internal/web/service/client_create_fanout_test.go index 80ced7c93..1d3b63d52 100644 --- a/internal/web/service/client_create_fanout_test.go +++ b/internal/web/service/client_create_fanout_test.go @@ -1,9 +1,16 @@ package service import ( + "context" + "fmt" + "strings" + "sync/atomic" "testing" + "time" + "github.com/mhsanaei/3x-ui/v3/internal/database" "github.com/mhsanaei/3x-ui/v3/internal/database/model" + "github.com/mhsanaei/3x-ui/v3/internal/web/runtime" ) func TestCreateAcrossManyInboundsUsesOneEmailSnapshot(t *testing.T) { @@ -82,3 +89,252 @@ func TestAttachAcrossManyInboundsUsesOneEmailSnapshot(t *testing.T) { t.Fatalf("linked inbounds = %d, want %d", len(linked), len(ids)) } } + +// barrierNodeRuntime holds every AddClient until fanout of them are inside it at +// once, recording the peak overlap; a sequential caller only ever reaches one. +type barrierNodeRuntime struct { + fakeNodeRuntime + fanout int32 + inFlight atomic.Int32 + maxPar atomic.Int32 + release chan struct{} + freed atomic.Bool + expired atomic.Bool +} + +func (b *barrierNodeRuntime) free() { + if b.freed.CompareAndSwap(false, true) { + close(b.release) + } +} + +func (b *barrierNodeRuntime) AddClient(ctx context.Context, ib *model.Inbound, c model.Client) error { + n := b.inFlight.Add(1) + for { + peak := b.maxPar.Load() + if n <= peak || b.maxPar.CompareAndSwap(peak, n) { + break + } + } + if n == b.fanout { + b.free() + } + select { + case <-b.release: + case <-time.After(5 * time.Second): + // Release everyone on the first timeout so a sequential regression + // fails once instead of stalling for fanout x the wait. + b.expired.Store(true) + b.free() + } + b.inFlight.Add(-1) + return b.fakeNodeRuntime.AddClient(ctx, ib, c) +} + +func fanoutNodeInbounds(t *testing.T, mgr *runtime.Manager, rt runtime.Runtime, n int, basePort int) []int { + t.Helper() + ids := make([]int, 0, n) + for i := range n { + node := &model.Node{ + Name: fmt.Sprintf("%s-%d", t.Name(), i), Address: "127.0.0.1", Port: 2096 + i, + ApiToken: "tok", Enable: true, Status: "online", + } + if err := database.GetDB().Create(node).Error; err != nil { + t.Fatalf("create node %d: %v", i, err) + } + mgr.SetRuntimeOverride(node.Id, rt) + ids = append(ids, nodeInbound(t, node.Id, basePort+i, nil).Id) + } + return ids +} + +// TestCreateAcrossNodesPushesConcurrently pins that a client spanning several +// node inbounds pushes to them at once, up to inboundFanoutConcurrency at a time. +func TestCreateAcrossNodesPushesConcurrently(t *testing.T) { + setupBulkDB(t) + startSerializedWriter(t) + mgr := useTestRuntimeManager(t) + + const nodes = inboundFanoutConcurrency + 1 + bar := &barrierNodeRuntime{fanout: inboundFanoutConcurrency, release: make(chan struct{})} + ids := fanoutNodeInbounds(t, mgr, bar, nodes, 40101) + + if _, err := (&ClientService{}).Create(&InboundService{}, &ClientCreatePayload{ + Client: model.Client{Email: "fanout@x", ID: "11111111-2222-3333-4444-555555555555", SubID: "sub-fanout", Enable: true}, + InboundIds: ids, + }); err != nil { + t.Fatalf("Create across %d node inbounds: %v", nodes, err) + } + + if got := bar.addClient.Load(); got != nodes { + t.Fatalf("AddClient pushes = %d, want %d", got, nodes) + } + if got := bar.maxPar.Load(); got < 2 || got != inboundFanoutConcurrency { + t.Fatalf("peak node pushes in flight = %d, want overlap at the %d cap (barrier timed out: %v)", + got, inboundFanoutConcurrency, bar.expired.Load()) + } +} + +// TestCreateRecoversPanicInOneInbound pins that a panicking inbound fails only +// itself: off the request goroutine nothing else would catch it. +func TestCreateRecoversPanicInOneInbound(t *testing.T) { + setupBulkDB(t) + startSerializedWriter(t) + mgr := useTestRuntimeManager(t) + + node := &model.Node{ + Name: t.Name(), Address: "127.0.0.1", Port: 2096, + ApiToken: "tok", Enable: true, Status: "online", + } + if err := database.GetDB().Create(node).Error; err != nil { + t.Fatalf("create node: %v", err) + } + mgr.SetRuntimeOverride(node.Id, &panicNodeRuntime{}) + boom := nodeInbound(t, node.Id, 40201, nil) + healthy := mkInbound(t, 40202, model.VLESS, `{"clients":[]}`) + + const uuid = "33333333-4444-5555-6666-777777777777" + _, err := (&ClientService{}).Create(&InboundService{}, &ClientCreatePayload{ + Client: model.Client{Email: "panic@x", ID: uuid, SubID: "sub-panic", Enable: true}, + InboundIds: []int{boom.Id, healthy.Id}, + }) + if err == nil { + t.Fatal("a panicking node runtime produced no error") + } + if want := fmt.Sprintf("inbound %d: panic:", boom.Id); !strings.Contains(err.Error(), want) { + t.Fatalf("error %q does not report %q", err, want) + } + if !settingsHoldUUID(t, &InboundService{}, healthy.Id, uuid) { + t.Fatalf("healthy inbound %d did not get the client", healthy.Id) + } +} + +// TestCreateLeavesHwidLimitAloneWhenCreateFails pins that a create the panel +// reported as failed never rewrites a device cap, so it can never retrim one. +func TestCreateLeavesHwidLimitAloneWhenCreateFails(t *testing.T) { + setupBulkDB(t) + startSerializedWriter(t) + svc := &ClientService{} + inboundSvc := &InboundService{} + + const vipUUID = "44444444-5555-6666-7777-888888888888" + seed := mkInbound(t, 41401, model.VLESS, `{"clients":[]}`) + if _, err := svc.Create(inboundSvc, &ClientCreatePayload{ + Client: model.Client{Email: "vip@x", ID: vipUUID, SubID: "sub-vip", Enable: true}, + InboundIds: []int{seed.Id}, + LimitHwid: 3, + }); err != nil { + t.Fatalf("seed Create: %v", err) + } + + broken := mkInbound(t, 41402, model.VLESS, `{"clients":`) + if _, err := svc.Create(inboundSvc, &ClientCreatePayload{ + Client: model.Client{Email: "vip@x", ID: vipUUID, SubID: "sub-vip", Enable: true}, + InboundIds: []int{broken.Id}, + LimitHwid: 1, + }); err == nil { + t.Fatal("re-adding to an unparsable inbound returned no error") + } + + if rec := lookupClientRecord(t, "vip@x"); rec.LimitHwid != 3 { + t.Fatalf("limit_hwid = %d, want the untouched 3: a failed create retrimmed a live client", rec.LimitHwid) + } + + // Same failure with the seeded inbound alongside it: that one is a dedup + // no-op returning no error, which must not read as "an inbound took it". + if _, err := svc.Create(inboundSvc, &ClientCreatePayload{ + Client: model.Client{Email: "vip@x", ID: vipUUID, SubID: "sub-vip", Enable: true}, + InboundIds: []int{seed.Id, broken.Id}, + LimitHwid: 1, + }); err == nil { + t.Fatal("re-adding over a no-op and an unparsable inbound returned no error") + } + if rec := lookupClientRecord(t, "vip@x"); rec.LimitHwid != 3 { + t.Fatalf("limit_hwid = %d, want the untouched 3: a no-op inbound counted as applied", rec.LimitHwid) + } + + // A brand new identity that only partly applies is left uncapped rather than + // capped, the deliberate safe side: the operator saw the error and retries. + healthy := mkInbound(t, 41403, model.VLESS, `{"clients":[]}`) + if _, err := svc.Create(inboundSvc, &ClientCreatePayload{ + Client: model.Client{Email: "fresh@x", ID: "55555555-6666-7777-8888-999999999999", SubID: "sub-fresh", Enable: true}, + InboundIds: []int{healthy.Id, broken.Id}, + LimitHwid: 5, + }); err == nil { + t.Fatal("creating over an unparsable inbound returned no error") + } + if rec := lookupClientRecord(t, "fresh@x"); rec.LimitHwid != 0 { + t.Fatalf("limit_hwid = %d, want 0 on a create that failed", rec.LimitHwid) + } +} + +func assertNamesFailedInbounds(t *testing.T, err error, broken []*model.Inbound, healthy *model.Inbound) { + t.Helper() + if err == nil { + t.Fatalf("applying %d unparsable inbounds returned no error", len(broken)) + } + for _, ib := range broken { + if want := fmt.Sprintf("inbound %d:", ib.Id); !strings.Contains(err.Error(), want) { + t.Fatalf("error %q does not name the failing %s", err, want) + } + } + if blamed := fmt.Sprintf("inbound %d:", healthy.Id); strings.Contains(err.Error(), blamed) { + t.Fatalf("error %q blames the healthy %s", err, blamed) + } +} + +// TestFanoutReportsEveryFailingInbound pins that no inbound aborts the others: +// each failure names its own inbound, and the healthy ones still get the client. +func TestFanoutReportsEveryFailingInbound(t *testing.T) { + const halfBadUUID = "22222222-3333-4444-5555-666666666666" + + t.Run("create", func(t *testing.T) { + setupBulkDB(t) + startSerializedWriter(t) + svc := &ClientService{} + inboundSvc := &InboundService{} + + broken := []*model.Inbound{ + mkInbound(t, 41201, model.VLESS, `{"clients":`), + mkInbound(t, 41202, model.VLESS, `{"clients":`), + } + healthy := mkInbound(t, 41203, model.VLESS, `{"clients":[]}`) + + _, err := svc.Create(inboundSvc, &ClientCreatePayload{ + Client: model.Client{Email: "halfbad@x", ID: halfBadUUID, SubID: "sub-halfbad", Enable: true}, + InboundIds: []int{broken[0].Id, broken[1].Id, healthy.Id}, + }) + assertNamesFailedInbounds(t, err, broken, healthy) + if !settingsHoldUUID(t, inboundSvc, healthy.Id, halfBadUUID) { + t.Fatalf("healthy inbound %d did not get the client", healthy.Id) + } + }) + + t.Run("attach", func(t *testing.T) { + setupBulkDB(t) + startSerializedWriter(t) + svc := &ClientService{} + inboundSvc := &InboundService{} + + seed := mkInbound(t, 41301, model.VLESS, `{"clients":[]}`) + if _, err := svc.Create(inboundSvc, &ClientCreatePayload{ + Client: model.Client{Email: "halfbad@x", ID: halfBadUUID, SubID: "sub-halfbad", Enable: true}, + InboundIds: []int{seed.Id}, + }); err != nil { + t.Fatalf("seed Create: %v", err) + } + + broken := []*model.Inbound{ + mkInbound(t, 41302, model.VLESS, `{"clients":`), + mkInbound(t, 41303, model.VLESS, `{"clients":`), + } + healthy := mkInbound(t, 41304, model.VLESS, `{"clients":[]}`) + + rec := lookupClientRecord(t, "halfbad@x") + _, err := svc.Attach(inboundSvc, rec.Id, []int{broken[0].Id, broken[1].Id, healthy.Id}) + assertNamesFailedInbounds(t, err, broken, healthy) + if !settingsHoldUUID(t, inboundSvc, healthy.Id, halfBadUUID) { + t.Fatalf("healthy inbound %d did not get the client", healthy.Id) + } + }) +} diff --git a/internal/web/service/client_crud.go b/internal/web/service/client_crud.go index d18f5bbca..a6fb387bb 100644 --- a/internal/web/service/client_crud.go +++ b/internal/web/service/client_crud.go @@ -6,7 +6,10 @@ import ( "errors" "fmt" "net/netip" + "runtime/debug" "strings" + "sync" + "sync/atomic" "time" "unicode" @@ -14,6 +17,7 @@ import ( "github.com/mhsanaei/3x-ui/v3/internal/database" "github.com/mhsanaei/3x-ui/v3/internal/database/model" + "github.com/mhsanaei/3x-ui/v3/internal/logger" "github.com/mhsanaei/3x-ui/v3/internal/util/common" "github.com/mhsanaei/3x-ui/v3/internal/util/random" "github.com/mhsanaei/3x-ui/v3/internal/xray" @@ -116,6 +120,8 @@ func (s *ClientService) GetClientsByTrafficReset(period string) ([]ClientResetCy return cycles, nil } +// Create applies the client to every requested inbound: one failing inbound no +// longer aborts the others, so the error can name several and needRestart holds. func (s *ClientService) Create(inboundSvc *InboundService, payload *ClientCreatePayload) (bool, error) { if payload == nil { return false, common.NewError("empty payload") @@ -194,14 +200,16 @@ func (s *ClientService) Create(inboundSvc *InboundService, payload *ClientCreate } } - needRestart := false + // Prepared before any inbound is written: fillProtocolDefaults mints the + // shared credentials on the first inbound and every later one reuses them. + adds := make([]*model.Inbound, 0, len(payload.InboundIds)) for _, ibId := range payload.InboundIds { inbound, getErr := inboundSvc.GetInbound(ibId) if getErr != nil { - return needRestart, getErr + return false, fmt.Errorf("inbound %d: %w", ibId, getErr) } if err := s.fillProtocolDefaults(&client, inbound); err != nil { - return needRestart, err + return false, fmt.Errorf("inbound %d: %w", ibId, err) } clientForInbound := client if ips, ok := client.AllowedIPsByInbound[ibId]; ok { @@ -217,23 +225,59 @@ func (s *ClientService) Create(inboundSvc *InboundService, payload *ClientCreate } settingsPayload, mErr := json.Marshal(map[string][]model.Client{"clients": {clientWithInboundFlow(clientForInbound, inbound)}}) if mErr != nil { - return needRestart, mErr - } - nr, addErr := s.AddInboundClient(inboundSvc, &model.Inbound{ - Id: ibId, - Settings: string(settingsPayload), - }) - if addErr != nil { - return needRestart, addErr - } - if nr { - needRestart = true + return false, fmt.Errorf("inbound %d: %w", ibId, mErr) } + adds = append(adds, &model.Inbound{Id: ibId, Settings: string(settingsPayload)}) } - if err := s.setClientLimitHwidByEmail(nil, client.Email, payload.LimitHwid); err != nil { - return needRestart, err + needRestart, fanoutErr := s.fanoutInboundClientAdds(inboundSvc, adds) + if fanoutErr != nil { + // Never on a failed create: this retrims the devices of an email that + // already existed, and a create the panel reported as failed must not. + return needRestart, fanoutErr } - return needRestart, nil + return needRestart, s.setClientLimitHwidByEmail(nil, client.Email, payload.LimitHwid) +} + +// inboundFanoutConcurrency caps how many inbounds one create/attach applies at +// once, so a client spanning many of them can't start an unbounded RPC burst. +const inboundFanoutConcurrency = 4 + +// fanoutInboundClientAdds applies one payload per inbound with the node pushes +// overlapping; unlike the sequential loop, one failure no longer stops the rest. +func (s *ClientService) fanoutInboundClientAdds(inboundSvc *InboundService, adds []*model.Inbound) (bool, error) { + var needRestart atomic.Bool + errs := make([]error, len(adds)) + sem := make(chan struct{}, inboundFanoutConcurrency) + var wg sync.WaitGroup + for i := range adds { + wg.Add(1) + sem <- struct{}{} + go func() { + defer wg.Done() + defer func() { <-sem }() + // Off the request goroutine gin's Recovery no longer covers this, + // so an unrecovered panic here would take the whole panel down. + defer func() { + if r := recover(); r != nil { + // The apply may already have committed, so ask for the + // restart the lost return value can no longer report. + needRestart.Store(true) + errs[i] = fmt.Errorf("inbound %d: panic: %v", adds[i].Id, r) + logger.Errorf("panic adding client to inbound %d: %v\n%s", adds[i].Id, r, debug.Stack()) + } + }() + nr, err := s.AddInboundClient(inboundSvc, adds[i]) + if nr { + needRestart.Store(true) + } + if err != nil { + errs[i] = fmt.Errorf("inbound %d: %w", adds[i].Id, err) + } + }() + } + wg.Wait() + + return needRestart.Load(), errors.Join(errs...) } func (s *ClientService) fillProtocolDefaults(c *model.Client, ib *model.Inbound) error { @@ -792,6 +836,8 @@ func addressesFitAmneziaWGInbound(addrs []string, ib *model.Inbound) bool { return true } +// Attach applies the client to every requested inbound: one failing inbound no +// longer aborts the others, so the error can name several and needRestart holds. func (s *ClientService) Attach(inboundSvc *InboundService, id int, inboundIds []int) (bool, error) { existing, err := s.GetByID(id) if err != nil { @@ -826,38 +872,29 @@ func (s *ClientService) Attach(inboundSvc *InboundService, id int, inboundIds [] clientWire.AllowedIPs = nil } - needRestart := false + adds := make([]*model.Inbound, 0, len(inboundIds)) for _, ibId := range inboundIds { if _, attached := have[ibId]; attached { continue } inbound, getErr := inboundSvc.GetInbound(ibId) if getErr != nil { - return needRestart, getErr + return false, fmt.Errorf("inbound %d: %w", ibId, getErr) } copyClient := *clientWire if !addressesFitAmneziaWGInbound(copyClient.AllowedIPs, inbound) { copyClient.AllowedIPs = nil } if err := s.fillProtocolDefaults(©Client, inbound); err != nil { - return needRestart, err + return false, fmt.Errorf("inbound %d: %w", ibId, err) } settingsPayload, mErr := json.Marshal(map[string][]model.Client{"clients": {clientWithInboundFlow(copyClient, inbound)}}) if mErr != nil { - return needRestart, mErr - } - nr, addErr := s.AddInboundClient(inboundSvc, &model.Inbound{ - Id: ibId, - Settings: string(settingsPayload), - }) - if addErr != nil { - return needRestart, addErr - } - if nr { - needRestart = true + return false, fmt.Errorf("inbound %d: %w", ibId, mErr) } + adds = append(adds, &model.Inbound{Id: ibId, Settings: string(settingsPayload)}) } - return needRestart, nil + return s.fanoutInboundClientAdds(inboundSvc, adds) } func (s *ClientService) CreateOne(inboundSvc *InboundService, inboundId int, client model.Client) (bool, error) { diff --git a/internal/web/service/node_bulk_dispatch_test.go b/internal/web/service/node_bulk_dispatch_test.go index 0ddc5c8ab..09e7aed04 100644 --- a/internal/web/service/node_bulk_dispatch_test.go +++ b/internal/web/service/node_bulk_dispatch_test.go @@ -80,15 +80,38 @@ func (f *fakeNodeRuntime) ResetClientTraffic(context.Context, *model.Inbound, st func (f *fakeNodeRuntime) ResetInboundTraffic(context.Context, *model.Inbound) error { return nil } func (f *fakeNodeRuntime) ResetAllTraffics(context.Context) error { return nil } -// setupNodeRuntime wires an online node + a fake runtime override and returns the -// node id and the fake so a test can drive the service node-dispatch path without -// a network node. -func setupNodeRuntime(t *testing.T) (int, *fakeNodeRuntime) { +// startSerializedWriter runs the single traffic-writer goroutine for the test, so +// concurrent service writes take the serialized path production uses. +func startSerializedWriter(t *testing.T) { + t.Helper() + resetTrafficWriterForTest(t) + StartTrafficWriter() +} + +// useTestRuntimeManager swaps in a fresh runtime.Manager for the test and puts +// the previous one back afterwards, so overrides can't leak between tests. +func useTestRuntimeManager(t *testing.T) *runtime.Manager { t.Helper() prev := runtime.GetManager() mgr := runtime.NewManager(runtime.LocalDeps{APIPort: func() int { return 0 }, SetNeedRestart: func() {}}) runtime.SetManager(mgr) t.Cleanup(func() { runtime.SetManager(prev) }) + return mgr +} + +// panicNodeRuntime panics on the per-client push, standing in for a bug in the +// apply path that would otherwise unwind straight out of a fanout goroutine. +type panicNodeRuntime struct{ fakeNodeRuntime } + +func (p *panicNodeRuntime) AddClient(context.Context, *model.Inbound, model.Client) error { + panic("boom from node runtime") +} + +// setupNodeRuntime wires an online node + a fake runtime override so a test can +// drive the service node-dispatch path without a network node. +func setupNodeRuntime(t *testing.T) (int, *fakeNodeRuntime) { + t.Helper() + mgr := useTestRuntimeManager(t) node := &model.Node{Name: "n1-" + t.Name(), Address: "127.0.0.1", Port: 2096, ApiToken: "tok", Enable: true, Status: "online"} if err := database.GetDB().Create(node).Error; err != nil {