mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-24 11:25:50 -06:00
fix(gateway): finish failed HTTP responses without hanging or crashing (#116874)
* fix(gateway): unify failed HTTP response finalization * test(gateway): avoid returning server-close promise executor --------- Co-authored-by: Peter Steinberger <steipete@macos.shared>
This commit is contained in:
committed by
GitHub
parent
102c2e1a64
commit
c19e55f226
@@ -28,6 +28,24 @@ export function setDefaultSecurityHeaders(
|
||||
}
|
||||
}
|
||||
|
||||
/** Finish a failed request without rewriting committed headers or orphaning its transport. */
|
||||
export function finishFailedGatewayHttpResponse(res: ServerResponse): void {
|
||||
if (res.destroyed || res.writableEnded) {
|
||||
return;
|
||||
}
|
||||
if (!res.headersSent) {
|
||||
res.statusCode = 500;
|
||||
res.setHeader("Content-Type", "text/plain; charset=utf-8");
|
||||
res.end("Internal Server Error");
|
||||
return;
|
||||
}
|
||||
|
||||
// Flush committed bytes before closing; truncated fixed-length bodies cannot reuse this socket.
|
||||
const socket = res.socket;
|
||||
res.end();
|
||||
socket?.end();
|
||||
}
|
||||
|
||||
export function sendJson(res: ServerResponse, status: number, body: unknown) {
|
||||
res.statusCode = status;
|
||||
res.setHeader("Content-Type", "application/json; charset=utf-8");
|
||||
|
||||
@@ -1,9 +1,10 @@
|
||||
// HTTP request trace tests ensure gateway request scope reaches logs and
|
||||
// diagnostic events for per-request debugging.
|
||||
import fs from "node:fs";
|
||||
import type { ServerResponse } from "node:http";
|
||||
import os from "node:os";
|
||||
import path from "node:path";
|
||||
import { afterEach, describe, expect, it } from "vitest";
|
||||
import { afterEach, describe, expect, it, vi } from "vitest";
|
||||
import {
|
||||
emitDiagnosticEvent,
|
||||
onDiagnosticEvent,
|
||||
@@ -103,3 +104,138 @@ describe("gateway HTTP request trace scope", () => {
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
describe("gateway HTTP request error cleanup", () => {
|
||||
it.each([
|
||||
{
|
||||
label: "partially written",
|
||||
writeResponse: (res: ServerResponse) => res.write("partial"),
|
||||
expectedBody: "partial",
|
||||
},
|
||||
{
|
||||
label: "already completed",
|
||||
writeResponse: (res: ServerResponse) => res.end("complete"),
|
||||
expectedBody: "complete",
|
||||
},
|
||||
{
|
||||
label: "fully written fixed-length",
|
||||
writeResponse: (res: ServerResponse) => {
|
||||
res.setHeader("Content-Length", "7");
|
||||
res.write("partial");
|
||||
},
|
||||
expectedBody: "partial",
|
||||
},
|
||||
{
|
||||
label: "fully written writeHead fixed-length",
|
||||
writeResponse: (res: ServerResponse) => {
|
||||
res.writeHead(200, { "Content-Length": "7" });
|
||||
res.write("partial");
|
||||
},
|
||||
expectedBody: "partial",
|
||||
},
|
||||
])(
|
||||
"finishes a $label response after its route throws",
|
||||
async ({ writeResponse, expectedBody }) => {
|
||||
const errorLog = vi.spyOn(console, "error").mockImplementation(() => {});
|
||||
const server = createGatewayHttpServer({
|
||||
clients: new Set(),
|
||||
controlUiEnabled: false,
|
||||
controlUiBasePath: "",
|
||||
openAiChatCompletionsEnabled: false,
|
||||
openResponsesEnabled: false,
|
||||
handleHooksRequest: async (_req, res) => {
|
||||
writeResponse(res);
|
||||
throw new Error("route failed after writing a response");
|
||||
},
|
||||
resolvedAuth,
|
||||
getRuntimeConfig: () => ({}),
|
||||
});
|
||||
const port = await listen(server);
|
||||
|
||||
try {
|
||||
const response = await fetch(`http://127.0.0.1:${port}/hooks/test`, {
|
||||
signal: AbortSignal.timeout(1_000),
|
||||
});
|
||||
|
||||
expect(response.status).toBe(200);
|
||||
expect(await response.text()).toBe(expectedBody);
|
||||
expect(errorLog).toHaveBeenCalledWith(
|
||||
"[gateway-http] unhandled error in request handler:",
|
||||
expect.any(Error),
|
||||
);
|
||||
} finally {
|
||||
server.closeAllConnections();
|
||||
await closeServer(server);
|
||||
errorLog.mockRestore();
|
||||
}
|
||||
},
|
||||
);
|
||||
|
||||
it.each([
|
||||
{
|
||||
label: "setHeader",
|
||||
setContentLength: (res: ServerResponse) => res.setHeader("Content-Length", "10"),
|
||||
},
|
||||
{
|
||||
label: "writeHead",
|
||||
setContentLength: (res: ServerResponse) => res.writeHead(200, { "Content-Length": "10" }),
|
||||
},
|
||||
])("closes an incomplete $label fixed-length response", async ({ setContentLength }) => {
|
||||
const errorLog = vi.spyOn(console, "error").mockImplementation(() => {});
|
||||
const server = createGatewayHttpServer({
|
||||
clients: new Set(),
|
||||
controlUiEnabled: false,
|
||||
controlUiBasePath: "",
|
||||
openAiChatCompletionsEnabled: false,
|
||||
openResponsesEnabled: false,
|
||||
handleHooksRequest: async (_req, res) => {
|
||||
setContentLength(res);
|
||||
res.write("partial");
|
||||
throw new Error("route failed before completing its fixed-length response");
|
||||
},
|
||||
resolvedAuth,
|
||||
getRuntimeConfig: () => ({}),
|
||||
});
|
||||
const port = await listen(server);
|
||||
|
||||
try {
|
||||
await expect(
|
||||
fetch(`http://127.0.0.1:${port}/hooks/test`, {
|
||||
signal: AbortSignal.timeout(1_000),
|
||||
}).then(async (response) => await response.text()),
|
||||
).rejects.toMatchObject({ name: "TypeError" });
|
||||
} finally {
|
||||
server.closeAllConnections();
|
||||
await closeServer(server);
|
||||
errorLog.mockRestore();
|
||||
}
|
||||
});
|
||||
|
||||
it("preserves the 500 response when its route fails before writing headers", async () => {
|
||||
const errorLog = vi.spyOn(console, "error").mockImplementation(() => {});
|
||||
const server = createGatewayHttpServer({
|
||||
clients: new Set(),
|
||||
controlUiEnabled: false,
|
||||
controlUiBasePath: "",
|
||||
openAiChatCompletionsEnabled: false,
|
||||
openResponsesEnabled: false,
|
||||
handleHooksRequest: async () => {
|
||||
throw new Error("route failed before writing a response");
|
||||
},
|
||||
resolvedAuth,
|
||||
getRuntimeConfig: () => ({}),
|
||||
});
|
||||
const port = await listen(server);
|
||||
|
||||
try {
|
||||
const response = await fetch(`http://127.0.0.1:${port}/hooks/test`);
|
||||
|
||||
expect(response.status).toBe(500);
|
||||
expect(await response.text()).toBe("Internal Server Error");
|
||||
} finally {
|
||||
server.closeAllConnections();
|
||||
await closeServer(server);
|
||||
errorLog.mockRestore();
|
||||
}
|
||||
});
|
||||
});
|
||||
|
||||
+16
-10
@@ -38,7 +38,11 @@ import {
|
||||
} from "./control-ui-routing.js";
|
||||
import type { ControlUiRootState } from "./control-ui.js";
|
||||
import type { AuthorizedGatewayHttpRequest } from "./http-auth-utils.js";
|
||||
import { sendGatewayAuthFailure, setDefaultSecurityHeaders } from "./http-common.js";
|
||||
import {
|
||||
finishFailedGatewayHttpResponse,
|
||||
sendGatewayAuthFailure,
|
||||
setDefaultSecurityHeaders,
|
||||
} from "./http-common.js";
|
||||
import { resolveRequestClientIp } from "./net.js";
|
||||
import {
|
||||
normalizePluginNodeCapabilityScopedUrl,
|
||||
@@ -544,13 +548,17 @@ export function createGatewayHttpServer(opts: {
|
||||
const getResolvedAuth = opts.getResolvedAuth ?? (() => resolvedAuth);
|
||||
const loadGatewayConfig = opts.getRuntimeConfig ?? getRuntimeConfig;
|
||||
const openAiCompatEnabled = openAiChatCompletionsEnabled || openResponsesEnabled;
|
||||
const handleServerRequest = (req: IncomingMessage, res: ServerResponse) => {
|
||||
void handleRequestWithTrace(req, res).catch((error: unknown) => {
|
||||
console.error("[gateway-http] failed to finalize request:", error);
|
||||
if (!res.destroyed) {
|
||||
res.destroy(error instanceof Error ? error : undefined);
|
||||
}
|
||||
});
|
||||
};
|
||||
const httpServer: HttpServer = opts.tlsOptions
|
||||
? createHttpsServer(opts.tlsOptions, (req, res) => {
|
||||
void handleRequestWithTrace(req, res);
|
||||
})
|
||||
: createHttpServer((req, res) => {
|
||||
void handleRequestWithTrace(req, res);
|
||||
});
|
||||
? createHttpsServer(opts.tlsOptions, handleServerRequest)
|
||||
: createHttpServer(handleServerRequest);
|
||||
|
||||
function handleRequestWithTrace(req: IncomingMessage, res: ServerResponse) {
|
||||
return runWithDiagnosticTraceContext(createDiagnosticTraceContext(), () =>
|
||||
@@ -979,9 +987,7 @@ export function createGatewayHttpServer(opts: {
|
||||
res.end("Not Found");
|
||||
} catch (err) {
|
||||
console.error("[gateway-http] unhandled error in request handler:", err);
|
||||
res.statusCode = 500;
|
||||
res.setHeader("Content-Type", "text/plain; charset=utf-8");
|
||||
res.end("Internal Server Error");
|
||||
finishFailedGatewayHttpResponse(res);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -520,6 +520,62 @@ describe("createGatewayPluginRequestHandler", () => {
|
||||
}
|
||||
});
|
||||
|
||||
it.each([
|
||||
{
|
||||
label: "setHeader",
|
||||
setContentLength: (res: ServerResponse) => res.setHeader("Content-Length", "10"),
|
||||
},
|
||||
{
|
||||
label: "writeHead",
|
||||
setContentLength: (res: ServerResponse) => res.writeHead(200, { "Content-Length": "10" }),
|
||||
},
|
||||
])(
|
||||
"closes an incomplete plugin $label fixed-length response after its route throws",
|
||||
async ({ setContentLength }) => {
|
||||
const log = createPluginLog();
|
||||
const handler = createGatewayPluginRequestHandler({
|
||||
registry: createTestRegistry({
|
||||
httpRoutes: [
|
||||
createRoute({
|
||||
path: "/incomplete",
|
||||
handler: async (_req, res) => {
|
||||
setContentLength(res);
|
||||
res.write("partial");
|
||||
throw new Error("boom");
|
||||
},
|
||||
}),
|
||||
],
|
||||
}),
|
||||
log,
|
||||
});
|
||||
const server = createServer((req, res) => {
|
||||
void handler(req, res);
|
||||
});
|
||||
|
||||
await new Promise<void>((resolve, reject) => {
|
||||
server.once("error", reject);
|
||||
server.listen(0, "127.0.0.1", () => resolve());
|
||||
});
|
||||
const address = server.address();
|
||||
if (!address || typeof address === "string") {
|
||||
throw new Error("server did not bind to a TCP port");
|
||||
}
|
||||
|
||||
try {
|
||||
await expect(
|
||||
fetch(`http://127.0.0.1:${address.port}/incomplete`, {
|
||||
signal: AbortSignal.timeout(500),
|
||||
}).then(async (response) => await response.text()),
|
||||
).rejects.toMatchObject({ name: "TypeError" });
|
||||
} finally {
|
||||
server.closeAllConnections();
|
||||
await new Promise<void>((resolve) => {
|
||||
server.close(() => resolve());
|
||||
});
|
||||
}
|
||||
},
|
||||
);
|
||||
|
||||
it("does not end a response the plugin already destroyed before throwing", async () => {
|
||||
const log = createPluginLog();
|
||||
const handler = createGatewayPluginRequestHandler({
|
||||
|
||||
@@ -10,6 +10,7 @@ import type { createSubsystemLogger } from "../../logging/subsystem.js";
|
||||
import type { PluginHttpRouteRegistration, PluginRegistry } from "../../plugins/registry.js";
|
||||
import { withPluginRuntimeGatewayRequestScope } from "../../plugins/runtime/gateway-request-scope.js";
|
||||
import { respondControlUiPluginAuthCookieProbe } from "../control-ui-plugin-auth-cookie.js";
|
||||
import { finishFailedGatewayHttpResponse } from "../http-common.js";
|
||||
import type { AuthorizedGatewayHttpRequest } from "../http-utils.js";
|
||||
import type { GatewayRequestContext, GatewayRequestOptions } from "../server-methods/types.js";
|
||||
import {
|
||||
@@ -272,13 +273,7 @@ export function createGatewayPluginRequestHandler(params: {
|
||||
}
|
||||
} catch (err) {
|
||||
log.warn(`plugin http route failed (${route.pluginId ?? "unknown"}): ${String(err)}`);
|
||||
if (!res.headersSent) {
|
||||
res.statusCode = 500;
|
||||
res.setHeader("Content-Type", "text/plain; charset=utf-8");
|
||||
res.end("Internal Server Error");
|
||||
} else if (!res.writableEnded && !res.destroyed) {
|
||||
res.end();
|
||||
}
|
||||
finishFailedGatewayHttpResponse(res);
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user