mirror of
https://github.com/openclaw/openclaw.git
synced 2026-08-02 03:11:39 +00:00
fix(gateway): preserve node invoke dispatch provenance (#117139)
* fix(gateway): preserve node invoke dispatch provenance Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6381c461-11aa-4700-8648-4da1393e1c59 * fix(gateway): preserve timeout precedence Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6381c461-11aa-4700-8648-4da1393e1c59 * fix(gateway): mark wake-path disconnect as not dispatched Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6381c461-11aa-4700-8648-4da1393e1c59 * fix(gateway): mark plugin dispatch after send Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6381c461-11aa-4700-8648-4da1393e1c59 * build(browser): regenerate Copilot runtime Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6381c461-11aa-4700-8648-4da1393e1c59 --------- Co-authored-by: Omar Shahine <10343873+omarshahine@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 6381c461-11aa-4700-8648-4da1393e1c59
This commit is contained in:
File diff suppressed because one or more lines are too long
36
packages/gateway-client/src/client.request-timeout.test.ts
Normal file
36
packages/gateway-client/src/client.request-timeout.test.ts
Normal file
@@ -0,0 +1,36 @@
|
||||
import { afterEach, expect, test, vi } from "vitest";
|
||||
import { GatewayClient, GatewayClientRequestTimeoutError } from "./client.js";
|
||||
import type { GatewayProtocolSocket } from "./protocol-client.js";
|
||||
|
||||
afterEach(() => {
|
||||
vi.useRealTimers();
|
||||
});
|
||||
|
||||
test("reports that a timed-out request crossed the transport send boundary", async () => {
|
||||
vi.useFakeTimers();
|
||||
const client = new GatewayClient({ requestTimeoutMs: 100 });
|
||||
const send = vi.fn();
|
||||
const socket: GatewayProtocolSocket = {
|
||||
isOpen: () => true,
|
||||
send,
|
||||
close: vi.fn(),
|
||||
};
|
||||
Object.assign(
|
||||
(client as unknown as { protocol: { socket: GatewayProtocolSocket | null } }).protocol,
|
||||
{ socket },
|
||||
);
|
||||
|
||||
const request = client.request("node.invoke", { nodeId: "node-1" });
|
||||
const outcome = request.catch((value: unknown) => value);
|
||||
expect(send).toHaveBeenCalledOnce();
|
||||
|
||||
await vi.advanceTimersByTimeAsync(100);
|
||||
|
||||
const error = await outcome;
|
||||
expect(error).toBeInstanceOf(GatewayClientRequestTimeoutError);
|
||||
expect(error).toMatchObject({
|
||||
method: "node.invoke",
|
||||
timeoutMs: 100,
|
||||
requestSent: true,
|
||||
});
|
||||
});
|
||||
@@ -37,12 +37,12 @@ import {
|
||||
import { buildDeviceAuthPayloadV3 } from "./device-auth.js";
|
||||
import {
|
||||
GatewayProtocolClient,
|
||||
GatewayProtocolRequestError,
|
||||
type GatewayProtocolCloseContext,
|
||||
type GatewayProtocolRequestOptions,
|
||||
type GatewayProtocolSocket,
|
||||
type GatewayProtocolSocketHandlers,
|
||||
} from "./protocol-client.js";
|
||||
import { GatewayProtocolRequestError } from "./protocol-request.js";
|
||||
import { shouldPauseGatewayReconnect } from "./reconnect-policy.js";
|
||||
import {
|
||||
DEFAULT_GATEWAY_REQUEST_TIMEOUT_MS,
|
||||
@@ -248,6 +248,20 @@ export class GatewayClientRequestError extends GatewayProtocolRequestError {
|
||||
}
|
||||
}
|
||||
|
||||
export class GatewayClientRequestTimeoutError extends Error {
|
||||
readonly method: string;
|
||||
readonly timeoutMs: number;
|
||||
readonly requestSent: boolean;
|
||||
|
||||
constructor(params: { method: string; timeoutMs: number; requestSent: boolean }) {
|
||||
super(`gateway request timeout for ${params.method}`);
|
||||
this.name = "GatewayClientRequestTimeoutError";
|
||||
this.method = params.method;
|
||||
this.timeoutMs = params.timeoutMs;
|
||||
this.requestSent = params.requestSent;
|
||||
}
|
||||
}
|
||||
|
||||
class GatewayClientTransientPreHelloCloseError extends Error {
|
||||
constructor() {
|
||||
super("gateway transient pre-hello clean close");
|
||||
@@ -402,7 +416,8 @@ export class GatewayClient {
|
||||
createSocket: (handlers) => this.createSocket(handlers),
|
||||
createRequestId: randomUUID,
|
||||
createRequestError: (error) => new GatewayClientRequestError(error),
|
||||
createRequestTimeoutError: (method) => new Error(`gateway request timeout for ${method}`),
|
||||
createRequestTimeoutError: (method, timeoutMs, requestSent) =>
|
||||
new GatewayClientRequestTimeoutError({ method, timeoutMs, requestSent }),
|
||||
createRequestAbortError: createGatewayRequestAbortError,
|
||||
buildConnectPlan: ({ nonce, challengeTs }) => {
|
||||
if (!nonce) {
|
||||
|
||||
@@ -84,7 +84,7 @@ type GatewayProtocolClientOptions<TPlan> = {
|
||||
createSocket: (handlers: GatewayProtocolSocketHandlers) => GatewayProtocolSocket;
|
||||
createRequestId: () => string;
|
||||
createRequestError?: (error: Partial<ErrorShape>) => GatewayProtocolRequestError;
|
||||
createRequestTimeoutError?: (method: string, timeoutMs: number) => Error;
|
||||
createRequestTimeoutError?: (method: string, timeoutMs: number, requestSent: boolean) => Error;
|
||||
createRequestAbortError?: (method: string) => Error;
|
||||
buildConnectPlan: (params: {
|
||||
nonce: string | null;
|
||||
@@ -226,6 +226,7 @@ export class GatewayProtocolClient<TPlan> {
|
||||
options?.timeoutMs === null ? undefined : (options?.timeoutMs ?? this.opts.requestTimeoutMs);
|
||||
return new Promise<T>((resolve, reject) => {
|
||||
let timeout: ReturnType<typeof setTimeout> | undefined;
|
||||
let requestSent = false;
|
||||
const pending: GatewayPendingRequest = {
|
||||
resolve: (value) => resolve(value as T),
|
||||
reject,
|
||||
@@ -238,9 +239,7 @@ export class GatewayProtocolClient<TPlan> {
|
||||
};
|
||||
const onAbort = () => {
|
||||
this.pending.delete(id);
|
||||
if (timeout) {
|
||||
clearTimeout(timeout);
|
||||
}
|
||||
pending.cleanup?.();
|
||||
this.finishRequestTiming(id, pending, false, "CLIENT_ABORTED");
|
||||
reject(
|
||||
this.opts.createRequestAbortError?.(method) ??
|
||||
@@ -263,11 +262,14 @@ export class GatewayProtocolClient<TPlan> {
|
||||
pending.cleanup = cleanup;
|
||||
if (timeoutMs !== undefined && timeoutMs >= 0) {
|
||||
timeout = setTimeout(() => {
|
||||
if (this.pending.get(id) !== pending) {
|
||||
return;
|
||||
}
|
||||
this.pending.delete(id);
|
||||
options?.signal?.removeEventListener("abort", onAbort);
|
||||
this.finishRequestTiming(id, pending, false, "CLIENT_TIMEOUT");
|
||||
reject(
|
||||
this.opts.createRequestTimeoutError?.(method, timeoutMs) ??
|
||||
this.opts.createRequestTimeoutError?.(method, timeoutMs, requestSent) ??
|
||||
new Error(`gateway request timed out after ${timeoutMs}ms: ${method}`),
|
||||
);
|
||||
}, timeoutMs);
|
||||
@@ -277,6 +279,7 @@ export class GatewayProtocolClient<TPlan> {
|
||||
this.pending.set(id, pending);
|
||||
try {
|
||||
socket.send(JSON.stringify({ type: "req", id, method, params }));
|
||||
requestSent = true;
|
||||
this.invoke("sent", () => options?.onSent?.());
|
||||
} catch (error) {
|
||||
this.pending.delete(id);
|
||||
|
||||
@@ -22,7 +22,7 @@ import type { OpenClawPluginNodeInvokePolicyContext } from "../plugins/types.js"
|
||||
import { closeOpenClawStateDatabaseForTest } from "../state/openclaw-state-db.js";
|
||||
import { ExecApprovalManager } from "./exec-approval-manager.js";
|
||||
import { applyPluginNodeInvokePolicy } from "./node-invoke-plugin-policy.js";
|
||||
import type { NodeSession } from "./node-registry.js";
|
||||
import type { NodeInvokeResult, NodeSession } from "./node-registry.js";
|
||||
import { listPendingOperatorApprovals } from "./operator-approval-store.js";
|
||||
import type { GatewayClient, GatewayRequestContext } from "./server-methods/types.js";
|
||||
|
||||
@@ -67,12 +67,19 @@ function createContext(opts?: {
|
||||
pluginApprovalIosPushDelivery?: GatewayRequestContext["pluginApprovalIosPushDelivery"];
|
||||
}) {
|
||||
const nodeSession = opts?.nodeSession ?? createNodeSession();
|
||||
const invoke = vi.fn(async () => ({
|
||||
ok: true,
|
||||
payload: { ok: true, value: 1 },
|
||||
payloadJSON: null,
|
||||
error: null,
|
||||
}));
|
||||
const invoke = vi.fn(
|
||||
async (params?: {
|
||||
onDispatchReady?: (invokeId: string) => void;
|
||||
}): Promise<NodeInvokeResult> => {
|
||||
params?.onDispatchReady?.("invoke-1");
|
||||
return {
|
||||
ok: true,
|
||||
payload: { ok: true, value: 1 },
|
||||
payloadJSON: null,
|
||||
error: null,
|
||||
};
|
||||
},
|
||||
);
|
||||
return {
|
||||
context: {
|
||||
getRuntimeConfig:
|
||||
@@ -272,6 +279,7 @@ describe("applyPluginNodeInvokePolicy", () => {
|
||||
params: DEMO_PARAMS,
|
||||
timeoutMs: undefined,
|
||||
idempotencyKey: undefined,
|
||||
onDispatchReady: expect.any(Function),
|
||||
});
|
||||
});
|
||||
|
||||
@@ -304,14 +312,15 @@ describe("applyPluginNodeInvokePolicy", () => {
|
||||
},
|
||||
);
|
||||
|
||||
it("marks plugin-owned work dispatched before calling the node transport", async () => {
|
||||
it("marks plugin-owned work dispatched only after the node transport accepts it", async () => {
|
||||
setDangerousDemoCommandRegistry([
|
||||
createDemoPolicy((ctx: OpenClawPluginNodeInvokePolicyContext) => ctx.invokeNode()),
|
||||
]);
|
||||
const { context, invoke } = createContext();
|
||||
const dispatchOrder: string[] = [];
|
||||
invoke.mockImplementationOnce(async () => {
|
||||
invoke.mockImplementationOnce(async (params) => {
|
||||
dispatchOrder.push("node transport");
|
||||
params?.onDispatchReady?.("invoke-1");
|
||||
return {
|
||||
ok: true,
|
||||
payload: { ok: true, value: 1 },
|
||||
@@ -330,7 +339,43 @@ describe("applyPluginNodeInvokePolicy", () => {
|
||||
});
|
||||
|
||||
expect(result).toMatchObject({ ok: true });
|
||||
expect(dispatchOrder).toStrictEqual(["dispatched", "node transport"]);
|
||||
expect(dispatchOrder).toStrictEqual(["node transport", "dispatched"]);
|
||||
});
|
||||
|
||||
it("keeps plugin-owned work pre-dispatch when the node transport rejects the send", async () => {
|
||||
setDangerousDemoCommandRegistry([
|
||||
createDemoPolicy((ctx: OpenClawPluginNodeInvokePolicyContext) => ctx.invokeNode()),
|
||||
]);
|
||||
const { context, invoke } = createContext();
|
||||
const onNodeCommandDispatched = vi.fn();
|
||||
invoke.mockResolvedValueOnce({
|
||||
ok: false,
|
||||
payload: null,
|
||||
payloadJSON: null,
|
||||
error: { code: "UNAVAILABLE", message: "failed to send invoke to node" },
|
||||
});
|
||||
|
||||
const result = await applyPluginNodeInvokePolicy({
|
||||
context,
|
||||
client: null,
|
||||
nodeSession: createNodeSession(),
|
||||
command: DEMO_COMMAND,
|
||||
params: DEMO_PARAMS,
|
||||
onNodeCommandDispatched,
|
||||
});
|
||||
|
||||
expect(result).toMatchObject({
|
||||
ok: false,
|
||||
code: "UNAVAILABLE",
|
||||
details: {
|
||||
nodeError: { code: "UNAVAILABLE", message: "failed to send invoke to node" },
|
||||
nodeCommandDispatched: false,
|
||||
},
|
||||
});
|
||||
expect(invoke).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ onDispatchReady: expect.any(Function) }),
|
||||
);
|
||||
expect(onNodeCommandDispatched).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("rejects expired plugin-owned work without dispatching it", async () => {
|
||||
|
||||
@@ -282,10 +282,6 @@ export async function applyPluginNodeInvokePolicy(params: {
|
||||
? Math.min(requestedTimeoutMs, remainingTimeoutMs)
|
||||
: remainingTimeoutMs
|
||||
: requestedTimeoutMs;
|
||||
// Once the registry owns the request, any failure is ambiguous to callers:
|
||||
// the node may have acted before the response was lost or rejected.
|
||||
nodeCommandDispatched = true;
|
||||
params.onNodeCommandDispatched?.();
|
||||
const res = await params.context.nodeRegistry.invoke({
|
||||
nodeId: params.nodeSession.nodeId,
|
||||
expectedConnId: params.nodeSession.connId,
|
||||
@@ -297,6 +293,12 @@ export async function applyPluginNodeInvokePolicy(params: {
|
||||
timeoutMs,
|
||||
...(params.signal ? { signal: params.signal } : {}),
|
||||
idempotencyKey: override.idempotencyKey ?? params.idempotencyKey,
|
||||
onDispatchReady: () => {
|
||||
// Only the registry knows that the transport send succeeded. Preserve
|
||||
// pre-send failures as retry-safe while making later failures ambiguous.
|
||||
nodeCommandDispatched = true;
|
||||
params.onNodeCommandDispatched?.();
|
||||
},
|
||||
});
|
||||
if (!res.ok) {
|
||||
return {
|
||||
|
||||
@@ -16,6 +16,7 @@ export type PendingInvoke = {
|
||||
error?: { code?: string; message?: string } | null;
|
||||
}) => void;
|
||||
reject: (err: Error) => void;
|
||||
deadlineAtMs?: number;
|
||||
hardTimer?: ReturnType<typeof setTimeout>;
|
||||
idleTimer?: ReturnType<typeof setTimeout>;
|
||||
idleTimeoutMs?: number;
|
||||
@@ -93,9 +94,14 @@ export class NodeInvokeStreamController {
|
||||
if (pending.connId !== connId) {
|
||||
continue;
|
||||
}
|
||||
this.clearTimers(pending);
|
||||
if (pending.deadlineAtMs !== undefined && Date.now() >= pending.deadlineAtMs) {
|
||||
this.settleTimeout(id, pending);
|
||||
continue;
|
||||
}
|
||||
if (!this.takePending(id, pending)) {
|
||||
continue;
|
||||
}
|
||||
this.options.disconnectPending(pending);
|
||||
this.options.pendingInvokes.delete(id);
|
||||
}
|
||||
}
|
||||
|
||||
@@ -104,8 +110,13 @@ export class NodeInvokeStreamController {
|
||||
if (!pending || pending.nodeId !== params.nodeId || pending.connId !== params.connId) {
|
||||
return false;
|
||||
}
|
||||
this.clearTimers(pending);
|
||||
this.options.pendingInvokes.delete(params.id);
|
||||
if (pending.deadlineAtMs !== undefined && Date.now() >= pending.deadlineAtMs) {
|
||||
this.settleTimeout(params.id, pending);
|
||||
return false;
|
||||
}
|
||||
if (!this.takePending(params.id, pending)) {
|
||||
return false;
|
||||
}
|
||||
if (!params.ok) {
|
||||
this.options.onFailedResult(pending);
|
||||
}
|
||||
@@ -125,29 +136,31 @@ export class NodeInvokeStreamController {
|
||||
idleTimeoutMs: number;
|
||||
signal?: AbortSignal;
|
||||
}): void {
|
||||
if (params.timeoutMs > 0) {
|
||||
params.pending.deadlineAtMs = Date.now() + params.timeoutMs;
|
||||
}
|
||||
this.options.pendingInvokes.set(params.requestId, params.pending);
|
||||
if (params.timeoutMs > 0) {
|
||||
params.pending.hardTimer = setTimeout(() => {
|
||||
this.sendInvokeCancel(params.requestId, params.pending);
|
||||
this.clearTimers(params.pending);
|
||||
this.options.pendingInvokes.delete(params.requestId);
|
||||
params.pending.resolve({
|
||||
ok: false,
|
||||
error: { code: "TIMEOUT", message: "node invoke timed out" },
|
||||
});
|
||||
this.settleTimeout(params.requestId, params.pending);
|
||||
}, params.timeoutMs);
|
||||
}
|
||||
if (params.pending.onProgress && params.idleTimeoutMs > 0) {
|
||||
params.pending.idleTimeoutMs = params.idleTimeoutMs;
|
||||
}
|
||||
this.options.pendingInvokes.set(params.requestId, params.pending);
|
||||
if (params.signal) {
|
||||
const onAbort = () => {
|
||||
if (this.options.pendingInvokes.get(params.requestId) !== params.pending) {
|
||||
if (
|
||||
params.pending.deadlineAtMs !== undefined &&
|
||||
Date.now() >= params.pending.deadlineAtMs
|
||||
) {
|
||||
this.settleTimeout(params.requestId, params.pending);
|
||||
return;
|
||||
}
|
||||
if (!this.takePending(params.requestId, params.pending)) {
|
||||
return;
|
||||
}
|
||||
this.sendInvokeCancel(params.requestId, params.pending);
|
||||
this.clearTimers(params.pending);
|
||||
this.options.pendingInvokes.delete(params.requestId);
|
||||
params.pending.resolve({
|
||||
ok: false,
|
||||
error: { code: "ABORTED", message: "node invoke cancelled" },
|
||||
@@ -225,12 +238,10 @@ export class NodeInvokeStreamController {
|
||||
|
||||
private createIdleTimer(requestId: string, pending: PendingInvoke) {
|
||||
return setTimeout(() => {
|
||||
if (this.options.pendingInvokes.get(requestId) !== pending) {
|
||||
if (!this.takePending(requestId, pending)) {
|
||||
return;
|
||||
}
|
||||
this.sendInvokeCancel(requestId, pending);
|
||||
this.clearTimers(pending);
|
||||
this.options.pendingInvokes.delete(requestId);
|
||||
pending.resolve({
|
||||
ok: false,
|
||||
error: { code: "IDLE_TIMEOUT", message: "node invoke produced no progress" },
|
||||
@@ -251,4 +262,24 @@ export class NodeInvokeStreamController {
|
||||
private sendInvokeCancel(requestId: string, pending: PendingInvoke): void {
|
||||
this.options.sendCancel(requestId, pending);
|
||||
}
|
||||
|
||||
private settleTimeout(requestId: string, pending: PendingInvoke): void {
|
||||
if (!this.takePending(requestId, pending)) {
|
||||
return;
|
||||
}
|
||||
this.sendInvokeCancel(requestId, pending);
|
||||
pending.resolve({
|
||||
ok: false,
|
||||
error: { code: "TIMEOUT", message: "node invoke timed out" },
|
||||
});
|
||||
}
|
||||
|
||||
private takePending(requestId: string, pending: PendingInvoke): boolean {
|
||||
if (this.options.pendingInvokes.get(requestId) !== pending) {
|
||||
return false;
|
||||
}
|
||||
this.options.pendingInvokes.delete(requestId);
|
||||
this.clearTimers(pending);
|
||||
return true;
|
||||
}
|
||||
}
|
||||
|
||||
@@ -987,7 +987,6 @@ describe("gateway/node-registry", () => {
|
||||
command: "debug.ping",
|
||||
timeoutMs: 0,
|
||||
});
|
||||
const disconnected = invoke.catch((error: unknown) => error);
|
||||
const request = JSON.parse(previousFrames[0] ?? "{}") as {
|
||||
payload?: { id?: string };
|
||||
};
|
||||
@@ -1009,7 +1008,13 @@ describe("gateway/node-registry", () => {
|
||||
message: "node connection changed during connectivity probe",
|
||||
},
|
||||
});
|
||||
await expect(disconnected).resolves.toEqual(new Error("node disconnected (debug.ping)"));
|
||||
await expect(invoke).resolves.toEqual({
|
||||
ok: false,
|
||||
error: {
|
||||
code: "DISCONNECTED",
|
||||
message: "node disconnected (debug.ping)",
|
||||
},
|
||||
});
|
||||
expect(
|
||||
registry.handleInvokeResult({
|
||||
id: request.payload?.id ?? "",
|
||||
@@ -1051,7 +1056,6 @@ describe("gateway/node-registry", () => {
|
||||
command: "system.run",
|
||||
timeoutMs: 0,
|
||||
});
|
||||
const oldDisconnected = oldInvoke.catch((err: unknown) => err);
|
||||
const oldRequest = JSON.parse(oldFrames[0] ?? "{}") as { payload?: { id?: string } };
|
||||
const newSession = registerNodeSession(registry, newClient, {});
|
||||
|
||||
@@ -1063,7 +1067,13 @@ describe("gateway/node-registry", () => {
|
||||
ok: true,
|
||||
}),
|
||||
).toBe(false);
|
||||
await expect(oldDisconnected).resolves.toEqual(new Error("node disconnected (system.run)"));
|
||||
await expect(oldInvoke).resolves.toEqual({
|
||||
ok: false,
|
||||
error: {
|
||||
code: "DISCONNECTED",
|
||||
message: "node disconnected (system.run)",
|
||||
},
|
||||
});
|
||||
expect(registry.get("node-1")).toBe(newSession);
|
||||
expect(registry.unregister("conn-old")).toBeNull();
|
||||
expect(registry.get("node-1")).toBe(newSession);
|
||||
@@ -1095,6 +1105,7 @@ describe("gateway/node-registry", () => {
|
||||
it("rejects invoke when the node connection changed before dispatch", async () => {
|
||||
const registry = createNodeRegistry();
|
||||
const replacementFrames: string[] = [];
|
||||
const onDispatchReady = vi.fn();
|
||||
registerNodeSession(registry, makeClient("conn-old", "node-1"), {});
|
||||
registerNodeSession(registry, makeClient("conn-new", "node-1", replacementFrames), {});
|
||||
|
||||
@@ -1103,12 +1114,14 @@ describe("gateway/node-registry", () => {
|
||||
nodeId: "node-1",
|
||||
expectedConnId: "conn-old",
|
||||
command: "system.run",
|
||||
onDispatchReady,
|
||||
}),
|
||||
).resolves.toEqual({
|
||||
ok: false,
|
||||
error: { code: "ROUTE_CHANGED", message: "node connection changed before dispatch" },
|
||||
});
|
||||
expect(replacementFrames).toEqual([]);
|
||||
expect(onDispatchReady).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("matches pending system.run events to the issuing connection", async () => {
|
||||
@@ -1280,7 +1293,7 @@ describe("gateway/node-registry", () => {
|
||||
await expect(invoke).resolves.toMatchObject({ ok: true });
|
||||
});
|
||||
|
||||
it("rejects zero-timeout invokes when the node disconnects", async () => {
|
||||
it("returns a structured result when a zero-timeout invoke disconnects", async () => {
|
||||
const registry = createNodeRegistry();
|
||||
registerNode(registry);
|
||||
const invoke = registry.invoke({
|
||||
@@ -1288,12 +1301,120 @@ describe("gateway/node-registry", () => {
|
||||
command: "debug.ping",
|
||||
timeoutMs: 0,
|
||||
});
|
||||
const disconnected = invoke.catch((error: unknown) => error);
|
||||
|
||||
expect(registry.unregister("conn-1")).toBe("node-1");
|
||||
const error = await disconnected;
|
||||
expect(error).toBeInstanceOf(Error);
|
||||
expect((error as Error).message).toBe("node disconnected (debug.ping)");
|
||||
await expect(invoke).resolves.toEqual({
|
||||
ok: false,
|
||||
error: {
|
||||
code: "DISCONNECTED",
|
||||
message: "node disconnected (debug.ping)",
|
||||
},
|
||||
});
|
||||
});
|
||||
|
||||
it("accepts results before the hard deadline and times out results at the deadline", async () => {
|
||||
vi.useFakeTimers();
|
||||
vi.setSystemTime(1_000);
|
||||
const registry = createNodeRegistry();
|
||||
const frames = registerNode(registry);
|
||||
const beforeDispatch = vi.fn();
|
||||
|
||||
const beforeDeadline = registry.invoke({
|
||||
nodeId: "node-1",
|
||||
command: "debug.ping",
|
||||
timeoutMs: 100,
|
||||
onDispatchReady: beforeDispatch,
|
||||
});
|
||||
const beforeRequest = JSON.parse(frames[0] ?? "{}") as { payload?: { id?: string } };
|
||||
vi.setSystemTime(1_099);
|
||||
expect(
|
||||
registry.handleInvokeResult({
|
||||
id: beforeRequest.payload?.id ?? "",
|
||||
nodeId: "node-1",
|
||||
connId: "conn-1",
|
||||
ok: true,
|
||||
}),
|
||||
).toBe(true);
|
||||
expect(beforeDispatch).toHaveBeenCalledOnce();
|
||||
await expect(beforeDeadline).resolves.toMatchObject({ ok: true });
|
||||
|
||||
vi.setSystemTime(2_000);
|
||||
const atDeadline = registry.invoke({
|
||||
nodeId: "node-1",
|
||||
command: "debug.ping",
|
||||
timeoutMs: 100,
|
||||
});
|
||||
const atRequest = JSON.parse(frames[1] ?? "{}") as { payload?: { id?: string } };
|
||||
vi.setSystemTime(2_100);
|
||||
const terminalResult = {
|
||||
id: atRequest.payload?.id ?? "",
|
||||
nodeId: "node-1",
|
||||
connId: "conn-1",
|
||||
ok: true,
|
||||
};
|
||||
|
||||
expect(registry.handleInvokeResult(terminalResult)).toBe(false);
|
||||
expect(registry.handleInvokeResult(terminalResult)).toBe(false);
|
||||
await expect(atDeadline).resolves.toEqual({
|
||||
ok: false,
|
||||
error: { code: "TIMEOUT", message: "node invoke timed out" },
|
||||
});
|
||||
});
|
||||
|
||||
it("prefers an elapsed hard deadline when disconnect beats the timer callback", async () => {
|
||||
vi.useFakeTimers();
|
||||
vi.setSystemTime(1_000);
|
||||
const registry = createNodeRegistry();
|
||||
registerNode(registry);
|
||||
const invoke = registry.invoke({
|
||||
nodeId: "node-1",
|
||||
command: "debug.ping",
|
||||
timeoutMs: 100,
|
||||
});
|
||||
|
||||
vi.setSystemTime(1_100);
|
||||
expect(registry.unregister("conn-1")).toBe("node-1");
|
||||
|
||||
await expect(invoke).resolves.toEqual({
|
||||
ok: false,
|
||||
error: { code: "TIMEOUT", message: "node invoke timed out" },
|
||||
});
|
||||
});
|
||||
|
||||
it("prefers an elapsed hard deadline when abort beats the timer callback", async () => {
|
||||
vi.useFakeTimers();
|
||||
vi.setSystemTime(1_000);
|
||||
const registry = createNodeRegistry();
|
||||
registerNode(registry);
|
||||
|
||||
const beforeDeadlineController = new AbortController();
|
||||
const beforeDeadline = registry.invoke({
|
||||
nodeId: "node-1",
|
||||
command: "debug.ping",
|
||||
timeoutMs: 100,
|
||||
signal: beforeDeadlineController.signal,
|
||||
});
|
||||
vi.setSystemTime(1_099);
|
||||
beforeDeadlineController.abort();
|
||||
await expect(beforeDeadline).resolves.toEqual({
|
||||
ok: false,
|
||||
error: { code: "ABORTED", message: "node invoke cancelled" },
|
||||
});
|
||||
|
||||
vi.setSystemTime(2_000);
|
||||
const atDeadlineController = new AbortController();
|
||||
const atDeadline = registry.invoke({
|
||||
nodeId: "node-1",
|
||||
command: "debug.ping",
|
||||
timeoutMs: 100,
|
||||
signal: atDeadlineController.signal,
|
||||
});
|
||||
vi.setSystemTime(2_100);
|
||||
atDeadlineController.abort();
|
||||
await expect(atDeadline).resolves.toEqual({
|
||||
ok: false,
|
||||
error: { code: "TIMEOUT", message: "node invoke timed out" },
|
||||
});
|
||||
});
|
||||
|
||||
it("orders streamed invoke progress and drops state after the final result", async () => {
|
||||
@@ -1561,12 +1682,17 @@ describe("gateway/node-registry", () => {
|
||||
idleTimeoutMs: 100,
|
||||
onProgress: () => {},
|
||||
});
|
||||
const disconnected = invoke.catch((error: unknown) => error);
|
||||
const request = JSON.parse(frames[0] ?? "{}") as { payload?: { id?: string } };
|
||||
const invokeId = request.payload?.id ?? "";
|
||||
|
||||
expect(registry.unregister("conn-1")).toBe("node-1");
|
||||
await expect(disconnected).resolves.toBeInstanceOf(Error);
|
||||
await expect(invoke).resolves.toEqual({
|
||||
ok: false,
|
||||
error: {
|
||||
code: "DISCONNECTED",
|
||||
message: "node disconnected (agent.cli.claude.run.v1)",
|
||||
},
|
||||
});
|
||||
expect(
|
||||
registry.handleInvokeProgress({
|
||||
invokeId,
|
||||
|
||||
@@ -302,7 +302,13 @@ export class NodeRegistry {
|
||||
},
|
||||
});
|
||||
} else {
|
||||
pending.reject(new Error(`node disconnected (${pending.command})`));
|
||||
pending.resolve({
|
||||
ok: false,
|
||||
error: {
|
||||
code: "DISCONNECTED",
|
||||
message: `node disconnected (${pending.command})`,
|
||||
},
|
||||
});
|
||||
}
|
||||
},
|
||||
});
|
||||
|
||||
@@ -329,6 +329,7 @@ describe("exec approvals gateway methods", () => {
|
||||
expectedPairingGeneration: "generation-1",
|
||||
command,
|
||||
params: { includeResolvedDefaults: true },
|
||||
onDispatchReady: expect.any(Function),
|
||||
});
|
||||
expect(respond).toHaveBeenCalledWith(true, payload, undefined);
|
||||
});
|
||||
@@ -419,6 +420,7 @@ describe("exec approvals gateway methods", () => {
|
||||
expectedPairingGeneration: "generation-1",
|
||||
command,
|
||||
params: {},
|
||||
onDispatchReady: expect.any(Function),
|
||||
});
|
||||
expect(respond).toHaveBeenCalledWith(true, payload, undefined);
|
||||
});
|
||||
@@ -480,6 +482,7 @@ describe("exec approvals gateway methods", () => {
|
||||
rules: [{ pattern: "hostname", action: "allow" }],
|
||||
baseHash: "sha256:current",
|
||||
},
|
||||
onDispatchReady: expect.any(Function),
|
||||
});
|
||||
expect(respond).toHaveBeenCalledWith(true, { updated: true, hash: "sha256:next" }, undefined);
|
||||
});
|
||||
@@ -560,6 +563,7 @@ describe("exec approvals gateway methods", () => {
|
||||
nodeId: "missing-node",
|
||||
command: "system.execApprovals.get",
|
||||
params: {},
|
||||
onDispatchReady: expect.any(Function),
|
||||
});
|
||||
expect(respond).toHaveBeenCalledWith(
|
||||
false,
|
||||
@@ -568,6 +572,7 @@ describe("exec approvals gateway methods", () => {
|
||||
code: "UNAVAILABLE",
|
||||
details: {
|
||||
nodeError: { code: "NOT_CONNECTED", message: "node not connected" },
|
||||
nodeCommandDispatched: false,
|
||||
},
|
||||
}),
|
||||
);
|
||||
|
||||
@@ -26,7 +26,7 @@ import { isNodeCommandAllowed, resolveNodeCommandAllowlist } from "../node-comma
|
||||
import type { NodeSession } from "../node-registry.js";
|
||||
import { resolveBaseHashParam } from "./base-hash.js";
|
||||
import {
|
||||
respondUnavailableOnNodeInvokeError,
|
||||
respondUnavailableOnNodeInvokeErrorWithProvenance,
|
||||
respondUnavailableOnThrow,
|
||||
safeParseJson,
|
||||
} from "./nodes.helpers.js";
|
||||
@@ -164,6 +164,7 @@ async function respondWithExecApprovalsNodePayload<TParams extends { nodeId: str
|
||||
}
|
||||
}
|
||||
await respondUnavailableOnThrow(params.respond, async () => {
|
||||
let nodeCommandDispatched = false;
|
||||
const res = await params.context.nodeRegistry.invoke({
|
||||
nodeId,
|
||||
...(nodeSession
|
||||
@@ -176,8 +177,15 @@ async function respondWithExecApprovalsNodePayload<TParams extends { nodeId: str
|
||||
: {}),
|
||||
command: params.command,
|
||||
params: params.commandParams(parsedParams, nodeSession),
|
||||
onDispatchReady: () => {
|
||||
nodeCommandDispatched = true;
|
||||
},
|
||||
});
|
||||
if (!respondUnavailableOnNodeInvokeError(params.respond, res)) {
|
||||
if (
|
||||
!respondUnavailableOnNodeInvokeErrorWithProvenance(params.respond, res, {
|
||||
nodeCommandDispatched,
|
||||
})
|
||||
) {
|
||||
return;
|
||||
}
|
||||
const payload = params.readPayload(res);
|
||||
|
||||
59
src/gateway/server-methods/nodes.helpers.test.ts
Normal file
59
src/gateway/server-methods/nodes.helpers.test.ts
Normal file
@@ -0,0 +1,59 @@
|
||||
import { describe, expect, it, vi } from "vitest";
|
||||
import { respondUnavailableOnNodeInvokeErrorWithProvenance } from "./nodes.helpers.js";
|
||||
import type { RespondFn } from "./types.js";
|
||||
|
||||
function createRespond(): ReturnType<typeof vi.fn<RespondFn>> {
|
||||
return vi.fn<RespondFn>();
|
||||
}
|
||||
|
||||
describe("respondUnavailableOnNodeInvokeErrorWithProvenance", () => {
|
||||
it("propagates proven pre-dispatch provenance", () => {
|
||||
const respond = createRespond();
|
||||
|
||||
expect(
|
||||
respondUnavailableOnNodeInvokeErrorWithProvenance(
|
||||
respond,
|
||||
{
|
||||
ok: false,
|
||||
error: { code: "NOT_CONNECTED", message: "node not connected" },
|
||||
},
|
||||
{ nodeCommandDispatched: false },
|
||||
),
|
||||
).toBe(false);
|
||||
|
||||
expect(respond).toHaveBeenCalledWith(
|
||||
false,
|
||||
undefined,
|
||||
expect.objectContaining({
|
||||
details: {
|
||||
nodeError: { code: "NOT_CONNECTED", message: "node not connected" },
|
||||
nodeCommandDispatched: false,
|
||||
},
|
||||
}),
|
||||
);
|
||||
});
|
||||
|
||||
it.each(["TIMEOUT", "DISCONNECTED"])("propagates post-dispatch provenance for %s", (code) => {
|
||||
const respond = createRespond();
|
||||
|
||||
respondUnavailableOnNodeInvokeErrorWithProvenance(
|
||||
respond,
|
||||
{
|
||||
ok: false,
|
||||
error: { code, message: "terminal node outcome" },
|
||||
},
|
||||
{ nodeCommandDispatched: true },
|
||||
);
|
||||
|
||||
expect(respond).toHaveBeenCalledWith(
|
||||
false,
|
||||
undefined,
|
||||
expect.objectContaining({
|
||||
details: {
|
||||
nodeError: { code, message: "terminal node outcome" },
|
||||
nodeCommandDispatched: true,
|
||||
},
|
||||
}),
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -49,6 +49,16 @@ export async function respondUnavailableOnThrow(respond: RespondFn, fn: () => Pr
|
||||
export function respondUnavailableOnNodeInvokeError<T extends { ok: boolean; error?: unknown }>(
|
||||
respond: RespondFn,
|
||||
res: T,
|
||||
): res is T & { ok: true } {
|
||||
return respondUnavailableOnNodeInvokeErrorWithProvenance(respond, res);
|
||||
}
|
||||
|
||||
export function respondUnavailableOnNodeInvokeErrorWithProvenance<
|
||||
T extends { ok: boolean; error?: unknown },
|
||||
>(
|
||||
respond: RespondFn,
|
||||
res: T,
|
||||
provenance?: { nodeCommandDispatched: boolean },
|
||||
): res is T & { ok: true } {
|
||||
if (res.ok) {
|
||||
return true;
|
||||
@@ -60,11 +70,15 @@ export function respondUnavailableOnNodeInvokeError<T extends { ok: boolean; err
|
||||
const nodeCode = normalizeOptionalString(nodeError?.code) ?? "";
|
||||
const nodeMessage = normalizeOptionalString(nodeError?.message) ?? "node invoke failed";
|
||||
const message = nodeCode ? `${nodeCode}: ${nodeMessage}` : nodeMessage;
|
||||
const details = {
|
||||
nodeError: res.error ?? null,
|
||||
...(provenance ? { nodeCommandDispatched: provenance.nodeCommandDispatched } : {}),
|
||||
};
|
||||
respond(
|
||||
false,
|
||||
undefined,
|
||||
errorShape(ErrorCodes.UNAVAILABLE, message, {
|
||||
details: { nodeError: res.error ?? null },
|
||||
details,
|
||||
}),
|
||||
);
|
||||
return false;
|
||||
|
||||
@@ -991,6 +991,11 @@ describe("node.invoke APNs wake path", () => {
|
||||
expect(call[0]).toBe(false);
|
||||
expect(call[2]?.code).toBe(ErrorCodes.UNAVAILABLE);
|
||||
expect(call[2]?.message).toBe("node not connected");
|
||||
expect(call[2]?.details).toEqual({
|
||||
code: "NOT_CONNECTED",
|
||||
nodeError: { code: "NOT_CONNECTED", message: "node not connected" },
|
||||
nodeCommandDispatched: false,
|
||||
});
|
||||
expect(mocks.sendApnsBackgroundWake).not.toHaveBeenCalled();
|
||||
expect(nodeRegistry.invoke).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
@@ -29,7 +29,7 @@ import { handleNodeInvokeProgress } from "./nodes.handlers.invoke-progress.js";
|
||||
import { handleNodeInvokeResult } from "./nodes.handlers.invoke-result.js";
|
||||
import {
|
||||
respondInvalidParams,
|
||||
respondUnavailableOnNodeInvokeError,
|
||||
respondUnavailableOnNodeInvokeErrorWithProvenance,
|
||||
respondUnavailableOnThrow,
|
||||
safeParseJson,
|
||||
} from "./nodes.helpers.js";
|
||||
@@ -196,30 +196,21 @@ export const nodeInvokeHandlers: GatewayRequestHandlers = {
|
||||
}
|
||||
const invokeDeadlineAtMs =
|
||||
typeof p.timeoutMs === "number" && p.timeoutMs > 0 ? Date.now() + p.timeoutMs : undefined;
|
||||
let pluginNodeCommandDispatched = false;
|
||||
let nodeCommandDispatched = false;
|
||||
const resolveRemainingInvokeTimeoutMs = () =>
|
||||
invokeDeadlineAtMs === undefined ? p.timeoutMs : Math.max(0, invokeDeadlineAtMs - Date.now());
|
||||
const respondIfInvokeExpired = (includeDispatchState = false) => {
|
||||
const respondIfInvokeExpired = () => {
|
||||
if (invokeDeadlineAtMs === undefined || resolveRemainingInvokeTimeoutMs() !== 0) {
|
||||
return false;
|
||||
}
|
||||
if (pluginNodeCommandDispatched || includeDispatchState) {
|
||||
respond(
|
||||
false,
|
||||
undefined,
|
||||
errorShape(ErrorCodes.UNAVAILABLE, "TIMEOUT: node invoke timed out", {
|
||||
details: {
|
||||
nodeError: { code: "TIMEOUT", message: "node invoke timed out" },
|
||||
nodeCommandDispatched: pluginNodeCommandDispatched,
|
||||
},
|
||||
}),
|
||||
);
|
||||
return true;
|
||||
}
|
||||
respondUnavailableOnNodeInvokeError(respond, {
|
||||
ok: false,
|
||||
error: { code: "TIMEOUT", message: "node invoke timed out" },
|
||||
});
|
||||
respondUnavailableOnNodeInvokeErrorWithProvenance(
|
||||
respond,
|
||||
{
|
||||
ok: false,
|
||||
error: { code: "TIMEOUT", message: "node invoke timed out" },
|
||||
},
|
||||
{ nodeCommandDispatched },
|
||||
);
|
||||
return true;
|
||||
};
|
||||
await respondUnavailableOnThrow(respond, async () => {
|
||||
@@ -406,7 +397,11 @@ export const nodeInvokeHandlers: GatewayRequestHandlers = {
|
||||
false,
|
||||
undefined,
|
||||
errorShape(ErrorCodes.UNAVAILABLE, "node not connected", {
|
||||
details: { code: "NOT_CONNECTED" },
|
||||
details: {
|
||||
code: "NOT_CONNECTED",
|
||||
nodeError: { code: "NOT_CONNECTED", message: "node not connected" },
|
||||
nodeCommandDispatched: false,
|
||||
},
|
||||
}),
|
||||
);
|
||||
return;
|
||||
@@ -487,7 +482,7 @@ export const nodeInvokeHandlers: GatewayRequestHandlers = {
|
||||
onNodeCommandDispatched: () => {
|
||||
// Deadline races must retain transport ownership so a command
|
||||
// already handed to the node is never advertised as retry-safe.
|
||||
pluginNodeCommandDispatched = true;
|
||||
nodeCommandDispatched = true;
|
||||
},
|
||||
idempotencyKey: p.idempotencyKey,
|
||||
isInvocationCurrent: () =>
|
||||
@@ -496,7 +491,7 @@ export const nodeInvokeHandlers: GatewayRequestHandlers = {
|
||||
invokeDeadlineAtMs,
|
||||
);
|
||||
if (policyResult === NODE_INVOKE_DEADLINE_EXPIRED) {
|
||||
respondIfInvokeExpired(true);
|
||||
respondIfInvokeExpired();
|
||||
return;
|
||||
}
|
||||
if (!(await continuePairingWork())) {
|
||||
@@ -599,6 +594,9 @@ export const nodeInvokeHandlers: GatewayRequestHandlers = {
|
||||
signal: invocationLifecycle,
|
||||
idempotencyKey: p.idempotencyKey,
|
||||
...(sessionKey ? { sessionKey } : {}),
|
||||
onDispatchReady: () => {
|
||||
nodeCommandDispatched = true;
|
||||
},
|
||||
});
|
||||
if (!(await continuePairingWork())) {
|
||||
return;
|
||||
@@ -670,7 +668,11 @@ export const nodeInvokeHandlers: GatewayRequestHandlers = {
|
||||
);
|
||||
return;
|
||||
}
|
||||
if (!respondUnavailableOnNodeInvokeError(respond, res)) {
|
||||
if (
|
||||
!respondUnavailableOnNodeInvokeErrorWithProvenance(respond, res, {
|
||||
nodeCommandDispatched,
|
||||
})
|
||||
) {
|
||||
return;
|
||||
}
|
||||
return;
|
||||
|
||||
@@ -476,7 +476,6 @@ describe("watch node HTTP transport", () => {
|
||||
command: "device.info",
|
||||
timeoutMs: 2_000,
|
||||
});
|
||||
const invokeAfterDisconnect = invoke.catch((error: unknown) => error);
|
||||
const pollResponse = await fetch(`${baseUrl}/poll`, {
|
||||
method: "POST",
|
||||
headers: { authorization: `Bearer ${String(connected.sessionToken)}` },
|
||||
@@ -517,7 +516,13 @@ describe("watch node HTTP transport", () => {
|
||||
nodeId: identity.deviceId,
|
||||
reason: "node pairing changed",
|
||||
});
|
||||
await expect(invokeAfterDisconnect).resolves.toBeInstanceOf(Error);
|
||||
await expect(invoke).resolves.toEqual({
|
||||
ok: false,
|
||||
error: {
|
||||
code: "DISCONNECTED",
|
||||
message: "node disconnected (device.info)",
|
||||
},
|
||||
});
|
||||
runtime.close();
|
||||
});
|
||||
|
||||
|
||||
Reference in New Issue
Block a user