Skip to content

Commit f7e4c31

Browse files
schmidt-sebastianleotianlizhan
schmidt-sebastian
authored and
leotianlizhan
committed
Clear the lastLimboFreeSnapshot version on existence filter mismatch (#9199)
1 parent 1d5e722 commit f7e4c31

File tree

4 files changed

+316
-22
lines changed

4 files changed

+316
-22
lines changed

‎Firestore/CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,7 @@
1+
# Unreleased
2+
- [fixed] Fixed an issue that can result in incomplete Query snapshots when an
3+
app is backgrounded during query execution.
4+
15
# v8.9.1
26
- [fixed] Fixed a bug in the AppCheck integration that caused the SDK to respond
37
to unrelated notifications (#8895).

‎Firestore/Example/Tests/SpecTests/json/existence_filter_spec_test.json

Lines changed: 236 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1072,6 +1072,242 @@
10721072
}
10731073
]
10741074
},
1075+
"Existence filter mismatch invalidates index-free query": {
1076+
"describeName": "Existence Filters:",
1077+
"itName": "Existence filter clears resume token",
1078+
"tags": [
1079+
"durable-persistence"
1080+
],
1081+
"config": {
1082+
"numClients": 1,
1083+
"useGarbageCollection": true
1084+
},
1085+
"steps": [
1086+
{
1087+
"userListen": {
1088+
"query": {
1089+
"filters": [
1090+
],
1091+
"orderBys": [
1092+
],
1093+
"path": "collection"
1094+
},
1095+
"targetId": 2
1096+
},
1097+
"expectedState": {
1098+
"activeTargets": {
1099+
"2": {
1100+
"queries": [
1101+
{
1102+
"filters": [
1103+
],
1104+
"orderBys": [
1105+
],
1106+
"path": "collection"
1107+
}
1108+
],
1109+
"resumeToken": ""
1110+
}
1111+
}
1112+
}
1113+
},
1114+
{
1115+
"watchAck": [
1116+
2
1117+
]
1118+
},
1119+
{
1120+
"watchEntity": {
1121+
"docs": [
1122+
{
1123+
"key": "collection/1",
1124+
"options": {
1125+
"hasCommittedMutations": false,
1126+
"hasLocalMutations": false
1127+
},
1128+
"value": {
1129+
"v": 1
1130+
},
1131+
"version": 1000
1132+
},
1133+
{
1134+
"key": "collection/2",
1135+
"options": {
1136+
"hasCommittedMutations": false,
1137+
"hasLocalMutations": false
1138+
},
1139+
"value": {
1140+
"v": 2
1141+
},
1142+
"version": 1000
1143+
}
1144+
],
1145+
"targets": [
1146+
2
1147+
]
1148+
}
1149+
},
1150+
{
1151+
"watchCurrent": [
1152+
[
1153+
2
1154+
],
1155+
"resume-token-1000"
1156+
]
1157+
},
1158+
{
1159+
"watchSnapshot": {
1160+
"targetIds": [
1161+
],
1162+
"version": 1000
1163+
},
1164+
"expectedSnapshotEvents": [
1165+
{
1166+
"added": [
1167+
{
1168+
"key": "collection/1",
1169+
"options": {
1170+
"hasCommittedMutations": false,
1171+
"hasLocalMutations": false
1172+
},
1173+
"value": {
1174+
"v": 1
1175+
},
1176+
"version": 1000
1177+
},
1178+
{
1179+
"key": "collection/2",
1180+
"options": {
1181+
"hasCommittedMutations": false,
1182+
"hasLocalMutations": false
1183+
},
1184+
"value": {
1185+
"v": 2
1186+
},
1187+
"version": 1000
1188+
}
1189+
],
1190+
"errorCode": 0,
1191+
"fromCache": false,
1192+
"hasPendingWrites": false,
1193+
"query": {
1194+
"filters": [
1195+
],
1196+
"orderBys": [
1197+
],
1198+
"path": "collection"
1199+
}
1200+
}
1201+
]
1202+
},
1203+
{
1204+
"watchFilter": [
1205+
[
1206+
2
1207+
],
1208+
"collection/1"
1209+
]
1210+
},
1211+
{
1212+
"watchSnapshot": {
1213+
"targetIds": [
1214+
],
1215+
"version": 2000
1216+
},
1217+
"expectedSnapshotEvents": [
1218+
{
1219+
"errorCode": 0,
1220+
"fromCache": true,
1221+
"hasPendingWrites": false,
1222+
"query": {
1223+
"filters": [
1224+
],
1225+
"orderBys": [
1226+
],
1227+
"path": "collection"
1228+
}
1229+
}
1230+
]
1231+
},
1232+
{
1233+
"restart": true,
1234+
"expectedState": {
1235+
"activeLimboDocs": [
1236+
],
1237+
"activeTargets": {
1238+
},
1239+
"enqueuedLimboDocs": [
1240+
]
1241+
}
1242+
},
1243+
{
1244+
"userListen": {
1245+
"query": {
1246+
"filters": [
1247+
],
1248+
"orderBys": [
1249+
],
1250+
"path": "collection"
1251+
},
1252+
"targetId": 2
1253+
},
1254+
"expectedSnapshotEvents": [
1255+
{
1256+
"added": [
1257+
{
1258+
"key": "collection/1",
1259+
"options": {
1260+
"hasCommittedMutations": false,
1261+
"hasLocalMutations": false
1262+
},
1263+
"value": {
1264+
"v": 1
1265+
},
1266+
"version": 1000
1267+
},
1268+
{
1269+
"key": "collection/2",
1270+
"options": {
1271+
"hasCommittedMutations": false,
1272+
"hasLocalMutations": false
1273+
},
1274+
"value": {
1275+
"v": 2
1276+
},
1277+
"version": 1000
1278+
}
1279+
],
1280+
"errorCode": 0,
1281+
"fromCache": true,
1282+
"hasPendingWrites": false,
1283+
"query": {
1284+
"filters": [
1285+
],
1286+
"orderBys": [
1287+
],
1288+
"path": "collection"
1289+
}
1290+
}
1291+
],
1292+
"expectedState": {
1293+
"activeTargets": {
1294+
"2": {
1295+
"queries": [
1296+
{
1297+
"filters": [
1298+
],
1299+
"orderBys": [
1300+
],
1301+
"path": "collection"
1302+
}
1303+
],
1304+
"resumeToken": ""
1305+
}
1306+
}
1307+
}
1308+
}
1309+
]
1310+
},
10751311
"Existence filter mismatch triggers re-run of query": {
10761312
"describeName": "Existence Filters:",
10771313
"itName": "Existence filter mismatch triggers re-run of query",

‎Firestore/core/src/local/local_store.cc

Lines changed: 20 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -263,6 +263,7 @@ model::DocumentMap LocalStore::ApplyRemoteEvent(
263263
for (const auto& entry : remote_event.target_changes()) {
264264
TargetId target_id = entry.first;
265265
const TargetChange& change = entry.second;
266+
const ByteString& resume_token = change.resume_token();
266267

267268
auto found = target_data_by_target_.find(target_id);
268269
if (found == target_data_by_target_.end()) {
@@ -277,24 +278,25 @@ model::DocumentMap LocalStore::ApplyRemoteEvent(
277278
target_cache_->RemoveMatchingKeys(change.removed_documents(), target_id);
278279
target_cache_->AddMatchingKeys(change.added_documents(), target_id);
279280

280-
// Update the resume token if the change includes one. Don't clear any
281-
// preexisting value. Bump the sequence number as well, so that documents
282-
// being removed now are ordered later than documents that were previously
283-
// removed from this target.
284-
const ByteString& resume_token = change.resume_token();
285-
// Update the resume token if the change includes one.
286-
if (!resume_token.empty()) {
287-
TargetData new_target_data =
288-
old_target_data
289-
.WithResumeToken(resume_token, remote_event.snapshot_version())
290-
.WithSequenceNumber(sequence_number);
291-
target_data_by_target_[target_id] = new_target_data;
292-
293-
// Update the target data if there are target changes (or if sufficient
294-
// time has passed since the last update).
295-
if (ShouldPersistTargetData(new_target_data, old_target_data, change)) {
296-
target_cache_->UpdateTarget(new_target_data);
297-
}
281+
TargetData new_target_data =
282+
old_target_data.WithSequenceNumber(sequence_number);
283+
if (remote_event.target_mismatches().find(target_id) !=
284+
remote_event.target_mismatches().end()) {
285+
new_target_data =
286+
new_target_data
287+
.WithResumeToken(ByteString{}, SnapshotVersion::None())
288+
.WithLastLimboFreeSnapshotVersion(SnapshotVersion::None());
289+
} else if (!resume_token.empty()) {
290+
new_target_data = old_target_data.WithResumeToken(
291+
resume_token, remote_event.snapshot_version());
292+
}
293+
294+
target_data_by_target_[target_id] = new_target_data;
295+
296+
// Update the target data if there are target changes (or if sufficient
297+
// time has passed since the last update).
298+
if (ShouldPersistTargetData(new_target_data, old_target_data, change)) {
299+
target_cache_->UpdateTarget(new_target_data);
298300
}
299301
}
300302

@@ -330,10 +332,6 @@ model::DocumentMap LocalStore::ApplyRemoteEvent(
330332
bool LocalStore::ShouldPersistTargetData(const TargetData& new_target_data,
331333
const TargetData& old_target_data,
332334
const TargetChange& change) const {
333-
// Avoid clearing any existing value
334-
HARD_ASSERT(!new_target_data.resume_token().empty(),
335-
"Attempted to persist target data with empty resume token");
336-
337335
// Always persist target data if we don't already have a resume token.
338336
if (old_target_data.resume_token().empty()) return true;
339337

‎Firestore/core/test/unit/local/local_store_test.cc

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
#include "Firestore/core/src/model/patch_mutation.h"
3737
#include "Firestore/core/src/model/set_mutation.h"
3838
#include "Firestore/core/src/model/transform_operation.h"
39+
#include "Firestore/core/src/remote/existence_filter.h"
3940
#include "Firestore/core/src/remote/remote_event.h"
4041
#include "Firestore/core/src/remote/watch_change.h"
4142
#include "Firestore/core/test/unit/remote/fake_target_metadata_provider.h"
@@ -70,6 +71,8 @@ using model::TargetId;
7071
using nanopb::ByteString;
7172
using nanopb::Message;
7273
using remote::DocumentWatchChange;
74+
using remote::ExistenceFilter;
75+
using remote::ExistenceFilterWatchChange;
7376
using remote::FakeTargetMetadataProvider;
7477
using remote::RemoteEvent;
7578
using remote::WatchChangeAggregator;
@@ -186,6 +189,24 @@ RemoteEvent UpdateRemoteEvent(
186189
removed_from_targets, {});
187190
}
188191

192+
/** Creates a remote event that inserts a list of documents. */
193+
RemoteEvent ExistenceFilterEvent(TargetId target_id,
194+
const DocumentKeySet& synced_keys,
195+
int remote_count,
196+
int version) {
197+
TargetData target_data(Query("foo").ToTarget(), target_id, 0,
198+
QueryPurpose::Listen);
199+
remote::FakeTargetMetadataProvider metadata_provider;
200+
metadata_provider.SetSyncedKeys(synced_keys, target_data);
201+
202+
ExistenceFilter existence_filter{remote_count};
203+
WatchChangeAggregator aggregator{&metadata_provider};
204+
ExistenceFilterWatchChange existence_filter_watch_change{existence_filter,
205+
target_id};
206+
aggregator.HandleExistenceFilter(existence_filter_watch_change);
207+
return aggregator.CreateRemoteEvent(testutil::Version(version));
208+
}
209+
189210
LocalViewChanges TestViewChanges(TargetId target_id,
190211
bool from_cache,
191212
std::vector<std::string> added_keys,
@@ -1168,6 +1189,41 @@ TEST_P(LocalStoreTest, UsesTargetMappingToExecuteQueries) {
11681189
FSTAssertQueryReturned("foo/a", "foo/b");
11691190
}
11701191

1192+
TEST_P(LocalStoreTest, IgnoresTargetMappingAfterExistenceFilterMismatch) {
1193+
if (IsGcEager()) return;
1194+
1195+
core::Query query =
1196+
Query("foo").AddingFilter(testutil::Filter("matches", "==", true));
1197+
TargetId target_id = AllocateQuery(query);
1198+
1199+
ExecuteQuery(query);
1200+
1201+
// Persist a mapping with a single document
1202+
ApplyRemoteEvent(
1203+
AddedRemoteEvent({Doc("foo/a", 10, Map("matches", true))}, {target_id}));
1204+
ApplyRemoteEvent(NoChangeEvent(target_id, 10));
1205+
UpdateViews(target_id, /* from_cache= */ false);
1206+
1207+
// At this point, we have not yet confirmed that the query is limbo free.
1208+
TargetData cached_target_data = GetTargetData(query);
1209+
ASSERT_EQ(testutil::Version(10),
1210+
cached_target_data.last_limbo_free_snapshot_version());
1211+
1212+
// Create an existence filter mismatch and verify that the last limbo free
1213+
// snapshot version is deleted
1214+
ApplyRemoteEvent(ExistenceFilterEvent(
1215+
target_id, DocumentKeySet{testutil::Key("foo/a")}, 2, 20));
1216+
cached_target_data = GetTargetData(query);
1217+
ASSERT_EQ(SnapshotVersion::None(),
1218+
cached_target_data.last_limbo_free_snapshot_version());
1219+
ASSERT_EQ(ByteString{}, cached_target_data.resume_token());
1220+
1221+
// Re-run the query as a collection scan
1222+
ExecuteQuery(query);
1223+
FSTAssertRemoteDocumentsRead(/* by_key */ 0, /* by_query= */ 1);
1224+
FSTAssertQueryReturned("foo/a");
1225+
}
1226+
11711227
TEST_P(LocalStoreTest, LastLimboFreeSnapshotIsAdvancedDuringViewProcessing) {
11721228
// This test verifies that the `last_limbo_free_snapshot` version for
11731229
// TargetData is advanced when we compute a limbo-free free view and that the

0 commit comments

Comments
 (0)