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

…30619) * 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]> (cherry picked from commit e432c23) Co-authored-by: Vladimir Razuvaev <[email protected]>
gatsbyjs · Apr 1, 2021 · 0eac672 · 0eac672
1 parent fa95904
commit 0eac672
Show file tree

Hide file tree

Showing 8 changed files with 178 additions and 35 deletions.
diff --git a/integration-tests/artifacts/__tests__/index.js b/integration-tests/artifacts/__tests__/index.js
@@ -3,14 +3,39 @@ const path = require(`path`)
 const { murmurhash } = require(`babel-plugin-remove-graphql-queries`)
 const { readPageData } = require(`gatsby/dist/utils/page-data`)
 const { stripIgnoredCharacters } = require(`gatsby/graphql`)
-const fs = require(`fs`)
+const fs = require(`fs-extra`)
 
 jest.setTimeout(100000)
 
 const publicDir = path.join(process.cwd(), `public`)
 
 const gatsbyBin = path.join(`node_modules`, `.bin`, `gatsby`)
 
+const manifest = {}
+
+function runGatsbyWithRunTestSetup(runNumber = 1) {
+  return function beforeAllImpl() {
+    return new Promise(resolve => {
+      const gatsbyProcess = spawn(gatsbyBin, [`build`], {
+        stdio: [`inherit`, `inherit`, `inherit`, `inherit`],
+        env: {
+          ...process.env,
+          NODE_ENV: `production`,
+          ARTIFACTS_RUN_SETUP: runNumber.toString(),
+        },
+      })
+
+      gatsbyProcess.on(`exit`, () => {
+        manifest[runNumber] = fs.readJSONSync(
+          path.join(process.cwd(), `.cache`, `build-manifest-for-test-1.json`)
+        )
+
+        resolve()
+      })
+    })
+  }
+}
+
 const titleQuery = `
   {
     site {
@@ -90,6 +115,24 @@ function assertFileExistenceForPagePaths({ pagePaths, type, shouldExist }) {
   )
 }
 
+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(async done => {
   const gatsbyCleanProcess = spawn(gatsbyBin, [`clean`], {
     stdio: [`inherit`, `inherit`, `inherit`, `inherit`],
@@ -105,20 +148,9 @@ beforeAll(async done => {
 })
 
 describe(`First run`, () => {
-  beforeAll(async done => {
-    const gatsbyProcess = spawn(gatsbyBin, [`build`], {
-      stdio: [`inherit`, `inherit`, `inherit`, `inherit`],
-      env: {
-        ...process.env,
-        NODE_ENV: `production`,
-        RUN_FOR_STALE_PAGE_ARTIFICATS: `1`,
-      },
-    })
+  const runNumber = 1
 
-    gatsbyProcess.on(`exit`, exitCode => {
-      done()
-    })
-  })
+  beforeAll(runGatsbyWithRunTestSetup(runNumber))
 
   describe(`Static Queries`, () => {
     test(`are written correctly when inline`, async () => {
@@ -272,26 +304,17 @@ describe(`First run`, () => {
       })
     })
   })
+
+  assertNodeCorrectness(runNumber)
 })
 
 describe(`Second run`, () => {
+  const runNumber = 2
+
   const expectedPages = [`stale-pages/stable`, `stale-pages/only-in-second`]
   const unexpectedPages = [`stale-pages/only-in-first`]
 
-  beforeAll(async done => {
-    const gatsbyProcess = spawn(gatsbyBin, [`build`], {
-      stdio: [`inherit`, `inherit`, `inherit`, `inherit`],
-      env: {
-        ...process.env,
-        NODE_ENV: `production`,
-        RUN_FOR_STALE_PAGE_ARTIFICATS: `2`,
-      },
-    })
-
-    gatsbyProcess.on(`exit`, exitCode => {
-      done()
-    })
-  })
+  beforeAll(runGatsbyWithRunTestSetup(runNumber))
 
   describe(`html files`, () => {
     const type = `html`
@@ -332,4 +355,6 @@ describe(`Second run`, () => {
       })
     })
   })
+
+  assertNodeCorrectness(runNumber)
 })
diff --git a/integration-tests/artifacts/gatsby-node.js b/integration-tests/artifacts/gatsby-node.js
@@ -1,4 +1,36 @@
-const isFirstRun = process.env.RUN_FOR_STALE_PAGE_ARTIFICATS !== `2`
+const path = require(`path`)
+const fs = require(`fs-extra`)
+
+const runNumber = parseInt(process.env.ARTIFACTS_RUN_SETUP, 10) || 1
+
+const isFirstRun = runNumber === 1
+
+exports.sourceNodes = ({ actions, createContentDigest, reporter, getNode }) => {
+  reporter.info(`Using test setup #${runNumber}`)
+
+  function createNodeHelper(type, nodePartial) {
+    const node = {
+      template: `default`,
+      ...nodePartial,
+      internal: {
+        type,
+        contentDigest: createContentDigest(nodePartial),
+      },
+    }
+    actions.createNode(node)
+  }
+
+  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}`,
+  })
+}
 
 exports.createPages = ({ actions }) => {
   function createPageHelper(dummyId) {
@@ -22,3 +54,19 @@ exports.createPages = ({ actions }) => {
     createPageHelper(`only-in-second`)
   }
 }
+
+let counter = 1
+exports.onPostBuild = ({ getNodes }) => {
+  console.log(`[test] onPostBuild`)
+
+  fs.writeJSONSync(
+    path.join(
+      process.cwd(),
+      `.cache`,
+      `build-manifest-for-test-${counter++}.json`
+    ),
+    {
+      allNodeCounters: getNodes().map(node => [node.id, node.internal.counter]),
+    }
+  )
+}
diff --git a/packages/gatsby/src/redux/__tests__/__snapshots__/index.js.snap b/packages/gatsby/src/redux/__tests__/__snapshots__/index.js.snap
@@ -79,6 +79,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
@@ -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
@@ -531,9 +531,17 @@ actions.deleteNodes = (nodes: any[], plugin: Plugin) => {
   return deleteNodesAction
 }
 
-// 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 = {}
 
@@ -633,9 +641,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 = []
@@ -793,6 +798,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
@@ -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
@@ -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
@@ -208,6 +208,7 @@ export interface IGatsbyState {
   status: {
     plugins: Record<string, IGatsbyPlugin>
     PLUGINS_HASH: Identifier
+    LAST_NODE_COUNTER: number
   }
   queries: {
     byNode: Map<Identifier, Set<Identifier>>