fix(gateway): close ask-fallback replay to cancelled approvals (#124381)

consumeAskFallback gated only on decision-less resolved records, so
run-aborted cancellations and dispatch-fencing revocations — which also
end decision-less — satisfied the timed-out predicate and could be
replayed as ask-fallback. Require the expired/no-route terminal so only
genuinely unanswered approvals re-admit.

Dispatch fencing now records its closure honestly: worker-dispatch
revocation routes through forceDenyDetailed(run-aborted, cancelled)
instead of expire(), which persisted a false 'timeout' terminal.

Rider cleanup: delete the write-only PendingEntry.reject/rejectPromise
plumbing and the lookupPendingId alias (single test caller moved to
lookupApprovalId).
This commit is contained in:
Peter Steinberger
2026-08-15 20:42:08 -07:00
committed by GitHub
parent 5943cd0362
commit e8e7598c5b
4 changed files with 42 additions and 20 deletions
@@ -81,4 +81,28 @@ describe("ExecApprovalManager timeout expiry publication", () => {
{ recordId: record.id, status: "expired", requestCommand: "echo expired" },
]);
});
it("rejects ask-fallback replay of a run-aborted cancellation", async () => {
installTimerMocks();
const manager = new ExecApprovalManager();
const record = manager.create({ command: "echo ok" }, 60_000, "approval-cancelled");
const decisionPromise = manager.register(record, 60_000);
// Dispatch fencing / run abort ends decision-less like a timeout, but its
// authority closed deliberately — replay must not re-admit through it.
const denied = manager.forceDenyDetailed(
"approval-cancelled",
"run-aborted",
{ kind: "system", id: "worker-dispatch" },
"cancelled",
);
expect(denied.outcome).toBe("denied");
await expect(decisionPromise).resolves.toBeNull();
expect(manager.getSnapshot("approval-cancelled")).toMatchObject({
status: "cancelled",
terminalReason: "run-aborted",
});
expect(manager.consumeAskFallback("approval-cancelled")).toBe(false);
});
});
+6 -10
View File
@@ -160,7 +160,6 @@ type ExecApprovalDurableLookup =
type PendingEntry<TPayload = ExecApprovalRequestPayload> = {
record: ExecApprovalRecord<TPayload>;
resolve: (decision: ExecApprovalDecision | null) => void;
reject: (err: Error) => void;
timer: ReturnType<typeof setTimeout> | null;
cleanupTimer: ReturnType<typeof setTimeout> | null;
handoffRetainCount: number;
@@ -357,16 +356,13 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
}
let resolvePromise: (decision: ExecApprovalDecision | null) => void;
let rejectPromise: (err: Error) => void;
const promise = new Promise<ExecApprovalDecision | null>((resolve, reject) => {
const promise = new Promise<ExecApprovalDecision | null>((resolve) => {
resolvePromise = resolve;
rejectPromise = reject;
});
// Create entry first so we can capture it in the closure (not re-fetch from map)
const entry: PendingEntry<TPayload> = {
record,
resolve: resolvePromise!,
reject: rejectPromise!,
timer: null,
cleanupTimer: null,
handoffRetainCount: 0,
@@ -1020,7 +1016,11 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
record.resolvedAtMs === undefined ||
record.decision !== undefined ||
record.consumedDecision !== undefined ||
record.askFallbackConsumed === true
record.askFallbackConsumed === true ||
// Only unanswered approvals (timeout or no delivery route) are
// re-admissible. Cancelled/fenced records also end decision-less, but
// their authority closed deliberately — never replay through them.
(record.status !== "expired" && record.terminalReason !== "no-route")
) {
return false;
}
@@ -1256,9 +1256,5 @@ export class ExecApprovalManager<TPayload = ExecApprovalRequestPayload> {
}
return { kind: "none" };
}
lookupPendingId(input: string): ExecApprovalIdLookupResult {
return this.lookupApprovalId(input);
}
}
/* oxlint-disable max-lines -- TODO: split this grandfathered oversized file. */
+10 -8
View File
@@ -372,14 +372,16 @@ export async function startGatewayCoreRuntime(input: {
for (const sessionKey of keys) {
revokeAttachGrantsForSession(sessionKey);
}
for (const record of execApprovalManager.listPendingRecords()) {
if (approvalRequestTargetsSession(record.request, keys, sessionId)) {
execApprovalManager.expire(record.id, "worker-dispatch");
}
}
for (const record of pluginApprovalManager.listPendingRecords()) {
if (approvalRequestTargetsSession(record.request, keys, sessionId)) {
pluginApprovalManager.expire(record.id, "worker-dispatch");
// Dispatch fencing closes approval authority deliberately: record it as a
// run-aborted cancellation, not a timeout, so ask-fallback replay cannot
// re-admit through the fenced record (consumeAskFallback admits only
// expired/no-route terminals).
const fenceResolver = { kind: "system", id: "worker-dispatch" } as const;
for (const manager of [execApprovalManager, pluginApprovalManager]) {
for (const record of manager.listPendingRecords()) {
if (approvalRequestTargetsSession(record.request, keys, sessionId)) {
manager.forceDenyDetailed(record.id, "run-aborted", fenceResolver, "cancelled");
}
}
}
};
@@ -3567,8 +3567,8 @@ describe("exec approval handlers", () => {
const pendingRecord = manager.create({ command: "echo new", host: "gateway" }, 2_000, "abcdef");
void manager.register(pendingRecord, 2_000);
expect(manager.lookupPendingId("abc")).toEqual({ kind: "none" });
expect(manager.lookupPendingId("abcdef")).toEqual({ kind: "exact", id: "abcdef" });
expect(manager.lookupApprovalId("abc")).toEqual({ kind: "none" });
expect(manager.lookupApprovalId("abcdef")).toEqual({ kind: "exact", id: "abcdef" });
});
it("stores versioned system.run binding and sorted env keys on approval request", async () => {