b/72533250: Fix issue with limbo resolutions triggering incorrect manufactured deletes. #1014

mikelehen · 2018-07-17T17:37:41Z

NOTE: This is a minimal fix for the issue we discussed; I think a cleaner fix is to move View.syncedDocuments out of the view. I'll poke at this but it'll probably take longer and be a bit more risky

This fixes an issue occurring when a limbo target receives a documentUpdate,
then a global snapshot, and then a CURRENT. Because there was a global
snapshot before the CURRENT, WatchChangeAggregator has no pending document
updates and calls SyncEngine.targetContainsDocument() to see if we got any
document from the backend for the target. See:

firebase-js-sdk/packages/firestore/src/remote/watch_change.ts

Line 422 in 6905339

!this.targetContainsDocument(targetId, key)

Prior to this change, targetContainsDocument() returned false because
it relies on our Views to track the contents of the target, and we don't
have Views for limbo targets. Thus WatchChangeAggregator incorrectly
manufactures a NoDocument document update which deletes data from our
cache.

The fix is to have SyncEngine track the fact that we did indeed get
a document for the limbo resolution and return true from
targetContainsDocument().

…ufactured deletes. This fixes an issue occurring when a limbo target receives a documentUpdate, then a global snapshot, and then a CURRENT. Because there was a global snapshot before the CURRENT, WatchChangeAggregator has no pending document updates and calls SyncEngine.targetContainsDocument() to see if we got any document from the backend for the target. See: https://github.com/firebase/firebase-js-sdk/blob/6905339235ad801291edc696dd75a08e80647f5b/packages/firestore/src/remote/watch_change.ts#L422 Prior to this change, targetContainsDocument() returned false because it relies on our Views to track the contents of the target, and we don't have Views for limbo targets. Thus WatchChangeAggregator incorrectly manufactures a NoDocument document update which deletes data from our cache. The fix is to have SyncEngine track the fact that we did indeed get a document for the limbo resolution and return true from targetContainsDocument().

wilhuff · 2018-07-17T17:59:49Z

packages/firestore/src/core/sync_engine.ts

+          limboResolution.receivedDocument = true;
+        } else if (targetChange.removedDocuments.size > 0) {
+          limboResolution.receivedDocument = false;
+        }


Isn't there an else case here too? If we got a snapshot but there's no document at all it won't be added, modified, or removed. How are we supposed to handle that here?

If not, can you add a comment here/assertion about this?

I reworked this code to be a bit more explicit about what's going on and what we're expecting. I think it's kind of gross because normally a targetChange is pretty flexible but we're only expecting very specific changes since this is a single-document lookup.

Let me know if you like this better and/or have suggestions.

schmidt-sebastian · 2018-07-18T00:14:57Z

packages/firestore/src/core/sync_engine.ts

