Fix missing document detachments when client is deactivated (#907)

This commit addresses the issue where documents attached to a client
are not being properly detached during deactivation. Specifically,
information about document detachments are not being reflected in the
database, leading to situations where documents remain attached to a
deactivated client.

By this fix, attempting to delete a document from the dashboard will
now correctly handle the scenario where the document is attached,
resolving the problem outlined in issue #758.

---------

Co-authored-by: Youngteac Hong <susukang98@gmail.com>

server/backend/database/client_info.go

@@ -32,6 +32,7 @@ var (
 	ErrDocumentNeverAttached   = errors.New("client has never attached the document")
 	ErrDocumentAlreadyAttached = errors.New("document already attached")
 	ErrDocumentAlreadyDetached = errors.New("document already detached")
+	ErrAttachedDocumentExists  = errors.New("attached document exits when deactivated")
 )
 
 // Below are statuses of the client.
@@ -223,6 +224,27 @@ func (i *ClientInfo) EnsureDocumentAttached(docID types.ID) error {
 	return nil
 }
 
+// EnsureDocumentsNotAttachedWhenDeactivated ensures that no documents are attached
+// when the client is deactivated.
+func (i *ClientInfo) EnsureDocumentsNotAttachedWhenDeactivated() error {
+	if i.Status != ClientDeactivated {
+		return nil
+	}
+
+	for docID := range i.Documents {
+		isAttached, err := i.IsAttached(docID)
+		if err != nil {
+			return err
+		}
+
+		if isAttached {
+			return ErrAttachedDocumentExists
+		}
+	}
+
+	return nil
+}
+
 // DeepCopy returns a deep copy of this client info.
 func (i *ClientInfo) DeepCopy() *ClientInfo {
 	if i == nil {

server/backend/database/client_info_test.go

@@ -145,4 +145,21 @@ func TestClientInfo(t *testing.T) {
 		err = clientInfo.AttachDocument(dummyDocID, false)
 		assert.ErrorIs(t, err, database.ErrDocumentAlreadyAttached)
 	})
+
+	t.Run("document detached when client deactivate test", func(t *testing.T) {
+		clientInfo := database.ClientInfo{
+			Status: database.ClientActivated,
+		}
+
+		err := clientInfo.AttachDocument(dummyDocID, false)
+		assert.NoError(t, err)
+		isAttached, err := clientInfo.IsAttached(dummyDocID)
+		assert.NoError(t, err)
+		assert.True(t, isAttached)
+
+		clientInfo.Deactivate()
+
+		err = clientInfo.EnsureDocumentsNotAttachedWhenDeactivated()
+		assert.Equal(t, database.ErrAttachedDocumentExists, err)
+	})
 }

server/backend/database/memory/database.go

@@ -523,7 +523,15 @@ func (d *DB) DeactivateClient(_ context.Context, refKey types.ClientRefKey) (*da
 	// the stored objects are returned instead of new objects. This can cause
 	// problems when directly modifying loaded objects. So, we need to DeepCopy.
 	clientInfo = clientInfo.DeepCopy()
+	for docID := range clientInfo.Documents {
+		if clientInfo.Documents[docID].Status == database.DocumentAttached {
+			if err := clientInfo.DetachDocument(docID); err != nil {
+				return nil, err
+			}
+		}
+	}
 	clientInfo.Deactivate()
+
 	if err := txn.Insert(tblClients, clientInfo); err != nil {
 		return nil, fmt.Errorf("update client: %w", err)
 	}

server/backend/database/mongo/client.go

@@ -520,15 +520,39 @@ func (c *Client) ActivateClient(ctx context.Context, projectID types.ID, key str
 	return &clientInfo, nil
 }
 
-// DeactivateClient deactivates the client of the given refKey.
+// DeactivateClient deactivates the client of the given refKey and updates document statuses as detached.
 func (c *Client) DeactivateClient(ctx context.Context, refKey types.ClientRefKey) (*database.ClientInfo, error) {
 	res := c.collection(ColClients).FindOneAndUpdate(ctx, bson.M{
 		"project_id": refKey.ProjectID,
 		"_id":        refKey.ClientID,
-	}, bson.M{
-		"$set": bson.M{
-			"status":     database.ClientDeactivated,
-			"updated_at": gotime.Now(),
+	}, bson.A{
+		bson.M{
+			"$set": bson.M{
+				"status":     database.ClientDeactivated,
+				"updated_at": gotime.Now(),
+				"documents": bson.M{
+					"$arrayToObject": bson.M{
+						"$map": bson.M{
+							"input": bson.M{"$objectToArray": "$documents"},
+							"as":    "doc",
+							"in": bson.M{
+								"k": "$$doc.k",
+								"v": bson.M{
+									"$cond": bson.M{
+										"if": bson.M{"$eq": bson.A{"$$doc.v.status", database.DocumentAttached}},
+										"then": bson.M{
+											"client_seq": 0,
+											"server_seq": 0,
+											"status":     database.DocumentDetached,
+										},
+										"else": "$$doc.v",
+									},
+								},
+							},
+						},
+					},
+				},
+			},
 		},
 	}, options.FindOneAndUpdate().SetReturnDocument(options.After))
 

server/backend/database/testcases/testcases.go

@@ -391,6 +391,56 @@ func RunActivateClientDeactivateClientTest(t *testing.T, db database.Database, p
 		assert.Equal(t, t.Name(), clientInfo.Key)
 		assert.Equal(t, database.ClientDeactivated, clientInfo.Status)
 	})
+
+	t.Run("ensure document detached when deactivate client test", func(t *testing.T) {
+		ctx := context.Background()
+
+		// 01. Create a client
+		clientInfo, err := db.ActivateClient(ctx, projectID, t.Name())
+		assert.NoError(t, err)
+		assert.Equal(t, t.Name(), clientInfo.Key)
+		assert.Equal(t, database.ClientActivated, clientInfo.Status)
+
+		// 02. Create documents and attach them to the client
+		for i := 0; i < 3; i++ {
+			docInfo, _ := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), helper.TestDocKey(t, i), true)
+			assert.NoError(t, clientInfo.AttachDocument(docInfo.ID, false))
+			clientInfo.Documents[docInfo.ID].ServerSeq = 1
+			clientInfo.Documents[docInfo.ID].ClientSeq = 1
+			assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo))
+
+			result, err := db.FindClientInfoByRefKey(ctx, clientInfo.RefKey())
+			assert.Equal(t, result.Documents[docInfo.ID].Status, database.DocumentAttached)
+			assert.Equal(t, result.Documents[docInfo.ID].ServerSeq, int64(1))
+			assert.Equal(t, result.Documents[docInfo.ID].ClientSeq, uint32(1))
+			assert.NoError(t, err)
+		}
+
+		// 03. Remove one document
+		docInfo, err := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), helper.TestDocKey(t, 2), true)
+		assert.NoError(t, err)
+		assert.NoError(t, clientInfo.RemoveDocument(docInfo.ID))
+		assert.NoError(t, db.UpdateClientInfoAfterPushPull(ctx, clientInfo, docInfo))
+
+		// 04. Deactivate the client
+		result, err := db.DeactivateClient(ctx, types.ClientRefKey{
+			ProjectID: projectID,
+			ClientID:  clientInfo.ID,
+		})
+		assert.NoError(t, err)
+
+		// 05. Check whether doc.Status is reflected properly.
+		// If it was `DocumentAttached`, it should be changed to `DocumentDetached`.
+		for i := 0; i < 2; i++ {
+			docInfo, err := db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), helper.TestDocKey(t, i), true)
+			assert.NoError(t, err)
+			assert.Equal(t, result.Documents[docInfo.ID].Status, database.DocumentDetached)
+		}
+		// If it was `DocumentRemoved`, it should be remained `DocumentRemoved`.
+		docInfo, err = db.FindDocInfoByKeyAndOwner(ctx, clientInfo.RefKey(), helper.TestDocKey(t, 2), true)
+		assert.NoError(t, err)
+		assert.Equal(t, result.Documents[docInfo.ID].Status, database.DocumentRemoved)
+	})
 }
 
 // RunUpdateProjectInfoTest runs the UpdateProjectInfo tests for the given db.

