From 265e4aeb9b6cbfcd07c75059508b7617b5169d73 Mon Sep 17 00:00:00 2001 From: Gar Date: Mon, 1 May 2023 13:22:34 -0700 Subject: [PATCH] fix: Trust the filesystem to move files The original lib/util/move-file.js has gone through many iterations over the last several years. It no longer represents anything close to the original intent. Most of the functionality it was originally trying to work around is now something that `@npmcli/fs#moveFile` does, and better. Trying to move a file by creating a hard link and unlinking the old file is an optimization best left to the operating system when you ask it to move a file. In fact the `@npmcli/fs#moveFile` method tries a *rename* first, which is the best choice all around. It also supports an "overwrite: false" flag so we can still error out if the destination already exists. This PR removes all of the old legacy code. --- lib/content/write.js | 12 ++- lib/util/move-file.js | 56 ------------ test/content/write.js | 16 ++++ test/util/move-file.js | 202 ----------------------------------------- 4 files changed, 25 insertions(+), 261 deletions(-) delete mode 100644 lib/util/move-file.js delete mode 100644 test/util/move-file.js 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') - } -)