diff --git a/packages/appkit/src/core/plugin-context.ts b/packages/appkit/src/core/plugin-context.ts index eda3f05a5..cae027284 100644 --- a/packages/appkit/src/core/plugin-context.ts +++ b/packages/appkit/src/core/plugin-context.ts @@ -2,6 +2,7 @@ import type express from "express"; import type { BasePlugin, IAppRequest, ToolProvider } from "shared"; import { createLogger } from "../logging/logger"; import { SpanStatusCode, TelemetryManager } from "../telemetry"; +import { forwardAsyncErrors } from "../utils/safe-handler"; const logger = createLogger("plugin-context"); @@ -287,7 +288,9 @@ export class PluginContext { if (typeof app[method] === "function") { (app[method] as (...a: unknown[]) => void)( route.path, - ...route.handlers, + // Forward sync throws and async rejections to the Express error + // middleware — Express 4 does not do this on its own. + ...route.handlers.map(forwardAsyncErrors), ); } }); @@ -299,7 +302,9 @@ export class PluginContext { ): void { if (!this.routeTarget) return; this.routeTarget.addExtension((app) => { - app.use(path, ...handlers); + // forwardAsyncErrors leaves 4-arg error middleware untouched, so error + // handlers passed through addMiddleware keep their Express semantics. + app.use(path, ...handlers.map(forwardAsyncErrors)); }); } } diff --git a/packages/appkit/src/core/tests/plugin-context.test.ts b/packages/appkit/src/core/tests/plugin-context.test.ts index 868cb183f..3f548ec57 100644 --- a/packages/appkit/src/core/tests/plugin-context.test.ts +++ b/packages/appkit/src/core/tests/plugin-context.test.ts @@ -75,7 +75,7 @@ describe("PluginContext", () => { expect(ctx.getPluginNames()).toEqual([]); }); - test("flushRoutes applies buffered routes via addExtension", () => { + test("flushRoutes applies buffered routes via addExtension", async () => { const handler = vi.fn(); ctx.addRoute("post", "/invocations", handler); @@ -87,10 +87,19 @@ describe("PluginContext", () => { const mockApp = { post: vi.fn() }; extensionFn(mockApp); - expect(mockApp.post).toHaveBeenCalledWith("/invocations", handler); + // Handlers are wrapped (forwardAsyncErrors) so async rejections reach + // the Express error middleware — assert the wrapper delegates. + expect(mockApp.post).toHaveBeenCalledWith( + "/invocations", + expect.any(Function), + ); + const wrapped = mockApp.post.mock.calls[0][1]; + const next = vi.fn(); + await wrapped("req", "res", next); + expect(handler).toHaveBeenCalledWith("req", "res", next); }); - test("addRoute called after registerAsRouteTarget applies immediately", () => { + test("addRoute called after registerAsRouteTarget applies immediately", async () => { const addExtension = vi.fn(); ctx.registerAsRouteTarget({ addExtension }); @@ -102,10 +111,13 @@ describe("PluginContext", () => { const mockApp = { get: vi.fn() }; extensionFn(mockApp); - expect(mockApp.get).toHaveBeenCalledWith("/health", handler); + expect(mockApp.get).toHaveBeenCalledWith("/health", expect.any(Function)); + const wrapped = mockApp.get.mock.calls[0][1]; + await wrapped("req", "res", vi.fn()); + expect(handler).toHaveBeenCalled(); }); - test("addRoute supports middleware chains", () => { + test("addRoute supports middleware chains", async () => { const auth = vi.fn(); const handler = vi.fn(); @@ -117,10 +129,19 @@ describe("PluginContext", () => { const extensionFn = addExtension.mock.calls[0][0]; const mockApp = { post: vi.fn() }; extensionFn(mockApp); - expect(mockApp.post).toHaveBeenCalledWith("/api", auth, handler); + expect(mockApp.post).toHaveBeenCalledWith( + "/api", + expect.any(Function), + expect.any(Function), + ); + const [, wrappedAuth, wrappedHandler] = mockApp.post.mock.calls[0]; + await wrappedAuth("req", "res", vi.fn()); + await wrappedHandler("req", "res", vi.fn()); + expect(auth).toHaveBeenCalled(); + expect(handler).toHaveBeenCalled(); }); - test("addMiddleware buffers and applies via use()", () => { + test("addMiddleware buffers and applies via use()", async () => { const handler = vi.fn(); ctx.addMiddleware("/api", handler); @@ -132,7 +153,46 @@ describe("PluginContext", () => { const mockApp = { use: vi.fn() }; extensionFn(mockApp); - expect(mockApp.use).toHaveBeenCalledWith("/api", handler); + expect(mockApp.use).toHaveBeenCalledWith("/api", expect.any(Function)); + const wrapped = mockApp.use.mock.calls[0][1]; + await wrapped("req", "res", vi.fn()); + expect(handler).toHaveBeenCalled(); + }); + + test("wrapped route handlers forward async rejections to next", async () => { + const failure = new Error("handler rejected"); + const handler = vi.fn().mockRejectedValue(failure); + const addExtension = vi.fn(); + ctx.registerAsRouteTarget({ addExtension }); + + ctx.addRoute("get", "/boom", handler); + + const extensionFn = addExtension.mock.calls[0][0]; + const mockApp = { get: vi.fn() }; + extensionFn(mockApp); + const wrapped = mockApp.get.mock.calls[0][1]; + const next = vi.fn(); + await wrapped("req", "res", next); + expect(next).toHaveBeenCalledWith(failure); + }); + + test("addMiddleware leaves 4-arg error middleware unwrapped", () => { + const errorMiddleware = ( + _err: unknown, + _req: unknown, + _res: unknown, + _next: unknown, + ) => {}; + const addExtension = vi.fn(); + ctx.registerAsRouteTarget({ addExtension }); + + ctx.addMiddleware("/api", errorMiddleware as never); + + const extensionFn = addExtension.mock.calls[0][0]; + const mockApp = { use: vi.fn() }; + extensionFn(mockApp); + // Express detects error middleware by arity — wrapping would break it. + expect(mockApp.use).toHaveBeenCalledWith("/api", errorMiddleware); }); test("multiple buffered routes are all applied on registration", () => { diff --git a/packages/appkit/src/plugin/plugin.ts b/packages/appkit/src/plugin/plugin.ts index c8138fd39..17030125e 100644 --- a/packages/appkit/src/plugin/plugin.ts +++ b/packages/appkit/src/plugin/plugin.ts @@ -25,6 +25,7 @@ import { TelemetryManager, } from "../telemetry"; import { deepMerge } from "../utils"; +import { forwardAsyncErrors } from "../utils/safe-handler"; import { DevFileReader } from "./dev-reader"; import type { ExecutionResult } from "./execution-result"; import { CacheInterceptor } from "./interceptors/cache"; @@ -666,7 +667,9 @@ export abstract class Plugin< ): void { const { name, method, path, handler } = config; - router[method](path, handler); + // Sync throws and async rejections are forwarded to the Express error + // middleware via next(err) — Express 4 does not do this on its own. + router[method](path, forwardAsyncErrors(handler)); const fullPath = `/api/${this.name}${path}`; this.registerEndpoint(name, fullPath); diff --git a/packages/appkit/src/plugins/agents/agents.ts b/packages/appkit/src/plugins/agents/agents.ts index c63ec094f..33e0b30df 100644 --- a/packages/appkit/src/plugins/agents/agents.ts +++ b/packages/appkit/src/plugins/agents/agents.ts @@ -771,9 +771,10 @@ export class AgentsPlugin extends Plugin implements ToolProvider { */ private mountInvokeRoutes() { if (!this.context) return; - const handler = (req: express.Request, res: express.Response) => { + // Return the promise so the forwardAsyncErrors wrapper applied by + // PluginContext.addRoute can forward rejections to the error middleware. + const handler = (req: express.Request, res: express.Response) => this._handleInvoke(req, res); - }; this.context.addRoute("post", "/invocations", handler); this.context.addRoute("post", "/responses", handler); } diff --git a/packages/appkit/src/plugins/server/index.ts b/packages/appkit/src/plugins/server/index.ts index e66abf5ad..ea878058d 100644 --- a/packages/appkit/src/plugins/server/index.ts +++ b/packages/appkit/src/plugins/server/index.ts @@ -5,7 +5,7 @@ import dotenv from "dotenv"; import express from "express"; import getPort, { portNumbers } from "get-port"; import type { PluginClientConfigs, PluginPhase } from "shared"; -import { ServerError } from "../../errors"; +import { AppKitError, ServerError } from "../../errors"; import { TelemetryReporter } from "../../internal-telemetry"; import { createLogger } from "../../logging/logger"; import { Plugin, toPlugin } from "../../plugin"; @@ -147,6 +147,12 @@ export class ServerPlugin extends Plugin { await this.setupFrontend(endpoints, pluginConfigs); + // Terminal error handler — registered after all routes, extensions, and + // frontend middleware so that errors forwarded via next(err) from any of + // them (e.g. async handler rejections forwarded by Plugin.route()) are + // turned into JSON error responses instead of hanging the request. + this.serverApplication.use(errorHandlerMiddleware); + const listenPort = await this.resolveListenPort(); const server = this.serverApplication.listen( @@ -192,6 +198,11 @@ export class ServerPlugin extends Plugin { * Call this inside the `onPluginsReady` callback of `createApp` to register * custom Express routes or middleware before the server starts listening. * + * Note: async handlers registered directly on the app must handle their own + * rejections — Express 4 does not forward rejected promises to the error + * middleware. Errors passed explicitly to `next(err)` are formatted by the + * terminal error middleware. + * * @param fn - A function that receives the express application. * @returns The server plugin instance for chaining. */ @@ -505,6 +516,87 @@ export function requestMetricsMiddleware( next(); } +/** + * Narrow an unknown thrown value to an Error that carries a numeric + * `statusCode` property in the HTTP error range (e.g. `ApiError` from + * `@databricks/sdk-experimental`). + */ +function hasHttpStatusCode( + error: unknown, +): error is Error & { statusCode: number } { + if (!(error instanceof Error) || !("statusCode" in error)) return false; + const statusCode = (error as Record).statusCode; + return ( + typeof statusCode === "number" && + Number.isInteger(statusCode) && + statusCode >= 400 && + statusCode <= 599 + ); +} + +/** + * Terminal Express error-handling middleware. + * + * Converts errors forwarded via `next(err)` (including async handler + * rejections forwarded by `Plugin.route()`) into JSON error responses, + * following the same status/message conventions as `Plugin.execute()`: + * - errors carrying an HTTP `statusCode` (including `AppKitError`) → that + * status; 5xx messages are masked in production to avoid leaking internals + * - anything else → 500 with a generic message in production + * + * 4xx errors are expected client errors (e.g. missing auth headers) and are + * logged at `warn` with just the message; 5xx/unknown errors are logged at + * `error` with the full error. + * + * @internal Exported for unit tests. + */ +export function errorHandlerMiddleware( + err: unknown, + req: express.Request, + res: express.Response, + next: express.NextFunction, +) { + const httpError = + err instanceof AppKitError || hasHttpStatusCode(err) ? err : null; + const statusCode = httpError ? httpError.statusCode : 500; + const isClientError = statusCode < 500; + + if (isClientError) { + logger.warn( + "Request failed for %s %s with status %d: %s", + req.method, + req.originalUrl, + statusCode, + err instanceof Error ? err.message : String(err), + ); + } else { + logger.error( + "Unhandled error for %s %s: %O", + req.method, + req.originalUrl, + err, + ); + } + + if (res.headersSent) { + next(err); + return; + } + + const isDev = process.env.NODE_ENV !== "production"; + + if (httpError) { + res.status(statusCode).json({ + error: isDev || isClientError ? httpError.message : "Server error", + }); + return; + } + + res.status(statusCode).json({ + error: isDev && err instanceof Error ? err.message : "Server error", + }); +} + /** * @internal */ diff --git a/packages/appkit/src/plugins/server/tests/error-middleware.test.ts b/packages/appkit/src/plugins/server/tests/error-middleware.test.ts new file mode 100644 index 000000000..9742e07e3 --- /dev/null +++ b/packages/appkit/src/plugins/server/tests/error-middleware.test.ts @@ -0,0 +1,223 @@ +import { afterEach, describe, expect, test, vi } from "vitest"; +import { + AuthenticationError, + ServerError, + ValidationError, +} from "../../../errors"; +import { errorHandlerMiddleware } from "../index"; + +const loggerSpies = vi.hoisted(() => ({ + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + event: vi.fn(), +})); + +vi.mock("../../../logging/logger", async (importOriginal) => { + const actual = + await importOriginal(); + return { ...actual, createLogger: () => loggerSpies }; +}); + +function makeReq() { + return { method: "GET", originalUrl: "/api/genie/default/messages" } as any; +} + +function makeRes(headersSent = false) { + const res: any = { headersSent }; + res.status = vi.fn().mockReturnValue(res); + res.json = vi.fn().mockReturnValue(res); + return res; +} + +describe("errorHandlerMiddleware", () => { + const originalNodeEnv = process.env.NODE_ENV; + + afterEach(() => { + process.env.NODE_ENV = originalNodeEnv; + vi.clearAllMocks(); + }); + + test("maps AppKitError to its statusCode with its message", () => { + const res = makeRes(); + const next = vi.fn(); + + errorHandlerMiddleware( + AuthenticationError.missingToken("user token"), + makeReq(), + res, + next, + ); + + expect(res.status).toHaveBeenCalledWith(401); + expect(res.json).toHaveBeenCalledWith({ + error: "Missing user token in request headers", + }); + expect(next).not.toHaveBeenCalled(); + }); + + test("preserves 4xx AppKitError messages in production (client-safe by design)", () => { + process.env.NODE_ENV = "production"; + const res = makeRes(); + + errorHandlerMiddleware( + new ValidationError("content is required"), + makeReq(), + res, + vi.fn(), + ); + + expect(res.status).toHaveBeenCalledWith(400); + expect(res.json).toHaveBeenCalledWith({ error: "content is required" }); + }); + + test("masks 5xx AppKitError messages in production", () => { + process.env.NODE_ENV = "production"; + const res = makeRes(); + + errorHandlerMiddleware( + new ServerError("internal connection details"), + makeReq(), + res, + vi.fn(), + ); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith({ error: "Server error" }); + }); + + test("preserves 5xx AppKitError messages outside production", () => { + process.env.NODE_ENV = "test"; + const res = makeRes(); + + errorHandlerMiddleware( + new ServerError("internal connection details"), + makeReq(), + res, + vi.fn(), + ); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith({ + error: "internal connection details", + }); + }); + + test("maps unknown errors to 500 with the message outside production", () => { + const res = makeRes(); + + errorHandlerMiddleware(new Error("boom"), makeReq(), res, vi.fn()); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith({ error: "boom" }); + }); + + test("masks unknown error messages in production", () => { + process.env.NODE_ENV = "production"; + const res = makeRes(); + + errorHandlerMiddleware( + new Error("internal details"), + makeReq(), + res, + vi.fn(), + ); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith({ error: "Server error" }); + }); + + test("preserves statusCode-bearing 4xx error messages in production", () => { + process.env.NODE_ENV = "production"; + const res = makeRes(); + const err = Object.assign(new Error("resource not found"), { + statusCode: 404, + }); + + errorHandlerMiddleware(err, makeReq(), res, vi.fn()); + + expect(res.status).toHaveBeenCalledWith(404); + expect(res.json).toHaveBeenCalledWith({ error: "resource not found" }); + }); + + test("masks statusCode-bearing 5xx error messages in production", () => { + process.env.NODE_ENV = "production"; + const res = makeRes(); + const err = Object.assign(new Error("upstream exploded"), { + statusCode: 502, + }); + + errorHandlerMiddleware(err, makeReq(), res, vi.fn()); + + expect(res.status).toHaveBeenCalledWith(502); + expect(res.json).toHaveBeenCalledWith({ error: "Server error" }); + }); + + test("ignores non-integer statusCode values and falls back to 500", () => { + const res = makeRes(); + const err = Object.assign(new Error("weird status"), { + statusCode: 404.5, + }); + + errorHandlerMiddleware(err, makeReq(), res, vi.fn()); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith({ error: "weird status" }); + }); + + test("logs 4xx errors at warn with the message, not the full error", () => { + const res = makeRes(); + const err = AuthenticationError.missingToken("user token"); + + errorHandlerMiddleware(err, makeReq(), res, vi.fn()); + + expect(loggerSpies.warn).toHaveBeenCalledTimes(1); + expect(loggerSpies.warn).toHaveBeenCalledWith( + expect.any(String), + "GET", + "/api/genie/default/messages", + 401, + err.message, + ); + expect(loggerSpies.error).not.toHaveBeenCalled(); + }); + + test("logs 5xx and unknown errors at error with the error object", () => { + const res = makeRes(); + const err = new Error("boom"); + + errorHandlerMiddleware(err, makeReq(), res, vi.fn()); + + expect(loggerSpies.error).toHaveBeenCalledTimes(1); + expect(loggerSpies.error).toHaveBeenCalledWith( + expect.any(String), + "GET", + "/api/genie/default/messages", + err, + ); + expect(loggerSpies.warn).not.toHaveBeenCalled(); + }); + + test("delegates to next(err) when headers are already sent", () => { + const res = makeRes(true); + const next = vi.fn(); + const err = new Error("late failure"); + + errorHandlerMiddleware(err, makeReq(), res, next); + + expect(next).toHaveBeenCalledWith(err); + expect(res.status).not.toHaveBeenCalled(); + expect(res.json).not.toHaveBeenCalled(); + }); + + test("handles non-Error thrown values with a generic 500", () => { + process.env.NODE_ENV = "production"; + const res = makeRes(); + + errorHandlerMiddleware("string error", makeReq(), res, vi.fn()); + + expect(res.status).toHaveBeenCalledWith(500); + expect(res.json).toHaveBeenCalledWith({ error: "Server error" }); + }); +}); diff --git a/packages/appkit/src/plugins/server/tests/server.integration.test.ts b/packages/appkit/src/plugins/server/tests/server.integration.test.ts index 0b67e4c06..b7171fe65 100644 --- a/packages/appkit/src/plugins/server/tests/server.integration.test.ts +++ b/packages/appkit/src/plugins/server/tests/server.integration.test.ts @@ -6,9 +6,10 @@ import { afterAll, beforeAll, describe, expect, test } from "vitest"; process.env.DATABRICKS_APP_PORT = "8000"; process.env.FLASK_RUN_HOST = "0.0.0.0"; -import type { PluginManifest } from "shared"; +import type { IAppResponse, IAppRouter, PluginManifest } from "shared"; import { ServiceContext } from "../../../context/service-context"; import { createApp } from "../../../core"; +import { AuthenticationError } from "../../../errors"; import { Plugin, toPlugin } from "../../../plugin"; import { server as serverPlugin } from "../index"; @@ -278,6 +279,204 @@ describe("createApp with async onPluginsReady callback", () => { }); }); +describe("ServerPlugin error handling for rejected async handlers", () => { + let server: Server; + let baseUrl: string; + let serviceContextMock: Awaited>; + let originalNodeEnv: string | undefined; + const TEST_PORT = 9879; + const unhandledRejections: unknown[] = []; + // Only count rejections raised by this suite's handlers — other suites in + // the same worker may legitimately produce unrelated rejections. + const suiteErrorMessages = new Set([ + "boom", + "buffered boom", + "returned promise boom", + AuthenticationError.missingToken("user token").message, + ]); + const onUnhandledRejection = (reason: unknown) => { + if (reason instanceof Error && suiteErrorMessages.has(reason.message)) { + unhandledRejections.push(reason); + } + }; + + beforeAll(async () => { + // The 401 test requires non-development (asUser must throw instead of + // falling back) and the "boom" assertion requires non-production (the + // message must not be masked) — pin NODE_ENV so both are deterministic. + originalNodeEnv = process.env.NODE_ENV; + process.env.NODE_ENV = "test"; + + setupDatabricksEnv(); + ServiceContext.reset(); + serviceContextMock = await mockServiceContext(); + process.on("unhandledRejection", onUnhandledRejection); + + // Plugin whose handlers reject before writing a response — mirrors the + // genie/serving pattern of `await this.asUser(req)._handleX(req, res)` + // hit without forwarded auth headers. + class ThrowingPlugin extends Plugin { + static manifest = { + name: "throwing-plugin", + displayName: "Throwing Plugin", + description: "Test plugin whose handlers reject", + resources: { required: [], optional: [] }, + } satisfies PluginManifest<"throwing-plugin">; + + async setup() { + // Registered through PluginContext's buffering API — exercises the + // applyRoute path, which must wrap handlers like Plugin.route() does. + this.context?.addRoute("get", "/buffered-error", async () => { + throw new Error("buffered boom"); + }); + + // Mirrors the agents plugin invoke shape: a non-async handler that + // calls an async rejecting method and RETURNS its promise. The + // forwardAsyncErrors wrapper can only catch the rejection if the + // promise is returned. + this.context?.addRoute("post", "/buffered-returned-promise", () => + this._rejectingMethod(), + ); + } + + async _rejectingMethod(): Promise { + throw new Error("returned promise boom"); + } + + injectRoutes(router: IAppRouter) { + this.route(router, { + name: "authError", + method: "get", + path: "/auth-error", + handler: async (req, res) => { + // asUser() throws AuthenticationError when the request has no + // x-forwarded-access-token header (outside development mode). + await this.asUser(req)._whoAmI(res); + }, + }); + + this.route(router, { + name: "genericError", + method: "get", + path: "/generic-error", + handler: async () => { + throw new Error("boom"); + }, + }); + } + + async _whoAmI(res: IAppResponse): Promise { + res.json({ ok: true }); + } + } + + const throwingPlugin = toPlugin(ThrowingPlugin); + + const app = await createApp({ + plugins: [ + serverPlugin({ + port: TEST_PORT, + host: "127.0.0.1", + }), + throwingPlugin({}), + ], + }); + + server = app.server.getServer(); + baseUrl = `http://127.0.0.1:${TEST_PORT}`; + + await new Promise((resolve) => setTimeout(resolve, 100)); + }); + + afterAll(async () => { + if (originalNodeEnv === undefined) { + delete process.env.NODE_ENV; + } else { + process.env.NODE_ENV = originalNodeEnv; + } + process.off("unhandledRejection", onUnhandledRejection); + serviceContextMock?.restore(); + if (server) { + await new Promise((resolve, reject) => { + server.close((err) => { + if (err) reject(err); + else resolve(); + }); + }); + } + }); + + test("handler throwing AuthenticationError returns 401 JSON without unhandled rejection", async () => { + const response = await fetch( + `${baseUrl}/api/throwing-plugin/auth-error`, + // No x-forwarded-access-token / x-forwarded-user headers. + ); + + expect(response.status).toBe(401); + expect(response.headers.get("content-type")).toContain("application/json"); + + const data = (await response.json()) as { error: string }; + expect(data.error).toBe( + AuthenticationError.missingToken("user token").message, + ); + + // Give a potential unhandledRejection a chance to fire. + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(unhandledRejections).toEqual([]); + }); + + test("handler throwing a generic error returns 500 JSON without unhandled rejection", async () => { + const response = await fetch( + `${baseUrl}/api/throwing-plugin/generic-error`, + ); + + expect(response.status).toBe(500); + expect(response.headers.get("content-type")).toContain("application/json"); + + const data = (await response.json()) as { error: string }; + // NODE_ENV is pinned to "test" in beforeAll, so the original message + // is surfaced; in production it would be masked as "Server error". + expect(data.error).toBe("boom"); + + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(unhandledRejections).toEqual([]); + }); + + test("buffered route with rejecting async handler returns 500 JSON without unhandled rejection", async () => { + // /buffered-error is mounted on the root app via PluginContext.addRoute, + // not via Plugin.route() — its handler must be wrapped by applyRoute. + const response = await fetch(`${baseUrl}/buffered-error`); + + expect(response.status).toBe(500); + expect(response.headers.get("content-type")).toContain("application/json"); + + const data = (await response.json()) as { error: string }; + expect(data.error).toBe("buffered boom"); + + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(unhandledRejections).toEqual([]); + }); + + test("buffered handler returning a rejecting method's promise returns 500 JSON without unhandled rejection", async () => { + // Locks in the contract relied on by the agents `/invocations` and + // `/responses` routes: a handler that returns its promise has rejections + // forwarded to the error middleware instead of escaping as + // unhandledRejection. + const response = await fetch(`${baseUrl}/buffered-returned-promise`, { + method: "POST", + }); + + expect(response.status).toBe(500); + expect(response.headers.get("content-type")).toContain("application/json"); + + const data = (await response.json()) as { error: string }; + expect(data.error).toBe("returned promise boom"); + + await new Promise((resolve) => setTimeout(resolve, 50)); + expect(unhandledRejections).toEqual([]); + }); +}); + describe("createApp without server plugin", () => { let serviceContextMock: Awaited>; let onPluginsReadyWasCalled = false; diff --git a/packages/appkit/src/utils/safe-handler.ts b/packages/appkit/src/utils/safe-handler.ts new file mode 100644 index 000000000..e451946f3 --- /dev/null +++ b/packages/appkit/src/utils/safe-handler.ts @@ -0,0 +1,34 @@ +import type express from "express"; + +/** + * Wrap an Express handler so synchronous throws and async rejections are + * forwarded to the error-handling middleware via `next(err)`. + * + * Express 4 does not catch rejected promises from async handlers — a + * rejection (e.g. `asUser()` throwing `AuthenticationError` before any + * response is written) escapes as an unhandledRejection, which leaves the + * request hanging and crashes Node by default. + * + * Error-handling middleware (arity 4, `(err, req, res, next)`) is returned + * unchanged: Express identifies it by function arity, so wrapping it would + * silently turn it into a regular handler. + * + * Note: handlers must RETURN their promise (e.g. `(req, res) => + * this._handle(req, res)` or `async (req, res) => { await ... }`) for + * rejections to be caught — a fire-and-forget body like + * `(req, res) => { this._handle(req, res); }` hides the promise from the + * wrapper and rejections escape as unhandledRejection again. + */ +export function forwardAsyncErrors( + handler: ( + req: express.Request, + res: express.Response, + next: express.NextFunction, + ) => unknown, +): express.RequestHandler { + if (handler.length > 3) return handler as express.RequestHandler; + return (req, res, next) => + Promise.resolve() + .then(() => handler(req, res, next)) + .catch(next); +}