+            limboResolution.receivedDocument,
+            'Received change for limbo target document without add.'
+          );
+        } else if (targetChange.removedDocuments.size > 0) {


Even after a couple second of staring at this, this line matches 335 and hence limboResolution.receivedDocument = false is never getting executed.

Err.... good catch, fixed. TBH I don't expect the modifiedDocuments or removedDocuments cases to ever be hit, but I included them for completeness.

schmidt-sebastian · 2018-07-18T00:14:59Z

packages/firestore/test/unit/specs/limbo_spec.test.ts

+          // Suppose docB was actually deleted server-side and so we receive an
+          // ack, a snapshot, CURRENT, and then another snapshot. This should
+          // resolve the limbo resolution and docB should disappear.
+          .watchAcks(docBQuery)


Is there a reason you are not using ackLimbo()?

I forgot it exists.

It seems to send the document, send current, send snapshot. And these tests are intentionally sending a snapshot before current, since that's the broken case.

wilhuff

LGTM

…ufactured deletes. [Port of firebase/firebase-js-sdk#1014] This fixes an issue occurring when a limbo target receives a documentUpdate, then a global snapshot, and then a CURRENT. Because there was a global snapshot before the CURRENT, WatchChangeAggregator has no pending document updates and calls SyncEngine.remoteKeysForTarget to see if we previously got any document from the backend for the target. See: https://github.com/firebase/firebase-js-sdk/blob/6905339235ad801291edc696dd75a08e80647f5b/packages/firestore/src/remote/watch_change.ts#L422 Prior to this change, remoteKeysForTarget returned empty because it relies on our Views to track the contents of the target, and we don't have Views for limbo targets. Thus WatchChangeAggregator incorrectly manufactures a NoDocument document update which deletes data from our cache. The fix is to have SyncEngine track the fact that we did indeed get a document for the limbo resolution and return it from remoteKeysForTarget.

…ufactured deletes. (#1556) [Port of firebase/firebase-js-sdk#1014] This fixes an issue occurring when a limbo target receives a documentUpdate, then a global snapshot, and then a CURRENT. Because there was a global snapshot before the CURRENT, WatchChangeAggregator has no pending document updates and calls SyncEngine.remoteKeysForTarget to see if we previously got any document from the backend for the target. See: https://github.com/firebase/firebase-js-sdk/blob/6905339235ad801291edc696dd75a08e80647f5b/packages/firestore/src/remote/watch_change.ts#L422 Prior to this change, remoteKeysForTarget returned empty because it relies on our Views to track the contents of the target, and we don't have Views for limbo targets. Thus WatchChangeAggregator incorrectly manufactures a NoDocument document update which deletes data from our cache. The fix is to have SyncEngine track the fact that we did indeed get a document for the limbo resolution and return it from remoteKeysForTarget.

@davideast

* Add @davideast as a CODEOWNER (#996) * Embed metadata directly into the RPC call (#979) * Embed metadata directly into the RPC call * [AUTOMATED]: Prettier Code Styling * Use ...args * [AUTOMATED]: Prettier Code Styling * Minimize diff * Add the OAuth assertion back in * Added missing type for optional database url. (#1001) * RxFire Realtime Database (#997) * initial database code * test setup * database tests * auditTrail and database tests * [AUTOMATED]: Prettier Code Styling * [AUTOMATED]: License Headers * Josh's comments. Database docs * [AUTOMATED]: Prettier Code Styling * Firestore docs * auth docs * declaration fixes * switch to peerDeps * [AUTOMATED]: Prettier Code Styling * test config * Expose array transforms and array contains queries. (#1004) Also remove test code that was combining multiple array contains queries since those were disallowed in 04c9c3a. * Move fieldFilter (free function) to Filter.create() (#988) This is a refactoring to unify filter creation across platforms. * Enable firestore sdk to talk to emulator (#1007) * Enable firestore sdk to talk to emulator * [AUTOMATED]: Prettier Code Styling * Revert firestore sdk changes * [AUTOMATED]: Prettier Code Styling * Revert credentials.ts * Cleanup * [AUTOMATED]: Prettier Code Styling * Set webSafe=false * Combine initializeTestApp and initializeFirestoreTestApp * [AUTOMATED]: Prettier Code Styling * Cleanup * [AUTOMATED]: Prettier Code Styling * Update major version since this is a breaking change that will cause the testing sdk to no longer work with old versions of the RTDB emulator * Completely remove admin sdk * Change version back to 0.1.0 * Setting GarbageSource in SyncEngine's constructor (#1010) * b/72533250: Fix issue with limbo resolutions triggering incorrect manufactured deletes. (#1014) This fixes an issue occurring when a limbo target receives a documentUpdate, then a global snapshot, and then a CURRENT. Because there was a global snapshot before the CURRENT, WatchChangeAggregator has no pending document updates and calls SyncEngine.targetContainsDocument() to see if we previously got any document from the backend for the target. See: https://github.com/firebase/firebase-js-sdk/blob/6905339235ad801291edc696dd75a08e80647f5b/packages/firestore/src/remote/watch_change.ts#L422 Prior to this change, targetContainsDocument() returned false because it relies on our Views to track the contents of the target, and we don't have Views for limbo targets. Thus WatchChangeAggregator incorrectly manufactures a NoDocument document update which deletes data from our cache. The fix is to have SyncEngine track the fact that we did indeed get a document for the limbo resolution and return true from targetContainsDocument(). * Updating yarn.lock * Add @firebase/util as a dep of @firebase/testing * Allow remote updates from watch to heal a cache with synthesized deletes in it (#1015) * Write a spec test for the busted cache * Modify spec test to demonstrate deletedDoc issue. (#1017) * Allow updates for targets where the document is modified * Fix getRemoteKeysForTarget() method name in comment. (#1020) While porting I noticed this was slightly wrong. targetContainsDocument() is the method in WatchChangeAggregator. The SyncEngine method I meant to reference is getRemoteKeysForTarget(). * Making sure we don't export 'experimental' (#1023) * Add a schema migration that drops the query cache (#1019) * Add a schema migration that drops the query cache This is a force fix for potential existence filter mismatches caused by firebase/firebase-ios-sdk#1548 The essential problem is that when resuming a query, the server is allowed to forget deletes. If the number of incorrectly synthesized deletes matches the number of server-forgotten deletes then the existence filter can give a false positive, preventing the cache from self healing. Dropping the query cache clears any client-side resume token which prevents a false positive existence filter mismatch. Note that the remote document cache and mutation queues are unaffected so any cached documents that do exist will still work while offline. * Implement review feedback * Add a release note for the fix to #1548 (#1024) * Ensure that we create an empty TargetGlobal row. (#1029) Ensure the v3 migration unconditionally creates the TargetGlobal row. Remove the no-longer-necessary v2 schema migration. * Remove unnecessary `any` (#1030) * Fix an errant any usage * [AUTOMATED]: Prettier Code Styling * Publish [email protected] * Unify local.QueryData with the other platforms (#1027) This makes it line up with it's own docs, and also the other platforms. * Fix to #1027 to allow SnapshotVersion == 0 (#1033) * Add iat to fake access token payload (#1022) * Add iat to fake access token payload * [AUTOMATED]: Prettier Code Styling * Simpler tests * [AUTOMATED]: Prettier Code Styling * Do not clobber iat * catch server error RESET_PASSWORD_EXCEED_LIMIT (#1037) * Merging Master into Multi-Tab (#1038)

mikelehen added bug api: firestore labels Jul 17, 2018

mikelehen assigned wilhuff Jul 17, 2018

mikelehen requested a review from wilhuff July 17, 2018 17:37

mikelehen requested a review from schmidt-sebastian as a code owner July 17, 2018 17:37

google-oss-bot added the needs-triage label Jul 17, 2018

wilhuff reviewed Jul 17, 2018

View reviewed changes

CR feedback and additional test.

0f11a3a

schmidt-sebastian reviewed Jul 18, 2018

View reviewed changes

typo

ebb450b

wilhuff mentioned this pull request Jul 18, 2018

Firestore: Deleting a couple of documents returned by a limited subcollection query triggers all documents to disappear locally firebase/firebase-ios-sdk#1548

Closed

wilhuff approved these changes Jul 18, 2018

View reviewed changes

wilhuff removed the needs-triage label Jul 18, 2018

wilhuff assigned mikelehen and unassigned wilhuff Jul 18, 2018

mikelehen merged commit 5c338d1 into master Jul 18, 2018

mikelehen deleted the mikelehen/limbo-deletes branch July 18, 2018 17:33

mikelehen mentioned this pull request Jul 18, 2018

b/72533250: Fix issue with limbo resolutions triggering incorrect manufactured deletes. firebase/firebase-ios-sdk#1556

Merged

firebase locked and limited conversation to collaborators Oct 18, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

b/72533250: Fix issue with limbo resolutions triggering incorrect manufactured deletes. #1014

b/72533250: Fix issue with limbo resolutions triggering incorrect manufactured deletes. #1014

mikelehen commented Jul 17, 2018

wilhuff Jul 17, 2018

mikelehen Jul 17, 2018

schmidt-sebastian Jul 18, 2018

mikelehen Jul 18, 2018

schmidt-sebastian Jul 18, 2018

mikelehen Jul 18, 2018

wilhuff left a comment

b/72533250: Fix issue with limbo resolutions triggering incorrect manufactured deletes. #1014

b/72533250: Fix issue with limbo resolutions triggering incorrect manufactured deletes. #1014

Conversation

mikelehen commented Jul 17, 2018

wilhuff Jul 17, 2018

Choose a reason for hiding this comment

mikelehen Jul 17, 2018

Choose a reason for hiding this comment

schmidt-sebastian Jul 18, 2018

Choose a reason for hiding this comment

mikelehen Jul 18, 2018

Choose a reason for hiding this comment

schmidt-sebastian Jul 18, 2018

Choose a reason for hiding this comment

mikelehen Jul 18, 2018

Choose a reason for hiding this comment

wilhuff left a comment

Choose a reason for hiding this comment