From e9a683d056c1a002838216c229305389e46d9791 Mon Sep 17 00:00:00 2001 From: Joost Koehoorn Date: Tue, 11 May 2021 22:30:35 +0200 Subject: [PATCH] fix(builtin): account for racy deletion of symlink in linker (#2662) On Windows, the linker has been prone to race conditions with respect to symlinking packages into the `node_modules` tree, causing ENOENT errors that causes the target to fail. Since the intention of `symlinkWithUnlink` and `deleteDirectory` is to unlink the directory anyway, we can simply ignore ENOENT errors for the directory that would be about to be unlinked. --- internal/linker/index.js | 45 ++++++++++++++++--- internal/linker/link_node_modules.ts | 66 ++++++++++++++++++++++------ 2 files changed, 90 insertions(+), 21 deletions(-) diff --git a/internal/linker/index.js b/internal/linker/index.js index 2a8ccd826e..c4d6a6f691 100644 --- a/internal/linker/index.js +++ b/internal/linker/index.js @@ -50,6 +50,30 @@ function gracefulLstat(path) { } }); } +function gracefulReadlink(path) { + try { + return fs.readlinkSync(path); + } + catch (e) { + if (e.code === 'ENOENT') { + return null; + } + throw e; + } +} +function gracefulReaddir(path) { + return __awaiter(this, void 0, void 0, function* () { + try { + return yield fs.promises.readdir(path); + } + catch (e) { + if (e.code === 'ENOENT') { + return []; + } + throw e; + } + }); +} function unlink(moduleName) { return __awaiter(this, void 0, void 0, function* () { const stat = yield gracefulLstat(moduleName); @@ -69,11 +93,12 @@ function unlink(moduleName) { function deleteDirectory(p) { return __awaiter(this, void 0, void 0, function* () { log_verbose("Deleting children of", p); - for (let entry of yield fs.promises.readdir(p)) { + for (let entry of yield gracefulReaddir(p)) { const childPath = path.join(p, entry); const stat = yield gracefulLstat(childPath); if (stat === null) { - throw Error(`File does not exist, but is listed as directory entry: ${childPath}`); + log_verbose(`File does not exist, but is listed as directory entry: ${childPath}`); + continue; } if (stat.isDirectory()) { yield deleteDirectory(childPath); @@ -277,11 +302,17 @@ function main(args, runfiles) { stats = yield gracefulLstat(p); } if (runfiles.manifest && execroot && stats !== null && stats.isSymbolicLink()) { - const symlinkPath = fs.readlinkSync(p).replace(/\\/g, '/'); - if (path.relative(symlinkPath, target) != '' && - !path.relative(execroot, symlinkPath).startsWith('..')) { - log_verbose(`Out-of-date symlink for ${p} to ${symlinkPath} detected. Target should be ${target}. Unlinking.`); - yield unlink(p); + const symlinkPathRaw = gracefulReadlink(p); + if (symlinkPathRaw !== null) { + const symlinkPath = symlinkPathRaw.replace(/\\/g, '/'); + if (path.relative(symlinkPath, target) != '' && + !path.relative(execroot, symlinkPath).startsWith('..')) { + log_verbose(`Out-of-date symlink for ${p} to ${symlinkPath} detected. Target should be ${target}. Unlinking.`); + yield unlink(p); + } + else { + log_verbose(`The symlink at ${p} no longer exists, so no need to unlink it.`); + } } } return symlink(target, p); diff --git a/internal/linker/link_node_modules.ts b/internal/linker/link_node_modules.ts index 412a9b45db..1ca96a1342 100644 --- a/internal/linker/link_node_modules.ts +++ b/internal/linker/link_node_modules.ts @@ -59,6 +59,36 @@ async function gracefulLstat(path: string): Promise { } } +/** + * Resolves a symlink to its linked path for a given path. Returns `null` if the path + * does not exist on disk. + */ +function gracefulReadlink(path: string): string|null { + try { + return fs.readlinkSync(path); + } catch (e) { + if (e.code === 'ENOENT') { + return null; + } + throw e; + } +} + +/** + * Lists the names of files and directories that exist in the given path. Returns an empty + * array if the path does not exist on disk. + */ +async function gracefulReaddir(path: string): Promise { + try { + return await fs.promises.readdir(path); + } catch (e) { + if (e.code === 'ENOENT') { + return []; + } + throw e; + } +} + /** * Deletes the given module name from the current working directory (i.e. symlink root). * If the module name resolves to a directory, the directory is deleted. Otherwise the @@ -81,11 +111,12 @@ async function unlink(moduleName: string) { /** Asynchronously deletes a given directory (with contents). */ async function deleteDirectory(p: string) { log_verbose("Deleting children of", p); - for (let entry of await fs.promises.readdir(p)) { + for (let entry of await gracefulReaddir(p)) { const childPath = path.join(p, entry); const stat = await gracefulLstat(childPath); if (stat === null) { - throw Error(`File does not exist, but is listed as directory entry: ${childPath}`); + log_verbose(`File does not exist, but is listed as directory entry: ${childPath}`); + continue; } if (stat.isDirectory()) { await deleteDirectory(childPath); @@ -413,18 +444,25 @@ export async function main(args: string[], runfiles: Runfiles) { // then this is guaranteed to be not an artifact from a previous linker run. If not we need to // check. if (runfiles.manifest && execroot && stats !== null && stats.isSymbolicLink()) { - const symlinkPath = fs.readlinkSync(p).replace(/\\/g, '/'); - if (path.relative(symlinkPath, target) != '' && - !path.relative(execroot, symlinkPath).startsWith('..')) { - // Left-over out-of-date symlink from previous run. This can happen if switching between - // root configuration options such as `--noenable_runfiles` and/or - // `--spawn_strategy=standalone`. It can also happen if two different targets link the same - // module name to different targets in a non-sandboxed environment. The latter will lead to - // undeterministic behavior. - // TODO: can we detect the latter case and throw an apprioriate error? - log_verbose(`Out-of-date symlink for ${p} to ${symlinkPath} detected. Target should be ${ - target}. Unlinking.`); - await unlink(p); + // Although `stats` suggests that the file exists as a symlink, it may have been deleted by + // another process. Only proceed unlinking if the file actually still exists. + const symlinkPathRaw = gracefulReadlink(p); + if (symlinkPathRaw !== null) { + const symlinkPath = symlinkPathRaw.replace(/\\/g, '/'); + if (path.relative(symlinkPath, target) != '' && + !path.relative(execroot, symlinkPath).startsWith('..')) { + // Left-over out-of-date symlink from previous run. This can happen if switching between + // root configuration options such as `--noenable_runfiles` and/or + // `--spawn_strategy=standalone`. It can also happen if two different targets link the + // same module name to different targets in a non-sandboxed environment. The latter will + // lead to undeterministic behavior. + // TODO: can we detect the latter case and throw an apprioriate error? + log_verbose(`Out-of-date symlink for ${p} to ${symlinkPath} detected. Target should be ${ + target}. Unlinking.`); + await unlink(p); + } else { + log_verbose(`The symlink at ${p} no longer exists, so no need to unlink it.`); + } } } return symlink(target, p);