diff --git a/integration-tests/artifacts/__tests__/index.js b/integration-tests/artifacts/__tests__/index.js index 69a7429fc7bfa..7ffb2a2e807f8 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, 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 {}, }, diff --git a/packages/gatsby/src/redux/__tests__/run-fast-filters.js b/packages/gatsby/src/redux/__tests__/run-fast-filters.js index 7cf0c2bbb5650..8b1c150ecc2ca 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 { @@ -459,3 +460,54 @@ 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` }, // matches id_2 and id_4 + deep: { flat: { search: { chain: { $eq: 500 } } } }, // matches id_2 + } + + const result = applyFastFilters( + createDbQueriesFromObject(filter), + [typeName], + new Map() + ) + + // Sanity-check + expect(result.length).toEqual(1) + expect(result[0].id).toEqual(`id_2`) + + // 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 same value that existing node id_4 has 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: getNode(`id_4`).internal.counter, + }, + } + store.dispatch({ + type: `CREATE_NODE`, + payload: badNode, + }) + + const run = () => + applyFastFilters(createDbQueriesFromObject(filter), [typeName], new Map()) + + expect(run).toThrow( + `Invariant violation: inconsistent node counters detected` + ) + }) +}) diff --git a/packages/gatsby/src/redux/actions/public.js b/packages/gatsby/src/redux/actions/public.js index 4308474ade49c..b0f31645d0ce1 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,9 +622,6 @@ const createNode = ( node.internal = {} } - NODE_COUNTER++ - node.internal.counter = NODE_COUNTER - // Ensure the new node has a children array. if (!node.array && !_.isArray(node.children)) { node.children = [] @@ -774,6 +779,8 @@ const createNode = ( .map(createDeleteAction) } + node.internal.counter = getNextNodeCounter() + updateNodeAction = { ...actionOptions, type: `CREATE_NODE`, 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 diff --git a/packages/gatsby/src/redux/reducers/status.ts b/packages/gatsby/src/redux/reducers/status.ts index 7913b60d40ea3..46b7222543898 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,9 @@ export const statusReducer = ( ), }, } + case `CREATE_NODE`: + state.LAST_NODE_COUNTER = action.payload.internal.counter + return state 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>