Skip to content

Commit

Permalink
Fix for 8474 - Prevent a possible condition of slow snapshots, caused…
Browse files Browse the repository at this point in the history 
… by a rapid series of document update(s) followed by a delete. (#8595)
  • Loading branch information
@MarkDuckworth
MarkDuckworth authored Oct 30, 2024
1 parent d4cb3f8 commit 0f5714b
Show file tree
Hide file tree
Showing 5 changed files with 352 additions and 8 deletions.
5 changes: 5 additions & 0 deletions .changeset/tasty-boxes-brake.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@firebase/firestore": patch
---

Prevent a possible condition of slow snapshots, caused by a rapid series of document update(s) followed by a delete.
34 changes: 30 additions & 4 deletions packages/firestore/src/remote/watch_change.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,7 @@ export class WatchChangeAggregator {

/** Keeps track of the documents to update since the last raised snapshot. */
private pendingDocumentUpdates = mutableDocumentMap();
private pendingDocumentUpdatesByTarget = documentTargetMap();

/** A mapping of document keys to their set of target IDs. */
private pendingDocumentTargetMapping = documentTargetMap();
Expand Down Expand Up @@ -587,16 +588,16 @@ export class WatchChangeAggregator {
if (targetState.current && targetIsDocumentTarget(targetData.target)) {
// Document queries for document that don't exist can produce an empty
// result set. To update our local cache, we synthesize a document
// delete if we have not previously received the document. This
// resolves the limbo state of the document, removing it from
// limboDocumentRefs.
// delete if we have not previously received the document for this
// target. This resolves the limbo state of the document, removing it
// from limboDocumentRefs.
//
// TODO(dimond): Ideally we would have an explicit lookup target
// instead resulting in an explicit delete message and we could
// remove this special logic.
const key = new DocumentKey(targetData.target.path);
if (
this.pendingDocumentUpdates.get(key) === null &&
!this.ensureDocumentUpdateByTarget(key).has(targetId) &&
!this.targetContainsDocument(targetId, key)
) {
this.removeDocumentFromTarget(
Expand Down Expand Up @@ -655,6 +656,7 @@ export class WatchChangeAggregator {
);

this.pendingDocumentUpdates = mutableDocumentMap();
this.pendingDocumentUpdatesByTarget = documentTargetMap();
this.pendingDocumentTargetMapping = documentTargetMap();
this.pendingTargetResets = new SortedMap<TargetId, TargetPurpose>(
primitiveComparator
Expand Down Expand Up @@ -685,6 +687,12 @@ export class WatchChangeAggregator {
document
);

this.pendingDocumentUpdatesByTarget =
this.pendingDocumentUpdatesByTarget.insert(
document.key,
this.ensureDocumentUpdateByTarget(document.key).add(targetId)
);

this.pendingDocumentTargetMapping =
this.pendingDocumentTargetMapping.insert(
document.key,
Expand Down Expand Up @@ -724,6 +732,12 @@ export class WatchChangeAggregator {
this.ensureDocumentTargetMapping(key).delete(targetId)
);

this.pendingDocumentTargetMapping =
this.pendingDocumentTargetMapping.insert(
key,
this.ensureDocumentTargetMapping(key).add(targetId)
);

if (updatedDocument) {
this.pendingDocumentUpdates = this.pendingDocumentUpdates.insert(
key,
Expand Down Expand Up @@ -782,6 +796,18 @@ export class WatchChangeAggregator {
return targetMapping;
}

private ensureDocumentUpdateByTarget(key: DocumentKey): SortedSet<TargetId> {
let targetMapping = this.pendingDocumentUpdatesByTarget.get(key);

if (!targetMapping) {
targetMapping = new SortedSet<TargetId>(primitiveComparator);
this.pendingDocumentUpdatesByTarget =
this.pendingDocumentUpdatesByTarget.insert(key, targetMapping);
}

return targetMapping;
}

/**
* Verifies that the user is still interested in this target (by calling
* `getTargetDataForTarget()`) and that we are not waiting for pending ADDs
Expand Down
280 changes: 280 additions & 0 deletions packages/firestore/test/unit/specs/limbo_spec.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1171,4 +1171,284 @@ describeSpec('Limbo Documents:', [], () => {
);
}
);

specTest(
'Fix #8474 - Limbo resolution for document is removed even if document updates for the document occurred before documentDelete in the global snapshot window',
[],
() => {
// onSnapshot(fullQuery)
const fullQuery = query('collection');

// getDocs(filterQuery)
const filterQuery = query('collection', filter('included', '==', true));

const docA = doc('collection/a', 1000, { key: 'a', included: false });
const docA2 = doc('collection/a', 1007, { key: 'a', included: true });
const docC = doc('collection/c', 1002, { key: 'c', included: true });

const limboQueryC = newQueryForPath(docC.key.path);

return (
spec()
// onSnapshot(fullQuery) - fullQuery is listening to documents in the collection for the full test
.userListens(fullQuery)
.watchAcksFull(fullQuery, 1001, docA, docC)
.expectEvents(fullQuery, {
fromCache: false,
added: [docA, docC]
})

// docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change)
.watchRemovesDoc(docC.key, fullQuery)
.watchCurrents(fullQuery, 'resume-token-1002')
.watchSnapshots(1002)
.expectLimboDocs(docC.key)
.expectEvents(fullQuery, {
fromCache: true
})

// User begins getDocs(filterQuery)
.userListensForGet(filterQuery)

// getDocs(filterQuery) will not resolve on the snapshot from cache
.expectEvents(filterQuery, {
fromCache: true,
added: [docC]
})

// Watch acks limbo and filter queries
.watchAcks(limboQueryC)
.watchAcks(filterQuery)

// Watch responds to limboQueryC - docC was deleted
.watchDeletesDoc(docC.key, 1009, limboQueryC)
.watchCurrents(limboQueryC, 'resume-token-1009')
.watchSnapshots(1009, [limboQueryC, fullQuery])

// However, docC is still in limbo because there has not been a global snapshot
.expectLimboDocs(docC.key)

// Rapid events of document update and delete caused by application
.watchRemovesDoc(docA.key, filterQuery)
.watchCurrents(filterQuery, 'resume-token-1004')
.watchSends({ affects: [filterQuery] }, docC)
.watchCurrents(filterQuery, 'resume-token-1005')
.watchRemovesDoc(docC.key, filterQuery)
.watchSends({ affects: [filterQuery] }, docA2)
.watchCurrents(filterQuery, 'resume-token-1007')

.watchSnapshots(1010, [fullQuery, limboQueryC])

// All changes are current and we get a global snapshot
.watchSnapshots(1010, [])

// Now docC is out of limbo
.expectLimboDocs()
.expectEvents(fullQuery, {
fromCache: false,
modified: [docA2],
removed: [docC]
})
// Now getDocs(filterQuery) can be resolved
.expectEvents(filterQuery, {
fromCache: false,
removed: [docC],
added: [docA2]
})

// No more expected events
.watchSnapshots(1100, [])
);
}
);

specTest(
'Fix #8474 - Limbo resolution for document is removed even if document updates for the document occurred in the global snapshot window and no document delete was received for the limbo resolution query',
[],
() => {
// onSnapshot(fullQuery)
const fullQuery = query('collection');

// getDocs(filterQuery)
const filterQuery = query('collection', filter('included', '==', true));

const docA = doc('collection/a', 1000, { key: 'a', included: false });
const docA2 = doc('collection/a', 1007, { key: 'a', included: true });
const docC = doc('collection/c', 1002, { key: 'c', included: true });

const limboQueryC = newQueryForPath(docC.key.path);

return (
spec()
// onSnapshot(fullQuery) - fullQuery is listening to documents in the collection for the full test
.userListens(fullQuery)
.watchAcksFull(fullQuery, 1001, docA, docC)
.expectEvents(fullQuery, {
fromCache: false,
added: [docA, docC]
})

// docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change)
.watchRemovesDoc(docC.key, fullQuery)
.watchCurrents(fullQuery, 'resume-token-1002')
.watchSnapshots(1002)
.expectLimboDocs(docC.key)
.expectEvents(fullQuery, {
fromCache: true
})

// User begins getDocs(filterQuery)
.userListensForGet(filterQuery)

// getDocs(filterQuery) will not resolve on the snapshot from cache
.expectEvents(filterQuery, {
fromCache: true,
added: [docC]
})

// Watch acks limbo and filter queries
.watchAcks(limboQueryC)
.watchAcks(filterQuery)

// Watch currents the limbo query, but did not send a document delete.
// This is and unexpected code path, but something that is called
// out as possible in the watch change aggregator.
.watchCurrents(limboQueryC, 'resume-token-1009')
.watchSnapshots(1009, [limboQueryC, fullQuery])

// However, docC is still in limbo because there has not been a global snapshot
.expectLimboDocs(docC.key)

// Rapid events of document update and delete caused by application
.watchRemovesDoc(docA.key, filterQuery)
.watchCurrents(filterQuery, 'resume-token-1004')
.watchSends({ affects: [filterQuery] }, docC)
.watchCurrents(filterQuery, 'resume-token-1005')
.watchRemovesDoc(docC.key, filterQuery)
.watchSends({ affects: [filterQuery] }, docA2)
.watchCurrents(filterQuery, 'resume-token-1007')

.watchSnapshots(1010, [fullQuery, limboQueryC])

// All changes are current and we get a global snapshot
.watchSnapshots(1010, [])

// Now docC is out of limbo
.expectLimboDocs()
.expectEvents(fullQuery, {
fromCache: false,
modified: [docA2],
removed: [docC]
})
// Now getDocs(filterQuery) can be resolved
.expectEvents(filterQuery, {
fromCache: false,
removed: [docC],
added: [docA2]
})

// No more expected events
.watchSnapshots(1100, [])
);
}
);

specTest(
'Fix #8474 - Handles code path of no ack for limbo resolution query before global snapshot',
[],
() => {
// onSnapshot(fullQuery)
const fullQuery = query('collection');

// getDocs(filterQuery)
const filterQuery = query('collection', filter('included', '==', true));

const docA = doc('collection/a', 1000, { key: 'a', included: false });
const docA2 = doc('collection/a', 1007, { key: 'a', included: true });
const docC = doc('collection/c', 1002, { key: 'c', included: true });

const limboQueryC = newQueryForPath(docC.key.path);

return (
spec()
// onSnapshot(fullQuery) - fullQuery is listening to documents in the collection for the full test
.userListens(fullQuery)
.watchAcksFull(fullQuery, 1001, docA, docC)
.expectEvents(fullQuery, {
fromCache: false,
added: [docA, docC]
})

// docC was deleted, this puts docC in limbo and causes a snapshot from cache (metadata-only change)
.watchRemovesDoc(docC.key, fullQuery)
.watchCurrents(fullQuery, 'resume-token-1002')
.watchSnapshots(1002)
.expectLimboDocs(docC.key)
.expectEvents(fullQuery, {
fromCache: true
})

// User begins getDocs(filterQuery)
.userListensForGet(filterQuery)

// getDocs(filterQuery) will not resolve on the snapshot from cache
.expectEvents(filterQuery, {
fromCache: true,
added: [docC]
})

// Watch filter query
.watchAcks(filterQuery)

// However, docC is still in limbo because there has not been a global snapshot
.expectLimboDocs(docC.key)

// Rapid events of document update and delete caused by application
.watchRemovesDoc(docA.key, filterQuery)
.watchCurrents(filterQuery, 'resume-token-1004')
.watchSends({ affects: [filterQuery] }, docC)
.watchCurrents(filterQuery, 'resume-token-1005')
.watchRemovesDoc(docC.key, filterQuery)
.watchSends({ affects: [filterQuery] }, docA2)
.watchCurrents(filterQuery, 'resume-token-1007')

.watchSnapshots(1010, [fullQuery, limboQueryC])

// All changes are current and we get a global snapshot
.watchSnapshots(1010, [])

.expectEvents(fullQuery, {
fromCache: true,
modified: [docA2]
})
// Now getDocs(filterQuery) can be resolved
.expectEvents(filterQuery, {
fromCache: true,
added: [docA2]
})

// Watch acks limbo query
.watchAcks(limboQueryC)

// Watch responds to limboQueryC - docC was deleted
.watchDeletesDoc(docC.key, 1009, limboQueryC)
.watchCurrents(limboQueryC, 'resume-token-1009')
.watchSnapshots(1100, [limboQueryC])

// No more expected events
.watchSnapshots(1101, [])

.expectLimboDocs()
.expectEvents(fullQuery, {
fromCache: false,
removed: [docC]
})
// Now getDocs(filterQuery) can be resolved
.expectEvents(filterQuery, {
fromCache: false,
removed: [docC]
})
);
}
);
});
Loading

0 comments on commit 0f5714b

Please sign in to comment.