From e8cd98f13517cfe702d1e7dc1056cf1eacd48930 Mon Sep 17 00:00:00 2001 From: Mila <107142260+milaGGL@users.noreply.github.com> Date: Wed, 22 Nov 2023 09:57:07 -0500 Subject: [PATCH] Fix docs unnecessarily undergo limbo resolution while resuming query bug (#12044) --- Firestore/CHANGELOG.md | 6 + .../Tests/SpecTests/json/limbo_spec_test.json | 405 +++++++++++++++++- .../Tests/SpecTests/json/limit_spec_test.json | 8 + Firestore/core/src/core/sync_engine.cc | 19 +- Firestore/core/src/core/view.cc | 20 +- Firestore/core/src/core/view.h | 14 +- 6 files changed, 433 insertions(+), 39 deletions(-) diff --git a/Firestore/CHANGELOG.md b/Firestore/CHANGELOG.md index ecd128d5869..ad6da5f8b21 100644 --- a/Firestore/CHANGELOG.md +++ b/Firestore/CHANGELOG.md @@ -1,3 +1,9 @@ +# Unreleased +- [fixed] Fixed an issue in the local cache synchronization logic where all + locally-cached documents that matched a resumed query would be unnecessarily + re-downloaded; with the fix it now only downloads the documents that are known + to be out-of-sync. (#12044) + # 10.18.0 - [fixed] Fix Firestore build for visionOS on Xcode 15.1. (#12023) diff --git a/Firestore/Example/Tests/SpecTests/json/limbo_spec_test.json b/Firestore/Example/Tests/SpecTests/json/limbo_spec_test.json index adffb61f510..6cb27ecc40d 100644 --- a/Firestore/Example/Tests/SpecTests/json/limbo_spec_test.json +++ b/Firestore/Example/Tests/SpecTests/json/limbo_spec_test.json @@ -7013,6 +7013,378 @@ } ] }, + "Limbo resolution should wait for full re-query result if there is an existence filter mismatch ": { + "describeName": "Limbo Documents:", + "itName": "Limbo resolution should wait for full re-query result if there is an existence filter mismatch ", + "tags": [ + ], + "config": { + "numClients": 1, + "useEagerGCForMemory": true + }, + "steps": [ + { + "userListen": { + "query": { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection" + }, + "targetId": 2 + }, + "expectedState": { + "activeTargets": { + "2": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection" + } + ], + "resumeToken": "" + } + } + } + }, + { + "watchAck": [ + 2 + ] + }, + { + "watchEntity": { + "docs": [ + { + "createTime": 0, + "key": "collection/a", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "v": 1 + }, + "version": 1000 + }, + { + "createTime": 0, + "key": "collection/b", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "v": 2 + }, + "version": 1000 + } + ], + "targets": [ + 2 + ] + } + }, + { + "watchCurrent": [ + [ + 2 + ], + "resume-token-1000" + ] + }, + { + "watchSnapshot": { + "targetIds": [ + ], + "version": 1000 + }, + "expectedSnapshotEvents": [ + { + "added": [ + { + "createTime": 0, + "key": "collection/a", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "v": 1 + }, + "version": 1000 + }, + { + "createTime": 0, + "key": "collection/b", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "v": 2 + }, + "version": 1000 + } + ], + "errorCode": 0, + "fromCache": false, + "hasPendingWrites": false, + "query": { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection" + } + } + ] + }, + { + "enableNetwork": false, + "expectedSnapshotEvents": [ + { + "errorCode": 0, + "fromCache": true, + "hasPendingWrites": false, + "query": { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection" + } + } + ], + "expectedState": { + "activeLimboDocs": [ + ], + "activeTargets": { + }, + "enqueuedLimboDocs": [ + ] + } + }, + { + "enableNetwork": true, + "expectedState": { + "activeTargets": { + "2": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection" + } + ], + "resumeToken": "resume-token-1000" + } + } + } + }, + { + "watchAck": [ + 2 + ] + }, + { + "watchFilter": { + "keys": [ + "collection/a" + ], + "targetIds": [ + 2 + ] + } + }, + { + "watchCurrent": [ + [ + 2 + ], + "resume-token-2000" + ] + }, + { + "watchSnapshot": { + "targetIds": [ + ], + "version": 2000 + }, + "expectedState": { + "activeTargets": { + "2": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection" + } + ], + "resumeToken": "", + "targetPurpose": "TargetPurposeExistenceFilterMismatch" + } + } + } + }, + { + "watchRemove": { + "targetIds": [ + 2 + ] + } + }, + { + "watchAck": [ + 2 + ] + }, + { + "watchEntity": { + "docs": [ + { + "createTime": 0, + "key": "collection/a", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "v": 1 + }, + "version": 1000 + } + ], + "targets": [ + 2 + ] + } + }, + { + "watchCurrent": [ + [ + 2 + ], + "resume-token-3000" + ] + }, + { + "watchSnapshot": { + "targetIds": [ + ], + "version": 3000 + }, + "expectedState": { + "activeLimboDocs": [ + "collection/b" + ], + "activeTargets": { + "1": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection/b" + } + ], + "resumeToken": "", + "targetPurpose": "TargetPurposeLimboResolution" + }, + "2": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection" + } + ], + "resumeToken": "", + "targetPurpose": "TargetPurposeExistenceFilterMismatch" + } + } + } + }, + { + "watchAck": [ + 1 + ] + }, + { + "watchCurrent": [ + [ + 1 + ], + "resume-token-3000" + ] + }, + { + "watchSnapshot": { + "targetIds": [ + ], + "version": 3000 + }, + "expectedSnapshotEvents": [ + { + "errorCode": 0, + "fromCache": false, + "hasPendingWrites": false, + "query": { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection" + }, + "removed": [ + { + "createTime": 0, + "key": "collection/b", + "options": { + "hasCommittedMutations": false, + "hasLocalMutations": false + }, + "value": { + "v": 2 + }, + "version": 1000 + } + ] + } + ], + "expectedState": { + "activeLimboDocs": [ + ], + "activeTargets": { + "2": { + "queries": [ + { + "filters": [ + ], + "orderBys": [ + ], + "path": "collection" + } + ], + "resumeToken": "", + "targetPurpose": "TargetPurposeExistenceFilterMismatch" + } + } + } + } + ] + }, "Limbo resolution throttling when a limbo listen is rejected.": { "describeName": "Limbo Documents:", "itName": "Limbo resolution throttling when a limbo listen is rejected.", @@ -8221,6 +8593,14 @@ ] } }, + { + "watchCurrent": [ + [ + 2 + ], + "resume-token-1001" + ] + }, { "watchSnapshot": { "targetIds": [ @@ -8278,22 +8658,7 @@ "path": "collection" } } - ] - }, - { - "watchCurrent": [ - [ - 2 - ], - "resume-token-1002" - ] - }, - { - "watchSnapshot": { - "targetIds": [ - ], - "version": 1002 - }, + ], "expectedState": { "activeLimboDocs": [ "collection/a1", @@ -8608,6 +8973,14 @@ ] } }, + { + "watchCurrent": [ + [ + 2 + ], + "resume-token-1001" + ] + }, { "watchSnapshot": { "targetIds": [ diff --git a/Firestore/Example/Tests/SpecTests/json/limit_spec_test.json b/Firestore/Example/Tests/SpecTests/json/limit_spec_test.json index fe1ed3c7c73..86ae7f214c9 100644 --- a/Firestore/Example/Tests/SpecTests/json/limit_spec_test.json +++ b/Firestore/Example/Tests/SpecTests/json/limit_spec_test.json @@ -5620,6 +5620,14 @@ ] } }, + { + "watchCurrent": [ + [ + 2 + ], + "resume-token-1004" + ] + }, { "watchSnapshot": { "targetIds": [ diff --git a/Firestore/core/src/core/sync_engine.cc b/Firestore/core/src/core/sync_engine.cc index 3a68dff69f8..24a4c3f7803 100644 --- a/Firestore/core/src/core/sync_engine.cc +++ b/Firestore/core/src/core/sync_engine.cc @@ -495,15 +495,24 @@ void SyncEngine::EmitNewSnapshotsAndNotifyLocalStore( } absl::optional target_changes; + bool targetIsPendingReset = false; if (maybe_remote_event.has_value()) { const RemoteEvent& remote_event = maybe_remote_event.value(); - auto it = remote_event.target_changes().find(query_view->target_id()); - if (it != remote_event.target_changes().end()) { - target_changes = it->second; + auto changes_iter = + remote_event.target_changes().find(query_view->target_id()); + if (changes_iter != remote_event.target_changes().end()) { + target_changes = changes_iter->second; + } + + auto mismatches_iter = + remote_event.target_mismatches().find(query_view->target_id()); + if (mismatches_iter != remote_event.target_mismatches().end()) { + targetIsPendingReset = true; } } - ViewChange view_change = - view.ApplyChanges(view_doc_changes, target_changes); + + ViewChange view_change = view.ApplyChanges(view_doc_changes, target_changes, + targetIsPendingReset); UpdateTrackedLimboDocuments(view_change.limbo_changes(), query_view->target_id()); diff --git a/Firestore/core/src/core/view.cc b/Firestore/core/src/core/view.cc index af31b0ee696..c812cb0861e 100644 --- a/Firestore/core/src/core/view.cc +++ b/Firestore/core/src/core/view.cc @@ -246,13 +246,9 @@ bool View::ShouldWaitForSyncedDocument(const Document& new_doc, !new_doc->has_local_mutations()); } -ViewChange View::ApplyChanges(const ViewDocumentChanges& doc_changes) { - return ApplyChanges(doc_changes, {}); -} - -ViewChange View::ApplyChanges( - const ViewDocumentChanges& doc_changes, - const absl::optional& target_change) { +ViewChange View::ApplyChanges(const ViewDocumentChanges& doc_changes, + const absl::optional& target_change, + bool targetIsPendingReset) { HARD_ASSERT(!doc_changes.needs_refill(), "Cannot apply changes that need a refill"); @@ -275,8 +271,14 @@ ViewChange View::ApplyChanges( }); ApplyTargetChange(target_change); - std::vector limbo_changes = UpdateLimboDocuments(); - bool synced = limbo_documents_.empty() && current_; + std::vector limbo_changes = + targetIsPendingReset ? std::vector{} + : UpdateLimboDocuments(); + + // We are at synced state if there is no limbo docs are waiting to be + // resolved, view is current with the backend, and the query is not pending + // to reset due to existence filter mismatch. + bool synced = limbo_documents_.empty() && current_ && !targetIsPendingReset; SyncState new_sync_state = synced ? SyncState::Synced : SyncState::Local; bool sync_state_changed = new_sync_state != sync_state_; sync_state_ = new_sync_state; diff --git a/Firestore/core/src/core/view.h b/Firestore/core/src/core/view.h index b3cdbce69fc..c6c41b3c8dc 100644 --- a/Firestore/core/src/core/view.h +++ b/Firestore/core/src/core/view.h @@ -161,14 +161,6 @@ class View { const absl::optional& previous_changes = absl::nullopt) const; - /** - * Updates the view with the given ViewDocumentChanges. - * - * @param doc_changes The set of changes to make to the view's docs. - * @return A new ViewChange with the given docs, changes, and sync state. - */ - ViewChange ApplyChanges(const core::ViewDocumentChanges& doc_changes); - /** * Updates the view with the given ViewDocumentChanges and updates limbo docs * and sync state from the given (optional) target change. @@ -176,11 +168,15 @@ class View { * @param doc_changes The set of changes to make to the view's docs. * @param target_change A target change to apply for computing limbo docs and * sync state. + * @param targetIsPendingReset - Whether the target is pending to reset due to + existence filter mismatch. If not explicitly specified, it is treated + equivalently to `false`. * @return A new ViewChange with the given docs, changes, and sync state. */ ViewChange ApplyChanges( const core::ViewDocumentChanges& doc_changes, - const absl::optional& target_change); + const absl::optional& target_change = absl::nullopt, + bool targetIsPendingReset = false); /** * Applies an OnlineState change to the view, potentially generating an