fix root-ownership race conditions in meta-test

isaacs · isaacs · commit 44a2b036b343 · 2019-10-07T16:25:04.000-07:00
Currently all of our tests verify on teardown that there are no root-owned files in the cache. However, owing to some race conditions and slippery stream event deferral behavior that won't be fixed until v7, occasionally cacache's chown doesn't get processed until _after_ the promise resolves and the test ends. As a result, sometimes this check occurs before the chown has happened, resulting in flaky hard-to-reproduce failures. The somewhat-kludgey solution here is to move the ownership check from t.teardown to process.on('exit'). In npm v7, we should move it back to t.teardown, because we should never have a test that resolves in such a way as to leave the cache in an invalid state. PR-URL: #262 Credit: @isaacs Close: #262 Reviewed-by: @isaacs
diff --git a/test/common-tap.js b/test/common-tap.js
@@ -60,29 +60,31 @@ const find = require('which').sync('find')
 require('tap').teardown(() => {
   // work around windows folder locking
   process.chdir(returnCwd)
-  try {
-    if (isSudo) {
-      // running tests as sudo.  ensure we didn't leave any root-owned
-      // files in the cache by mistake.
-      const args = [ commonCache, '-uid', '0' ]
-      const found = spawnSync(find, args)
-      const output = found && found.stdout && found.stdout.toString()
-      if (output.length) {
-        const er = new Error('Root-owned files left in cache!')
-        er.testName = main
-        er.files = output.trim().split('\n')
-        throw er
+  process.on('exit', () => {
+    try {
+      if (isSudo) {
+        // running tests as sudo.  ensure we didn't leave any root-owned
+        // files in the cache by mistake.
+        const args = [ commonCache, '-uid', '0' ]
+        const found = spawnSync(find, args)
+        const output = found && found.stdout && found.stdout.toString()
+        if (output.length) {
+          const er = new Error('Root-owned files left in cache!')
+          er.testName = main
+          er.files = output.trim().split('\n')
+          throw er
+        }
+      }
+      if (!process.env.NO_TEST_CLEANUP) {
+        rimraf.sync(exports.pkg)
+        rimraf.sync(commonCache)
+      }
+    } catch (e) {
+      if (process.platform !== 'win32') {
+        throw e
       }
     }
-    if (!process.env.NO_TEST_CLEANUP) {
-      rimraf.sync(exports.pkg)
-      rimraf.sync(commonCache)
-    }
-  } catch (e) {
-    if (process.platform !== 'win32') {
-      throw e
-    }
-  }
+  })
 })
 
 var port = exports.port = 15443 + testId
diff --git a/test/tap/meta-test-flaky-root-ownership-test.js b/test/tap/meta-test-flaky-root-ownership-test.js
@@ -0,0 +1,20 @@
+const t = require('tap')
+if (!process.getuid || process.getuid() !== 0 || !process.env.SUDO_UID || !process.env.SUDO_GID) {
+  t.pass('this test only runs in sudo mode')
+  t.end()
+  process.exit(0)
+}
+
+const common = require('../common-tap.js')
+const fs = require('fs')
+const mkdirp = require('mkdirp')
+mkdirp.sync(common.cache + '/root/owned')
+fs.writeFileSync(common.cache + '/root/owned/file.txt', 'should be chowned')
+const chown = require('chownr')
+
+// this will fire after t.teardown() but before process.on('exit')
+setTimeout(() => {
+  chown.sync(common.cache, +process.env.SUDO_UID, +process.env.SUDO_GID)
+}, 100)
+
+t.pass('this is fine')
