Skip to content
Draft
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions src/main/src/downloading/downloader.ts
Original file line number Diff line number Diff line change
Expand Up @@ -142,16 +142,24 @@ export async function download<T>(
const fd = await open(dest, flag);
handle = { fd, path: dest };
} catch (err) {
throw new DownloadError({ code: "fs-error", path: dest }, `Failed to open ${dest}`, err);
const errno = getErrorCode(err) ?? undefined;
throw new DownloadError(
{ code: "fs-error", path: dest, errno },
errno ? `Failed to open ${dest} (${errno})` : `Failed to open ${dest}`,
err,
);
}

if (checkpoint && probe.size) {
try {
await handle.fd.truncate(probe.size);
} catch (err) {
const errno = getErrorCode(err) ?? undefined;
throw new DownloadError(
{ code: "fs-error", path: handle.path },
`Failed to truncate ${handle.path}`,
{ code: "fs-error", path: handle.path, errno },
errno
? `Failed to truncate ${handle.path} (${errno})`
: `Failed to truncate ${handle.path}`,
err,
);
}
Expand Down Expand Up @@ -432,9 +440,12 @@ async function downloadStream(
if (progress) progress.bytesWritten += result.bytesWritten;
writePosition += result.bytesWritten;
} catch (err) {
const errno = getErrorCode(err) ?? undefined;
throw new DownloadError(
{ code: "fs-error", path: handle.path },
`Failed to write to ${handle.path}`,
{ code: "fs-error", path: handle.path, errno },
errno
? `Failed to write to ${handle.path} (${errno})`
: `Failed to write to ${handle.path}`,
err,
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -422,6 +422,8 @@ class DownloadView extends ComponentEx<IDownloadViewProps, IComponentState> {
}
const urlInvalid = ["moved permanently", "forbidden", "gone"];
const title = resume ? "Failed to resume download" : "Failed to start download";
// Fall back to `err.code` for raw Node errors that reach here directly.
const errno: string | undefined = err.payload?.errno ?? err.code;
if (err instanceof ProcessCanceled) {
this.props.onShowError(title, err, undefined, false);
} else if (err instanceof UserCanceled) {
Expand Down Expand Up @@ -526,9 +528,9 @@ class DownloadView extends ComponentEx<IDownloadViewProps, IComponentState> {
undefined,
false,
);
} else if (err.code === "ENOSPC") {
} else if (errno === "ENOSPC") {
this.props.onShowError(title, "The disk is full", undefined, false);
} else if (err.code === "EBADF") {
} else if (errno === "EBADF") {
this.props.onShowError(
title,
"Failed to write to disk. If you use a removable media or " +
Expand Down
35 changes: 35 additions & 0 deletions src/shared/src/download-errors.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import { describe, expect, it } from "vitest";

import { downloadErrorToWire, wireToDownloadError } from "./download-errors";
import { DownloadError } from "./types/errors";

const fsErrorErrno = (err: unknown): string | undefined =>
err instanceof DownloadError && err.payload.code === "fs-error" ? err.payload.errno : undefined;

describe("download-errors wire codec", () => {
it("preserves the fs-error errno across the wire", () => {
const wire = downloadErrorToWire(
new DownloadError(
{ code: "fs-error", path: "C:/tmp/__vortex_tmp_0001", errno: "ENOSPC" },
"Failed to write to C:/tmp/__vortex_tmp_0001 (ENOSPC)",
),
);
const wireErrno = wire.payload.code === "fs-error" ? wire.payload.errno : undefined;
expect(wireErrno).toBe("ENOSPC");

const rebuilt = wireToDownloadError(wire);
expect(rebuilt).toBeInstanceOf(DownloadError);
expect(fsErrorErrno(rebuilt)).toBe("ENOSPC");
expect(rebuilt.message).toContain("ENOSPC");
});

it("tolerates an fs-error with no errno", () => {
const rebuilt = wireToDownloadError(
downloadErrorToWire(
new DownloadError({ code: "fs-error", path: "C:/tmp/x" }, "Failed to write to C:/tmp/x"),
),
);
expect(rebuilt).toBeInstanceOf(DownloadError);
expect(fsErrorErrno(rebuilt)).toBeUndefined();
});
});
2 changes: 1 addition & 1 deletion src/shared/src/types/errors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export type DownloadErrorPayload =
| { code: "precondition-failed"; url: URL }
| { code: "protocol-violation"; url: URL }
| { code: "is-html"; url: URL }
| { code: "fs-error"; path: string }
| { code: "fs-error"; path: string; errno?: string }

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really despise this. Node's is incredibly inconsistent when it comes to error data and the error codes it provides are terrible. This is why I went to great lengths for the new FS abstraction to use proper meaningful errors:

interface ParsedNodeError {
code: FileSystemErrorCode;
isTransient: boolean;
originalCode: string;
}
function parseNodeError(err: unknown): ParsedNodeError {
if (!(err instanceof Error)) {
return { code: "generic", isTransient: false, originalCode: "" };
}
// https://nodejs.org/api/errors.html
// NOTE(erri120): Node.js is inconsistent when it comes to data on Errors.
// The error code that we care about is on err.info.code or err.code if err.info doesn't exist
if (!("code" in err) || typeof err.code !== "string") {
return { code: "generic", isTransient: false, originalCode: "" };
}
const originalCode =
"info" in err &&
typeof err.info === "object" &&
err.info !== null &&
"code" in err.info &&
typeof err.info.code === "string"
? err.info.code
: err.code;
// NOTE(erri120): Node.js uses POSIX error names as codes
// https://www.man7.org/linux/man-pages/man3/errno.3.html
// https://nodejs.org/api/errors.html#common-system-errors
if (originalCode === "EACCES" || originalCode === "EPERM") {
// EACCES: Permission denied (POSIX.1-2001).
// EPERM: Operation not permitted (POSIX.1-2001).
return { code: "no permissions", isTransient: false, originalCode };
} else if (originalCode === "ENOENT") {
// ENOENT: No such file or directory (POSIX.1-2001).
return { code: "not found", isTransient: false, originalCode };
} else if (originalCode === "EEXIST") {
// EEXIST: File exists (POSIX.1-2001).
return { code: "already exists", isTransient: false, originalCode };
} else if (originalCode === "ENOSPC") {
// ENOSPC: No space left on device (POSIX.1-2001)
return { code: "no space", isTransient: false, originalCode };
} else if (originalCode === "ENOTDIR") {
// ENOTDIR: Not a directory (POSIX.1-2001).
return { code: "not a directory", isTransient: false, originalCode };
} else if (originalCode === "EISDIR") {
// EISDIR: Is a directory (POSIX.1-2001).
return { code: "not a file", isTransient: false, originalCode };
} else if (originalCode === "ENOTEMPTY") {
// ENOTEMPTY: Directory not empty (POSIX.1-2001).
return { code: "directory not empty", isTransient: false, originalCode };
} else if (originalCode === "EMFILE" || originalCode === "EBUSY") {
// EMFILE: Too many open files (POSIX.1-2001).
// EBUSY: Device or resource busy (POSIX.1-2001)
return { code: "generic", isTransient: true, originalCode };
} else {
return { code: "generic", isTransient: false, originalCode };
}
}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We would need to add support for FileSystemError inside the downloader, because right not it's not properly setup. It seems to be a way bigger PR comparing to this. I mainly wanted to get diagnostics immediately to see whether we have any errors that should be handled.

So the question is, do you want to properly add support for FileSystemError instead of doing this diagnostics PR? I'm fine with going the proper route here, this is not a critical thing AFAIK

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@erri120 I did exactly that, what's you opinion on this approach?

| { code: "resolver-error" };

export class DownloadError extends Error {
Expand Down