server/clients/clients.go

@@ -49,29 +49,25 @@ func Deactivate(
 	db database.Database,
 	refKey types.ClientRefKey,
 ) (*database.ClientInfo, error) {
-	clientInfo, err := db.FindClientInfoByRefKey(ctx, refKey)
+	// TODO(hackerwins): We need to remove the presence of the client from the document.
+	// Be careful that housekeeping is executed by the leader. And documents are sharded
+	// by the servers in the cluster. So, we need to consider the case where the leader is
+	// not the same as the server that handles the document.
+
+	// TODO(raararaara): When deactivating a client, we need to update three DB properties
+	// (ClientInfo.Status, ClientInfo.Documents, SyncedSeq) in DB.
+	// Updating the sub-properties of ClientInfo guarantees atomicity as it involves a single MongoDB document.
+	// However, SyncedSeqs are stored in separate documents, so we can't ensure atomic updates for both.
+	// Currently, if SyncedSeqs update fails, it mainly impacts GC efficiency without causing major issues.
+	// We need to consider implementing a correction logic to remove SyncedSeqs in the future.
+	clientInfo, err := db.DeactivateClient(ctx, refKey)
 	if err != nil {
 		return nil, err
 	}
 
+	// TODO(raararaara): We're currently updating SyncedSeq one by one. This approach is similar
+	// to n+1 query problem. We need to investigate if we can optimize this process by using a single query in the future.
 	for docID, clientDocInfo := range clientInfo.Documents {
-		isAttached, err := clientInfo.IsAttached(docID)
-		if err != nil {
-			return nil, err
-		}
-		if !isAttached {
-			continue
-		}
-
-		if err := clientInfo.DetachDocument(docID); err != nil {
-			return nil, err
-		}
-
-		// TODO(hackerwins): We need to remove the presence of the client from the document.
-		// Be careful that housekeeping is executed by the leader. And documents are sharded
-		// by the servers in the cluster. So, we need to consider the case where the leader is
-		// not the same as the server that handles the document.
-
 		if err := db.UpdateSyncedSeq(
 			ctx,
 			clientInfo,
@@ -85,7 +81,7 @@ func Deactivate(
 		}
 	}
 
-	return db.DeactivateClient(ctx, refKey)
+	return clientInfo, err
 }
 
 // FindActiveClientInfo find the active client info by the given ref key.