Skip to content

Commit a7c45e3

Browse files
author
yongkangc
committed
fix(agents): hand off active input at delivery boundary
Prevents stale predecessor restoration during active-input admission.
1 parent 0c69ba7 commit a7c45e3

11 files changed

Lines changed: 330 additions & 153 deletions

File tree

integration-tests/support/integration-fixture.ts

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { mkdir, mkdtemp, readFile, rm, utimes, writeFile } from 'node:fs/promises';
1+
import { appendFile, mkdir, mkdtemp, readFile, rm, utimes, writeFile } from 'node:fs/promises';
22
import { tmpdir } from 'node:os';
33
import { dirname, join, resolve } from 'node:path';
44
import { fileURLToPath } from 'node:url';
@@ -258,14 +258,35 @@ export class IntegrationFixture {
258258
await this.#startReplacementGarcon();
259259
}
260260

261-
async #removeFinalNativeUserRow(input: { chatId: string; clientRequestId: string }): Promise<void> {
261+
async appendDirectOpenAiNativeMessage(input: {
262+
chatId: string;
263+
role: 'user' | 'assistant';
264+
content: string;
265+
clientRequestId?: string;
266+
turnId?: string;
267+
}): Promise<void> {
268+
const nativePath = await this.#directOpenAiNativePath(input.chatId);
269+
const raw = await readFile(nativePath, 'utf8');
270+
if (raw.length > 0 && !raw.endsWith('\n')) {
271+
throw new Error('Direct native transcript has an incomplete tail.');
272+
}
273+
await appendFile(nativePath, `${JSON.stringify({
274+
role: input.role,
275+
content: input.content,
276+
timestamp: new Date().toISOString(),
277+
...(input.clientRequestId ? { clientRequestId: input.clientRequestId } : {}),
278+
...(input.turnId ? { turnId: input.turnId } : {}),
279+
})}\n`, 'utf8');
280+
}
281+
282+
async #directOpenAiNativePath(chatId: string): Promise<string> {
262283
const registry = JSON.parse(
263284
await readFile(join(this.dirs.workspace, 'chats.json'), 'utf8'),
264285
) as { sessions?: Record<string, Record<string, unknown>> };
265-
const chat = registry.sessions?.[input.chatId];
266-
if (!chat) throw new Error(`Chat ${input.chatId} was not persisted before crash.`);
286+
const chat = registry.sessions?.[chatId];
287+
if (!chat) throw new Error(`Chat ${chatId} was not persisted before restart.`);
267288
if (chat.agentId !== DIRECT_OPENAI_CHAT_COMPLETIONS_COMPATIBLE_AGENT_ID) {
268-
throw new Error(`Chat ${input.chatId} is not a direct OpenAI-compatible chat.`);
289+
throw new Error(`Chat ${chatId} is not a direct OpenAI-compatible chat.`);
269290
}
270291
const nativeSession = chat.nativeSession && typeof chat.nativeSession === 'object'
271292
? chat.nativeSession as Record<string, unknown>
@@ -290,9 +311,13 @@ export class IntegrationFixture {
290311
|| !nativePath
291312
|| resolve(nativePath) !== expectedPath
292313
) {
293-
throw new Error(`Chat ${input.chatId} has an unexpected native transcript path.`);
314+
throw new Error(`Chat ${chatId} has an unexpected native transcript path.`);
294315
}
316+
return expectedPath;
317+
}
295318

319+
async #removeFinalNativeUserRow(input: { chatId: string; clientRequestId: string }): Promise<void> {
320+
const expectedPath = await this.#directOpenAiNativePath(input.chatId);
296321
const raw = await readFile(expectedPath, 'utf8');
297322
if (!raw.endsWith('\n')) throw new Error('Direct native transcript has an incomplete tail.');
298323
const lines = raw.split('\n').filter((line) => line.length > 0);

integration-tests/tests/server/persistence-lifecycle.test.ts

