diff --git a/internal/web/runtime/remote.go b/internal/web/runtime/remote.go index 998e39ff5..2e81579c7 100644 --- a/internal/web/runtime/remote.go +++ b/internal/web/runtime/remote.go @@ -339,7 +339,10 @@ func (r *Remote) DeleteUser(ctx context.Context, ib *model.Inbound, email string } id, err := r.resolveRemoteID(ctx, ib.Tag) if err != nil { - return nil + // Can't confirm the delete reached the node — surface it so the caller + // marks the node dirty and a reconcile converges, instead of silently + // dropping the delete and letting the next snapshot resurrect the client. + return fmt.Errorf("remote DeleteUser: resolve tag %q: %w", ib.Tag, err) } body := map[string]any{"inboundIds": []int{id}} _, err = r.do(ctx, http.MethodPost, diff --git a/internal/web/service/client_inbound_apply.go b/internal/web/service/client_inbound_apply.go index 3d6c30eb1..881f0e1ca 100644 --- a/internal/web/service/client_inbound_apply.go +++ b/internal/web/service/client_inbound_apply.go @@ -742,15 +742,17 @@ func (s *ClientService) DelInboundClientByEmail(inboundSvc *InboundService, inbo } } - if needApiDel { - rt, push, dirty, perr := inboundSvc.nodePushPlan(oldInbound) - if perr != nil { - return false, perr - } - if dirty { - markDirty = true - } - if oldInbound.NodeID == nil { + if oldInbound.NodeID == nil { + // Local inbound: a disabled client isn't in the running Xray, so only + // a live one (needApiDel) needs an API removal. + if needApiDel { + rt, push, dirty, perr := inboundSvc.nodePushPlan(oldInbound) + if perr != nil { + return false, perr + } + if dirty { + markDirty = true + } if !push { needRestart = true } else if err1 := rt.RemoveUser(context.Background(), oldInbound, email); err1 == nil { @@ -762,7 +764,19 @@ func (s *ClientService) DelInboundClientByEmail(inboundSvc *InboundService, inbo logger.Debug("Error in deleting client on", rt.Name(), ":", email) needRestart = true } - } else if push { + } + } else { + // Node inbound: propagate the delete regardless of the enable flag — + // the node's own DB still carries a disabled client and would + // resurrect it on the next snapshot otherwise. + rt, push, dirty, perr := inboundSvc.nodePushPlan(oldInbound) + if perr != nil { + return false, perr + } + if dirty { + markDirty = true + } + if push { if err1 := rt.DeleteUser(context.Background(), oldInbound, email); err1 != nil { logger.Warning("Error in deleting client on", rt.Name(), ":", err1) markDirty = true diff --git a/internal/web/service/node_dirty_test.go b/internal/web/service/node_dirty_test.go index 4fdc0453f..5c06c4c97 100644 --- a/internal/web/service/node_dirty_test.go +++ b/internal/web/service/node_dirty_test.go @@ -65,6 +65,48 @@ func TestSetRemoteTraffic_DirtyPreservesConfig(t *testing.T) { } } +// Deleting a *disabled* client attached to a node inbound must still propagate +// to the node. The node's own DB carries the (disabled) client, so the central +// panel has to mark the node dirty (→ reconcile) instead of dropping the delete +// and letting the next traffic snapshot resurrect the client. Regression for +// the enable-flag gate that used to skip the node path entirely (#5352). +func TestDelInboundClientByEmail_DisabledNodeClientMarksDirty(t *testing.T) { + setupConflictDB(t) + db := database.GetDB() + + // Offline node so nodePushPlan reports dirty without needing a live runtime. + node := &model.Node{Name: "n1", Address: "127.0.0.1", Port: 2096, ApiToken: "tok", Enable: true, Status: "offline"} + if err := db.Create(node).Error; err != nil { + t.Fatalf("create node: %v", err) + } + id := node.Id + + central := &model.Inbound{ + UserId: 1, + NodeID: &id, + Tag: "in-443-tcp", + Enable: true, + Port: 443, + Protocol: model.VLESS, + Settings: `{"clients":[{"email":"a@x","enable":false}]}`, + } + if err := db.Create(central).Error; err != nil { + t.Fatalf("create inbound: %v", err) + } + + inboundSvc := &InboundService{} + clientSvc := &ClientService{} + if _, err := clientSvc.DelInboundClientByEmail(inboundSvc, central.Id, "a@x", false); err != nil { + t.Fatalf("DelInboundClientByEmail: %v", err) + } + + if _, _, dirty, _, err := (&NodeService{}).NodeSyncState(id); err != nil { + t.Fatalf("NodeSyncState: %v", err) + } else if !dirty { + t.Fatal("deleting a disabled node client must mark the node dirty (#5352)") + } +} + // ClearNodeDirty must be a compare-and-swap on config_dirty_at so a concurrent // edit that re-dirties the node during a reconcile is not silently cleared. func TestNodeDirty_ClearIsCASOnDirtyAt(t *testing.T) {