fix(patch): stop a workdir cleanup race from masking update errors (#9798)

`npm patch update` builds a throwaway git repo in a temp dir to rebase
the existing patch, then removes it. Two problems made that removal
flaky:

Since git 2.48, `maintenance.autoDetach` defaults to true, so `git
commit` detaches `git maintenance run --auto` and returns while that
background process is still writing under `.git/objects/pack`. Removing
the workdir then races it and fails with `ENOTEMPTY`.

Both cleanup sites removed the workdir with an unguarded `await
rm(...)`, so that failure replaced the command's real error, a
`EPATCHBASE` rebase failure surfaced as `ENOTEMPTY`, and on the success
path a cleanup failure would have failed an otherwise-complete update.

The throwaway repo's git invocations now pass `-c gc.auto=0 -c
maintenance.auto=false` so git never spawns background maintenance, and
both cleanup sites go through a helper that retries the removal and
never throws.

## References

Reported in #9793 (comment)
as an unrelated CI failure.

Author: manzoorwanijk

lib/commands/patch.js

@@ -408,7 +408,7 @@ class Patch extends BaseCommand {
         pkgJson, patched, entry, name: entry.name, newVersion, newKey, diff, installedVersions,
       })
     } finally {
-      await rm(rebase.work, { recursive: true, force: true })
+      await this.#removeWork(rebase.work)
     }
   }
 
@@ -561,13 +561,24 @@ class Patch extends BaseCommand {
       }
       return { work, repo, newDir, conflicted }
     } catch (er) {
-      await rm(work, { recursive: true, force: true })
+      await this.#removeWork(work)
       throw er
     }
   }
 
+  // Remove the throwaway rebase workdir.
+  // Retries because git can briefly hold files there, and never throws so a cleanup failure cannot mask the real error.
+  async #removeWork (work) {
+    try {
+      await rm(work, { recursive: true, force: true, maxRetries: 10, retryDelay: 100 })
+    } catch (er) {
+      log.verbose('patch', `could not remove ${work}: ${er.message}`)
+    }
+  }
+
   #git (args, cwd) {
-    return git.spawn(args, {
+    // The throwaway repo never needs maintenance and a detached auto-gc would race the workdir cleanup.
+    return git.spawn(['-c', 'gc.auto=0', '-c', 'maintenance.auto=false', ...args], {
       cwd,
       env: { ...process.env, GIT_CONFIG_GLOBAL: '/dev/null', GIT_CONFIG_SYSTEM: '/dev/null' },
     })

test/lib/commands/patch.js

@@ -1,4 +1,5 @@
 const fs = require('node:fs')
+const fsPromises = require('node:fs/promises')
 const path = require('node:path')
 const t = require('tap')
 const Arborist = require('@npmcli/arborist')
@@ -909,6 +910,64 @@ t.test('update: a patch that no longer applies to its baseline rejects with EPAT
   await t.rejects(npm.exec('patch', ['update', name]), { code: 'EPATCHBASE' })
 })
 
+// fs/promises whose rm always fails for the rebase workdir, mimicking a concurrent git process leaving it non-empty.
+const failingWorkdirRm = {
+  'node:fs/promises': {
+    ...fsPromises,
+    rm: async (p, opts) => {
+      if (path.basename(p).startsWith('npm-patch-rebase-')) {
+        throw Object.assign(new Error(`ENOTEMPTY: directory not empty, rmdir '${p}'`), { code: 'ENOTEMPTY' })
+      }
+      return fsPromises.rm(p, opts)
+    },
+  },
+}
+
+t.test('update: a failed workdir cleanup does not mask the real error', async t => {
+  const name = 'upd-drift-rm'
+  const { npm, registry } = await loadMockNpm(t, {
+    config: { 'ignore-scripts': true, audit: false },
+    strictRegistryNock: false,
+    prefixDir: rootWith({ [name]: '^1.0.0' }),
+    mocks: failingWorkdirRm,
+  })
+  await setupVersions(npm, registry, name, { '1.0.0': 'real\n', '2.0.0': 'real2\n' })
+  await npm.exec('install', [])
+  fs.mkdirSync(path.join(npm.prefix, 'patches'), { recursive: true })
+  fs.writeFileSync(path.join(npm.prefix, 'patches', `${name}@1.0.0.patch`),
+    '--- a/index.js\t\n+++ b/index.js\t\n@@ -1,1 +1,1 @@\n-NOT-THE-REAL-LINE\n+changed\n')
+  const pkg = readJson(path.join(npm.prefix, 'package.json'))
+  pkg.patchedDependencies = { [`${name}@1.0.0`]: `patches/${name}@1.0.0.patch` }
+  fs.writeFileSync(path.join(npm.prefix, 'package.json'), JSON.stringify(pkg))
+  npm.config.set('to', '2.0.0')
+  await t.rejects(npm.exec('patch', ['update', name]), { code: 'EPATCHBASE' })
+})
+
+t.test('update: a failed workdir cleanup does not fail a successful update', async t => {
+  const name = 'upd-exact-rm'
+  const { npm, joinedOutput, outputs, registry } = await loadMockNpm(t, {
+    config: { 'ignore-scripts': true, audit: false },
+    strictRegistryNock: false,
+    prefixDir: rootWith({ [name]: '^1.0.0' }),
+    mocks: failingWorkdirRm,
+  })
+  await setupVersions(npm, registry, name, { '1.0.0': 'a\nb\nc\n', '2.0.0': 'a\nb\nCC\n' })
+  await npm.exec('install', [])
+
+  outputs.length = 0
+  await npm.exec('patch', ['add', name])
+  const editDir = joinedOutput().match(/directory: (.+)/)[1].trim()
+  fs.writeFileSync(path.join(editDir, 'index.js'), 'AA\nb\nc\n')
+  await npm.exec('patch', ['commit', editDir])
+
+  npm.config.set('to', '2.0.0')
+  await npm.exec('patch', ['update', name])
+
+  const pkg = readJson(path.join(npm.prefix, 'package.json'))
+  t.same(pkg.patchedDependencies, { [`${name}@2.0.0`]: `patches/${name}@2.0.0.patch` },
+    'selector renamed to the new version')
+})
+
 t.test('update: when the new version already contains the patch, reports EPATCHEMPTY', async t => {
   const name = 'upd-empty'
   const { npm, joinedOutput, outputs, registry } = await loadMockNpm(t, {