Lines changed: 38 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ describe('persistence lifecycle', () => {
6969
await withIntegrationFixture('ephemeral-queue-restart', async (fixture) => {
7070
const chatId = fixture.newChatId();
7171
const held = fixture.fakeProviders.openAi.holdNext({ lastUserText: 'ephemeral-active' });
72-
await fixture.client.startDirectChat({
72+
const active = await fixture.client.startDirectChat({
7373
chatId,
7474
content: 'ephemeral-active',
7575
projectPath: fixture.dirs.project,
@@ -80,14 +80,45 @@ describe('persistence lifecycle', () => {
8080
const paused = await fixture.client.pauseQueue(chatId);
8181
expect(paused.control.queue.entries.map((entry) => entry.content)).toEqual(['discard-on-restart']);
8282
expect(paused.control.queue.pause?.kind).toBe('manual');
83+
expect((await fixture.client.reconnectState([chatId])).processing).toEqual({
84+
outcome: 'snapshot',
85+
runningChatIds: [chatId],
86+
});
8387

8488
const activeAborted = held.expectAbort();
85-
await fixture.restartGarcon();
89+
await fixture.restartGarcon({
90+
beforeStart: () => fixture.appendDirectOpenAiNativeMessage({
91+
chatId,
92+
role: 'assistant',
93+
content: 'terminal persisted after disconnect',
94+
clientRequestId: active.clientRequestId,
95+
turnId: active.turnId,
96+
}),
97+
});
8698
await activeAborted;
8799
held.releaseTruncatedStream();
88100

89-
const control = await fixture.client.getExecutionControl(chatId);
90-
expect(control).toEqual({
101+
const restarted = await fixture.client.reconnectState([chatId]);
102+
expect(restarted.processing).toEqual({
103+
outcome: 'snapshot',
104+
runningChatIds: [],
105+
});
106+
expect(restarted.controlResults).toEqual([{
107+
chatId,
108+
outcome: 'snapshot',
109+
control: {
110+
queue: {
111+
entries: [],
112+
dispatchingEntryId: null,
113+
recentlyDispatched: [],
114+
pause: null,
115+
reorderRevision: 0,
116+
},
117+
version: 0,
118+
updatedAt: null,
119+
},
120+
}]);
121+
expect(await fixture.client.getExecutionControl(chatId)).toEqual({
91122
queue: {
92123
entries: [],
93124
dispatchingEntryId: null,
@@ -98,9 +129,12 @@ describe('persistence lifecycle', () => {
98129
version: 0,
99130
updatedAt: null,
100131
});
132+
const reloaded = await fixture.client.reloadChat(chatId);
133+
expect(assistantContents(reloaded.messages)).toContain('terminal persisted after disconnect');
101134
const restored = await fixture.client.getMessages(chatId);
102135
expect(restored.pendingUserInputs).toEqual([]);
103136
expect(countUserContent(restored.messages, 'discard-on-restart')).toBe(0);
137+
expect(assistantContents(restored.messages)).toContain('terminal persisted after disconnect');
104138

105139
const next = await fixture.client.runDirectChat({
106140
chatId,

server-agents/codex/src/agents/codex/__tests__/execution.test.js

Lines changed: 82 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,20 @@ function startRequest(overrides = {}) {
6767
};
6868
}
6969

70+
function activeInputRequest(operation, beforeDelivery = async () => undefined) {
71+
return startRequest({
72+
agentSessionId: 'thread-1',
73+
nativeSession: {
74+
ownerId: 'codex',
75+
schemaVersion: 1,
76+
value: { path: '/tmp/thread-1.jsonl', agentSessionId: 'thread-1' },
77+
},
78+
operation,
79+
beforeDelivery,
80+
carryOver: undefined,
81+
});
82+
}
83+
7084
describe('CodexExecution', () => {
7185
it('preserves admission, endpoint configuration, session identity, and event identity', async () => {
7286
const runtime = createRuntime();
@@ -170,4 +184,72 @@ describe('CodexExecution', () => {
170184
await expect(execution.start(startRequest({ prompt: '/goal clear' })))
171185
.rejects.toMatchObject({ code: 'INVALID_SETTINGS' });
172186
});
187+
188+
for (const outcome of ['decline', 'failure']) {
189+
it(`keeps the predecessor operation visible when active input has a pre-boundary ${outcome}`, async () => {
190+
const runtime = createRuntime();
191+
const execution = new CodexExecution(
192+
createHost(),
193+
runtime,
194+
createPathNativeSessionCodec('codex'),
195+
createConfig(),
196+
);
197+
const predecessor = startRequest().operation;
198+
const successor = { ...predecessor, clientRequestId: 'request-2', turnId: 'turn-2' };
199+
const next = { ...predecessor, clientRequestId: 'request-3', turnId: 'turn-3' };
200+
const events = [];
201+
execution.subscribe((event) => events.push(event));
202+
await execution.start(startRequest());
203+
runtime.submitActiveInput.mockImplementation(async () => {
204+
runtime.emitFinished('chat-1', 0, {
205+
clientRequestId: predecessor.clientRequestId,
206+
turnId: predecessor.turnId,
207+
});
208+
if (outcome === 'failure') throw new Error('failed before delivery boundary');
209+
return false;
210+
});
211+
212+
const activeInput = execution.submitActiveInput(activeInputRequest(successor));
213+
if (outcome === 'failure') await expect(activeInput).rejects.toThrow('failed before delivery boundary');
214+
else await expect(activeInput).resolves.toBe(false);
215+
expect(events).toContainEqual(expect.objectContaining({
216+
type: 'finished',
217+
operation: predecessor,
218+
}));
219+
220+
await execution.resume(activeInputRequest(next));
221+
expect(runtime.runTurn).toHaveBeenCalledOnce();
222+
});
223+
}
224+
225+
it('retains successor ownership after a post-boundary delivery failure', async () => {
226+
const runtime = createRuntime();
227+
const execution = new CodexExecution(
228+
createHost(),
229+
runtime,
230+
createPathNativeSessionCodec('codex'),
231+
createConfig(),
232+
);
233+
const predecessor = startRequest().operation;
234+
const successor = { ...predecessor, clientRequestId: 'request-2', turnId: 'turn-2' };
235+
const events = [];
236+
execution.subscribe((event) => events.push(event));
237+
await execution.start(startRequest());
238+
runtime.submitActiveInput.mockImplementation(async (_request, beforeDelivery) => {
239+
await beforeDelivery();
240+
throw new Error('delivery outcome unknown');
241+
});
242+
243+
await expect(execution.submitActiveInput(activeInputRequest(successor)))
244+
.rejects.toThrow('delivery outcome unknown');
245+
runtime.emitFailed('chat-1', 'delivery failed', {
246+
clientRequestId: successor.clientRequestId,
247+
turnId: successor.turnId,
248+
});
249+
250+
expect(events).toContainEqual(expect.objectContaining({
251+
type: 'failed',
252+
operation: successor,
253+
}));
254+
});
173255
});

server-agents/codex/src/agents/codex/execution.ts

Lines changed: 14 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -98,23 +98,21 @@ export class CodexExecution implements AgentExecution {
9898
async submitActiveInput(
9999
request: Parameters<NonNullable<AgentExecution['submitActiveInput']>>[0],
100100
): Promise<boolean> {
101-
this.#operations.register(request.chatId, request.operation);
102-
try {
103-
const runtimeRequest = prepareResumeRequest(
104-
request,
105-
await this.#runtimeConfiguration(request),
106-
this.nativeSessions,
107-
);
108-
const accepted = await this.runtime.submitActiveInput(
109-
runtimeRequest,
101+
const predecessor = this.#operations.current(request.chatId);
102+
const runtimeRequest = prepareResumeRequest(
103+
request,
104+
await this.#runtimeConfiguration(request),
105+
this.nativeSessions,
106+
);
107+
return this.runtime.submitActiveInput(
108+
runtimeRequest,
109+
() => this.#operations.handoff(
110+
request.chatId,
111+
predecessor,
112+
request.operation,
110113
request.beforeDelivery,
111-
);
112-
if (!accepted) this.#operations.finish(request.chatId, request.operation);
113-
return accepted;
114-
} catch (error) {
115-
this.#operations.finish(request.chatId, request.operation);
116-
throw error;
117-
}
114+
),
115+
);
118116
}
119117

120118
async compact(

server-agents/common/src/execution/operation-tracker.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,27 @@ export class AgentOperationTracker {
88
this.#operations.set(chatId, operation);
99
}
1010

11+
async handoff(
12+
chatId: string,
13+
predecessor: AgentOperationIdentity | null,
14+
successor: AgentOperationIdentity,
15+
commit: () => Promise<void>,
16+
): Promise<void> {
17+
if ((this.#operations.get(chatId) ?? null) !== predecessor) {
18+
throw new Error(`Cannot hand off operation for chat ${chatId} after its active operation changed`);
19+
}
20+
this.#operations.set(chatId, successor);
21+
try {
22+
await commit();
23+
} catch (error) {
24+
if (this.#operations.get(chatId) === successor) {
25+
if (predecessor) this.#operations.set(chatId, predecessor);
26+
else this.#operations.delete(chatId);
27+
}
28+
throw error;
29+
}
30+
}
31+
1132
current(
1233
chatId: string,
1334
metadata?: RuntimeEventMetadata,

server/agents/__tests__/event-bus.test.js

Lines changed: 36 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ describe('AgentEventBus', () => {
6060
const { bus } = makeBus();
6161
bus.trackTurn('chat-1', operation('turn-1'));
6262
bus.markTurnAbortable('chat-1', operation('turn-1'));
63-
bus.replaceTurn('chat-1', operation('turn-2'));
63+
await bus.handoffTurn('chat-1', operation('turn-1'), operation('turn-2'), async () => undefined);
6464
const controller = new AbortController();
6565
const waiting = bus.waitUntilTurnAbortable('chat-1', operation('turn-2'), controller.signal);
6666
controller.abort();
@@ -79,15 +79,48 @@ describe('AgentEventBus', () => {
7979
expect(bus.getActiveTurn('chat-1')?.turnId).toBe('turn-1');
8080
});
8181

82-
it('allows an explicit active-input identity replacement', () => {
82+
it('commits an explicit active-input identity handoff at its delivery boundary', async () => {
8383
const { bus } = makeBus();
8484
bus.trackTurn('chat-1', operation('turn-1'));
8585

86-
bus.replaceTurn('chat-1', operation('turn-2'));
86+
await bus.handoffTurn('chat-1', operation('turn-1'), operation('turn-2'), async () => {
87+
expect(bus.getActiveTurn('chat-1')?.turnId).toBe('turn-2');
88+
});
8789

8890
expect(bus.getActiveTurn('chat-1')?.turnId).toBe('turn-2');
8991
});
9092

93+
it('restores the predecessor only when the delivery boundary fails while the successor owns it', async () => {
94+
const { bus } = makeBus();
95+
bus.trackTurn('chat-1', operation('turn-1'));
96+
bus.markTurnAbortable('chat-1', operation('turn-1'));
97+
98+
await expect(bus.handoffTurn(
99+
'chat-1',
100+
operation('turn-1'),
101+
operation('turn-2'),
102+
async () => { throw new Error('registration failed'); },
103+
)).rejects.toThrow('registration failed');
104+
105+
expect(bus.getActiveTurn('chat-1')?.turnId).toBe('turn-1');
106+
await expect(bus.waitUntilTurnAbortable('chat-1', operation('turn-1'))).resolves.toBe(true);
107+
});
108+
109+
it('rejects a handoff after the predecessor identity has changed', async () => {
110+
const { bus } = makeBus();
111+
bus.trackTurn('chat-1', operation('turn-1'));
112+
bus.settleTurn('chat-1', operation('turn-1'));
113+
bus.trackTurn('chat-1', operation('turn-3'));
114+
115+
await expect(bus.handoffTurn(
116+
'chat-1',
117+
operation('turn-1'),
118+
operation('turn-2'),
119+
async () => undefined,
120+
)).rejects.toThrow('active turn changed');
121+
expect(bus.getActiveTurn('chat-1')?.turnId).toBe('turn-3');
122+
});
123+
91124
it('retains exact identity through duplicate terminal events until settlement', () => {
92125
const { bus, emit } = makeBus();
93126
const terminals = [];

0 commit comments

Comments
 (0)