Skip to content

Commit afcbdd7

Browse files
openperfsteipete
openperf
and
steipete
authored
fix(infra/agents): session-routing guard for coalesced gateway restart continuations (#86742) (#87323)
* fix(infra/agents): session-routing guard for coalesced gateway restart continuations (#86742) When two sessions issue gateway.restart with continuationMessage close together, the scheduler Path B updatePendingRestartEmitHooks unconditionally overwrote the existing pending hooks, silently dropping the first sessions continuation and potentially routing the second sessions continuation back to the first session (CWE-200 finding flagged by aisle-research-bot on prior attempt #74443). Add a session-routing guard: scheduleGatewaySigusr1Restart now accepts an optional sessionKey and tracks the pending restarts owning session. Coalesced callers from a different session are rejected at the hook- update step and the new ScheduledRestart.emitHooksQueued: false field surfaces the drop to the caller. The gateway tool propagates this as continuationQueued: false in the tool response, matching #83370 narrow report-only surface. Same-session debounce/replace and legacy hookless callers behave the same as before. Refs #86742 * fix(infra): preserve queued restart continuation on forced bypass * fix(infra): make forced restart hook preservation explicit * fix(infra): guard restart continuation ownership before reschedule * fix(infra): report hookless coalesced restarts accurately * fix(infra): trust runtime session for restart sentinel routing * fix(infra): preserve earlier restart reschedule semantics * fix(agents): trust runtime session for update continuations * fix(infra): preserve hookless forced restart continuations --------- Co-authored-by: Peter Steinberger <[email protected]>
1 parent 2b43315 commit afcbdd7

10 files changed

Lines changed: 546 additions & 45 deletions

File tree

‎apps/shared/OpenClawKit/Sources/OpenClawProtocol/GatewayModels.swift‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -616,9 +616,9 @@ public struct SendParams: Codable, Sendable {
616616
message: String?,
617617
mediaurl: String?,
618618
mediaurls: [String]?,
619-
buffer: String?,
620-
filename: String?,
621-
contenttype: String?,
619+
buffer: String? = nil,
620+
filename: String? = nil,
621+
contenttype: String? = nil,
622622
asvoice: Bool?,
623623
gifplayback: Bool?,
624624
channel: String?,

‎scripts/protocol-gen-swift.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -42,6 +42,7 @@ const STRICT_LITERAL_STRUCTS = new Set([
4242
]);
4343

4444
const DEFAULTED_OPTIONAL_INIT_PARAM_ENTRIES: readonly [string, readonly string[]][] = [
45+
["SendParams", ["buffer", "filename", "contentType"]],
4546
["SessionOperationEvent", ["agentId"]],
4647
["SessionsCompactionListParams", ["agentId"]],
4748
["SessionsCompactionGetParams", ["agentId"]],

‎src/agents/tools/gateway-tool.test.ts‎

Lines changed: 102 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -11,11 +11,13 @@ const {
1111
extractDeliveryInfoMock,
1212
formatDoctorNonInteractiveHintMock,
1313
isRestartEnabledMock,
14+
callGatewayToolMock,
1415
removeRestartSentinelFileMock,
1516
scheduleGatewaySigusr1RestartMock,
1617
writeRestartSentinelMock,
1718
} = vi.hoisted(() => ({
1819
isRestartEnabledMock: vi.fn(() => true),
20+
callGatewayToolMock: vi.fn(async () => ({ ok: true })),
1921
extractDeliveryInfoMock: vi.fn(() => ({
2022
deliveryContext: {
2123
channel: "slack",
@@ -31,8 +33,14 @@ const {
3133
writeRestartSentinelMock: vi.fn(async (_payload: RestartSentinelPayload) => "/tmp/restart"),
3234
removeRestartSentinelFileMock: vi.fn(async (_path: string | null | undefined) => undefined),
3335
scheduleGatewaySigusr1RestartMock: vi.fn((_opts?: ScheduleGatewayRestartArgs) => ({
34-
scheduled: true,
36+
ok: true,
37+
pid: 123,
38+
signal: "SIGUSR1" as const,
3539
delayMs: 250,
40+
mode: "emit" as const,
41+
coalesced: false,
42+
cooldownMsApplied: 0,
43+
emitHooksQueued: true,
3644
})),
3745
}));
3846

@@ -67,7 +75,7 @@ vi.mock("../../logging/subsystem.js", () => ({
6775
}));
6876

6977
vi.mock("./gateway.js", () => ({
70-
callGatewayTool: vi.fn(),
78+
callGatewayTool: callGatewayToolMock,
7179
readGatewayCallOptions: vi.fn(() => ({})),
7280
}));
7381

@@ -110,7 +118,18 @@ describe("gateway tool restart continuation", () => {
110118
writeRestartSentinelMock.mockResolvedValue("/tmp/restart");
111119
removeRestartSentinelFileMock.mockClear();
112120
scheduleGatewaySigusr1RestartMock.mockReset();
113-
scheduleGatewaySigusr1RestartMock.mockReturnValue({ scheduled: true, delayMs: 250 });
121+
scheduleGatewaySigusr1RestartMock.mockReturnValue({
122+
ok: true,
123+
pid: 123,
124+
signal: "SIGUSR1",
125+
delayMs: 250,
126+
mode: "emit",
127+
coalesced: false,
128+
cooldownMsApplied: 0,
129+
emitHooksQueued: true,
130+
});
131+
callGatewayToolMock.mockReset();
132+
callGatewayToolMock.mockResolvedValue({ ok: true });
114133
});
115134

116135
it("does not expose system-event continuations to the agent tool", async () => {
@@ -187,9 +206,62 @@ describe("gateway tool restart continuation", () => {
187206
const restartArgs = requireScheduledRestartArgs();
188207
expect(restartArgs.delayMs).toBe(250);
189208
expect(restartArgs.reason).toBe("continue after reboot");
209+
expect(restartArgs.sessionKey).toBe("agent:main:main");
190210
expect(typeof restartArgs.emitHooks?.beforeEmit).toBe("function");
191211
expect(typeof restartArgs.emitHooks?.afterEmitRejected).toBe("function");
192-
expect(result?.details).toEqual({ scheduled: true, delayMs: 250 });
212+
expect(result?.details).toMatchObject({
213+
ok: true,
214+
delayMs: 250,
215+
coalesced: false,
216+
emitHooksQueued: true,
217+
continuationQueued: true,
218+
});
219+
});
220+
221+
it("uses the runtime session, not model-supplied params, for scheduler ownership and sentinel routing (#86742)", async () => {
222+
const tool = createGatewayTool({
223+
agentSessionKey: "agent:main:session-A",
224+
config: {},
225+
});
226+
227+
await tool.execute?.("tool-call-1", {
228+
action: "restart",
229+
sessionKey: "agent:main:session-B",
230+
continuationMessage: "Reply after restart",
231+
});
232+
233+
expect(requireScheduledRestartArgs().sessionKey).toBe("agent:main:session-A");
234+
await requireScheduledRestartArgs().emitHooks?.beforeEmit?.();
235+
expect(requireRestartSentinelPayload().sessionKey).toBe("agent:main:session-A");
236+
});
237+
238+
it("reports continuationQueued=false when a coalesced restart belongs to another session (#86742)", async () => {
239+
scheduleGatewaySigusr1RestartMock.mockReturnValue({
240+
ok: true,
241+
pid: 123,
242+
signal: "SIGUSR1",
243+
delayMs: 0,
244+
mode: "emit",
245+
coalesced: true,
246+
cooldownMsApplied: 0,
247+
emitHooksQueued: false,
248+
});
249+
const tool = createGatewayTool({
250+
agentSessionKey: "agent:main:main",
251+
config: {},
252+
});
253+
254+
const result = await tool.execute?.("tool-call-1", {
255+
action: "restart",
256+
continuationMessage: "Reply after restart",
257+
});
258+
259+
expect(writeRestartSentinelMock).not.toHaveBeenCalled();
260+
expect(result?.details).toMatchObject({
261+
coalesced: true,
262+
emitHooksQueued: false,
263+
continuationQueued: false,
264+
});
193265
});
194266

195267
it.each([-1, 1.5, "soon"])("rejects invalid restart delayMs value %s", async (delayMs) => {
@@ -278,4 +350,30 @@ describe("gateway tool restart continuation", () => {
278350

279351
expect(removeRestartSentinelFileMock).toHaveBeenCalledWith("/tmp/restart");
280352
});
353+
354+
it("uses the runtime session for update.run continuation routing (#86742)", async () => {
355+
const tool = createGatewayTool({
356+
agentSessionKey: "agent:main:session-A",
357+
config: {},
358+
});
359+
360+
await tool.execute?.("tool-call-update", {
361+
action: "update.run",
362+
sessionKey: "agent:main:session-B",
363+
continuationMessage: "Reply after update restart",
364+
note: "Updating now",
365+
restartDelayMs: 0,
366+
});
367+
368+
expect(callGatewayToolMock).toHaveBeenCalledWith(
369+
"update.run",
370+
expect.objectContaining({ timeoutMs: expect.any(Number) }),
371+
expect.objectContaining({
372+
sessionKey: "agent:main:session-A",
373+
continuationMessage: "Reply after update restart",
374+
note: "Updating now",
375+
restartDelayMs: 0,
376+
}),
377+
);
378+
});
281379
});

‎src/agents/tools/gateway-tool.ts‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -395,8 +395,8 @@ export function createGatewayTool(opts?: {
395395
throw new Error("Gateway restart is disabled (commands.restart=false).");
396396
}
397397
const sessionKey =
398-
normalizeOptionalString(params.sessionKey) ??
399-
normalizeOptionalString(opts?.agentSessionKey);
398+
normalizeOptionalString(opts?.agentSessionKey) ??
399+
normalizeOptionalString(params.sessionKey);
400400
const delayMs = readNonNegativeIntegerParam(params, "delayMs");
401401
const reason = normalizeOptionalString(params.reason)?.slice(0, 200);
402402
const note = normalizeOptionalString(params.note);
@@ -429,6 +429,9 @@ export function createGatewayTool(opts?: {
429429
const scheduled = scheduleGatewaySigusr1Restart({
430430
delayMs,
431431
reason,
432+
// Ownership and sentinel routing use the same trusted session identity,
433+
// so model-supplied params cannot queue work into another session.
434+
sessionKey,
432435
emitHooks: {
433436
beforeEmit: async () => {
434437
sentinelPath = await writeRestartSentinel(payload);
@@ -438,7 +441,10 @@ export function createGatewayTool(opts?: {
438441
},
439442
},
440443
});
441-
return jsonResult(scheduled);
444+
return jsonResult({
445+
...scheduled,
446+
...(payload.continuation ? { continuationQueued: scheduled.emitHooksQueued } : {}),
447+
});
442448
}
443449

444450
const gatewayOpts = readGatewayCallOptions(params);
@@ -449,8 +455,8 @@ export function createGatewayTool(opts?: {
449455
restartDelayMs: number | undefined;
450456
} => {
451457
const sessionKey =
452-
normalizeOptionalString(params.sessionKey) ??
453-
normalizeOptionalString(opts?.agentSessionKey);
458+
normalizeOptionalString(opts?.agentSessionKey) ??
459+
normalizeOptionalString(params.sessionKey);
454460
const note = normalizeOptionalString(params.note);
455461
const restartDelayMs = readNonNegativeIntegerParam(params, "restartDelayMs");
456462
return { sessionKey, note, restartDelayMs };

‎src/auto-reply/reply/commands-session-restart.test.ts‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -180,6 +180,18 @@ describe("handleRestartCommand", () => {
180180
}
181181
});
182182

183+
it("threads sessionKey into scheduleGatewaySigusr1Restart so cross-session coalescing is rejected (#86742)", async () => {
184+
const handler = () => {};
185+
process.on("SIGUSR1", handler);
186+
try {
187+
await handleRestartCommand(restartCommandParams(), true);
188+
const scheduledArgs = mocks.scheduleGatewaySigusr1Restart.mock.calls.at(-1)?.[0];
189+
expect(scheduledArgs?.sessionKey).toBe("agent:main:telegram:direct:123:thread:thread-1");
190+
} finally {
191+
process.removeListener("SIGUSR1", handler);
192+
}
193+
});
194+
183195
it("rejects authorized non-owner restart commands", async () => {
184196
const result = await handleRestartCommand(
185197
restartCommandParams({

‎src/auto-reply/reply/commands-session.ts‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -706,6 +706,10 @@ export const handleRestartCommand: CommandHandler = async (params, allowTextComm
706706
let sentinelPath: string | null = null;
707707
scheduleGatewaySigusr1Restart({
708708
reason: "/restart",
709+
// Sibling session-routing guard: /restart writes a session-scoped sentinel
710+
// with continuation, so the scheduler must own the pending slot under the
711+
// same key to avoid cross-session continuation overwrite (#86742).
712+
sessionKey: sentinelPayload?.sessionKey,
709713
emitHooks: sentinelPayload
710714
? {
711715
beforeEmit: async () => {

0 commit comments

Comments
 (0)