From 75deb126064bc7dbe8e56a08d99bceb644a2613d Mon Sep 17 00:00:00 2001 From: Vincent Koc Date: Mon, 27 Apr 2026 22:04:09 -0700 Subject: [PATCH] fix(gateway): avoid approval route config load Avoid eager runtime config loading in the gateway approval path and unref approval cleanup grace timers. --- src/gateway/exec-approval-manager.test.ts | 61 +++++++++++++++++++ src/gateway/exec-approval-manager.ts | 20 ++++-- .../server-methods/approval-shared.test.ts | 56 +++++++++++++++++ src/gateway/server-methods/approval-shared.ts | 11 ++-- 4 files changed, 140 insertions(+), 8 deletions(-) create mode 100644 src/gateway/exec-approval-manager.test.ts create mode 100644 src/gateway/server-methods/approval-shared.test.ts diff --git a/src/gateway/exec-approval-manager.test.ts b/src/gateway/exec-approval-manager.test.ts new file mode 100644 index 00000000000..9b742a57267 --- /dev/null +++ b/src/gateway/exec-approval-manager.test.ts @@ -0,0 +1,61 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { ExecApprovalManager } from "./exec-approval-manager.js"; + +type TimeoutCallback = Parameters[0]; +type MockTimerHandle = ReturnType & { + unref: ReturnType; +}; + +describe("ExecApprovalManager", () => { + afterEach(() => { + vi.restoreAllMocks(); + }); + + function installTimerMocks() { + const timers: Array<{ + delay: number | undefined; + handle: MockTimerHandle; + }> = []; + + vi.spyOn(globalThis, "setTimeout").mockImplementation((( + callback: TimeoutCallback, + delay?: number, + ) => { + void callback; + const handle = { unref: vi.fn() } as unknown as MockTimerHandle; + timers.push({ delay, handle }); + return handle; + }) as unknown as typeof setTimeout); + vi.spyOn(globalThis, "clearTimeout").mockImplementation( + (() => undefined) as typeof clearTimeout, + ); + + return timers; + } + + it("does not keep resolved approval cleanup timers ref'd", async () => { + const timers = installTimerMocks(); + const manager = new ExecApprovalManager(); + const record = manager.create({ command: "echo ok" }, 60_000, "approval-resolve"); + const decisionPromise = manager.register(record, 60_000); + + expect(manager.resolve("approval-resolve", "allow-once")).toBe(true); + await expect(decisionPromise).resolves.toBe("allow-once"); + + const cleanupTimer = timers.find((timer) => timer.delay === 15_000); + expect(cleanupTimer?.handle.unref).toHaveBeenCalledTimes(1); + }); + + it("does not keep expired approval cleanup timers ref'd", async () => { + const timers = installTimerMocks(); + const manager = new ExecApprovalManager(); + const record = manager.create({ command: "echo ok" }, 60_000, "approval-expire"); + const decisionPromise = manager.register(record, 60_000); + + expect(manager.expire("approval-expire")).toBe(true); + await expect(decisionPromise).resolves.toBeNull(); + + const cleanupTimer = timers.find((timer) => timer.delay === 15_000); + expect(cleanupTimer?.handle.unref).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/gateway/exec-approval-manager.ts b/src/gateway/exec-approval-manager.ts index 2b439e28cd4..2579efbc511 100644 --- a/src/gateway/exec-approval-manager.ts +++ b/src/gateway/exec-approval-manager.ts @@ -8,6 +8,18 @@ import { normalizeLowercaseStringOrEmpty } from "../shared/string-coerce.js"; // Grace period to keep resolved entries for late awaitDecision calls const RESOLVED_ENTRY_GRACE_MS = 15_000; +function unrefTimer(timer: ReturnType): void { + const unref = (timer as { unref?: () => void }).unref; + if (typeof unref === "function") { + unref.call(timer); + } +} + +function scheduleResolvedEntryCleanup(cleanup: () => void): void { + const timer = setTimeout(cleanup, RESOLVED_ENTRY_GRACE_MS); + unrefTimer(timer); +} + export type ExecApprovalRequestPayload = InfraExecApprovalRequestPayload; export type ExecApprovalRecord = { @@ -117,12 +129,12 @@ export class ExecApprovalManager { // Resolve the promise first, then delete after a grace period. // This allows in-flight awaitDecision calls to find the resolved entry. pending.resolve(decision); - setTimeout(() => { + scheduleResolvedEntryCleanup(() => { // Only delete if the entry hasn't been replaced if (this.pending.get(recordId) === pending) { this.pending.delete(recordId); } - }, RESOLVED_ENTRY_GRACE_MS); + }); return true; } @@ -139,11 +151,11 @@ export class ExecApprovalManager { pending.record.decision = undefined; pending.record.resolvedBy = resolvedBy ?? null; pending.resolve(null); - setTimeout(() => { + scheduleResolvedEntryCleanup(() => { if (this.pending.get(recordId) === pending) { this.pending.delete(recordId); } - }, RESOLVED_ENTRY_GRACE_MS); + }); return true; } diff --git a/src/gateway/server-methods/approval-shared.test.ts b/src/gateway/server-methods/approval-shared.test.ts new file mode 100644 index 00000000000..c4fa1973cc3 --- /dev/null +++ b/src/gateway/server-methods/approval-shared.test.ts @@ -0,0 +1,56 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { ExecApprovalManager } from "../exec-approval-manager.js"; +import { handlePendingApprovalRequest } from "./approval-shared.js"; +import type { GatewayRequestContext } from "./types.js"; + +const hasApprovalTurnSourceRouteMock = vi.hoisted(() => vi.fn(() => true)); + +vi.mock("../../infra/approval-turn-source.js", () => ({ + hasApprovalTurnSourceRoute: hasApprovalTurnSourceRouteMock, +})); + +describe("handlePendingApprovalRequest", () => { + afterEach(() => { + hasApprovalTurnSourceRouteMock.mockClear(); + }); + + it("does not resolve turn-source routes when approval clients are already available", async () => { + const manager = new ExecApprovalManager(); + const record = manager.create( + { + command: "echo ok", + turnSourceChannel: "feishu", + turnSourceAccountId: "work", + }, + 60_000, + "approval-with-client", + ); + const decisionPromise = manager.register(record, 60_000); + const respond = vi.fn(); + const requestPromise = handlePendingApprovalRequest({ + manager, + record, + decisionPromise, + respond, + context: { + broadcast: vi.fn(), + hasExecApprovalClients: () => true, + } as unknown as GatewayRequestContext, + requestEventName: "exec.approval.requested", + requestEvent: { + id: record.id, + request: record.request, + createdAtMs: record.createdAtMs, + expiresAtMs: record.expiresAtMs, + }, + twoPhase: true, + deliverRequest: () => false, + }); + + await Promise.resolve(); + expect(hasApprovalTurnSourceRouteMock).not.toHaveBeenCalled(); + + expect(manager.resolve(record.id, "allow-once")).toBe(true); + await requestPromise; + }); +}); diff --git a/src/gateway/server-methods/approval-shared.ts b/src/gateway/server-methods/approval-shared.ts index 7b74a3f3d49..fabd220e0ec 100644 --- a/src/gateway/server-methods/approval-shared.ts +++ b/src/gateway/server-methods/approval-shared.ts @@ -163,12 +163,15 @@ export async function handlePendingApprovalRequest< params.context.broadcast(params.requestEventName, params.requestEvent, { dropIfSlow: true }); const hasApprovalClients = params.context.hasExecApprovalClients?.(params.clientConnId) ?? false; - const hasTurnSourceRoute = hasApprovalTurnSourceRoute({ - turnSourceChannel: params.record.request.turnSourceChannel, - turnSourceAccountId: params.record.request.turnSourceAccountId, - }); const deliveredResult = params.deliverRequest(); const delivered = isPromiseLike(deliveredResult) ? await deliveredResult : deliveredResult; + const hasTurnSourceRoute = + !hasApprovalClients && + !delivered && + hasApprovalTurnSourceRoute({ + turnSourceChannel: params.record.request.turnSourceChannel, + turnSourceAccountId: params.record.request.turnSourceAccountId, + }); if (!hasApprovalClients && !hasTurnSourceRoute && !delivered) { params.manager.expire(params.record.id, "no-approval-route");