From 6b9078d9c1a1d4df0c7d184d146edc8eb0f0fef5 Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Wed, 31 Mar 2021 20:27:59 +0700 Subject: [PATCH 01/11] add failing test --- .../src/redux/__tests__/run-fast-filters.js | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/packages/gatsby/src/redux/__tests__/run-fast-filters.js b/packages/gatsby/src/redux/__tests__/run-fast-filters.js index 7cf0c2bbb5650..07bee61b5b09d 100644 --- a/packages/gatsby/src/redux/__tests__/run-fast-filters.js +++ b/packages/gatsby/src/redux/__tests__/run-fast-filters.js @@ -459,3 +459,56 @@ describe(`applyFastFilters`, () => { expect(result.length).toBe(2) }) }) + +describe(`edge cases (yay)`, () => { + beforeAll(() => { + store.dispatch({ type: `DELETE_CACHE` }) + mockNodes().forEach(node => + actions.createNode(node, { name: `test` })(store.dispatch) + ) + }) + + it(`throws when node counters are messed up`, () => { + const filter = { + slog: { $eq: `def` }, + deep: { flat: { search: { chain: { $eq: 500 } } } }, + } + + const result = applyFastFilters( + createDbQueriesFromObject(filter), + [typeName], + new Map() + ) + + // Sanity-check + expect(result.length).toEqual(1) + expect(result[0].id).toEqual(`id_2`) + + // Simulating first node creation after process restart. + // It will get counter === 0 that will mess up fast filter results intersection + const badNode = { + id: `bad-node`, + deep: { flat: { search: { chain: 500 } } }, + internal: { + type: typeName, + contentDigest: `bad-node`, + counter: 0, + }, + } + store.dispatch({ + type: `CREATE_NODE`, + payload: badNode, + }) + + const result2 = applyFastFilters( + createDbQueriesFromObject(filter), + [typeName], + new Map() + ) + + // Sanity-check + expect(result2).toEqual([]) + expect(result2.length).toEqual(1) + expect(result2[0].id).toEqual(`id_2`) + }) +}) From 253d5dbc85ecc6ced22ff276b59a7cb8492d88b8 Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Wed, 31 Mar 2021 20:55:14 +0700 Subject: [PATCH 02/11] actually failing test --- .../src/redux/__tests__/run-fast-filters.js | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/packages/gatsby/src/redux/__tests__/run-fast-filters.js b/packages/gatsby/src/redux/__tests__/run-fast-filters.js index 07bee61b5b09d..6aa6e91bfe698 100644 --- a/packages/gatsby/src/redux/__tests__/run-fast-filters.js +++ b/packages/gatsby/src/redux/__tests__/run-fast-filters.js @@ -470,8 +470,8 @@ describe(`edge cases (yay)`, () => { it(`throws when node counters are messed up`, () => { const filter = { - slog: { $eq: `def` }, - deep: { flat: { search: { chain: { $eq: 500 } } } }, + slog: { $eq: `def` }, // matches id_2 and id_4 + deep: { flat: { search: { chain: { $eq: 500 } } } }, // matches id_2 } const result = applyFastFilters( @@ -484,15 +484,17 @@ describe(`edge cases (yay)`, () => { expect(result.length).toEqual(1) expect(result[0].id).toEqual(`id_2`) - // Simulating first node creation after process restart. - // It will get counter === 0 that will mess up fast filter results intersection + // After process restart node.internal.counter is reset and conflicts with counters from the previous run + // in some situations this leads to incorrect intersection of filtered results. + // Below we set node.internal.counter to 4 which conflicts with existing node id_4 and leads + // to bad intersection of filtered results const badNode = { id: `bad-node`, deep: { flat: { search: { chain: 500 } } }, internal: { type: typeName, contentDigest: `bad-node`, - counter: 0, + counter: 4, }, } store.dispatch({ @@ -505,9 +507,6 @@ describe(`edge cases (yay)`, () => { [typeName], new Map() ) - - // Sanity-check - expect(result2).toEqual([]) expect(result2.length).toEqual(1) expect(result2[0].id).toEqual(`id_2`) }) From 042d9f76c8e95529240150c674aff482c9a9dd6e Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Wed, 31 Mar 2021 21:06:09 +0700 Subject: [PATCH 03/11] make test independent of other tests --- packages/gatsby/src/redux/__tests__/run-fast-filters.js | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/packages/gatsby/src/redux/__tests__/run-fast-filters.js b/packages/gatsby/src/redux/__tests__/run-fast-filters.js index 6aa6e91bfe698..1ba7235983ba8 100644 --- a/packages/gatsby/src/redux/__tests__/run-fast-filters.js +++ b/packages/gatsby/src/redux/__tests__/run-fast-filters.js @@ -3,6 +3,7 @@ const { applyFastFilters, } = require(`../run-fast-filters`) const { store } = require(`../index`) +const { getNode } = require(`../nodes`) const { createDbQueriesFromObject } = require(`../../db/common/query`) const { actions } = require(`../actions`) const { @@ -494,7 +495,7 @@ describe(`edge cases (yay)`, () => { internal: { type: typeName, contentDigest: `bad-node`, - counter: 4, + counter: getNode(`id_4`).internal.counter, }, } store.dispatch({ From fa69c7ae93a2a0ae9692d64839d2bceb020c1a49 Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Wed, 31 Mar 2021 23:41:33 +0700 Subject: [PATCH 04/11] add invariant for filter intersection assumptions --- .../gatsby/src/redux/__tests__/run-fast-filters.js | 11 +++++------ packages/gatsby/src/redux/nodes.ts | 5 +++++ 2 files changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/gatsby/src/redux/__tests__/run-fast-filters.js b/packages/gatsby/src/redux/__tests__/run-fast-filters.js index 1ba7235983ba8..1ec661e9e8744 100644 --- a/packages/gatsby/src/redux/__tests__/run-fast-filters.js +++ b/packages/gatsby/src/redux/__tests__/run-fast-filters.js @@ -503,12 +503,11 @@ describe(`edge cases (yay)`, () => { payload: badNode, }) - const result2 = applyFastFilters( - createDbQueriesFromObject(filter), - [typeName], - new Map() + const run = () => + applyFastFilters(createDbQueriesFromObject(filter), [typeName], new Map()) + + expect(run).toThrow( + `Invariant violation: inconsistent node counters detected` ) - expect(result2.length).toEqual(1) - expect(result2[0].id).toEqual(`id_2`) }) }) diff --git a/packages/gatsby/src/redux/nodes.ts b/packages/gatsby/src/redux/nodes.ts index fb19a2f2d9ea5..0af81b8e5c214 100644 --- a/packages/gatsby/src/redux/nodes.ts +++ b/packages/gatsby/src/redux/nodes.ts @@ -1104,6 +1104,11 @@ export function intersectNodesByCounter( } else if (counterA > counterB) { pointerB++ } else { + if (nodeA !== nodeB) { + throw new Error( + `Invariant violation: inconsistent node counters detected` + ) + } // nodeA===nodeB. Make sure we didn't just add this node already. // Since input arrays are sorted, the same node should be grouped // back to back, so even if both input arrays contained the same node From 0860f4c47cf1a0f4bc48aa70d49362d035cd9d60 Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Thu, 1 Apr 2021 03:07:28 +0700 Subject: [PATCH 05/11] add failing integration test --- .../artifacts/__tests__/index.js | 32 +++++++++++++++++++ integration-tests/artifacts/gatsby-node.js | 15 ++++++++- 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/integration-tests/artifacts/__tests__/index.js b/integration-tests/artifacts/__tests__/index.js index 69a7429fc7bfa..b5618b612b6d9 100644 --- a/integration-tests/artifacts/__tests__/index.js +++ b/integration-tests/artifacts/__tests__/index.js @@ -232,6 +232,24 @@ function assertHTMLCorrectness(runNumber) { }) } +function assertNodeCorrectness(runNumber) { + describe(`node correctness`, () => { + it(`nodes do not have repeating counters`, () => { + const seenCounters = new Map() + const duplicates = {} + // Just a convenience step to display node ids with duplicate counters + manifest[runNumber].allNodeCounters.forEach(([id, counter]) => { + if (seenCounters.has(counter)) { + duplicates.push({ counter, nodeIds: [id, seenCounters.get(counter)] }) + } + seenCounters.set(counter, id) + }) + expect(manifest[runNumber].allNodeCounters.length).toBeGreaterThan(0) + expect(duplicates).toEqual([]) + }) + }) +} + beforeAll(done => { fs.removeSync(path.join(__dirname, `__debug__`)) @@ -454,6 +472,8 @@ describe(`First run (baseline)`, () => { assertWebpackBundleChanges({ browser: true, ssr: true, runNumber }) assertHTMLCorrectness(runNumber) + + assertNodeCorrectness(runNumber) }) describe(`Second run (different pages created, data changed)`, () => { @@ -541,6 +561,8 @@ describe(`Second run (different pages created, data changed)`, () => { assertWebpackBundleChanges({ browser: false, ssr: false, runNumber }) assertHTMLCorrectness(runNumber) + + assertNodeCorrectness(runNumber) }) describe(`Third run (js change, all pages are recreated)`, () => { @@ -632,6 +654,8 @@ describe(`Third run (js change, all pages are recreated)`, () => { assertWebpackBundleChanges({ browser: true, ssr: true, runNumber }) assertHTMLCorrectness(runNumber) + + assertNodeCorrectness(runNumber) }) describe(`Fourth run (gatsby-browser change - cache get invalidated)`, () => { @@ -718,6 +742,8 @@ describe(`Fourth run (gatsby-browser change - cache get invalidated)`, () => { assertWebpackBundleChanges({ browser: true, ssr: true, runNumber }) assertHTMLCorrectness(runNumber) + + assertNodeCorrectness(runNumber) }) describe(`Fifth run (.cache is deleted but public isn't)`, () => { @@ -792,6 +818,8 @@ describe(`Fifth run (.cache is deleted but public isn't)`, () => { assertWebpackBundleChanges({ browser: true, ssr: true, runNumber }) assertHTMLCorrectness(runNumber) + + assertNodeCorrectness(runNumber) }) describe(`Sixth run (ssr-only change - only ssr compilation hash changes)`, () => { @@ -882,6 +910,8 @@ describe(`Sixth run (ssr-only change - only ssr compilation hash changes)`, () = assertWebpackBundleChanges({ browser: false, ssr: true, runNumber }) assertHTMLCorrectness(runNumber) + + assertNodeCorrectness(runNumber) }) describe(`Seventh run (no change in any file that is bundled, we change untracked file, but previous build used unsafe method so all should rebuild)`, () => { @@ -970,4 +1000,6 @@ describe(`Seventh run (no change in any file that is bundled, we change untracke assertWebpackBundleChanges({ browser: false, ssr: false, runNumber }) assertHTMLCorrectness(runNumber) + + assertNodeCorrectness(runNumber) }) diff --git a/integration-tests/artifacts/gatsby-node.js b/integration-tests/artifacts/gatsby-node.js index be1fb7fed430a..415f2f0cbf5e6 100644 --- a/integration-tests/artifacts/gatsby-node.js +++ b/integration-tests/artifacts/gatsby-node.js @@ -38,6 +38,7 @@ exports.sourceNodes = ({ createContentDigest, webhookBody, reporter, + getNode, }) => { if (webhookBody && webhookBody.runNumber) { runNumber = webhookBody.runNumber @@ -115,6 +116,17 @@ exports.sourceNodes = ({ label: `This is${isFirstRun ? `` : ` not`} a first run`, // this will be queried - we want to invalidate html here }) + for (let prevRun = 1; prevRun < runNumber; prevRun++) { + const node = getNode(`node-created-in-run-${prevRun}`) + if (node) { + actions.touchNode(node) + } + } + createNodeHelper(`NodeCounterTest`, { + id: `node-created-in-run-${runNumber}`, + label: `Node created in run ${runNumber}`, + }) + for (const prevNode of previouslyCreatedNodes.values()) { if (!currentlyCreatedNodes.has(prevNode.id)) { actions.deleteNode({ node: prevNode }) @@ -186,7 +198,7 @@ exports.onPreBuild = () => { } let counter = 1 -exports.onPostBuild = async ({ graphql }) => { +exports.onPostBuild = async ({ graphql, getNodes }) => { console.log(`[test] onPostBuild`) if (!didRemoveTrailingSlashForTestedPage) { @@ -212,6 +224,7 @@ exports.onPostBuild = async ({ graphql }) => { `build-manifest-for-test-${counter++}.json` ), { + allNodeCounters: getNodes().map(node => [node.id, node.internal.counter]), allPages: data.allSitePage.nodes.map(node => node.path), changedBrowserCompilationHash, changedSsrCompilationHash, From 8e2928db917e55f771390d36e58d9894d4d22d5e Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Thu, 1 Apr 2021 03:17:54 +0700 Subject: [PATCH 06/11] =?UTF-8?q?fix=20test=20=F0=9F=A4=A6=E2=80=8D?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- integration-tests/artifacts/__tests__/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/integration-tests/artifacts/__tests__/index.js b/integration-tests/artifacts/__tests__/index.js index b5618b612b6d9..7ffb2a2e807f8 100644 --- a/integration-tests/artifacts/__tests__/index.js +++ b/integration-tests/artifacts/__tests__/index.js @@ -236,7 +236,7 @@ function assertNodeCorrectness(runNumber) { describe(`node correctness`, () => { it(`nodes do not have repeating counters`, () => { const seenCounters = new Map() - const duplicates = {} + const duplicates = [] // Just a convenience step to display node ids with duplicate counters manifest[runNumber].allNodeCounters.forEach(([id, counter]) => { if (seenCounters.has(counter)) { From f1332e6f7839dcccb18e87bfe11b130a067e7bde Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Thu, 1 Apr 2021 03:51:14 +0700 Subject: [PATCH 07/11] actually fix this heisenbug --- packages/gatsby/src/redux/actions/public.js | 17 ++++++++++++----- packages/gatsby/src/redux/reducers/status.ts | 6 ++++++ packages/gatsby/src/redux/types.ts | 1 + 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/packages/gatsby/src/redux/actions/public.js b/packages/gatsby/src/redux/actions/public.js index 4308474ade49c..b8226c0269a15 100644 --- a/packages/gatsby/src/redux/actions/public.js +++ b/packages/gatsby/src/redux/actions/public.js @@ -512,9 +512,17 @@ actions.deleteNode = (node: any, plugin?: Plugin) => { } } -// We add a counter to internal to make sure we maintain insertion order for -// backends that don't do that out of the box -let NODE_COUNTER = 0 +// We add a counter to node.internal for fast comparisons/intersections +// of various node slices. The counter must increase even across builds. +function getNextNodeCounter() { + const lastNodeCounter = store.getState().status.LAST_NODE_COUNTER ?? 0 + if (lastNodeCounter >= Number.MAX_SAFE_INTEGER) { + throw new Error( + `Could not create more nodes. Maximum node count is reached: ${lastNodeCounter}` + ) + } + return lastNodeCounter + 1 +} const typeOwners = {} @@ -614,8 +622,7 @@ const createNode = ( node.internal = {} } - NODE_COUNTER++ - node.internal.counter = NODE_COUNTER + node.internal.counter = getNextNodeCounter() // Ensure the new node has a children array. if (!node.array && !_.isArray(node.children)) { diff --git a/packages/gatsby/src/redux/reducers/status.ts b/packages/gatsby/src/redux/reducers/status.ts index 7913b60d40ea3..f308e3651c5be 100644 --- a/packages/gatsby/src/redux/reducers/status.ts +++ b/packages/gatsby/src/redux/reducers/status.ts @@ -3,6 +3,7 @@ import { ActionsUnion, IGatsbyState } from "../types" const defaultState: IGatsbyState["status"] = { PLUGINS_HASH: ``, + LAST_NODE_COUNTER: 0, plugins: {}, } @@ -42,6 +43,11 @@ export const statusReducer = ( ), }, } + case `CREATE_NODE`: + return { + ...state, + LAST_NODE_COUNTER: action.payload.internal.counter, + } default: return state } diff --git a/packages/gatsby/src/redux/types.ts b/packages/gatsby/src/redux/types.ts index 92b533e1e4d28..5afd6318aa720 100644 --- a/packages/gatsby/src/redux/types.ts +++ b/packages/gatsby/src/redux/types.ts @@ -228,6 +228,7 @@ export interface IGatsbyState { status: { plugins: Record PLUGINS_HASH: Identifier + LAST_NODE_COUNTER: number } queries: { byNode: Map> From 720119c78bcefbdba862785c3c890bfefeef5afd Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Thu, 1 Apr 2021 04:16:07 +0700 Subject: [PATCH 08/11] update redux state snapshot --- packages/gatsby/src/redux/__tests__/__snapshots__/index.js.snap | 1 + 1 file changed, 1 insertion(+) diff --git a/packages/gatsby/src/redux/__tests__/__snapshots__/index.js.snap b/packages/gatsby/src/redux/__tests__/__snapshots__/index.js.snap index 3c4f40a03c188..883da4c9ec1e0 100644 --- a/packages/gatsby/src/redux/__tests__/__snapshots__/index.js.snap +++ b/packages/gatsby/src/redux/__tests__/__snapshots__/index.js.snap @@ -91,6 +91,7 @@ Object { "staticQueriesByTemplate": Map {}, "staticQueryComponents": Map {}, "status": Object { + "LAST_NODE_COUNTER": 0, "PLUGINS_HASH": "", "plugins": Object {}, }, From 61aa8f82e7432f453a8005d313f4252164123a03 Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Thu, 1 Apr 2021 16:18:47 +0700 Subject: [PATCH 09/11] perf: mutate status state directly Co-authored-by: Michal Piechowiak --- packages/gatsby/src/redux/reducers/status.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/packages/gatsby/src/redux/reducers/status.ts b/packages/gatsby/src/redux/reducers/status.ts index f308e3651c5be..46b7222543898 100644 --- a/packages/gatsby/src/redux/reducers/status.ts +++ b/packages/gatsby/src/redux/reducers/status.ts @@ -44,10 +44,8 @@ export const statusReducer = ( }, } case `CREATE_NODE`: - return { - ...state, - LAST_NODE_COUNTER: action.payload.internal.counter, - } + state.LAST_NODE_COUNTER = action.payload.internal.counter + return state default: return state } From 2ad24abfef2ecbe78a386867c9455d42db8474d3 Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Thu, 1 Apr 2021 16:24:48 +0700 Subject: [PATCH 10/11] only newly created nodes should get a counter --- packages/gatsby/src/redux/actions/public.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/gatsby/src/redux/actions/public.js b/packages/gatsby/src/redux/actions/public.js index b8226c0269a15..b0f31645d0ce1 100644 --- a/packages/gatsby/src/redux/actions/public.js +++ b/packages/gatsby/src/redux/actions/public.js @@ -622,8 +622,6 @@ const createNode = ( node.internal = {} } - node.internal.counter = getNextNodeCounter() - // Ensure the new node has a children array. if (!node.array && !_.isArray(node.children)) { node.children = [] @@ -781,6 +779,8 @@ const createNode = ( .map(createDeleteAction) } + node.internal.counter = getNextNodeCounter() + updateNodeAction = { ...actionOptions, type: `CREATE_NODE`, From c100e55c06c0650504ecd942240c8dc7b75770e8 Mon Sep 17 00:00:00 2001 From: Vladimir Razuvaev Date: Thu, 1 Apr 2021 17:37:10 +0700 Subject: [PATCH 11/11] tweak comment Co-authored-by: Michal Piechowiak --- packages/gatsby/src/redux/__tests__/run-fast-filters.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/gatsby/src/redux/__tests__/run-fast-filters.js b/packages/gatsby/src/redux/__tests__/run-fast-filters.js index 1ec661e9e8744..8b1c150ecc2ca 100644 --- a/packages/gatsby/src/redux/__tests__/run-fast-filters.js +++ b/packages/gatsby/src/redux/__tests__/run-fast-filters.js @@ -487,7 +487,7 @@ describe(`edge cases (yay)`, () => { // After process restart node.internal.counter is reset and conflicts with counters from the previous run // in some situations this leads to incorrect intersection of filtered results. - // Below we set node.internal.counter to 4 which conflicts with existing node id_4 and leads + // Below we set node.internal.counter to same value that existing node id_4 has and leads // to bad intersection of filtered results const badNode = { id: `bad-node`,