Skip to content

Commit

Permalink
fix(builtin): account for racy deletion of symlink in linker (#2662)
Browse files Browse the repository at this point in the history
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.
JoostK authored May 11, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
1 parent 911529f commit e9a683d
Showing 2 changed files with 90 additions and 21 deletions.
45 changes: 38 additions & 7 deletions internal/linker/index.js
Original file line number Diff line number Diff line change
@@ -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);
66 changes: 52 additions & 14 deletions internal/linker/link_node_modules.ts
Original file line number Diff line number Diff line change
@@ -59,6 +59,36 @@ async function gracefulLstat(path: string): Promise<fs.Stats|null> {
}
}

/**
* 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<string[]> {
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);

0 comments on commit e9a683d

Please sign in to comment.