fix(gatsby): fix incorrect intersection of filtered results (#30594)

* add failing test

* actually failing test

* make test independent of other tests

* add invariant for filter intersection assumptions

* add failing integration test

* fix test 🤦‍

* actually fix this heisenbug

* update redux state snapshot

* perf: mutate status state directly

Co-authored-by: Michal Piechowiak <[email protected]>

* only newly created nodes should get a counter

* tweak comment

Co-authored-by: Michal Piechowiak <[email protected]>

Co-authored-by: Michal Piechowiak <[email protected]>

Author: vladar

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)
 })

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,

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 {},
   },

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`
+    )
+  })
+})

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`,

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

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
   }

packages/gatsby/src/redux/types.ts

@@ -228,6 +228,7 @@ export interface IGatsbyState {
   status: {
     plugins: Record<string, IGatsbyPlugin>
     PLUGINS_HASH: Identifier
+    LAST_NODE_COUNTER: number
   }
   queries: {
     byNode: Map<Identifier, Set<Identifier>>