From 7ad9e719f6ee1ea7e7d8f6fdb1360b592b333595 Mon Sep 17 00:00:00 2001 From: JOOHOJANG <46807540+JOOHOJANG@users.noreply.github.com> Date: Fri, 6 Dec 2024 17:28:58 +0900 Subject: [PATCH] Preserve Detached Client's Lamport in Version Vector (#1090) Following the GC error discovered in #1089, we've temporarily modified the version vector handling to retain the detached client's lamport across local and minimum version vectors. This change provides a stopgap solution while we investigate a more comprehensive approach to garbage collection. --------- Co-authored-by: Youngteac Hong --- design/garbage-collection.md | 108 +------------------ pkg/document/document.go | 12 +-- pkg/document/internal_document.go | 10 -- pkg/document/time/version_vector.go | 18 +--- server/backend/database/memory/database.go | 35 ++---- server/backend/database/mongo/client.go | 36 ++----- test/integration/gc_test.go | 117 ++++++++++++++++++++- 7 files changed, 131 insertions(+), 205 deletions(-) diff --git a/design/garbage-collection.md b/design/garbage-collection.md index 421b8c920..7e44b8c18 100644 --- a/design/garbage-collection.md +++ b/design/garbage-collection.md @@ -50,9 +50,7 @@ Conceptually, min version vector is version vector that is uniformly applied to if (removedAt.lamport <= minVersionVector[removedAt.actor]) { runGC() -} else if (removedAt.lamport < minVersionVector.minLamport()) { - runGC() -} +} ``` ``` @@ -68,110 +66,6 @@ min([c1:2, c2:3, c3:4], [c1:3, c2:1, c3:5, c4:3]) => [c1:2, c2:1, c3:4, c4:0] ``` -### How we handle if min version vector includes detached client's lamport. -We have to wipe out detached client's lamport from every version vector in db and other client's local version vector. -![detached-user-version-vector](media/detached-user-version-vector.jpg) - -For example, -``` -// initial state -c1.versionVector = {c1:4, c2:3, c3: 4} -c2.versionVector = {c1:3, c2:4, c3: 5} -c3.versionVector = {c1:3, c2:3, c3: 6} - -db.versionVector = { - c1: {c1:4, c2:3, c3: 4}, - c2: {c1:3, c2:4, c3: 5}, - c3: {c1:3, c2:3, c3: 6} -} - -// process -1. c1 detach and remove its version vector from db. - -db.versionVector = { - c2: {c1:3, c2:4, c3: 5} - c3: {c1:3, c2:3, c3: 6} -} - -2. compute minVersionVector -min(c2.vv, c3.vv) = min({c1:3, c2:4, c3: 5}, {c1:3, c2:3, c3:5}) = {c1:3, c2:3, c3:5} - -``` -as you can see from above, c1's lamport is still inside minVersionVector, and also in every client's local document's version vector too. - -So we need to filter detached client's lamport from -1. db.version vector -2. other client's local version vector. - -But it causes n+1 query problem to remove lamport from db.versionVector. So we choose remove only client's version vector from table, and filter minVersionVector by active clients. - -``` -// initial state -db.versionVector = { - c1: {c1:3, c2:4, c3: 5}, - c2: {c1:3, c2:3, c3: 6} -} -min(c1.vv, c2.vv) = min({c1:3, c2:4, c3: 5}, {c1:3, c2:3, c3:5}) = -{c1:3, c2:3, c3:5} -c1, c2 are acitve(attached). - -minVersionVector = {c1:3, c2:3, c3:5}.Filter([c3]) = {c1:3, c2:3} -``` - -After client receive this minVersionVector, it will filter its version vector to remove detached client's lamport. -The next pushpull request will contains filtered version vector so that eventually db.version vector will store attached client's version vector only. -![filter-version-vector](media/filter-version-vector.jpg) - - - -### Why `removedAt.lamport <= minVersionVector[removedAt.actor]` is not enough to run GC -Let's consider the following scenario. - -Users A, B, and C are participating in a collaborative editing session. User C deletes a specific node and immediately detaches the document. In this situation, the node deleted by C remains in the document, and C's version vector is removed from the version vector table in the database. - -Previously, we stated that to find the minimum version vector, we query all vectors in the version vector table in the database and take the minimum value. After C detaches, if we create the minimum version vector by querying the version vector table, the resulting minimum version vector will not contain C’s information. - -Our existing garbage collection (GC) algorithm performs GC when the condition removedAt.lamport <= minVersionVector[removedAt.actor] is satisfied. However, if the actor who deleted the node does not exist in the minimum version vector, this logic will not function. - -Therefore, the algorithm needs to be designed so that GC is performed even in situations where the actor who deleted the node is not present in the minimum version vector. - -### Is it safe to run GC in condition `removedAt.lamport < minVersionVector.minLamport()` -We can understand this by considering the definitions of the version vector and the minimum version vector. - -A version vector indicates the editing progress of a user’s document, including how much of other users’ edits have been incorporated. For example, if A’s version vector is `[A:5, B:4, C:2]`, it means that A’s document reflects changes up to 4 made by B and up to 2 made by C. - -Expanding this further, let’s assume three users have the following version vectors: - -- A: `[A:5, B:4, C:2]` -- B: `[A:3, B:4, C:2]` -- C: `[A:2, B:1, C:3]` - -We assume that C deleted a specific node with their last change. - -In this situation, if C detaches from the document, only A’s and B’s version vectors remain, and the minimum version vector would become `[A:3, B:4]`. When can we perform garbage collection (GC) to delete the node removed by C at `[A:2, B:1, C:3]`? - -By examining the minimum version vector at this point, we can consider two scenarios: - -1. Only A and B were participating in the editing from the beginning. -2. There was another user besides A and B, but that user has now detached. - -In the first scenario, the existing algorithm that operates when `removedAt.lamport <= minVersionVector[removedAt.actor]` applies, so we don’t need to address it further. - -The second scenario presents a potential issue, as a node removed by someone else remains as a tombstone. To remove this tombstone, we need a minimum guarantee. - -If we express the execution criterion of the GC algorithm in semantic terms, it would be: - -> "The point at which all users are aware that a specific node has been removed." - -From the moment C detaches, information about C is removed from each version vector. So, how can we know that C deleted a specific node? Since there’s no direct way to determine this in the minimum version vector due to the lack of information, we need to verify this fact indirectly. - -From the perspective of the version vector and the minimum version vector, this means that the minimum value in the minimum version vector should be greater than removedAt. - -Of course, it’s possible for a specific client to have a timestamp greater than removedAt without knowing that C deleted the node. However, this case can be addressed by calculating the minimum lamport value in the minimum version vector. - -What’s essential here is having a consistent criterion. If we take the node’s removedAt as this criterion, and if a lamport value greater than this criterion exists in the minimum version vector, then it is safe to delete the node. - -![remove-detached-clients-tombstone](media/remove-datached-clients-tombstone.jpg) ## An example of garbage collection: ### State 1 diff --git a/pkg/document/document.go b/pkg/document/document.go index f5a4eb734..b1396778c 100644 --- a/pkg/document/document.go +++ b/pkg/document/document.go @@ -221,17 +221,7 @@ func (d *Document) ApplyChangePack(pack *change.Pack) error { d.GarbageCollect(pack.VersionVector) } - // 05. Remove detached client's lamport from version vector if it exists - if pack.VersionVector != nil && !hasSnapshot { - actorIDs, err := pack.VersionVector.Keys() - if err != nil { - return err - } - - d.doc.changeID = d.doc.changeID.SetVersionVector(d.doc.changeID.VersionVector().Filter(actorIDs)) - } - - // 06. Update the status. + // 05. Update the status. if pack.IsRemoved { d.SetStatus(StatusRemoved) } diff --git a/pkg/document/internal_document.go b/pkg/document/internal_document.go index 849c28a43..3f54c5cc8 100644 --- a/pkg/document/internal_document.go +++ b/pkg/document/internal_document.go @@ -188,16 +188,6 @@ func (d *InternalDocument) ApplyChangePack(pack *change.Pack, disableGC bool) er } } - // 04. Remove detached client's lamport from version vector if it exists - if pack.VersionVector != nil && !hasSnapshot { - actorIDs, err := pack.VersionVector.Keys() - if err != nil { - return err - } - - d.changeID = d.changeID.SetVersionVector(d.changeID.VersionVector().Filter(actorIDs)) - } - return nil } diff --git a/pkg/document/time/version_vector.go b/pkg/document/time/version_vector.go index 3253e1820..75741a965 100644 --- a/pkg/document/time/version_vector.go +++ b/pkg/document/time/version_vector.go @@ -18,7 +18,6 @@ package time import ( "bytes" - "math" "sort" "strconv" "strings" @@ -118,9 +117,7 @@ func (v VersionVector) EqualToOrAfter(other *Ticket) bool { clientLamport, ok := v[other.actorID.bytes] if !ok { - minLamport := v.MinLamport() - - return minLamport > other.lamport + return false } return clientLamport >= other.lamport @@ -176,19 +173,6 @@ func (v VersionVector) Max(other VersionVector) VersionVector { return maxVV } -// MinLamport returns min lamport value in version vector. -func (v VersionVector) MinLamport() int64 { - var minLamport int64 = math.MaxInt64 - - for _, value := range v { - if value < minLamport { - minLamport = value - } - } - - return minLamport -} - // MaxLamport returns max lamport value in version vector. func (v VersionVector) MaxLamport() int64 { var maxLamport int64 = -1 diff --git a/server/backend/database/memory/database.go b/server/backend/database/memory/database.go index fb6cfb066..5d3517d2e 100644 --- a/server/backend/database/memory/database.go +++ b/server/backend/database/memory/database.go @@ -1347,24 +1347,10 @@ func (d *DB) UpdateAndFindMinSyncedVersionVector( docRefKey types.DocRefKey, versionVector time.VersionVector, ) (time.VersionVector, error) { - // 01. Prepare attachedActorIDs including the current client. - // If some clients are detached, we should remove them from the min version vector. - // For this, we use attachedActorIDs to filter the min version vector. - var attachedActorIDs []*time.ActorID - attached, err := clientInfo.IsAttached(docRefKey.DocID) - if err != nil { - return nil, err - } + // TODO(JOOHOJANG): We have to consider removing detached client's lamport + // from min version vector. - if attached { - actorID, err := clientInfo.ID.ToActorID() - if err != nil { - return nil, err - } - attachedActorIDs = append(attachedActorIDs, actorID) - } - - // 02. Find all version vectors of the given document from DB. + // 01. Find all version vectors of the given document from DB. txn := d.db.Txn(false) defer txn.Abort() iterator, err := txn.Get(tblVersionVectors, "doc_id", docRefKey.DocID.String()) @@ -1372,22 +1358,16 @@ func (d *DB) UpdateAndFindMinSyncedVersionVector( return nil, fmt.Errorf("find all version vectors: %w", err) } - // 03. Compute min version vector. + // 02. Compute min version vector. var minVersionVector time.VersionVector - // 03-1. Compute min version vector of other clients and collect attachedActorIDs. + // 02-1. Compute min version vector of other clients and collect attachedActorIDs. for raw := iterator.Next(); raw != nil; raw = iterator.Next() { vvi := raw.(*database.VersionVectorInfo) if clientInfo.ID == vvi.ClientID { continue } - actorID, err := vvi.ClientID.ToActorID() - if err != nil { - return nil, err - } - attachedActorIDs = append(attachedActorIDs, actorID) - if minVersionVector == nil { minVersionVector = vvi.VersionVector continue @@ -1399,11 +1379,10 @@ func (d *DB) UpdateAndFindMinSyncedVersionVector( minVersionVector = versionVector } - // 03-2. Compute min version vector with current client's version vector and filter detached clients. + // 02-2. Compute min version vector with current client's version vector. minVersionVector = minVersionVector.Min(versionVector) - minVersionVector = minVersionVector.Filter(attachedActorIDs) - // 04. Update current client's version vector. If the client is detached, remove it. + // 03. Update current client's version vector. If the client is detached, remove it. // This is only for the current client and does not affect the version vector of other clients. if err = d.UpdateVersionVector(ctx, clientInfo, docRefKey, versionVector); err != nil { return nil, err diff --git a/server/backend/database/mongo/client.go b/server/backend/database/mongo/client.go index de70d1308..236d55961 100644 --- a/server/backend/database/mongo/client.go +++ b/server/backend/database/mongo/client.go @@ -1235,26 +1235,11 @@ func (c *Client) UpdateAndFindMinSyncedVersionVector( docRefKey types.DocRefKey, versionVector time.VersionVector, ) (time.VersionVector, error) { + // TODO(JOOHOJANG): We have to consider removing detached client's lamport + // from min version vector. var versionVectorInfos []database.VersionVectorInfo - // 01. Prepare attachedActorIDs including the current client. - // If some clients are detached, we should remove them from the min version vector. - // For this, we use attachedActorIDs to filter the min version vector. - var attachedActorIDs []*time.ActorID - attached, err := clientInfo.IsAttached(docRefKey.DocID) - if err != nil { - return nil, err - } - - if attached { - currentActorID, err := clientInfo.ID.ToActorID() - if err != nil { - return nil, err - } - attachedActorIDs = append(attachedActorIDs, currentActorID) - } - - // 02. Find all version vectors of the given document from DB. + // 01. Find all version vectors of the given document from DB. cursor, err := c.collection(ColVersionVectors).Find(ctx, bson.M{ "project_id": docRefKey.ProjectID, "doc_id": docRefKey.DocID, @@ -1267,21 +1252,15 @@ func (c *Client) UpdateAndFindMinSyncedVersionVector( return nil, fmt.Errorf("decode version vectors: %w", err) } - // 03. Compute min version vector. + // 02. Compute min version vector. var minVersionVector time.VersionVector - // 03-1. Compute min version vector of other clients and collect attachedActorIDs. + // 02-1. Compute min version vector of other clients and collect attachedActorIDs. for _, vvi := range versionVectorInfos { if clientInfo.ID == vvi.ClientID { continue } - actorID, err := vvi.ClientID.ToActorID() - if err != nil { - return nil, err - } - attachedActorIDs = append(attachedActorIDs, actorID) - if minVersionVector == nil { minVersionVector = vvi.VersionVector continue @@ -1293,11 +1272,10 @@ func (c *Client) UpdateAndFindMinSyncedVersionVector( minVersionVector = versionVector } - // 03-2. Compute min version vector with current client's version vector and filter detached clients. + // 02-2. Compute min version vector with current client's version vector. minVersionVector = minVersionVector.Min(versionVector) - minVersionVector = minVersionVector.Filter(attachedActorIDs) - // 04. Update current client's version vector. If the client is detached, remove it. + // 03. Update current client's version vector. If the client is detached, remove it. // This is only for the current client and does not affect the version vector of other clients. if err = c.UpdateVersionVector(ctx, clientInfo, docRefKey, versionVector); err != nil { return nil, err diff --git a/test/integration/gc_test.go b/test/integration/gc_test.go index 39c9f2c78..4fc6195e6 100644 --- a/test/integration/gc_test.go +++ b/test/integration/gc_test.go @@ -442,8 +442,10 @@ func TestGarbageCollection(t *testing.T) { assert.NoError(t, c1.Sync(ctx)) // remove c2 lamport from d1.vv after GC - // d1.vv = [c1:5], minvv = [c1:4], db.vv {c1: [c1:4]} - assert.Equal(t, true, checkVV(d1.VersionVector(), versionOf(d1.ActorID(), 5))) + // d1.vv = [c1:5, c2:4], minvv = [c1:4, c2:4], db.vv {c1: [c1:4]} + // TODO(JOOHOJANG): We have to consider removing detached client's lamport + // from version vector. + assert.Equal(t, true, checkVV(d1.VersionVector(), versionOf(d1.ActorID(), 5), versionOf(d2.ActorID(), 4))) assert.Equal(t, 0, d1.GarbageLen()) assert.Equal(t, 6, d2.GarbageLen()) }) @@ -1190,6 +1192,115 @@ func TestGarbageCollection(t *testing.T) { assert.NoError(t, c2.Sync(ctx)) assert.Equal(t, 0, d2.GarbageLen()) - assert.Equal(t, 1, len(d2.VersionVector())) + // TODO(JOOHOJANG): We have to consider removing detached client's lamport + // from version vector. + assert.Equal(t, 2, len(d2.VersionVector())) + }) + + t.Run("detach gc test", func(t *testing.T) { + clients := activeClients(t, 3) + c1, c2, c3 := clients[0], clients[1], clients[2] + defer deactivateAndCloseClients(t, clients) + ctx := context.Background() + d1 := document.New(helper.TestDocKey(t)) + d2 := document.New(helper.TestDocKey(t)) + d3 := document.New(helper.TestDocKey(t)) + + assert.NoError(t, c1.Attach(ctx, d1)) + assert.NoError(t, c2.Attach(ctx, d2)) + assert.NoError(t, c3.Attach(ctx, d3)) + assert.NoError(t, c3.Sync(ctx)) + assert.NoError(t, c2.Sync(ctx)) + assert.NoError(t, c2.Sync(ctx)) + assert.NoError(t, c1.Sync(ctx)) + assert.NoError(t, c1.Sync(ctx)) + + assert.Equal(t, true, checkVV(d1.VersionVector(), versionOf(d1.ActorID(), 3), versionOf(d2.ActorID(), 1), versionOf(d3.ActorID(), 1))) + assert.Equal(t, true, checkVV(d2.VersionVector(), versionOf(d1.ActorID(), 1), versionOf(d2.ActorID(), 3), versionOf(d3.ActorID(), 1))) + assert.Equal(t, true, checkVV(d3.VersionVector(), versionOf(d1.ActorID(), 1), versionOf(d2.ActorID(), 1), versionOf(d3.ActorID(), 3))) + + err := d1.Update(func(root *json.Object, p *presence.Presence) error { + root.SetNewText("text").Edit(0, 0, "a").Edit(1, 1, "b").Edit(2, 2, "c") + return nil + }, "insert abc") + assert.NoError(t, err) + + assert.NoError(t, c1.Sync(ctx)) + assert.NoError(t, c2.Sync(ctx)) + assert.NoError(t, c2.Sync(ctx)) + assert.NoError(t, c3.Sync(ctx)) + assert.NoError(t, c3.Sync(ctx)) + + assert.Equal(t, true, checkVV(d1.VersionVector(), versionOf(d1.ActorID(), 4), versionOf(d2.ActorID(), 1), versionOf(d3.ActorID(), 1))) + assert.Equal(t, true, checkVV(d2.VersionVector(), versionOf(d1.ActorID(), 4), versionOf(d2.ActorID(), 5), versionOf(d3.ActorID(), 1))) + assert.Equal(t, true, checkVV(d3.VersionVector(), versionOf(d1.ActorID(), 4), versionOf(d2.ActorID(), 1), versionOf(d3.ActorID(), 5))) + + err = d3.Update(func(root *json.Object, p *presence.Presence) error { + root.GetText("text").Edit(1, 3, "") // delete bc + return nil + }, "delete bc") + assert.NoError(t, err) + err = d1.Update(func(root *json.Object, p *presence.Presence) error { + root.GetText("text").Edit(0, 0, "1") // 1abc + return nil + }, "insert 1") + assert.NoError(t, err) + err = d1.Update(func(root *json.Object, p *presence.Presence) error { + root.GetText("text").Edit(0, 0, "2") // 21abc + return nil + }, "insert 2") + assert.NoError(t, err) + err = d1.Update(func(root *json.Object, p *presence.Presence) error { + root.GetText("text").Edit(0, 0, "3") // 321abc + return nil + }, "insert 3") + assert.NoError(t, err) + err = d2.Update(func(root *json.Object, p *presence.Presence) error { + root.GetText("text").Edit(3, 3, "x") // abcx + return nil + }, "insert x") + assert.NoError(t, err) + err = d2.Update(func(root *json.Object, p *presence.Presence) error { + root.GetText("text").Edit(4, 4, "y") // abcxy + return nil + }, "insert x") + assert.NoError(t, err) + + assert.NoError(t, c1.Sync(ctx)) + assert.NoError(t, c2.Sync(ctx)) + assert.NoError(t, c1.Sync(ctx)) + + assert.Equal(t, `{"text":[{"val":"3"},{"val":"2"},{"val":"1"},{"val":"a"},{"val":"b"},{"val":"c"},{"val":"x"},{"val":"y"}]}`, d1.Marshal()) + assert.Equal(t, `{"text":[{"val":"3"},{"val":"2"},{"val":"1"},{"val":"a"},{"val":"b"},{"val":"c"},{"val":"x"},{"val":"y"}]}`, d2.Marshal()) + assert.Equal(t, true, checkVV(d1.VersionVector(), versionOf(d1.ActorID(), 9), versionOf(d2.ActorID(), 7), versionOf(d3.ActorID(), 1))) + assert.Equal(t, true, checkVV(d2.VersionVector(), versionOf(d1.ActorID(), 7), versionOf(d2.ActorID(), 10), versionOf(d3.ActorID(), 1))) + assert.Equal(t, true, checkVV(d3.VersionVector(), versionOf(d1.ActorID(), 4), versionOf(d2.ActorID(), 1), versionOf(d3.ActorID(), 6))) + + assert.NoError(t, c3.Detach(ctx, d3)) + err = d2.Update(func(root *json.Object, p *presence.Presence) error { + root.GetText("text").Edit(5, 5, "z") // 321abzcxy + return nil + }, "insert y") + assert.NoError(t, err) + assert.NoError(t, c1.Sync(ctx)) + + assert.Equal(t, 2, d1.GarbageLen()) + assert.NoError(t, c1.Sync(ctx)) + assert.Equal(t, 2, d1.GarbageLen()) + + assert.NoError(t, c2.Sync(ctx)) + assert.NoError(t, c1.Sync(ctx)) + // TODO(JOOHOJANG): We have to consider removing detached client's lamport + // from version vector. + assert.Equal(t, true, checkVV(d1.VersionVector(), versionOf(d1.ActorID(), 12), versionOf(d2.ActorID(), 11), versionOf(d3.ActorID(), 7))) + assert.Equal(t, true, checkVV(d2.VersionVector(), versionOf(d1.ActorID(), 7), versionOf(d2.ActorID(), 13), versionOf(d3.ActorID(), 7))) + assert.Equal(t, `{"text":[{"val":"3"},{"val":"2"},{"val":"1"},{"val":"a"},{"val":"z"},{"val":"x"},{"val":"y"}]}`, d1.Marshal()) + assert.Equal(t, `{"text":[{"val":"3"},{"val":"2"},{"val":"1"},{"val":"a"},{"val":"z"},{"val":"x"},{"val":"y"}]}`, d2.Marshal()) + assert.Equal(t, 2, d1.GarbageLen()) + assert.Equal(t, 2, d2.GarbageLen()) + assert.NoError(t, c2.Sync(ctx)) + assert.NoError(t, c1.Sync(ctx)) + assert.Equal(t, 0, d1.GarbageLen()) + assert.Equal(t, 0, d2.GarbageLen()) }) }