From c9f536d3b54e6652b1345c22b3d7e6998583dbf9 Mon Sep 17 00:00:00 2001 From: Dan Van Brunt Date: Fri, 30 May 2025 10:20:56 -0400 Subject: [PATCH 1/7] feat(image-adapter): improve error handling and status codes for image optimization failures --- .../adapters/image-optimization-adapter.ts | 76 ++++++++++++++++++- 1 file changed, 74 insertions(+), 2 deletions(-) diff --git a/packages/open-next/src/adapters/image-optimization-adapter.ts b/packages/open-next/src/adapters/image-optimization-adapter.ts index 1058e4dd..5fe6c4e6 100644 --- a/packages/open-next/src/adapters/image-optimization-adapter.ts +++ b/packages/open-next/src/adapters/image-optimization-adapter.ts @@ -115,9 +115,42 @@ export async function defaultHandler( return buildSuccessResponse(result, options?.streamCreator, etag); } catch (e: any) { error("Failed to optimize image", e); + + // Determine appropriate status code based on error + let statusCode = 500; // Default to 500 for unknown errors + let errorMessage = "Internal server error"; + + // Check if error has HTTP status information + if (e && typeof e === 'object') { + if ('statusCode' in e && typeof e.statusCode === 'number') { + statusCode = e.statusCode; + errorMessage = `HTTP Error: ${statusCode}`; + } else if ('code' in e) { + const code = e.code as string; + if (code === 'ENOTFOUND' || code === 'ECONNREFUSED') { + statusCode = 404; + errorMessage = `Image not found: ${e.message}`; + } + } + + if (e.message && typeof e.message === 'string') { + // Try to extract status codes from error messages + if (e.message.includes('403') || e.message.includes('Access Denied')) { + statusCode = 403; + errorMessage = `Access denied: ${e.message}`; + } else if (e.message.includes('404') || e.message.includes('Not Found')) { + statusCode = 404; + errorMessage = `Image not found: ${e.message}`; + } else { + errorMessage = e.message; + } + } + } + return buildFailureResponse( - "Internal server error", + errorMessage, options?.streamCreator, + statusCode ); } } @@ -255,7 +288,46 @@ async function downloadHandler( try { // Case 1: remote image URL => download the image from the URL if (url.href.toLowerCase().match(/^https?:\/\//)) { - pipeRes(https.get(url), res); + const request = https.get(url, (response) => { + // Check for HTTP error status codes + if (response.statusCode && response.statusCode >= 400) { + error(`Failed to get image: HTTP ${response.statusCode}`); + res.statusCode = response.statusCode; + res.end(); + return; + } + // IncomingMessage is a Readable stream, not a Writable + // We need to pipe it directly to the response + response.pipe(res) + .once("close", () => { + if (!res.headersSent) { + res.statusCode = 200; + } + res.end(); + }) + .once("error", (pipeErr) => { + error("Failed to get image during piping", pipeErr); + if (!res.headersSent) { + res.statusCode = 400; + } + res.end(); + }); + }); + + request.on('error', (err: NodeJS.ErrnoException) => { + error("Failed to get image", err); + // Handle common network errors + if (err && typeof err === 'object' && 'code' in err) { + if (err.code === 'ENOTFOUND' || err.code === 'ECONNREFUSED') { + res.statusCode = 404; + } else { + res.statusCode = 400; + } + } else { + res.statusCode = 400; + } + res.end(); + }); } // Case 2: local image => download the image from S3 else { From dcfe64b2af3e650f3619a287a1583a4e226f7f0a Mon Sep 17 00:00:00 2001 From: Dan Van Brunt Date: Fri, 30 May 2025 13:15:40 -0400 Subject: [PATCH 2/7] lint --- .../adapters/image-optimization-adapter.ts | 38 ++++++++++--------- 1 file changed, 21 insertions(+), 17 deletions(-) diff --git a/packages/open-next/src/adapters/image-optimization-adapter.ts b/packages/open-next/src/adapters/image-optimization-adapter.ts index 5fe6c4e6..7a2dc9ca 100644 --- a/packages/open-next/src/adapters/image-optimization-adapter.ts +++ b/packages/open-next/src/adapters/image-optimization-adapter.ts @@ -115,30 +115,33 @@ export async function defaultHandler( return buildSuccessResponse(result, options?.streamCreator, etag); } catch (e: any) { error("Failed to optimize image", e); - + // Determine appropriate status code based on error let statusCode = 500; // Default to 500 for unknown errors let errorMessage = "Internal server error"; - + // Check if error has HTTP status information - if (e && typeof e === 'object') { - if ('statusCode' in e && typeof e.statusCode === 'number') { + if (e && typeof e === "object") { + if ("statusCode" in e && typeof e.statusCode === "number") { statusCode = e.statusCode; errorMessage = `HTTP Error: ${statusCode}`; - } else if ('code' in e) { + } else if ("code" in e) { const code = e.code as string; - if (code === 'ENOTFOUND' || code === 'ECONNREFUSED') { + if (code === "ENOTFOUND" || code === "ECONNREFUSED") { statusCode = 404; errorMessage = `Image not found: ${e.message}`; } } - - if (e.message && typeof e.message === 'string') { + + if (e.message && typeof e.message === "string") { // Try to extract status codes from error messages - if (e.message.includes('403') || e.message.includes('Access Denied')) { + if (e.message.includes("403") || e.message.includes("Access Denied")) { statusCode = 403; errorMessage = `Access denied: ${e.message}`; - } else if (e.message.includes('404') || e.message.includes('Not Found')) { + } else if ( + e.message.includes("404") || + e.message.includes("Not Found") + ) { statusCode = 404; errorMessage = `Image not found: ${e.message}`; } else { @@ -146,11 +149,11 @@ export async function defaultHandler( } } } - + return buildFailureResponse( errorMessage, options?.streamCreator, - statusCode + statusCode, ); } } @@ -298,7 +301,8 @@ async function downloadHandler( } // IncomingMessage is a Readable stream, not a Writable // We need to pipe it directly to the response - response.pipe(res) + response + .pipe(res) .once("close", () => { if (!res.headersSent) { res.statusCode = 200; @@ -313,12 +317,12 @@ async function downloadHandler( res.end(); }); }); - - request.on('error', (err: NodeJS.ErrnoException) => { + + request.on("error", (err: NodeJS.ErrnoException) => { error("Failed to get image", err); // Handle common network errors - if (err && typeof err === 'object' && 'code' in err) { - if (err.code === 'ENOTFOUND' || err.code === 'ECONNREFUSED') { + if (err && typeof err === "object" && "code" in err) { + if (err.code === "ENOTFOUND" || err.code === "ECONNREFUSED") { res.statusCode = 404; } else { res.statusCode = 400; From 6d36c0a59dcf4b45426a4356df190662bf88e5a2 Mon Sep 17 00:00:00 2001 From: Dan Van Brunt Date: Fri, 30 May 2025 17:05:20 -0400 Subject: [PATCH 3/7] refactor: improve error handling for image optimization to distinguish between client and server errors --- .../adapters/image-optimization-adapter.ts | 112 ++++++++++++++---- 1 file changed, 86 insertions(+), 26 deletions(-) diff --git a/packages/open-next/src/adapters/image-optimization-adapter.ts b/packages/open-next/src/adapters/image-optimization-adapter.ts index 7a2dc9ca..7430a266 100644 --- a/packages/open-next/src/adapters/image-optimization-adapter.ts +++ b/packages/open-next/src/adapters/image-optimization-adapter.ts @@ -28,6 +28,7 @@ import { emptyReadableStream, toReadableStream } from "utils/stream.js"; import type { OpenNextHandlerOptions } from "types/overrides.js"; import { createGenericHandler } from "../core/createGenericHandler.js"; import { resolveImageLoader } from "../core/resolve.js"; +import { IgnorableError } from "../utils/error.js"; import { debug, error } from "./logger.js"; import { optimizeImage } from "./plugins/image-optimization/image-optimization.js"; import { setNodeEnv } from "./util.js"; @@ -114,44 +115,54 @@ export async function defaultHandler( ); return buildSuccessResponse(result, options?.streamCreator, etag); } catch (e: any) { - error("Failed to optimize image", e); - - // Determine appropriate status code based on error + // Determine if this is a client error (4xx) or server error (5xx) let statusCode = 500; // Default to 500 for unknown errors - let errorMessage = "Internal server error"; + const isClientError = + e && + typeof e === "object" && + (("statusCode" in e && + typeof e.statusCode === "number" && + e.statusCode >= 400 && + e.statusCode < 500) || + e.code === "ENOTFOUND" || + e.code === "ECONNREFUSED" || + (e.message && + (e.message.includes("403") || + e.message.includes("404") || + e.message.includes("Access Denied") || + e.message.includes("Not Found")))); + + // Only log actual server errors as errors, log client errors as debug + if (isClientError) { + debug("Client error in image optimization", e); + } else { + error("Failed to optimize image", e); + } - // Check if error has HTTP status information + // Determine appropriate status code based on error type if (e && typeof e === "object") { if ("statusCode" in e && typeof e.statusCode === "number") { statusCode = e.statusCode; - errorMessage = `HTTP Error: ${statusCode}`; } else if ("code" in e) { const code = e.code as string; if (code === "ENOTFOUND" || code === "ECONNREFUSED") { statusCode = 404; - errorMessage = `Image not found: ${e.message}`; } - } - - if (e.message && typeof e.message === "string") { - // Try to extract status codes from error messages + } else if (e.message) { if (e.message.includes("403") || e.message.includes("Access Denied")) { statusCode = 403; - errorMessage = `Access denied: ${e.message}`; } else if ( e.message.includes("404") || e.message.includes("Not Found") ) { statusCode = 404; - errorMessage = `Image not found: ${e.message}`; - } else { - errorMessage = e.message; } } } + // Pass through the original error message from Next.js return buildFailureResponse( - errorMessage, + e.message || "Internal server error", options?.streamCreator, statusCode, ); @@ -318,18 +329,25 @@ async function downloadHandler( }); }); - request.on("error", (err: NodeJS.ErrnoException) => { - error("Failed to get image", err); - // Handle common network errors - if (err && typeof err === "object" && "code" in err) { - if (err.code === "ENOTFOUND" || err.code === "ECONNREFUSED") { - res.statusCode = 404; - } else { - res.statusCode = 400; - } + request.on("error", (err: Error & { code?: string }) => { + // For network errors, these are typically client errors (bad URL, etc.) + // so log as debug instead of error to avoid false alarms + const isClientError = + err.code === "ENOTFOUND" || err.code === "ECONNREFUSED"; + + if (isClientError) { + // Log the full error for debugging but don't expose it to the client + debug("Client error fetching image", { + code: err.code, + message: err.message, + }); + res.statusCode = 404; // Not Found for DNS or connection errors } else { - res.statusCode = 400; + error("Failed to get image", err); + res.statusCode = 400; // Bad Request for other errors } + + // Don't send the error message back to the client res.end(); }); } @@ -357,6 +375,48 @@ async function downloadHandler( } } } catch (e: any) { + // Check if this is a client error (like 404, 403, etc.) + const isClientError = + e && + typeof e === "object" && + (("statusCode" in e && + typeof e.statusCode === "number" && + e.statusCode >= 400 && + e.statusCode < 500) || + e.code === "ENOTFOUND" || + e.code === "ECONNREFUSED" || + (e.message && + (e.message.includes("403") || + e.message.includes("404") || + e.message.includes("Access Denied") || + e.message.includes("Not Found")))); + + if (isClientError) { + debug("Client error downloading image", e); + // Just pass through the original error to preserve Next.js's error handling + // but wrap it in IgnorableError to prevent it from being logged as an error + const clientError = new IgnorableError( + e.message || "Client error downloading image", + ); + + // Preserve the original status code or set an appropriate one + if (e && typeof e === "object") { + if ("statusCode" in e && typeof e.statusCode === "number") { + (clientError as any).statusCode = e.statusCode; + } else if (e.code === "ENOTFOUND" || e.code === "ECONNREFUSED") { + (clientError as any).statusCode = 404; + } else if (e.message?.includes("403")) { + (clientError as any).statusCode = 403; + } else if (e.message?.includes("404")) { + (clientError as any).statusCode = 404; + } else { + (clientError as any).statusCode = 400; + } + } + + throw clientError; + } + error("Failed to download image", e); throw e; } From e7a51a1e2bcbc0199c869fca42fd475bfaeff620 Mon Sep 17 00:00:00 2001 From: Dan Van Brunt Date: Mon, 2 Jun 2025 10:40:39 -0400 Subject: [PATCH 4/7] refactor: improve error handling in image optimization with status codes and error classification --- .../adapters/image-optimization-adapter.ts | 178 +++++++++++------- packages/open-next/src/utils/error.ts | 16 +- 2 files changed, 121 insertions(+), 73 deletions(-) diff --git a/packages/open-next/src/adapters/image-optimization-adapter.ts b/packages/open-next/src/adapters/image-optimization-adapter.ts index 7430a266..013fa73f 100644 --- a/packages/open-next/src/adapters/image-optimization-adapter.ts +++ b/packages/open-next/src/adapters/image-optimization-adapter.ts @@ -28,7 +28,11 @@ import { emptyReadableStream, toReadableStream } from "utils/stream.js"; import type { OpenNextHandlerOptions } from "types/overrides.js"; import { createGenericHandler } from "../core/createGenericHandler.js"; import { resolveImageLoader } from "../core/resolve.js"; -import { IgnorableError } from "../utils/error.js"; +import { + FatalError, + IgnorableError, + RecoverableError, +} from "../utils/error.js"; import { debug, error } from "./logger.js"; import { optimizeImage } from "./plugins/image-optimization/image-optimization.js"; import { setNodeEnv } from "./util.js"; @@ -83,10 +87,8 @@ export async function defaultHandler( // We return a 400 here if imageParams returns an errorMessage // https://github.com/vercel/next.js/blob/512d8283054407ab92b2583ecce3b253c3be7b85/packages/next/src/server/next-server.ts#L937-L941 if ("errorMessage" in imageParams) { - error( - "Error during validation of image params", - imageParams.errorMessage, - ); + const clientError = new RecoverableError(imageParams.errorMessage, 400); + error("Error during validation of image params", clientError); return buildFailureResponse( imageParams.errorMessage, options?.streamCreator, @@ -115,56 +117,17 @@ export async function defaultHandler( ); return buildSuccessResponse(result, options?.streamCreator, etag); } catch (e: any) { - // Determine if this is a client error (4xx) or server error (5xx) - let statusCode = 500; // Default to 500 for unknown errors - const isClientError = - e && - typeof e === "object" && - (("statusCode" in e && - typeof e.statusCode === "number" && - e.statusCode >= 400 && - e.statusCode < 500) || - e.code === "ENOTFOUND" || - e.code === "ECONNREFUSED" || - (e.message && - (e.message.includes("403") || - e.message.includes("404") || - e.message.includes("Access Denied") || - e.message.includes("Not Found")))); + // Determine if this is a client error (4xx) and convert it to appropriate error type + const classifiedError = classifyError(e); - // Only log actual server errors as errors, log client errors as debug - if (isClientError) { - debug("Client error in image optimization", e); - } else { - error("Failed to optimize image", e); - } + // Log with the appropriate level based on the error type + error("Image optimization error", classifiedError); - // Determine appropriate status code based on error type - if (e && typeof e === "object") { - if ("statusCode" in e && typeof e.statusCode === "number") { - statusCode = e.statusCode; - } else if ("code" in e) { - const code = e.code as string; - if (code === "ENOTFOUND" || code === "ECONNREFUSED") { - statusCode = 404; - } - } else if (e.message) { - if (e.message.includes("403") || e.message.includes("Access Denied")) { - statusCode = 403; - } else if ( - e.message.includes("404") || - e.message.includes("Not Found") - ) { - statusCode = 404; - } - } - } - - // Pass through the original error message from Next.js + // Pass through the error message from Next.js return buildFailureResponse( - e.message || "Internal server error", + classifiedError.message || "Internal server error", options?.streamCreator, - statusCode, + classifiedError.statusCode, ); } } @@ -173,6 +136,63 @@ export async function defaultHandler( // Helper functions // ////////////////////// +/** + * Classifies an error and converts it to the appropriate OpenNext error type + * with the correct status code and logging level. + */ +function classifyError(e: any): IgnorableError | RecoverableError | FatalError { + // Default values + let statusCode = 500; + const message = e?.message || "Internal server error"; + + // If it's already an OpenNext error, return it directly + if (e && typeof e === "object" && "__openNextInternal" in e) { + return e; + } + + // Determine if this is a client error (4xx) or server error (5xx) + const isClientError = + e && + typeof e === "object" && + (("statusCode" in e && + typeof e.statusCode === "number" && + e.statusCode >= 400 && + e.statusCode < 500) || + e.code === "ENOTFOUND" || + e.code === "ECONNREFUSED" || + (e.message && + (e.message.includes("403") || + e.message.includes("404") || + e.message.includes("Access Denied") || + e.message.includes("Not Found")))); + + // Determine appropriate status code based on error type + if (e && typeof e === "object") { + if ("statusCode" in e && typeof e.statusCode === "number") { + statusCode = e.statusCode; + } else if ("code" in e) { + const code = e.code as string; + if (code === "ENOTFOUND" || code === "ECONNREFUSED") { + statusCode = 404; + } + } else if (e.message) { + if (e.message.includes("403") || e.message.includes("Access Denied")) { + statusCode = 403; + } else if (e.message.includes("404") || e.message.includes("Not Found")) { + statusCode = 404; + } + } + } + + // Client errors (4xx) are wrapped as IgnorableError to prevent noise in monitoring + if (isClientError || statusCode < 500) { + return new IgnorableError(message, statusCode); + } + + // Server errors (5xx) are marked as FatalError to ensure proper monitoring + return new FatalError(message, statusCode); +} + function validateImageParams( headers: OutgoingHttpHeaders, query?: InternalEvent["query"], @@ -305,11 +325,24 @@ async function downloadHandler( const request = https.get(url, (response) => { // Check for HTTP error status codes if (response.statusCode && response.statusCode >= 400) { - error(`Failed to get image: HTTP ${response.statusCode}`); + // Create an IgnorableError with appropriate status code + const clientError = new IgnorableError( + response.statusMessage || `HTTP error ${response.statusCode}`, + response.statusCode, + ); + + // Log the error using proper error logger to handle it correctly + error("Client error fetching image", clientError, { + status: response.statusCode, + statusText: response.statusMessage, + url: url.href, + }); + res.statusCode = response.statusCode; res.end(); return; } + // IncomingMessage is a Readable stream, not a Writable // We need to pipe it directly to the response response @@ -320,8 +353,12 @@ async function downloadHandler( } res.end(); }) - .once("error", (pipeErr) => { - error("Failed to get image during piping", pipeErr); + .once("error", (pipeErr: Error) => { + const clientError = new IgnorableError( + `Error during image piping: ${pipeErr.message}`, + 400, + ); + error("Failed to get image during piping", clientError); if (!res.headersSent) { res.statusCode = 400; } @@ -330,24 +367,25 @@ async function downloadHandler( }); request.on("error", (err: Error & { code?: string }) => { - // For network errors, these are typically client errors (bad URL, etc.) - // so log as debug instead of error to avoid false alarms + // For network errors, convert to appropriate error type based on error code const isClientError = err.code === "ENOTFOUND" || err.code === "ECONNREFUSED"; - - if (isClientError) { - // Log the full error for debugging but don't expose it to the client - debug("Client error fetching image", { - code: err.code, - message: err.message, - }); - res.statusCode = 404; // Not Found for DNS or connection errors - } else { - error("Failed to get image", err); - res.statusCode = 400; // Bad Request for other errors - } - - // Don't send the error message back to the client + const statusCode = isClientError ? 404 : 400; + + // Create appropriate error type + const clientError = new IgnorableError( + err.message || `Error fetching image: ${err.code || "unknown error"}`, + statusCode, + ); + + // Log with error function but it will be handled properly based on error type + error("Error fetching image", clientError, { + code: err.code, + message: err.message, + url: url.href, + }); + + res.statusCode = statusCode; res.end(); }); } diff --git a/packages/open-next/src/utils/error.ts b/packages/open-next/src/utils/error.ts index c1e8dc9c..352dea36 100644 --- a/packages/open-next/src/utils/error.ts +++ b/packages/open-next/src/utils/error.ts @@ -3,6 +3,7 @@ export interface BaseOpenNextError { readonly canIgnore: boolean; // 0 - debug, 1 - warn, 2 - error readonly logLevel: 0 | 1 | 2; + readonly statusCode?: number; } // This is an error that can be totally ignored @@ -11,9 +12,12 @@ export class IgnorableError extends Error implements BaseOpenNextError { readonly __openNextInternal = true; readonly canIgnore = true; readonly logLevel = 0; - constructor(message: string) { + readonly statusCode: number; + + constructor(message: string, statusCode = 400) { super(message); this.name = "IgnorableError"; + this.statusCode = statusCode; } } @@ -23,9 +27,12 @@ export class RecoverableError extends Error implements BaseOpenNextError { readonly __openNextInternal = true; readonly canIgnore = true; readonly logLevel = 1; - constructor(message: string) { + readonly statusCode: number; + + constructor(message: string, statusCode = 400) { super(message); this.name = "RecoverableError"; + this.statusCode = statusCode; } } @@ -34,9 +41,12 @@ export class FatalError extends Error implements BaseOpenNextError { readonly __openNextInternal = true; readonly canIgnore = false; readonly logLevel = 2; - constructor(message: string) { + readonly statusCode: number; + + constructor(message: string, statusCode = 500) { super(message); this.name = "FatalError"; + this.statusCode = statusCode; } } From eeea47c0cb9aefd4c89ad950bc8f93f1631131b3 Mon Sep 17 00:00:00 2001 From: Dan Van Brunt Date: Mon, 2 Jun 2025 12:33:05 -0400 Subject: [PATCH 5/7] refactor: centralize error handling in image optimization adapter with unified error classification --- .../adapters/image-optimization-adapter.ts | 123 ++++++------------ 1 file changed, 40 insertions(+), 83 deletions(-) diff --git a/packages/open-next/src/adapters/image-optimization-adapter.ts b/packages/open-next/src/adapters/image-optimization-adapter.ts index 013fa73f..5b483041 100644 --- a/packages/open-next/src/adapters/image-optimization-adapter.ts +++ b/packages/open-next/src/adapters/image-optimization-adapter.ts @@ -31,7 +31,7 @@ import { resolveImageLoader } from "../core/resolve.js"; import { FatalError, IgnorableError, - RecoverableError, + type RecoverableError, } from "../utils/error.js"; import { debug, error } from "./logger.js"; import { optimizeImage } from "./plugins/image-optimization/image-optimization.js"; @@ -87,7 +87,8 @@ export async function defaultHandler( // We return a 400 here if imageParams returns an errorMessage // https://github.com/vercel/next.js/blob/512d8283054407ab92b2583ecce3b253c3be7b85/packages/next/src/server/next-server.ts#L937-L941 if ("errorMessage" in imageParams) { - const clientError = new RecoverableError(imageParams.errorMessage, 400); + // Use IgnorableError for client-side validation issues (logLevel 0) to prevent monitoring alerts + const clientError = new IgnorableError(imageParams.errorMessage, 400); error("Error during validation of image params", clientError); return buildFailureResponse( imageParams.errorMessage, @@ -137,59 +138,48 @@ export async function defaultHandler( ////////////////////// /** - * Classifies an error and converts it to the appropriate OpenNext error type - * with the correct status code and logging level. + * Classifies an error as either a client error (IgnorableError) or server error (FatalError) + * to ensure proper logging behavior without triggering false monitoring alerts. + * + * The primary goal is to preserve the original error information while ensuring + * client errors don't trigger monitoring alerts. */ function classifyError(e: any): IgnorableError | RecoverableError | FatalError { - // Default values - let statusCode = 500; - const message = e?.message || "Internal server error"; - // If it's already an OpenNext error, return it directly if (e && typeof e === "object" && "__openNextInternal" in e) { return e; } - // Determine if this is a client error (4xx) or server error (5xx) - const isClientError = + // Preserve the original message + const message = e?.message || "Internal server error"; + + // Preserve the original status code if available, otherwise use a default + let statusCode = 500; + if ( e && typeof e === "object" && - (("statusCode" in e && - typeof e.statusCode === "number" && - e.statusCode >= 400 && - e.statusCode < 500) || - e.code === "ENOTFOUND" || - e.code === "ECONNREFUSED" || - (e.message && - (e.message.includes("403") || - e.message.includes("404") || - e.message.includes("Access Denied") || - e.message.includes("Not Found")))); - - // Determine appropriate status code based on error type - if (e && typeof e === "object") { - if ("statusCode" in e && typeof e.statusCode === "number") { - statusCode = e.statusCode; - } else if ("code" in e) { - const code = e.code as string; - if (code === "ENOTFOUND" || code === "ECONNREFUSED") { - statusCode = 404; - } - } else if (e.message) { - if (e.message.includes("403") || e.message.includes("Access Denied")) { - statusCode = 403; - } else if (e.message.includes("404") || e.message.includes("Not Found")) { - statusCode = 404; - } - } + "statusCode" in e && + typeof e.statusCode === "number" + ) { + statusCode = e.statusCode; } - // Client errors (4xx) are wrapped as IgnorableError to prevent noise in monitoring - if (isClientError || statusCode < 500) { + // Simple check for client errors - anything with a 4xx status code + // or common error codes that indicate client issues + const isClientError = + (statusCode >= 400 && statusCode < 500) || + (e && + typeof e === "object" && + (e.code === "ENOTFOUND" || + e.code === "ECONNREFUSED" || + e.code === "ETIMEDOUT")); + + // Wrap client errors as IgnorableError to prevent monitoring alerts + if (isClientError) { return new IgnorableError(message, statusCode); } - // Server errors (5xx) are marked as FatalError to ensure proper monitoring + // Server errors are marked as FatalError to ensure proper monitoring return new FatalError(message, statusCode); } @@ -413,49 +403,16 @@ async function downloadHandler( } } } catch (e: any) { - // Check if this is a client error (like 404, 403, etc.) - const isClientError = - e && - typeof e === "object" && - (("statusCode" in e && - typeof e.statusCode === "number" && - e.statusCode >= 400 && - e.statusCode < 500) || - e.code === "ENOTFOUND" || - e.code === "ECONNREFUSED" || - (e.message && - (e.message.includes("403") || - e.message.includes("404") || - e.message.includes("Access Denied") || - e.message.includes("Not Found")))); - - if (isClientError) { - debug("Client error downloading image", e); - // Just pass through the original error to preserve Next.js's error handling - // but wrap it in IgnorableError to prevent it from being logged as an error - const clientError = new IgnorableError( - e.message || "Client error downloading image", - ); - - // Preserve the original status code or set an appropriate one - if (e && typeof e === "object") { - if ("statusCode" in e && typeof e.statusCode === "number") { - (clientError as any).statusCode = e.statusCode; - } else if (e.code === "ENOTFOUND" || e.code === "ECONNREFUSED") { - (clientError as any).statusCode = 404; - } else if (e.message?.includes("403")) { - (clientError as any).statusCode = 403; - } else if (e.message?.includes("404")) { - (clientError as any).statusCode = 404; - } else { - (clientError as any).statusCode = 400; - } - } + // Use our centralized error classification function + const classifiedError = classifyError(e); - throw clientError; - } + // Log error with appropriate level based on error type + // This will automatically downgrade client errors to debug level + error("Failed to download image", classifiedError); - error("Failed to download image", e); - throw e; + // Since we're in a middleware (adapter) called by Next.js's image optimizer, + // we should throw the properly classified error to let Next.js handle it appropriately + // The Next.js image optimizer will use the status code and normalize the error message + throw classifiedError; } } From 6f0044b67e54cfad2dcc3a42535da390fd6b9ef7 Mon Sep 17 00:00:00 2001 From: Dan Van Brunt Date: Thu, 5 Jun 2025 13:32:21 -0400 Subject: [PATCH 6/7] refactor: improve error handling and logging for image optimization adapter --- .../adapters/image-optimization-adapter.ts | 281 ++++++++---------- 1 file changed, 125 insertions(+), 156 deletions(-) diff --git a/packages/open-next/src/adapters/image-optimization-adapter.ts b/packages/open-next/src/adapters/image-optimization-adapter.ts index 5b483041..9ecce6d2 100644 --- a/packages/open-next/src/adapters/image-optimization-adapter.ts +++ b/packages/open-next/src/adapters/image-optimization-adapter.ts @@ -28,11 +28,7 @@ import { emptyReadableStream, toReadableStream } from "utils/stream.js"; import type { OpenNextHandlerOptions } from "types/overrides.js"; import { createGenericHandler } from "../core/createGenericHandler.js"; import { resolveImageLoader } from "../core/resolve.js"; -import { - FatalError, - IgnorableError, - type RecoverableError, -} from "../utils/error.js"; +import { IgnorableError } from "../utils/error.js"; import { debug, error } from "./logger.js"; import { optimizeImage } from "./plugins/image-optimization/image-optimization.js"; import { setNodeEnv } from "./util.js"; @@ -87,9 +83,10 @@ export async function defaultHandler( // We return a 400 here if imageParams returns an errorMessage // https://github.com/vercel/next.js/blob/512d8283054407ab92b2583ecce3b253c3be7b85/packages/next/src/server/next-server.ts#L937-L941 if ("errorMessage" in imageParams) { - // Use IgnorableError for client-side validation issues (logLevel 0) to prevent monitoring alerts - const clientError = new IgnorableError(imageParams.errorMessage, 400); - error("Error during validation of image params", clientError); + error( + "Error during validation of image params", + imageParams.errorMessage, + ); return buildFailureResponse( imageParams.errorMessage, options?.streamCreator, @@ -118,17 +115,19 @@ export async function defaultHandler( ); return buildSuccessResponse(result, options?.streamCreator, etag); } catch (e: any) { - // Determine if this is a client error (4xx) and convert it to appropriate error type - const classifiedError = classifyError(e); + // All image-related errors should be treated as client errors + // Extract status code from error or default to 400 Bad Request + const statusCode = e.statusCode || 400; + const errorMessage = e.message || "Failed to process image request"; - // Log with the appropriate level based on the error type - error("Image optimization error", classifiedError); + // Create an IgnorableError for proper monitoring classification + const clientError = new IgnorableError(errorMessage, statusCode); + error("Failed to optimize image", clientError); - // Pass through the error message from Next.js return buildFailureResponse( - classifiedError.message || "Internal server error", + errorMessage, options?.streamCreator, - classifiedError.statusCode, + statusCode, // Use the appropriate status code (preserving original when available) ); } } @@ -137,52 +136,6 @@ export async function defaultHandler( // Helper functions // ////////////////////// -/** - * Classifies an error as either a client error (IgnorableError) or server error (FatalError) - * to ensure proper logging behavior without triggering false monitoring alerts. - * - * The primary goal is to preserve the original error information while ensuring - * client errors don't trigger monitoring alerts. - */ -function classifyError(e: any): IgnorableError | RecoverableError | FatalError { - // If it's already an OpenNext error, return it directly - if (e && typeof e === "object" && "__openNextInternal" in e) { - return e; - } - - // Preserve the original message - const message = e?.message || "Internal server error"; - - // Preserve the original status code if available, otherwise use a default - let statusCode = 500; - if ( - e && - typeof e === "object" && - "statusCode" in e && - typeof e.statusCode === "number" - ) { - statusCode = e.statusCode; - } - - // Simple check for client errors - anything with a 4xx status code - // or common error codes that indicate client issues - const isClientError = - (statusCode >= 400 && statusCode < 500) || - (e && - typeof e === "object" && - (e.code === "ENOTFOUND" || - e.code === "ECONNREFUSED" || - e.code === "ETIMEDOUT")); - - // Wrap client errors as IgnorableError to prevent monitoring alerts - if (isClientError) { - return new IgnorableError(message, statusCode); - } - - // Server errors are marked as FatalError to ensure proper monitoring - return new FatalError(message, statusCode); -} - function validateImageParams( headers: OutgoingHttpHeaders, query?: InternalEvent["query"], @@ -295,124 +248,140 @@ async function downloadHandler( // directly. debug("downloadHandler url", url); - // Reads the output from the Writable and writes to the response - const pipeRes = (w: Writable, res: ServerResponse) => { - w.pipe(res) + /** + * Helper function to handle image errors consistently with appropriate response + * @param e The error object + * @param res The server response object + * @param isInternalImage Whether the error is from an internal image (S3) or external image + */ + function handleImageError( + e: any, + res: ServerResponse, + isInternalImage: boolean, + ) { + let originalStatus = e.statusCode || e.$metadata?.httpStatusCode || 500; + let message = e.message || "Failed to process image request"; + + // Special handling for S3 ListBucket permission errors + // AWS SDK v3 nests error details deeply within the error object + const isListBucketError = + (message.includes("s3:ListBucket") && message.includes("AccessDenied")) || + e.error?.message?.includes("s3:ListBucket") || + (e.Code === "AccessDenied" && e.Message?.includes("s3:ListBucket")); + + if (isListBucketError) { + message = "Image not found or access denied"; + // For S3 ListBucket errors, ensure we're using 403 (the actual AWS error) + if (originalStatus === 500 && e.$metadata?.httpStatusCode === 403) { + originalStatus = 403; + } + + // Log using IgnorableError to classify as client error + const clientError = new IgnorableError(message, originalStatus); + error("S3 ListBucket permission error", clientError); + } else { + // Log all other errors as client errors + const clientError = new IgnorableError(message, originalStatus); + error("Failed to process image", clientError); + } + + // For external images, throw if not ListBucket error + // Next.js will preserve the status code for external images + if (!isInternalImage && !isListBucketError) { + const formattedError = new Error(message); + // @ts-ignore: Add statusCode property to Error + formattedError.statusCode = originalStatus >= 500 ? 400 : originalStatus; + throw formattedError; + } + + // Different handling for internal vs external images + const finalStatus = originalStatus >= 500 ? 400 : originalStatus; + res.statusCode = finalStatus; + + // For internal images, we want to trigger Next.js's "internal response invalid" message + if (isInternalImage) { + // For internal images, don't set Content-Type to trigger Next.js's default error handling + // This should result in "url parameter is valid but internal response is invalid" + + // Still include error details in headers for debugging only + const errorMessage = isListBucketError ? "Access denied" : message; + res.setHeader("x-nextjs-internal-error", errorMessage); + res.end(); + } else { + // For external images, maintain existing behavior with text/plain + res.setHeader("Content-Type", "text/plain"); + + if (isListBucketError) { + res.end("Access denied"); + } else { + res.end(message); + } + } + } + + // Pipes data from a writable stream to the server response + function pipeStream( + stream: Writable, + res: ServerResponse, + isInternalImage: boolean, + ) { + stream + .pipe(res) .once("close", () => { res.statusCode = 200; res.end(); }) .once("error", (err) => { - error("Failed to get image", err); - res.statusCode = 400; - res.end(); + error("Error streaming image data", err); + handleImageError(err, res, isInternalImage); }); - }; + } + // Main handler logic with clearer error paths try { - // Case 1: remote image URL => download the image from the URL + // EXTERNAL IMAGE HANDLING if (url.href.toLowerCase().match(/^https?:\/\//)) { - const request = https.get(url, (response) => { - // Check for HTTP error status codes - if (response.statusCode && response.statusCode >= 400) { - // Create an IgnorableError with appropriate status code - const clientError = new IgnorableError( - response.statusMessage || `HTTP error ${response.statusCode}`, - response.statusCode, - ); - - // Log the error using proper error logger to handle it correctly - error("Client error fetching image", clientError, { - status: response.statusCode, - statusText: response.statusMessage, - url: url.href, - }); - - res.statusCode = response.statusCode; - res.end(); - return; - } - - // IncomingMessage is a Readable stream, not a Writable - // We need to pipe it directly to the response - response - .pipe(res) - .once("close", () => { - if (!res.headersSent) { - res.statusCode = 200; - } - res.end(); - }) - .once("error", (pipeErr: Error) => { - const clientError = new IgnorableError( - `Error during image piping: ${pipeErr.message}`, - 400, - ); - error("Failed to get image during piping", clientError); - if (!res.headersSent) { - res.statusCode = 400; - } - res.end(); - }); - }); - - request.on("error", (err: Error & { code?: string }) => { - // For network errors, convert to appropriate error type based on error code - const isClientError = - err.code === "ENOTFOUND" || err.code === "ECONNREFUSED"; - const statusCode = isClientError ? 404 : 400; - - // Create appropriate error type - const clientError = new IgnorableError( - err.message || `Error fetching image: ${err.code || "unknown error"}`, - statusCode, - ); - - // Log with error function but it will be handled properly based on error type - error("Error fetching image", clientError, { - code: err.code, - message: err.message, - url: url.href, - }); - - res.statusCode = statusCode; - res.end(); - }); + try { + pipeStream(https.get(url), res, false); + } catch (e: any) { + handleImageError(e, res, false); + } + return; } - // Case 2: local image => download the image from S3 - else { - // Download image from S3 - // note: S3 expects keys without leading `/` + // INTERNAL IMAGE HANDLING (S3) + try { const response = await loader.load(url.href); + // Handle empty response body if (!response.body) { - throw new Error("Empty response body from the S3 request."); - } + const message = "Empty response body from the S3 request."; + const clientError = new IgnorableError(message, 400); + error("Empty response from S3", clientError); - // @ts-ignore - pipeRes(response.body, res); + res.statusCode = 400; + res.setHeader("Content-Type", "text/plain"); + res.end(message); + return; + } - // Respect the bucket file's content-type and cache-control - // imageOptimizer will use this to set the results.maxAge + // Set headers from the response if (response.contentType) { res.setHeader("Content-Type", response.contentType); } if (response.cacheControl) { res.setHeader("Cache-Control", response.cacheControl); } + + // Stream the image to the client + // @ts-ignore + pipeStream(response.body, res, true); + } catch (e: any) { + // Direct response for all internal image errors + handleImageError(e, res, true); } } catch (e: any) { - // Use our centralized error classification function - const classifiedError = classifyError(e); - - // Log error with appropriate level based on error type - // This will automatically downgrade client errors to debug level - error("Failed to download image", classifiedError); - - // Since we're in a middleware (adapter) called by Next.js's image optimizer, - // we should throw the properly classified error to let Next.js handle it appropriately - // The Next.js image optimizer will use the status code and normalize the error message - throw classifiedError; + // Catch-all for any unexpected errors + handleImageError(e, res, true); } } From aa3678367b4c2ca64d95d4e48f5dca4c23d3f5e1 Mon Sep 17 00:00:00 2001 From: Dan Van Brunt Date: Mon, 9 Jun 2025 09:52:38 -0400 Subject: [PATCH 7/7] chore: Added changeset --- .changeset/big-laws-jam.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/big-laws-jam.md diff --git a/.changeset/big-laws-jam.md b/.changeset/big-laws-jam.md new file mode 100644 index 00000000..3bc4808d --- /dev/null +++ b/.changeset/big-laws-jam.md @@ -0,0 +1,5 @@ +--- +"@opennextjs/aws": major +--- + +Improved error handling in image optimization adapter for S3 ListBucket permission errors