Skip to content

fix(cache): make dummy cache error ignorable #641

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

fix(cache): make dummy cache error ignorable #641

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
5 changes: 5 additions & 0 deletions .changeset/great-houses-study.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@opennextjs/aws": patch
---

feat(cache): make dummy cache error ignorable
25 changes: 21 additions & 4 deletions packages/open-next/src/adapters/cache.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { IgnorableError, isIgnorableError } from "utils/error";
import { isBinaryContentType } from "./binary";
import { debug, error, warn } from "./logger";

Expand Down Expand Up @@ -192,7 +193,11 @@ export default class S3Cache {
} as CacheHandlerValue;
} catch (e) {
// We can usually ignore errors here as they are usually due to cache not being found
debug("Failed to get fetch cache", e);
if (isIgnorableError(e)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to do this here, error should already be handling this

opennextjs-aws/packages/open-next/src/adapters/logger.ts

Lines 43 to 67 in 4a500ee

export function error(...args: any[]) {
// we try to catch errors from the aws-sdk client and downplay some of them
if (
args.some((arg: AwsSdkClientCommandErrorLog) => isDownplayedErrorLog(arg))
) {
debug(...args);
} else if (args.some((arg) => arg.__openNextInternal)) {
// In case of an internal error, we log it with the appropriate log level
const error = args.find(
(arg) => arg.__openNextInternal,
) as BaseOpenNextError;
if (error.logLevel === 0) {
debug(...args);
return;
}
if (error.logLevel === 1) {
warn(...args);
return;
}
console.error(...args);
return;
} else {
console.error(...args);
}
}

Having only the error function deciding which error to log is probably better (and easier to read)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I better understand what you meant now (I didn't noticed the handling of internal errors in the logger).

Still I find having IgnorableErrors and OPEN_NEXT_ERROR_LOG_LEVEL a bit overkill.

Let me think more about this.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think IgnorableError alone are enough in most cases, debug mode should never be used outside of when you debug so you should never see such errors in production.
The OPEN_NEXT_ERROR_LOG_LEVEL has 1 main use imo, it's for adding the ability to mask RecoverableError logs, but it could also be used to mask IgnorableError in debug mode, which could be useful for us when testing something that has nothing to do with these errors

debug("Failed to get fetch cache", e.message);
} else {
debug("Failed to get fetch cache", e);
}
return null;
}
}
Expand Down Expand Up @@ -271,7 +276,11 @@ export default class S3Cache {
return null;
} catch (e) {
// We can usually ignore errors here as they are usually due to cache not being found
debug("Failed to get body cache", e);
if (isIgnorableError(e)) {
debug("Failed to get body cache", e.message);
} else {
debug("Failed to get body cache", e);
}
return null;
}
}
Expand Down Expand Up @@ -409,7 +418,11 @@ export default class S3Cache {
}
debug("Finished setting cache");
} catch (e) {
error("Failed to set cache", e);
if (isIgnorableError(e)) {
debug("Failed to set cache", e.message);
} else {
error("Failed to set cache", e);
}
} finally {
// We need to resolve the promise even if there was an error
detachedPromise?.resolve();
Expand Down Expand Up @@ -456,7 +469,11 @@ export default class S3Cache {
await globalThis.tagCache.writeTags(toInsert);
}
} catch (e) {
error("Failed to revalidate tag", e);
if (isIgnorableError(e)) {
debug("Failed to revalidate tag", e.message);
} else {
error("Failed to revalidate tag", e);
}
}
}
}
7 changes: 4 additions & 3 deletions packages/open-next/src/overrides/incrementalCache/dummy.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,16 @@
import type { IncrementalCache } from "types/overrides";
import { IgnorableError } from "utils/error";

const dummyIncrementalCache: IncrementalCache = {
name: "dummy",
get: async () => {
throw new Error("Dummy cache is not implemented");
throw new IgnorableError('"Dummy" cache does not cache anything');
},
set: async () => {
throw new Error("Dummy cache is not implemented");
throw new IgnorableError('"Dummy" cache does not cache anything');
},
delete: async () => {
throw new Error("Dummy cache is not implemented");
throw new IgnorableError('"Dummy" cache does not cache anything');
},
};

Expand Down
11 changes: 10 additions & 1 deletion packages/open-next/src/utils/error.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ export interface BaseOpenNextError {
readonly logLevel: 0 | 1 | 2;
}

const IGNORABLE_ERROR_NAME = "IgnorableError";

// This is an error that can be totally ignored
// It don't even need to be logged, or only in debug mode
export class IgnorableError extends Error implements BaseOpenNextError {
Expand All @@ -13,10 +15,17 @@ export class IgnorableError extends Error implements BaseOpenNextError {
readonly logLevel = 0;
constructor(message: string) {
super(message);
this.name = "IgnorableError";
this.name = IGNORABLE_ERROR_NAME;
}
}

// Note that `e instanceof IgnorableError` might not work across bundles
// embedding a distinct implementation of `IgnorableError`.
export function isIgnorableError(error: unknown): error is IgnorableError {
const anyError = error as any;
return anyError.__openNextInternal && anyError.name === IGNORABLE_ERROR_NAME;
}

// This is an error that can be recovered from
// It should be logged but the process can continue
export class RecoverableError extends Error implements BaseOpenNextError {
Expand Down