diff --git a/lib/content/write.js b/lib/content/write.js index 2d5d184..74b87fb 100644 --- a/lib/content/write.js +++ b/lib/content/write.js @@ -4,7 +4,7 @@ const events = require('events') const contentPath = require('./path') const fs = require('fs/promises') -const moveFile = require('../util/move-file') +const { moveFile } = require('@npmcli/fs') const { Minipass } = require('minipass') const Pipeline = require('minipass-pipeline') const Flush = require('minipass-flush') @@ -161,8 +161,14 @@ async function moveToDestination (tmp, cache, sri, opts) { const destDir = path.dirname(destination) await fs.mkdir(destDir, { recursive: true }) - await moveFile(tmp.target, destination) - tmp.moved = true + try { + await moveFile(tmp.target, destination, { overwrite: false }) + tmp.moved = true + } catch (err) { + if (!err.message.startsWith('The destination file exists')) { + throw Object.assign(err, { code: 'EEXIST' }) + } + } } function sizeError (expected, found) { diff --git a/lib/util/move-file.js b/lib/util/move-file.js deleted file mode 100644 index eb3ba76..0000000 --- a/lib/util/move-file.js +++ /dev/null @@ -1,56 +0,0 @@ -'use strict' - -const fs = require('fs/promises') -const { moveFile: move } = require('@npmcli/fs') -const pinflight = require('promise-inflight') - -module.exports = moveFile - -async function moveFile (src, dest) { - const isWindows = process.platform === 'win32' - - // This isn't quite an fs.rename -- the assumption is that - // if `dest` already exists, and we get certain errors while - // trying to move it, we should just not bother. - // - // In the case of cache corruption, users will receive an - // EINTEGRITY error elsewhere, and can remove the offending - // content their own way. - // - // Note that, as the name suggests, this strictly only supports file moves. - try { - await fs.link(src, dest) - } catch (err) { - if (isWindows && err.code === 'EPERM') { - // XXX This is a really weird way to handle this situation, as it - // results in the src file being deleted even though the dest - // might not exist. Since we pretty much always write files to - // deterministic locations based on content hash, this is likely - // ok (or at worst, just ends in a future cache miss). But it would - // be worth investigating at some time in the future if this is - // really what we want to do here. - } else if (err.code === 'EEXIST' || err.code === 'EBUSY') { - // file already exists, so whatever - } else { - throw err - } - } - try { - await Promise.all([ - fs.unlink(src), - !isWindows && fs.chmod(dest, '0444'), - ]) - } catch (e) { - return pinflight('cacache-move-file:' + dest, async () => { - await fs.stat(dest).catch((err) => { - if (err.code !== 'ENOENT') { - // Something else is wrong here. Bail bail bail - throw err - } - }) - // file doesn't already exist! let's try a rename -> copy fallback - // only delete if it successfully copies - return move(src, dest) - }) - } -} diff --git a/test/content/write.js b/test/content/write.js index e3b561d..fae3cd9 100644 --- a/test/content/write.js +++ b/test/content/write.js @@ -233,6 +233,22 @@ t.test('cleans up tmp on successful completion', async t => { }) }) +t.test('Handles moveFile error other than EEXIST', async t => { + const write = t.mock('../../lib/content/write.js', { + '@npmcli/fs': { + moveFile: async () => { + throw new Error('Unknown error') + }, + }, + }) + const CONTENT = 'foobarbaz' + const CACHE = t.testdir() + await t.rejects( + write.stream(CACHE).end(CONTENT).promise(), + { message: 'Unknown error' } + ) +}) + t.test('cleans up tmp on streaming error', (t) => { const CONTENT = 'foobarbaz' const CACHE = t.testdir() diff --git a/test/util/move-file.js b/test/util/move-file.js deleted file mode 100644 index 2c35b34..0000000 --- a/test/util/move-file.js +++ /dev/null @@ -1,202 +0,0 @@ -'use strict' - -const { - chmod, - open, - readFile, - readdir, - stat, -} = require('fs/promises') -const fs = require('fs') -const path = require('path') -const t = require('tap') - -const moveFile = require('../../lib/util/move-file') - -t.test('move a file', function (t) { - const testDir = t.testdir({ - src: 'foo', - }) - return moveFile(testDir + '/src', testDir + '/dest') - .then(() => { - return readFile(testDir + '/dest', 'utf8') - }) - .then((data) => { - t.equal(data, 'foo', 'file data correct') - return stat(testDir + '/src').catch((err) => { - t.ok(err, 'src read error') - t.equal(err.code, 'ENOENT', 'src does not exist') - }) - }) -}) - -t.test('does not clobber existing files', function (t) { - const testDir = t.testdir({ - src: 'foo', - dest: 'bar', - }) - return moveFile(testDir + '/src', testDir + '/dest') - .then(() => { - return readFile(testDir + '/dest', 'utf8') - }) - .then((data) => { - t.equal(data, 'bar', 'conflicting file left intact') - return stat(testDir + '/src').catch((err) => { - t.ok(err, 'src read error') - t.equal(err.code, 'ENOENT', 'src file still deleted') - }) - }) -}) - -t.test('does not move a file into an existing directory', async t => { - const testDir = t.testdir({ - src: 'foo', - dest: {}, - }) - await moveFile(testDir + '/src', testDir + '/dest') - const files = await readdir(testDir + '/dest') - t.equal(files.length, 0, 'directory remains empty') -}) - -t.test('does not error if destination file is open', function (t) { - const testDir = t.testdir({ - src: 'foo', - dest: 'bar', - }) - - return open(testDir + '/dest', 'r+').then((fh) => { - return moveFile(testDir + '/src', testDir + '/dest') - .then(() => { - return fh.close() - }) - .then(() => { - return readFile(testDir + '/dest', 'utf8') - }) - .then((data) => { - t.equal(data, 'bar', 'destination left intact') - return stat(testDir + '/src').catch((err) => { - t.ok(err, 'src read error') - t.equal(err.code, 'ENOENT', 'src does not exist') - }) - }) - }) -}) - -t.test('fallback to renaming on missing files post-move', async t => { - const testDir = t.testdir({ - src: 'foo', - }) - - const missingFileError = new Error('ENOENT') - missingFileError.code = 'ENOENT' - const mockFS = { - ...fs.promises, - async unlink (path) { - throw missingFileError - }, - async stat (path) { - throw missingFileError - }, - } - const mockedMoveFile = t.mock('../../lib/util/move-file', { - 'fs/promises': mockFS, - }) - - await mockedMoveFile(testDir + '/src', testDir + '/dest') - const data = await readFile(testDir + '/dest', 'utf8') - t.equal(data, 'foo', 'file data correct') - await t.rejects( - stat(testDir + '/src'), - { code: 'ENOENT' }, - './src does not exist' - ) -}) - -t.test('non ENOENT error on move fallback', async function (t) { - const testDir = t.testdir({ - src: 'foo', - }) - - const missingFileError = new Error('ENOENT') - missingFileError.code = 'ENOENT' - const otherError = new Error('UNKNOWN') - otherError.code = 'OTHER' - const mockFS = { - ...fs.promises, - async unlink (path) { - throw missingFileError - }, - async stat (path) { - throw otherError - }, - - } - const mockedMoveFile = t.mock('../../lib/util/move-file', { - 'fs/promises': mockFS, - }) - - await t.rejects( - mockedMoveFile(testDir + '/src', testDir + '/dest'), - { code: 'OTHER' }, - 'throws other error' - ) -}) - -t.test('verify weird EPERM on Windows behavior', t => { - const processPlatform = process.platform - Object.defineProperty(process, 'platform', { value: 'win32' }) - t.teardown(() => { - Object.defineProperty(process, 'platform', { value: processPlatform }) - }) - - let calledMonkeyPatch = false - const mockFS = { - ...fs.promises, - link: async (src, dest) => { - calledMonkeyPatch = true - throw Object.assign(new Error('yolo'), { code: 'EPERM' }) - }, - } - - const mockedMoveFile = t.mock('../../lib/util/move-file.js', { - 'fs/promises': mockFS, - }) - - const testDir = t.testdir({ - eperm: { - src: 'epermmy', - }, - }) - - return mockedMoveFile(testDir + '/eperm/src', testDir + '/eperm/dest') - .then(() => t.ok(calledMonkeyPatch, 'called the patched fs.link fn')) - .then(() => t.rejects(readFile('eperm/dest'), { - code: 'ENOENT', - }, 'destination file did not get written')) - .then(() => t.rejects(readFile('eperm/src'), { - code: 'ENOENT', - }, 'src file did get deleted')) -}) - -t.test( - 'errors if dest is not writable', - { - skip: process.platform === 'win32', - }, - async t => { - const testDir = t.testdir({ - src: 'foo', - dest: {}, - }) - - await chmod(testDir + '/dest', parseInt('400', 8)) - await t.rejects( - moveFile(testDir + '/src', path.join(testDir + '/dest', 'file')), - { code: 'EACCES' }, - 'error is about permissions' - ) - - const data = await readFile(testDir + '/src', 'utf8') - t.equal(data, 'foo', 'src contents left intact') - } -)