mirror of
https://github.com/MHSanaei/3x-ui.git
synced 2026-08-22 02:47:14 +00:00
fix(nodes): propagate single-client deletion to remote nodes (#5352)
Deleting a client attached to a remote-node inbound could silently fail to reach the node, so the node's next traffic snapshot resurrected the client once the 90s delete tombstone expired. Two paths in the single-client delete (Delete -> DelInboundClientByEmail): - A disabled client was skipped entirely: the node-propagation and mark-dirty block sat behind the client's enable flag (needApiDel), so a disabled client on a node never detached and never marked the node dirty. The bulk and multi-client delete paths already handle the node case independently of enable state; mirror that structure here. - Remote.DeleteUser returned nil when resolveRemoteID failed, hiding the failure from the caller so the node was never marked dirty. Surface the error like AddClient/UpdateUser do, so the caller marks the node dirty and the next reconcile converges. Add a regression test asserting a disabled node client's deletion marks the node dirty.
This commit is contained in:
@@ -339,7 +339,10 @@ func (r *Remote) DeleteUser(ctx context.Context, ib *model.Inbound, email string
|
|||||||
}
|
}
|
||||||
id, err := r.resolveRemoteID(ctx, ib.Tag)
|
id, err := r.resolveRemoteID(ctx, ib.Tag)
|
||||||
if err != nil {
|
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}}
|
body := map[string]any{"inboundIds": []int{id}}
|
||||||
_, err = r.do(ctx, http.MethodPost,
|
_, err = r.do(ctx, http.MethodPost,
|
||||||
|
|||||||
@@ -742,15 +742,17 @@ func (s *ClientService) DelInboundClientByEmail(inboundSvc *InboundService, inbo
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
if needApiDel {
|
if oldInbound.NodeID == nil {
|
||||||
rt, push, dirty, perr := inboundSvc.nodePushPlan(oldInbound)
|
// Local inbound: a disabled client isn't in the running Xray, so only
|
||||||
if perr != nil {
|
// a live one (needApiDel) needs an API removal.
|
||||||
return false, perr
|
if needApiDel {
|
||||||
}
|
rt, push, dirty, perr := inboundSvc.nodePushPlan(oldInbound)
|
||||||
if dirty {
|
if perr != nil {
|
||||||
markDirty = true
|
return false, perr
|
||||||
}
|
}
|
||||||
if oldInbound.NodeID == nil {
|
if dirty {
|
||||||
|
markDirty = true
|
||||||
|
}
|
||||||
if !push {
|
if !push {
|
||||||
needRestart = true
|
needRestart = true
|
||||||
} else if err1 := rt.RemoveUser(context.Background(), oldInbound, email); err1 == nil {
|
} 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)
|
logger.Debug("Error in deleting client on", rt.Name(), ":", email)
|
||||||
needRestart = true
|
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 {
|
if err1 := rt.DeleteUser(context.Background(), oldInbound, email); err1 != nil {
|
||||||
logger.Warning("Error in deleting client on", rt.Name(), ":", err1)
|
logger.Warning("Error in deleting client on", rt.Name(), ":", err1)
|
||||||
markDirty = true
|
markDirty = true
|
||||||
|
|||||||
@@ -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
|
// 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.
|
// edit that re-dirties the node during a reconcile is not silently cleared.
|
||||||
func TestNodeDirty_ClearIsCASOnDirtyAt(t *testing.T) {
|
func TestNodeDirty_ClearIsCASOnDirtyAt(t *testing.T) {
|
||||||
|
|||||||
Reference in New Issue
Block a user