From f072d0448d412203cc728274a4814dd374868053 Mon Sep 17 00:00:00 2001 From: Sanaei Date: Mon, 7 Sep 2026 01:51:01 +0200 Subject: [PATCH] fix(clients): flag the restart a partly-applied edit or delete still needs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 63b46cd6 made a multi-inbound client op apply its inbounds concurrently and stop aborting at the first failure, so an error can now come back together with needRestart=true: the inbounds that succeeded committed real changes and their Xray still needs the restart. That commit taught the two callers it converted — create and attach — to read the flag before the error check. d34ec97f then routed Update, Delete, Detach and DeleteByEmail's record-less fallback through the same fanout but touched no caller, so on a master with several nodes a partly-applied edit or delete returned (true, err) into a handler that returned on err first. Xray was never flagged for the work that landed and notifyClientsChanged never fired, so the running config kept serving the pre-edit client set and every open panel showed stale rows until something else happened to trigger a restart. Read needRestart before the error check in update, delete and detach, and broadcast on needRestart || err == nil — the same shape create and attach have had since 63b46cd6. The predicate is a strict superset of the old err == nil, and needRestart is only ever assigned after a runSerializedTx commit, so it firing genuinely means something landed. The three handlers are pinned by a new controller test each: one client on two inbounds, the second one's settings JSON corrupted so the op commits on one and fails on the other, asserting both the success:false response and the restart flag. All three fail without the change. The API docs for update, del and detach now describe the partial-application contract, as add and attach already did. Detach ends at the fanout so every one of its errors carries the inbound prefix; update and delete write the client record afterwards, and a failure there is reported without one. --- .../content/docs/en/reference/api/clients.mdx | 26 ++++ docs/public/openapi.json | 3 + frontend/public/openapi.json | 3 + frontend/src/pages/api-docs/endpoints.ts | 6 + internal/web/controller/client.go | 42 ++++-- .../controller/client_partial_apply_test.go | 141 ++++++++++++++++++ 6 files changed, 209 insertions(+), 12 deletions(-) create mode 100644 internal/web/controller/client_partial_apply_test.go diff --git a/docs/content/docs/en/reference/api/clients.mdx b/docs/content/docs/en/reference/api/clients.mdx index 14568fbde..b4696ccf4 100644 --- a/docs/content/docs/en/reference/api/clients.mdx +++ b/docs/content/docs/en/reference/api/clients.mdx @@ -570,6 +570,25 @@ _openapi: 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: 'The inbounds are applied concurrently and independently: one that + fails no longer stops the others. Every inbound error names the + inbound it came from (`inbound 7: `), and several failures + are reported together, one per line. So a `success:false` response can + still have applied the edit to the remaining inbounds. The client + record is written after the inbounds, so a failure there is reported + without an `inbound :` prefix and leaves the inbound edits in + place.' + heading: update-an-existing-client-by-email-changes-propagate-to-every-attached-inbound-body-is-the-json-client-payload--supply-the-full-set-of-fields-you-want-to-keep-the-server-replaces-the-row-it-does-not-patch + - content: 'The inbounds are applied concurrently and independently: one that + fails no longer stops the others. Every inbound error names the + inbound it came from (`inbound 7: `), and several failures + are reported together, one per line. So a `success:false` response can + still have removed the client from the remaining inbounds; the client + record is kept in that case, so re-running the call retries exactly + the leftovers. The record and traffic rows are dropped after the + inbounds, so a failure there is reported without an `inbound :` + prefix and leaves the client already removed from every inbound.' + heading: delete-a-client-by-email-removes-it-from-every-attached-inbound-and-drops-its-traffic-record-unless-keeptraffic1-is-passed - content: '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 @@ -579,6 +598,13 @@ _openapi: 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 + - content: 'The inbounds are applied concurrently and independently: one that + fails no longer stops the others. Every inbound error names the + inbound it came from (`inbound 7: `), and several failures + are reported together, one per line. So a `success:false` response can + still have detached the remaining inbounds. Detach writes nothing + beyond the inbounds, so every error carries the prefix.' + heading: detach-a-client-from-one-or-more-inbounds-without-deleting-the-client --- {/* This file was generated by Fumadocs. Do not edit this file directly. Any changes should be made by running the generation command again. */} diff --git a/docs/public/openapi.json b/docs/public/openapi.json index 90d60585d..59d8dca40 100644 --- a/docs/public/openapi.json +++ b/docs/public/openapi.json @@ -7591,6 +7591,7 @@ ], "summary": "Update an existing client by email. Changes propagate to every attached inbound. Body is the JSON client payload — supply the full set of fields you want to keep (the server replaces the row, it does not patch).", "operationId": "post_panel_api_clients_update_email", + "description": "The inbounds are applied concurrently and independently: one that fails no longer stops the others. Every inbound error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. So a `success:false` response can still have applied the edit to the remaining inbounds. The client record is written after the inbounds, so a failure there is reported without an `inbound :` prefix and leaves the inbound edits in place.", "parameters": [ { "name": "email", @@ -7654,6 +7655,7 @@ ], "summary": "Delete a client by email. Removes it from every attached inbound and drops its traffic record unless keepTraffic=1 is passed.", "operationId": "post_panel_api_clients_del_email", + "description": "The inbounds are applied concurrently and independently: one that fails no longer stops the others. Every inbound error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. So a `success:false` response can still have removed the client from the remaining inbounds; the client record is kept in that case, so re-running the call retries exactly the leftovers. The record and traffic rows are dropped after the inbounds, so a failure there is reported without an `inbound :` prefix and leaves the client already removed from every inbound.", "parameters": [ { "name": "email", @@ -7781,6 +7783,7 @@ ], "summary": "Detach a client from one or more inbounds without deleting the client.", "operationId": "post_panel_api_clients_email_detach", + "description": "The inbounds are applied concurrently and independently: one that fails no longer stops the others. Every inbound error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. So a `success:false` response can still have detached the remaining inbounds. Detach writes nothing beyond the inbounds, so every error carries the prefix.", "parameters": [ { "name": "email", diff --git a/frontend/public/openapi.json b/frontend/public/openapi.json index 90d60585d..59d8dca40 100644 --- a/frontend/public/openapi.json +++ b/frontend/public/openapi.json @@ -7591,6 +7591,7 @@ ], "summary": "Update an existing client by email. Changes propagate to every attached inbound. Body is the JSON client payload — supply the full set of fields you want to keep (the server replaces the row, it does not patch).", "operationId": "post_panel_api_clients_update_email", + "description": "The inbounds are applied concurrently and independently: one that fails no longer stops the others. Every inbound error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. So a `success:false` response can still have applied the edit to the remaining inbounds. The client record is written after the inbounds, so a failure there is reported without an `inbound :` prefix and leaves the inbound edits in place.", "parameters": [ { "name": "email", @@ -7654,6 +7655,7 @@ ], "summary": "Delete a client by email. Removes it from every attached inbound and drops its traffic record unless keepTraffic=1 is passed.", "operationId": "post_panel_api_clients_del_email", + "description": "The inbounds are applied concurrently and independently: one that fails no longer stops the others. Every inbound error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. So a `success:false` response can still have removed the client from the remaining inbounds; the client record is kept in that case, so re-running the call retries exactly the leftovers. The record and traffic rows are dropped after the inbounds, so a failure there is reported without an `inbound :` prefix and leaves the client already removed from every inbound.", "parameters": [ { "name": "email", @@ -7781,6 +7783,7 @@ ], "summary": "Detach a client from one or more inbounds without deleting the client.", "operationId": "post_panel_api_clients_email_detach", + "description": "The inbounds are applied concurrently and independently: one that fails no longer stops the others. Every inbound error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. So a `success:false` response can still have detached the remaining inbounds. Detach writes nothing beyond the inbounds, so every error carries the prefix.", "parameters": [ { "name": "email", diff --git a/frontend/src/pages/api-docs/endpoints.ts b/frontend/src/pages/api-docs/endpoints.ts index 165a7e695..0d10968c6 100644 --- a/frontend/src/pages/api-docs/endpoints.ts +++ b/frontend/src/pages/api-docs/endpoints.ts @@ -1151,6 +1151,8 @@ export const sections: readonly Section[] = [ path: '/panel/api/clients/update/:email', summary: 'Update an existing client by email. Changes propagate to every attached inbound. Body is the JSON client payload — supply the full set of fields you want to keep (the server replaces the row, it does not patch).', + description: + 'The inbounds are applied concurrently and independently: one that fails no longer stops the others. Every inbound error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. So a `success:false` response can still have applied the edit to the remaining inbounds. The client record is written after the inbounds, so a failure there is reported without an `inbound :` prefix and leaves the inbound edits in place.', params: [ { name: 'email', @@ -1167,6 +1169,8 @@ export const sections: readonly Section[] = [ path: '/panel/api/clients/del/:email', summary: 'Delete a client by email. Removes it from every attached inbound and drops its traffic record unless keepTraffic=1 is passed.', + description: + 'The inbounds are applied concurrently and independently: one that fails no longer stops the others. Every inbound error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. So a `success:false` response can still have removed the client from the remaining inbounds; the client record is kept in that case, so re-running the call retries exactly the leftovers. The record and traffic rows are dropped after the inbounds, so a failure there is reported without an `inbound :` prefix and leaves the client already removed from every inbound.', params: [ { name: 'email', in: 'path', type: 'string', desc: 'Client email (unique identifier).' }, { @@ -1200,6 +1204,8 @@ export const sections: readonly Section[] = [ method: 'POST', path: '/panel/api/clients/:email/detach', summary: 'Detach a client from one or more inbounds without deleting the client.', + description: + 'The inbounds are applied concurrently and independently: one that fails no longer stops the others. Every inbound error names the inbound it came from (`inbound 7: `), and several failures are reported together, one per line. So a `success:false` response can still have detached the remaining inbounds. Detach writes nothing beyond the inbounds, so every error carries the prefix.', 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 d6224ed99..852793798 100644 --- a/internal/web/controller/client.go +++ b/internal/web/controller/client.go @@ -215,30 +215,42 @@ func (a *ClientController) update(c *gin.Context) { } inboundFilter := parseInboundIdsQuery(c.Query("inboundIds")) needRestart, err := a.clientService.UpdateByEmail(&a.inboundService, email, req.Client, req.LimitHwid, inboundFilter...) + // Flagged before the error check: a partly-applied edit leaves the change + // committed on the inbounds that succeeded, and those still need the restart. + if needRestart { + a.xrayService.SetToNeedRestart() + } + // A partly-applied call committed real changes; 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.inboundClientUpdateSuccess"), pendingNodeObj(a.clientService.HasPendingNode(&a.inboundService, email)), nil) - if needRestart { - a.xrayService.SetToNeedRestart() - } - notifyClientsChanged() } func (a *ClientController) delete(c *gin.Context) { email := c.Param("email") keepTraffic := c.Query("keepTraffic") == "1" needRestart, err := a.clientService.DeleteByEmail(&a.inboundService, email, keepTraffic) + // Flagged before the error check: a partly-applied delete already removed + // the client from the inbounds that succeeded, and those need the restart. + if needRestart { + a.xrayService.SetToNeedRestart() + } + // A partly-applied call committed real removals; 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 } jsonMsg(c, I18nWeb(c, "pages.inbounds.toasts.inboundClientDeleteSuccess"), nil) - if needRestart { - a.xrayService.SetToNeedRestart() - } - notifyClientsChanged() } type attachDetachBody struct { @@ -640,15 +652,21 @@ func (a *ClientController) detach(c *gin.Context) { return } needRestart, err := a.clientService.DetachByEmailMany(&a.inboundService, email, body.InboundIds) + // Flagged before the error check: a partly-applied detach already removed + // the client from the inbounds that succeeded, and those need the restart. + if needRestart { + a.xrayService.SetToNeedRestart() + } + // A partly-applied call committed real removals; 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.inboundClientDeleteSuccess"), pendingNodeObj(a.inboundService.AnyNodePending(body.InboundIds)), nil) - if needRestart { - a.xrayService.SetToNeedRestart() - } - notifyClientsChanged() } type bulkResetRequest struct { diff --git a/internal/web/controller/client_partial_apply_test.go b/internal/web/controller/client_partial_apply_test.go new file mode 100644 index 000000000..fba3b37ab --- /dev/null +++ b/internal/web/controller/client_partial_apply_test.go @@ -0,0 +1,141 @@ +package controller + +import ( + "bytes" + "encoding/json" + "net/http" + "net/http/httptest" + "path/filepath" + "testing" + + "github.com/gin-gonic/gin" + + "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/entity" + "github.com/mhsanaei/3x-ui/v3/internal/web/service" +) + +// seedPartlyApplyingClient puts one client on two inbounds and corrupts the second +// one's settings, so a later op succeeds on one inbound and fails on the other. +func seedPartlyApplyingClient(t *testing.T, email string, basePort int) (healthyID, brokenID int) { + t.Helper() + dbDir := t.TempDir() + t.Setenv("XUI_DB_FOLDER", dbDir) + if err := database.InitDB(filepath.Join(dbDir, "x-ui.db")); err != nil { + t.Fatalf("InitDB: %v", err) + } + t.Cleanup(func() { _ = database.CloseDB() }) + + db := database.GetDB() + ids := make([]int, 0, 2) + for i := range 2 { + ib := &model.Inbound{ + UserId: 1, Enable: true, Port: basePort + i, + Tag: "in-" + string(rune('a'+i)) + "-partial", + Protocol: model.VLESS, Settings: `{"clients": []}`, + StreamSettings: `{"network":"tcp","security":"none"}`, + } + if err := db.Create(ib).Error; err != nil { + t.Fatalf("create inbound %d: %v", i, err) + } + ids = append(ids, ib.Id) + } + + if _, err := (&service.ClientService{}).Create(&service.InboundService{}, &service.ClientCreatePayload{ + Client: model.Client{Email: email, ID: "11111111-2222-3333-4444-555555555555", SubID: "sub-" + email, Enable: true}, + InboundIds: ids, + }); err != nil { + t.Fatalf("seed Create across both inbounds: %v", err) + } + + if err := db.Model(&model.Inbound{}).Where("id = ?", ids[1]). + Update("settings", `{"clients":`).Error; err != nil { + t.Fatalf("corrupt inbound %d settings: %v", ids[1], err) + } + return ids[0], ids[1] +} + +func postCtx(t *testing.T, email string, body any) (*gin.Context, *httptest.ResponseRecorder) { + t.Helper() + gin.SetMode(gin.TestMode) + w := httptest.NewRecorder() + c, _ := gin.CreateTestContext(w) + c.Params = gin.Params{{Key: "email", Value: email}} + payload := []byte("{}") + if body != nil { + var err error + if payload, err = json.Marshal(body); err != nil { + t.Fatalf("marshal body: %v", err) + } + } + c.Request = httptest.NewRequest(http.MethodPost, "/", bytes.NewReader(payload)) + c.Request.Header.Set("Content-Type", "application/json") + return c, w +} + +// assertPartialApply pins that the op really failed on one inbound, so a green +// test cannot be a plain full success that never exercised the error path. +func assertPartialApply(t *testing.T, w *httptest.ResponseRecorder) { + t.Helper() + var msg entity.Msg + if err := json.Unmarshal(w.Body.Bytes(), &msg); err != nil { + t.Fatalf("decode response %q: %v", w.Body.String(), err) + } + if msg.Success { + t.Fatalf("response reports success=true, want the partial apply to report failure: %q", w.Body.String()) + } +} + +// TestUpdateHandlerFlagsRestartOnPartialApply pins that an edit committed on some +// inbounds and failed on others still flags Xray, as create/attach already did. +func TestUpdateHandlerFlagsRestartOnPartialApply(t *testing.T) { + const email = "partial-update@example.com" + seedPartlyApplyingClient(t, email, 43310) + + a := &ClientController{} + a.xrayService.IsNeedRestartAndSetFalse() + c, w := postCtx(t, email, map[string]any{ + "email": email, "id": "11111111-2222-3333-4444-555555555555", + "subId": "sub-" + email, "enable": true, "comment": "edited", + }) + a.update(c) + + assertPartialApply(t, w) + if !a.xrayService.IsNeedRestartAndSetFalse() { + t.Fatal("a partly-applied client edit left Xray unflagged for restart") + } +} + +// TestDeleteHandlerFlagsRestartOnPartialApply is the delete-side twin: the +// removals that landed still need the restart the error path used to discard. +func TestDeleteHandlerFlagsRestartOnPartialApply(t *testing.T) { + const email = "partial-delete@example.com" + seedPartlyApplyingClient(t, email, 43320) + + a := &ClientController{} + a.xrayService.IsNeedRestartAndSetFalse() + c, w := postCtx(t, email, nil) + a.delete(c) + + assertPartialApply(t, w) + if !a.xrayService.IsNeedRestartAndSetFalse() { + t.Fatal("a partly-applied client delete left Xray unflagged for restart") + } +} + +// TestDetachHandlerFlagsRestartOnPartialApply covers the third converted path. +func TestDetachHandlerFlagsRestartOnPartialApply(t *testing.T) { + const email = "partial-detach@example.com" + healthyID, brokenID := seedPartlyApplyingClient(t, email, 43330) + + a := &ClientController{} + a.xrayService.IsNeedRestartAndSetFalse() + c, w := postCtx(t, email, attachDetachBody{InboundIds: []int{healthyID, brokenID}}) + a.detach(c) + + assertPartialApply(t, w) + if !a.xrayService.IsNeedRestartAndSetFalse() { + t.Fatal("a partly-applied client detach left Xray unflagged for restart") + } +}