fix(gateway): avoid approval route config load

Avoid eager runtime config loading in the gateway approval path and unref approval cleanup grace timers.
This commit is contained in:
Vincent Koc
2026-04-27 22:04:09 -07:00
committed by GitHub
parent ece523a2b0
commit 75deb12606
4 changed files with 140 additions and 8 deletions

View File

@@ -0,0 +1,61 @@
import { afterEach, describe, expect, it, vi } from "vitest";
import { ExecApprovalManager } from "./exec-approval-manager.js";
type TimeoutCallback = Parameters<typeof setTimeout>[0];
type MockTimerHandle = ReturnType<typeof setTimeout> & {
unref: ReturnType<typeof vi.fn>;
};
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);
});
});

View File

@@ -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<typeof setTimeout>): 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<TPayload = ExecApprovalRequestPayload> = {
@@ -117,12 +129,12 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
// 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<TPayload = ExecApprovalRequestPayload> {
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;
}

View File

@@ -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;
});
});

View File

@@ -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");