Skip to content

Commit

Permalink
fix root-ownership race conditions in meta-test
Browse files Browse the repository at this point in the history
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
  • Loading branch information
isaacs committed Oct 7, 2019
1 parent dcff367 commit 44a2b03
Show file tree
Hide file tree
Showing 2 changed files with 43 additions and 21 deletions.
44 changes: 23 additions & 21 deletions test/common-tap.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
20 changes: 20 additions & 0 deletions test/tap/meta-test-flaky-root-ownership-test.js
Original file line number Diff line number Diff line change
@@ -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')

0 comments on commit 44a2b03

Please sign in to comment.