From 0f5714ba5baab119a73355c0fd86db5a44cd3d20 Mon Sep 17 00:00:00 2001 From: Mark Duckworth <1124037+MarkDuckworth@users.noreply.github.com> Date: Wed, 30 Oct 2024 10:05:19 -0600 Subject: [PATCH 1/4] Fix for 8474 - Prevent a possible condition of slow snapshots, caused by a rapid series of document update(s) followed by a delete. (#8595) --- .changeset/tasty-boxes-brake.md | 5 + packages/firestore/src/remote/watch_change.ts | 34 ++- .../test/unit/specs/limbo_spec.test.ts | 280 ++++++++++++++++++ .../firestore/test/unit/specs/spec_builder.ts | 28 +- packages/firestore/test/util/helpers.ts | 13 +- 5 files changed, 352 insertions(+), 8 deletions(-) create mode 100644 .changeset/tasty-boxes-brake.md diff --git a/.changeset/tasty-boxes-brake.md b/.changeset/tasty-boxes-brake.md new file mode 100644 index 00000000000..beb8b626f36 --- /dev/null +++ b/.changeset/tasty-boxes-brake.md @@ -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. diff --git a/packages/firestore/src/remote/watch_change.ts b/packages/firestore/src/remote/watch_change.ts index 38e10a23e35..0c69163095f 100644 --- a/packages/firestore/src/remote/watch_change.ts +++ b/packages/firestore/src/remote/watch_change.ts @@ -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(); @@ -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( @@ -655,6 +656,7 @@ export class WatchChangeAggregator { ); this.pendingDocumentUpdates = mutableDocumentMap(); + this.pendingDocumentUpdatesByTarget = documentTargetMap(); this.pendingDocumentTargetMapping = documentTargetMap(); this.pendingTargetResets = new SortedMap( primitiveComparator @@ -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, @@ -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, @@ -782,6 +796,18 @@ export class WatchChangeAggregator { return targetMapping; } + private ensureDocumentUpdateByTarget(key: DocumentKey): SortedSet { + let targetMapping = this.pendingDocumentUpdatesByTarget.get(key); + + if (!targetMapping) { + targetMapping = new SortedSet(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 diff --git a/packages/firestore/test/unit/specs/limbo_spec.test.ts b/packages/firestore/test/unit/specs/limbo_spec.test.ts index 0a4052cc72b..f6043a7fc9b 100644 --- a/packages/firestore/test/unit/specs/limbo_spec.test.ts +++ b/packages/firestore/test/unit/specs/limbo_spec.test.ts @@ -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] + }) + ); + } + ); }); diff --git a/packages/firestore/test/unit/specs/spec_builder.ts b/packages/firestore/test/unit/specs/spec_builder.ts index d79cca9cd82..80dcd6519de 100644 --- a/packages/firestore/test/unit/specs/spec_builder.ts +++ b/packages/firestore/test/unit/specs/spec_builder.ts @@ -51,7 +51,7 @@ import { forEach } from '../../../src/util/obj'; import { ObjectMap } from '../../../src/util/obj_map'; import { isNullOrUndefined } from '../../../src/util/types'; import { firestore } from '../../util/api_helpers'; -import { TestSnapshotVersion } from '../../util/helpers'; +import { deletedDoc, TestSnapshotVersion } from '../../util/helpers'; import { RpcError } from './spec_rpc_error'; import { @@ -315,6 +315,15 @@ export class SpecBuilder { return this; } + /** Listen to query using the same options as executing a getDoc or getDocs */ + userListensForGet(query: Query, resume?: ResumeSpec): this { + this.addUserListenStep(query, resume, { + includeMetadataChanges: true, + waitForSyncWhenOnline: true + }); + return this; + } + userListensToCache(query: Query, resume?: ResumeSpec): this { this.addUserListenStep(query, resume, { source: Source.Cache }); return this; @@ -821,6 +830,23 @@ export class SpecBuilder { return this; } + watchDeletesDoc( + key: DocumentKey, + version: TestSnapshotVersion, + ...targets: Query[] + ): this { + this.nextStep(); + this.currentStep = { + watchEntity: { + doc: SpecBuilder.docToSpec( + deletedDoc(key.path.canonicalString(), version) + ), + removedTargets: targets.map(query => this.getTargetId(query)) + } + }; + return this; + } + watchFilters( queries: Query[], docs: DocumentKey[] = [], diff --git a/packages/firestore/test/util/helpers.ts b/packages/firestore/test/util/helpers.ts index dded004ebfd..c5865c3e0f7 100644 --- a/packages/firestore/test/util/helpers.ts +++ b/packages/firestore/test/util/helpers.ts @@ -172,12 +172,19 @@ export function doc( } export function deletedDoc( - keyStr: string, + keyStrOrDocumentKey: string | DocumentKey, ver: TestSnapshotVersion ): MutableDocument { - return MutableDocument.newNoDocument(key(keyStr), version(ver)).setReadTime( + if ( + keyStrOrDocumentKey instanceof String || + typeof keyStrOrDocumentKey === 'string' + ) { + keyStrOrDocumentKey = key(keyStrOrDocumentKey as string); + } + return MutableDocument.newNoDocument( + keyStrOrDocumentKey, version(ver) - ); + ).setReadTime(version(ver)); } export function unknownDoc( From caf30900795c577be7f8db3c7437ee3f3992aee6 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Wed, 30 Oct 2024 15:12:32 -0400 Subject: [PATCH 2/4] Upgrade ts-node to 10.19.2 (#8531) --- package.json | 2 +- packages/firestore-compat/package.json | 2 +- packages/firestore/package.json | 2 +- yarn.lock | 8 ++++---- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/package.json b/package.json index 147ef5e7b24..62fe45a3256 100644 --- a/package.json +++ b/package.json @@ -152,7 +152,7 @@ "sqlite3": "5.1.6", "terser": "5.16.1", "ts-loader": "9.5.1", - "ts-node": "10.9.1", + "ts-node": "10.9.2", "tsec": "0.2.8", "tslint": "6.1.3", "typedoc": "0.16.11", diff --git a/packages/firestore-compat/package.json b/packages/firestore-compat/package.json index 9734a26a650..5015eea131d 100644 --- a/packages/firestore-compat/package.json +++ b/packages/firestore-compat/package.json @@ -60,7 +60,7 @@ "rollup-plugin-terser": "7.0.2", "rollup-plugin-typescript2": "0.31.2", "@rollup/plugin-node-resolve": "13.3.0", - "ts-node": "10.9.1", + "ts-node": "10.9.2", "typescript": "5.5.4" }, "license": "Apache-2.0", diff --git a/packages/firestore/package.json b/packages/firestore/package.json index f1a3462b5f7..896aaa55d6e 100644 --- a/packages/firestore/package.json +++ b/packages/firestore/package.json @@ -124,7 +124,7 @@ "rollup-plugin-terser": "7.0.2", "rollup-plugin-typescript2": "0.31.2", "rollup-plugin-dts": "5.3.1", - "ts-node": "10.9.1", + "ts-node": "10.9.2", "typescript": "5.5.4" }, "repository": { diff --git a/yarn.lock b/yarn.lock index 03539a92d43..bd59a038213 100644 --- a/yarn.lock +++ b/yarn.lock @@ -17002,10 +17002,10 @@ ts-loader@9.5.1: semver "^7.3.4" source-map "^0.7.4" -ts-node@10.9.1: - version "10.9.1" - resolved "https://registry.npmjs.org/ts-node/-/ts-node-10.9.1.tgz" - integrity sha512-NtVysVPkxxrwFGUUxGYhfux8k78pQB3JqYBXlLRZgdGUqTO5wU/UyHop5p70iEbGhB7q5KmiZiU0Y3KlJrScEw== +ts-node@10.9.2: + version "10.9.2" + resolved "https://registry.npmjs.org/ts-node/-/ts-node-10.9.2.tgz#70f021c9e185bccdca820e26dc413805c101c71f" + integrity sha512-f0FFpIdcHgn8zcPSbf1dRevwt047YMnaiJM3u2w2RewrB+fob/zePZcrOyQoLMMO7aBIddLcQIEK5dYjkLnGrQ== dependencies: "@cspotcode/source-map-support" "^0.8.0" "@tsconfig/node10" "^1.0.7" From 274e9a56e92a0efc0cd755cb9ea332277b4ec843 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Tue, 5 Nov 2024 17:25:54 -0500 Subject: [PATCH 3/4] Remove unused `convertPropertiesForEnclosingClass" (#8618) --- repo-scripts/prune-dts/prune-dts.ts | 55 ----------------------------- 1 file changed, 55 deletions(-) diff --git a/repo-scripts/prune-dts/prune-dts.ts b/repo-scripts/prune-dts/prune-dts.ts index 1654e0fa6cc..70cfc2933bb 100644 --- a/repo-scripts/prune-dts/prune-dts.ts +++ b/repo-scripts/prune-dts/prune-dts.ts @@ -268,61 +268,6 @@ function prunePrivateImports< } } -/** - * Iterates over the provided symbols and returns named declarations for these - * symbols if they are missing from `currentClass`. This allows us to merge - * class hierarchies for classes whose inherited types are not part of the - * public API. - * - * This method relies on a private API in TypeScript's `codefix` package. - */ -function convertPropertiesForEnclosingClass( - program: ts.Program, - host: ts.CompilerHost, - sourceFile: ts.SourceFile, - parentClassSymbols: ts.Symbol[], - currentClass: ts.ClassDeclaration -): ts.ClassElement[] { - const newMembers: ts.ClassElement[] = []; - // The `codefix` package is not public but it does exactly what we want. It's - // the same package that is used by VSCode to fill in missing members, which - // is what we are using it for in this script. `codefix` handles missing - // properties, methods and correctly deduces generics. - // eslint-disable-next-line @typescript-eslint/no-explicit-any - (ts as any).codefix.createMissingMemberNodes( - currentClass, - parentClassSymbols, - sourceFile, - { program, host }, - /* userPreferences= */ {}, - /* importAdder= */ undefined, - (missingMember: ts.ClassElement) => { - const originalSymbol = parentClassSymbols.find( - symbol => - symbol.escapedName == - (missingMember.name as ts.Identifier).escapedText - ); - if (originalSymbol) { - const jsDocComment = extractJSDocComment(originalSymbol, newMembers); - if (jsDocComment) { - ts.setSyntheticLeadingComments(missingMember, [ - { - kind: ts.SyntaxKind.MultiLineCommentTrivia, - text: `*\n${jsDocComment}\n`, - hasTrailingNewLine: true, - pos: -1, - end: -1 - } - ]); - } - - newMembers.push(missingMember); - } - } - ); - return newMembers; -} - /** * Iterates over the provided symbols and returns named declarations for these * symbols if they are missing from `currentInterface`. This allows us to merge From e3e20784503347ba5cab1ead3a7b73df87125cb4 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Wed, 6 Nov 2024 09:57:57 -0500 Subject: [PATCH 4/4] Consolidate CI test output into a single string (#8489) * Consolidate CI test output into a single string Having CI test process stdout and stderr in a single string makes it easier to read when looking through failures, since you can see the test output alongside the error messages. Without this, any stderr output written during a test will be captured seperately from the test output, so when we then log it after a test failure, we can't tell which test logged which errors. * Prefix stdout and stderr output --- scripts/run_tests_in_ci.js | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/scripts/run_tests_in_ci.js b/scripts/run_tests_in_ci.js index 96f85979165..0cf6a7f5d3c 100644 --- a/scripts/run_tests_in_ci.js +++ b/scripts/run_tests_in_ci.js @@ -61,8 +61,7 @@ const argv = yargs.options({ const dir = path.resolve(myPath); const { name } = require(`${dir}/package.json`); - let stdout = ''; - let stderr = ''; + let testProcessOutput = ''; try { if (process.env?.BROWSERS) { for (const package in crossBrowserPackages) { @@ -74,19 +73,18 @@ const argv = yargs.options({ const testProcess = spawn('yarn', ['--cwd', dir, scriptName]); testProcess.childProcess.stdout.on('data', data => { - stdout += data.toString(); + testProcessOutput += '[stdout]' + data.toString(); }); testProcess.childProcess.stderr.on('data', data => { - stderr += data.toString(); + testProcessOutput += '[stderr]' + data.toString(); }); await testProcess; console.log('Success: ' + name); - writeLogs('Success', name, stdout + '\n' + stderr); + writeLogs('Success', name, testProcessOutput); } catch (e) { console.error('Failure: ' + name); - console.log(stdout); - console.error(stderr); + console.error(testProcessOutput); if (process.env.CHROME_VERSION_NOTES) { console.error(); @@ -94,7 +92,7 @@ const argv = yargs.options({ console.error(); } - writeLogs('Failure', name, stdout + '\n' + stderr); + writeLogs('Failure', name, testProcessOutput); process.exit(1); }