Skip to content

Better diagnostics for download errors#23635

Draft
Aragas wants to merge 2 commits into
release/v2.3from
task/LAZ-697
Draft

Better diagnostics for download errors#23635
Aragas wants to merge 2 commits into
release/v2.3from
task/LAZ-697

Conversation

@Aragas

@Aragas Aragas commented Jul 6, 2026

Copy link
Copy Markdown
Member

We still don't know the roo cases, lets make sure the code is being passed
Closes LAZ-697

@Aragas Aragas self-assigned this Jul 6, 2026
@Aragas
Aragas requested a review from a team as a code owner July 6, 2026 12:41
Comment thread src/shared/src/types/errors.ts Outdated
| { 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?

…other packages

Moved FS errors into the contract package
FS errors now use our semantic error handling
@Aragas
Aragas marked this pull request as draft July 8, 2026 14:59
@github-actions

Copy link
Copy Markdown

This PR has been marked as stale due to inactivity.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants