[node-modules] Fixes race between mkdirp and remove (#1108)

* Fixes race between `mkdirp` and `remove` during module linking by doing all `remove` first.

* Get rid of keepNodeModules by keeping inner node_modules by default during all the operations

* Add assertion check that removed directory has node_modules

Author: larixer

.yarn/versions/37861a83.yml

@@ -0,0 +1,20 @@
+releases:
+  "@yarnpkg/cli": prerelease
+  "@yarnpkg/plugin-node-modules": prerelease
+
+declined:
+  - "@yarnpkg/plugin-constraints"
+  - "@yarnpkg/plugin-dlx"
+  - "@yarnpkg/plugin-essentials"
+  - "@yarnpkg/plugin-init"
+  - "@yarnpkg/plugin-interactive-tools"
+  - "@yarnpkg/plugin-npm-cli"
+  - "@yarnpkg/plugin-pack"
+  - "@yarnpkg/plugin-patch"
+  - "@yarnpkg/plugin-pnp"
+  - "@yarnpkg/plugin-stage"
+  - "@yarnpkg/plugin-typescript"
+  - "@yarnpkg/plugin-version"
+  - "@yarnpkg/plugin-workspace-tools"
+  - "@yarnpkg/core"
+  - "@yarnpkg/doctor"

packages/plugin-node-modules/sources/NodeModulesLinker.ts

@@ -317,7 +317,10 @@ function getLocationMap(installState: NodeModulesLocatorMap) {
   return locationMap;
 }
 
-const removeDir = async (dir: PortablePath, options?: {innerLoop?: boolean, excludeNodeModules?: boolean}): Promise<any> => {
+const removeDir = async (dir: PortablePath, options?: {innerLoop?: boolean}): Promise<any> => {
+  if (dir.split(ppath.sep).indexOf(NODE_MODULES) < 0)
+    throw new Error(`Assertion failed: trying to remove dir that doesn't contain node_modules: ${dir}`);
+
   try {
     if (!options || !options.innerLoop) {
       const stats = await xfs.lstatPromise(dir);
@@ -330,7 +333,7 @@ const removeDir = async (dir: PortablePath, options?: {innerLoop?: boolean, excl
     for (const entry of entries) {
       const targetPath = ppath.join(dir, toFilename(entry.name));
       if (entry.isDirectory()) {
-        if (entry.name !== NODE_MODULES || !options || !options.excludeNodeModules) {
+        if (entry.name !== NODE_MODULES || (options && options.innerLoop)) {
           await removeDir(targetPath, {innerLoop: true});
         }
       } else {
@@ -447,7 +450,7 @@ const buildLocationTree = (locatorMap: NodeModulesLocatorMap | null, {skipPrefix
 const symlinkPromise = async (srcDir: PortablePath, dstDir: PortablePath) =>
   xfs.symlinkPromise(process.platform !== 'win32' ? ppath.relative(ppath.dirname(dstDir), srcDir) : srcDir, dstDir, process.platform === 'win32' ? 'junction' : undefined);
 
-const copyPromise = async (dstDir: PortablePath, srcDir: PortablePath, {baseFs}: {baseFs: FakeFS<PortablePath>}) => {
+const copyPromise = async (dstDir: PortablePath, srcDir: PortablePath, {baseFs, innerLoop}: {baseFs: FakeFS<PortablePath>, innerLoop?: boolean}) => {
   await xfs.mkdirpPromise(dstDir);
   const entries = await baseFs.readdirPromise(srcDir, {withFileTypes: true});
 
@@ -472,7 +475,9 @@ const copyPromise = async (dstDir: PortablePath, srcDir: PortablePath, {baseFs}:
     const srcPath = ppath.join(srcDir, toFilename(entry.name));
     const dstPath = ppath.join(dstDir, toFilename(entry.name));
     if (entry.isDirectory()) {
-      await copyPromise(dstPath, srcPath, {baseFs});
+      if (entry.name !== NODE_MODULES || innerLoop) {
+        await copyPromise(dstPath, srcPath, {baseFs, innerLoop: true});
+      }
     } else {
       await copy(dstPath, srcPath, entry);
     }
@@ -574,10 +579,9 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
   const locationTree = buildLocationTree(installState, {skipPrefix: project.cwd});
 
   const addQueue: Promise<void>[] = [];
-  const addModule = async ({srcDir, dstDir, linkType, keepNodeModules}: {srcDir: PortablePath, dstDir: PortablePath, linkType: LinkType, keepNodeModules: boolean}) => {
+  const addModule = async ({srcDir, dstDir, linkType}: {srcDir: PortablePath, dstDir: PortablePath, linkType: LinkType}) => {
     const promise: Promise<any> = (async () => {
       try {
-        await removeDir(dstDir, {excludeNodeModules: keepNodeModules});
         if (linkType === LinkType.SOFT) {
           await xfs.mkdirpPromise(ppath.dirname(dstDir));
           await symlinkPromise(ppath.resolve(srcDir), dstDir);
@@ -597,14 +601,12 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
     }
   };
 
-  const cloneModule = async (srcDir: PortablePath, dstDir: PortablePath, options?: { keepSrcNodeModules?: boolean, keepDstNodeModules?: boolean, innerLoop?: boolean }) => {
+  const cloneModule = async (srcDir: PortablePath, dstDir: PortablePath, options?: { innerLoop?: boolean }) => {
     const promise: Promise<any> = (async () => {
-      const cloneDir = async (srcDir: PortablePath, dstDir: PortablePath, options?: { keepSrcNodeModules?: boolean, keepDstNodeModules?: boolean, innerLoop?: boolean }) => {
+      const cloneDir = async (srcDir: PortablePath, dstDir: PortablePath, options?: { innerLoop?: boolean }) => {
         try {
-          if (!options || !options.innerLoop) {
-            await removeDir(dstDir, {excludeNodeModules: options && options.keepDstNodeModules});
+          if (!options || !options.innerLoop)
             await xfs.mkdirpPromise(dstDir);
-          }
 
           const entries = await xfs.readdirPromise(srcDir, {withFileTypes: true});
           for (const entry of entries) {
@@ -614,13 +616,13 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
             const src = ppath.join(srcDir, entry.name);
             const dst = ppath.join(dstDir, entry.name);
 
-            if (entry.name !== NODE_MODULES || !options || !options.keepSrcNodeModules) {
-              if (entry.isDirectory()) {
+            if (entry.isDirectory()) {
+              if (entry.name !== NODE_MODULES || (options && options.innerLoop)) {
                 await xfs.mkdirpPromise(dst);
-                await cloneDir(src, dst, {keepSrcNodeModules: false, keepDstNodeModules: false, innerLoop: true});
-              } else {
-                await xfs.copyFilePromise(src, dst, fs.constants.COPYFILE_FICLONE);
+                await cloneDir(src, dst, {innerLoop: true});
               }
+            } else {
+              await xfs.copyFilePromise(src, dst, fs.constants.COPYFILE_FICLONE);
             }
           }
         } catch (e) {
@@ -661,39 +663,53 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
 
 
   // Delete locations that no longer exist
-  const deleteList: PortablePath[] = [];
+  const deleteList = new Set<PortablePath>();
+  // Delete locations of inner node_modules
+  const innerDeleteList = new Set<PortablePath>();
   for (const {locations} of preinstallState.locatorMap.values()) {
     for (const location of locations) {
       const {locationRoot, segments} = parseLocation(location, {
         skipPrefix: project.cwd,
       });
 
+      let prevNode = prevLocationTree.get(locationRoot);
       let node = locationTree.get(locationRoot);
       let curLocation = locationRoot;
-      if (!node) {
-        deleteList.push(curLocation);
-      } else {
+      if (node) {
         for (const segment of segments) {
           // '.' segment exists only for top-level locator, skip it
           if (segment === '.')
             continue;
 
           curLocation = ppath.join(curLocation, segment);
           node = node.children.get(segment);
+          if (prevNode)
+            prevNode = prevNode.children.get(segment);
+
           if (!node) {
-            deleteList.push(curLocation);
+            deleteList.add(curLocation);
+            // If previous install had inner node_modules folder, we should explicitely list it for
+            // `removeDir` to delete it, but we need to delete it first, so we add it to inner delete list
+            if (prevNode && prevNode.children.has(NODE_MODULES))
+              innerDeleteList.add(ppath.join(curLocation, NODE_MODULES));
             break;
           }
         }
       }
     }
   }
 
+  // Handle inner node_modules deletions first
+  for (const dstDir of innerDeleteList)
+    await deleteModule(dstDir);
+  await Promise.all(deleteQueue);
+  deleteQueue.length = 0;
+
   for (const dstDir of deleteList)
     await deleteModule(dstDir);
 
   // Update changed locations
-  const addList: Array<{srcDir: PortablePath, dstDir: PortablePath, linkType: LinkType, keepNodeModules: boolean}> = [];
+  const addList: Array<{srcDir: PortablePath, dstDir: PortablePath, linkType: LinkType}> = [];
   for (const [prevLocator, {locations}] of preinstallState.locatorMap.entries()) {
     for (const location of locations) {
       const {locationRoot, segments} = parseLocation(location, {
@@ -715,9 +731,9 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
           const srcDir = info.target;
           const dstDir = curLocation;
           const linkType = info.linkType;
-          const keepNodeModules = node.children.size > 0;
           if (srcDir !== dstDir) {
-            addList.push({srcDir, dstDir, linkType, keepNodeModules});
+            deleteList.add(dstDir);
+            addList.push({srcDir, dstDir, linkType});
           }
         }
       }
@@ -747,13 +763,15 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
         node = node!.children.get(segment);
 
       if (!prevTreeNode) {
-        addList.push({srcDir, dstDir, linkType, keepNodeModules: node!.children.size > 0});
+        deleteList.add(dstDir);
+        addList.push({srcDir, dstDir, linkType});
       } else {
         for (const segment of segments) {
           curLocation = ppath.join(curLocation, segment);
           prevTreeNode = prevTreeNode.children.get(segment);
           if (!prevTreeNode) {
-            addList.push({srcDir, dstDir, linkType, keepNodeModules: node!.children.size > 0});
+            deleteList.add(dstDir);
+            addList.push({srcDir, dstDir, linkType});
             break;
           }
         }
@@ -765,18 +783,15 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
   const reportedProgress = report.reportProgress(progress);
 
   try {
-    const persistedLocations = new Map<PortablePath, {
-      dstDir: PortablePath,
-      keepNodeModules: boolean,
-    }>();
+    const persistedLocations = new Map<PortablePath, PortablePath>();
 
     // For the first pass we'll only want to install a single copy for each
     // source directory. We'll later use the resulting install directories for
     // the other instances of the same package (this will avoid us having to
     // crawl the zip archives for each package).
     for (const entry of addList) {
       if (entry.linkType === LinkType.SOFT || !persistedLocations.has(entry.srcDir)) {
-        persistedLocations.set(entry.srcDir, {dstDir: entry.dstDir, keepNodeModules: entry.keepNodeModules});
+        persistedLocations.set(entry.srcDir, entry.dstDir);
         await addModule({...entry});
       }
     }
@@ -787,9 +802,9 @@ async function persistNodeModules(preinstallState: InstallState, installState: N
 
     // Second pass: clone module duplicates
     for (const entry of addList) {
-      const locationInfo = persistedLocations.get(entry.srcDir)!;
-      if (entry.linkType !== LinkType.SOFT && entry.dstDir !== locationInfo.dstDir) {
-        await cloneModule(locationInfo.dstDir, entry.dstDir, {keepSrcNodeModules: locationInfo.keepNodeModules, keepDstNodeModules: entry.keepNodeModules});
+      const persistedDir = persistedLocations.get(entry.srcDir)!;
+      if (entry.linkType !== LinkType.SOFT && entry.dstDir !== persistedDir) {
+        await cloneModule(persistedDir, entry.dstDir);
       }
     }