fix: Prevent node snapshots from resurrecting bulk-deleted clients (#6382)

Fixes #6356

Co-authored-by: Matt Van Horn <455140+mvanhorn@users.noreply.github.com>

internal/web/service/client_bulk.go

@@ -908,8 +908,9 @@ func (s *ClientService) bulkDelInboundClients(
 		return res
 	}
 
-	// Match by email — the client's stable identity (see Delete). Removes every
+	// Match by email — the client's stable identity (see Delete). The link-derived
-	// entry carrying a wanted email, independent of credential drift.
+	// set is deletion intent: an email already absent from settings is successful,
+	// while foundEmails tracks entries that still need settings-specific cleanup.
 	wantedEmails := make(map[string]struct{}, len(emails))
 	for _, email := range emails {
 		if records[email] == nil {
@@ -939,12 +940,6 @@ func (s *ClientService) bulkDelInboundClients(
 		newClients = append(newClients, client)
 	}
 
-	for email := range wantedEmails {
-		if !foundEmails[email] {
-			res.perEmailSkipped[email] = "Client Not Found In Inbound"
-		}
-	}
-
 	db := database.GetDB()
 	newClients = compactOrphans(db, newClients)
 	if newClients == nil {
@@ -953,7 +948,7 @@ func (s *ClientService) bulkDelInboundClients(
 	settings["clients"] = newClients
 	newSettings, err := json.MarshalIndent(settings, "", "  ")
 	if err != nil {
-		for email := range foundEmails {
+		for email := range wantedEmails {
 			if _, skip := res.perEmailSkipped[email]; !skip {
 				res.perEmailSkipped[email] = err.Error()
 			}
@@ -991,9 +986,8 @@ func (s *ClientService) bulkDelInboundClients(
 		var sharedErr error
 		sharedSet, sharedErr = inboundSvc.emailsUsedByOtherInbounds(foundList, inboundId)
 		if sharedErr != nil {
-			for email := range foundEmails {
+			for email := range wantedEmails {
 				res.perEmailSkipped[email] = sharedErr.Error()
-				delete(foundEmails, email)
 			}
 			return res
 		}
@@ -1046,7 +1040,7 @@ func (s *ClientService) bulkDelInboundClients(
 		return nil
 	})
 	if txErr != nil {
-		for email := range foundEmails {
+		for email := range wantedEmails {
 			if _, skip := res.perEmailSkipped[email]; !skip {
 				res.perEmailSkipped[email] = txErr.Error()
 			}
@@ -1071,12 +1065,21 @@ func (s *ClientService) bulkDelInboundClients(
 				}
 			}
 		}
-	} else if len(foundEmails) <= nodeBulkPushThreshold {
+	} else {
+		dispatchEmails := make([]string, 0, len(wantedEmails))
+		for email := range wantedEmails {
+			if _, skip := res.perEmailSkipped[email]; !skip {
+				dispatchEmails = append(dispatchEmails, email)
+			}
+		}
+		if len(dispatchEmails) > nodeBulkPushThreshold {
+			return res
+		}
 		rt, push, _, perr := inboundSvc.nodePushPlan(oldInbound)
 		if perr != nil {
 			logger.Warning("BulkDelete: node runtime lookup after commit failed:", perr)
 		} else if push {
-			for email := range foundEmails {
+			for _, email := range dispatchEmails {
 				if err1 := rt.DeleteClient(context.Background(), email); err1 != nil {
 					logger.Warning("Error in deleting client on", rt.Name(), ":", err1)
 				}

internal/web/service/node_bulk_dispatch_test.go

@@ -234,6 +234,9 @@ func TestNodeBulkDeleteDoesNotPushBeforeFailedCommit(t *testing.T) {
 	if got := fake.deleteClient.Load() + fake.deleteUser.Load(); got != 0 {
 		t.Fatalf("failed transaction pushed %d delete call(s) to the node, want 0", got)
 	}
+	if isClientEmailTombstoned(client.Email) {
+		t.Fatal("failed bulk delete left a live tombstone")
+	}
 }
 
 func TestNodeBulkSmallDeleteRemovesWholeRemoteClient(t *testing.T) {
@@ -241,6 +244,10 @@ func TestNodeBulkSmallDeleteRemovesWholeRemoteClient(t *testing.T) {
 	nodeID, fake := setupNodeRuntime(t)
 	client := model.Client{ID: uuid.NewString(), Email: "full-delete@x", Enable: true}
 	nodeInbound(t, nodeID, 30024, []model.Client{client})
+	var record model.ClientRecord
+	if err := database.GetDB().Where("email = ?", client.Email).First(&record).Error; err != nil {
+		t.Fatalf("load client record: %v", err)
+	}
 
 	result, _, err := (&ClientService{}).BulkDelete(&InboundService{}, []string{client.Email}, true)
 	if err != nil {
@@ -255,6 +262,104 @@ func TestNodeBulkSmallDeleteRemovesWholeRemoteClient(t *testing.T) {
 	if got := fake.deleteUser.Load(); got != 0 {
 		t.Fatalf("remote DeleteUser detach calls = %d, want 0 for full deletion", got)
 	}
+	var records, links int64
+	if err := database.GetDB().Model(&model.ClientRecord{}).Where("email = ?", client.Email).Count(&records).Error; err != nil {
+		t.Fatalf("count client records: %v", err)
+	}
+	if err := database.GetDB().Model(&model.ClientInbound{}).Where("client_id = ?", record.Id).Count(&links).Error; err != nil {
+		t.Fatalf("count client links: %v", err)
+	}
+	if records != 0 || links != 0 {
+		t.Fatalf("bulk delete left records=%d links=%d, want 0/0", records, links)
+	}
+}
+
+func TestNodeBulkDeleteTreatsMissingSettingsClientAsAlreadyDeleted(t *testing.T) {
+	setupBulkDB(t)
+	nodeID, fake := setupNodeRuntime(t)
+	client := model.Client{ID: uuid.NewString(), Email: "drifted-delete@x", Enable: true}
+	ib := nodeInbound(t, nodeID, 30025, []model.Client{client})
+
+	// Simulate a stale normalized link after the client has already disappeared
+	// from the inbound settings JSON.
+	if err := database.GetDB().Model(&model.Inbound{}).Where("id = ?", ib.Id).
+		Update("settings", clientsSettings(t, nil)).Error; err != nil {
+		t.Fatalf("drift inbound settings: %v", err)
+	}
+
+	result, _, err := (&ClientService{}).BulkDelete(&InboundService{}, []string{client.Email}, true)
+	if err != nil {
+		t.Fatalf("BulkDelete: %v", err)
+	}
+	if result.Deleted != 1 || len(result.Skipped) != 0 {
+		t.Fatalf("BulkDelete result = %+v, want one deleted client", result)
+	}
+	if got := fake.deleteClient.Load(); got != 1 {
+		t.Fatalf("remote DeleteClient calls = %d, want 1", got)
+	}
+	var records, links int64
+	if err := database.GetDB().Model(&model.ClientRecord{}).Where("email = ?", client.Email).Count(&records).Error; err != nil {
+		t.Fatalf("count client records: %v", err)
+	}
+	if err := database.GetDB().Model(&model.ClientInbound{}).Where("inbound_id = ?", ib.Id).Count(&links).Error; err != nil {
+		t.Fatalf("count client links: %v", err)
+	}
+	if records != 0 || links != 0 {
+		t.Fatalf("bulk delete left records=%d links=%d, want 0/0", records, links)
+	}
+	if !isClientEmailTombstoned(client.Email) {
+		t.Fatal("successful bulk delete withdrew the client tombstone")
+	}
+	t.Cleanup(func() { withdrawClientTombstones(client.Email) })
+}
+
+func TestNodeBulkDeleteCompletesAcrossPresentAndMissingSettings(t *testing.T) {
+	setupBulkDB(t)
+	nodeID, fake := setupNodeRuntime(t)
+	client := model.Client{ID: uuid.NewString(), Email: "mixed-delete@x", Enable: true}
+	drifted := nodeInbound(t, nodeID, 30026, []model.Client{client})
+	nodeInbound(t, nodeID, 30027, []model.Client{client})
+
+	if err := database.GetDB().Model(&model.Inbound{}).Where("id = ?", drifted.Id).
+		Update("settings", clientsSettings(t, nil)).Error; err != nil {
+		t.Fatalf("drift inbound settings: %v", err)
+	}
+
+	result, _, err := (&ClientService{}).BulkDelete(&InboundService{}, []string{client.Email}, true)
+	if err != nil {
+		t.Fatalf("BulkDelete: %v", err)
+	}
+	if result.Deleted != 1 || len(result.Skipped) != 0 {
+		t.Fatalf("BulkDelete result = %+v, want one deleted client", result)
+	}
+	if got := fake.deleteClient.Load(); got != 2 {
+		t.Fatalf("remote DeleteClient calls = %d, want one per node inbound", got)
+	}
+}
+
+func TestNodeBulkDeleteMalformedSettingsWithdrawsTombstone(t *testing.T) {
+	setupBulkDB(t)
+	nodeID, fake := setupNodeRuntime(t)
+	client := model.Client{ID: uuid.NewString(), Email: "malformed-delete@x", Enable: true}
+	ib := nodeInbound(t, nodeID, 30028, []model.Client{client})
+	if err := database.GetDB().Model(&model.Inbound{}).Where("id = ?", ib.Id).
+		Update("settings", `{"clients":[`).Error; err != nil {
+		t.Fatalf("break inbound settings: %v", err)
+	}
+
+	result, _, err := (&ClientService{}).BulkDelete(&InboundService{}, []string{client.Email}, true)
+	if err != nil {
+		t.Fatalf("BulkDelete: %v", err)
+	}
+	if result.Deleted != 0 || len(result.Skipped) != 1 {
+		t.Fatalf("BulkDelete result = %+v, want one skipped client", result)
+	}
+	if got := fake.deleteClient.Load() + fake.deleteUser.Load(); got != 0 {
+		t.Fatalf("malformed settings pushed %d delete call(s) to the node, want 0", got)
+	}
+	if isClientEmailTombstoned(client.Email) {
+		t.Fatal("failed bulk delete left a live tombstone")
+	}
 }
 
 func TestNodeUpdateInboundClientNoopSkipsRuntimeAndDirty(t *testing.T) {
@@ -351,8 +456,8 @@ func TestNodeBulk_LargeDeleteFoldsToDirty(t *testing.T) {
 		t.Fatalf("BulkDelete: %v", err)
 	}
 
-	if got := fake.deleteUser.Load(); got != 0 {
+	if got := fake.deleteClient.Load() + fake.deleteUser.Load(); got != 0 {
-		t.Fatalf("large delete streamed %d DeleteUser RPCs, want 0 (should fold to dirty)", got)
+		t.Fatalf("large delete streamed %d delete RPCs, want 0 (should fold to dirty)", got)
 	}
 	if _, _, dirty, _, err := (&NodeService{}).NodeSyncState(nodeID); err != nil {
 		t.Fatalf("NodeSyncState: %v", err)