Skip to content

Commit beaeeff

Browse files
StiensWoutclaudemaria-rcks
authored
fix(acp): bound unread raw notification retention (#19585)
Co-authored-by: Claude Opus 5.5 <[email protected]> Co-authored-by: maria-rcks <[email protected]>
1 parent 5cb2a7d commit beaeeff

4 files changed

Lines changed: 275 additions & 38 deletions

File tree

‎packages/effect-acp/src/agent.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,11 @@ export class AcpAgent extends Context.Service<
3535
AcpAgent,
3636
{
3737
readonly raw: {
38+
/**
39+
* Stream of inbound ACP notifications observed on the connection. Unread
40+
* ones are kept best-effort: the newest 32 whose JSON lines fit in 16Mi
41+
* characters, always including the latest. Ends when the connection does.
42+
*/
3843
readonly notifications: Stream.Stream<AcpProtocol.AcpIncomingNotification>;
3944
readonly request: (
4045
method: string,

‎packages/effect-acp/src/client.ts‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -60,6 +60,11 @@ export interface AcpClientOptions {
6060
}
6161

6262
type AcpClientRaw = {
63+
/**
64+
* Inbound notifications. Unread ones are kept best-effort: the newest 32 whose
65+
* JSON lines fit in 16Mi characters, always including the latest. Ends with the
66+
* connection.
67+
*/
6368
readonly notifications: Stream.Stream<AcpProtocol.AcpIncomingNotification>;
6469
readonly request: (method: string, payload: unknown) => Effect.Effect<unknown, AcpError.AcpError>;
6570
readonly notify: (method: string, payload: unknown) => Effect.Effect<void, AcpError.AcpError>;

‎packages/effect-acp/src/protocol.test.ts‎

Lines changed: 177 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,6 +62,16 @@ const JsonRpcErrorResponse = Schema.Struct({
6262
});
6363
const encodeUnknownJsonString = Schema.encodeUnknownSync(Schema.fromJsonString(Schema.Unknown));
6464
const encoder = new TextEncoder();
65+
const MIB = 1024 * 1024;
66+
const decodeIndex = Schema.decodeUnknownSync(Schema.Struct({ index: Schema.Number }));
67+
const largeNotification = (index: number, textLength: number) =>
68+
encoder.encode(
69+
`${encodeUnknownJsonString({
70+
jsonrpc: "2.0",
71+
method: "x/large",
72+
params: { index, text: "x".repeat(textLength) },
73+
})}\n`,
74+
);
6575
const mockPeerPath = Effect.map(Effect.service(Path.Path), (path) =>
6676
path.join(import.meta.dirname, "../test/fixtures/acp-mock-peer.ts"),
6777
);
@@ -148,6 +158,173 @@ it.layer(NodeServices.layer)("effect-acp protocol", (it) => {
148158
}),
149159
);
150160

161+
it.effect("keeps only recent raw notifications after their callbacks run", () =>
162+
Effect.gen(function* () {
163+
const { stdio, input } = yield* makeInMemoryStdio();
164+
const handled = yield* Deferred.make<void>();
165+
let handledCount = 0;
166+
const transport = yield* AcpProtocol.makeAcpPatchedProtocol({
167+
stdio,
168+
serverRequestMethods: new Set(),
169+
onNotification: () =>
170+
Effect.sync(() => ++handledCount).pipe(
171+
Effect.flatMap((count) =>
172+
count === 64 ? Deferred.succeed(handled, undefined).pipe(Effect.asVoid) : Effect.void,
173+
),
174+
),
175+
});
176+
177+
const messages = Array.from({ length: 64 }, (_, index) =>
178+
encodeUnknownJsonString({
179+
jsonrpc: "2.0",
180+
method: "x/performance",
181+
params: { index },
182+
}),
183+
);
184+
yield* Queue.offer(input, encoder.encode(`${messages.join("\n")}\n`));
185+
yield* Deferred.await(handled);
186+
187+
const retained = yield* transport.incoming.pipe(Stream.take(32), Stream.runCollect);
188+
189+
assert.equal(handledCount, 64);
190+
assert.equal(retained.length, 32);
191+
assert.deepEqual(retained[0]?.params, { index: 32 });
192+
assert.deepEqual(retained[31]?.params, { index: 63 });
193+
}),
194+
);
195+
196+
it.effect("keeps the newest unread raw notifications whose JSON fits in 16Mi characters", () =>
197+
Effect.gen(function* () {
198+
const { stdio, input } = yield* makeInMemoryStdio();
199+
const handled = yield* Queue.unbounded<number>();
200+
const transport = yield* AcpProtocol.makeAcpPatchedProtocol({
201+
stdio,
202+
serverRequestMethods: new Set(),
203+
onNotification: (notification) =>
204+
Queue.offer(handled, decodeIndex(notification.params).index).pipe(Effect.asVoid),
205+
});
206+
207+
// Ten 3 MiB notifications: only the newest five fit in the budget.
208+
for (let index = 0; index < 10; index++) {
209+
yield* Queue.offer(input, largeNotification(index, 3 * MIB));
210+
assert.equal(yield* Queue.take(handled), index);
211+
}
212+
213+
const retained = yield* transport.incoming.pipe(Stream.take(5), Stream.runCollect);
214+
assert.deepEqual(
215+
retained.map((notification) => decodeIndex(notification.params).index),
216+
[5, 6, 7, 8, 9],
217+
);
218+
}),
219+
);
220+
221+
it.effect("keeps a single unread raw notification that alone exceeds the budget", () =>
222+
Effect.gen(function* () {
223+
const { stdio, input } = yield* makeInMemoryStdio();
224+
const handled = yield* Queue.unbounded<number>();
225+
const transport = yield* AcpProtocol.makeAcpPatchedProtocol({
226+
stdio,
227+
serverRequestMethods: new Set(),
228+
onNotification: (notification) =>
229+
Queue.offer(handled, decodeIndex(notification.params).index).pipe(Effect.asVoid),
230+
});
231+
232+
yield* Queue.offer(input, largeNotification(0, 1));
233+
yield* Queue.take(handled);
234+
yield* Queue.offer(input, largeNotification(1, 17 * MIB));
235+
yield* Queue.take(handled);
236+
237+
const [retained] = yield* transport.incoming.pipe(Stream.take(1), Stream.runCollect);
238+
assert.equal(decodeIndex(retained?.params).index, 1);
239+
}),
240+
);
241+
242+
it.effect("delivers every raw notification to a reader that keeps up, then ends", () =>
243+
Effect.gen(function* () {
244+
const { stdio, input } = yield* makeInMemoryStdio();
245+
const transport = yield* AcpProtocol.makeAcpPatchedProtocol({
246+
stdio,
247+
serverRequestMethods: new Set(),
248+
});
249+
const received = yield* Queue.unbounded<number>();
250+
const reader = yield* transport.incoming.pipe(
251+
Stream.map((notification) => decodeIndex(notification.params).index),
252+
Stream.tap((index) => Queue.offer(received, index)),
253+
Stream.runCollect,
254+
Effect.forkScoped,
255+
);
256+
257+
// 30 MiB in total, well past the unread budget.
258+
for (let index = 0; index < 10; index++) {
259+
yield* Queue.offer(input, largeNotification(index, 3 * MIB));
260+
assert.equal(yield* Queue.take(received), index);
261+
}
262+
yield* Queue.end(input);
263+
264+
assert.deepEqual(yield* Fiber.join(reader), [0, 1, 2, 3, 4, 5, 6, 7, 8, 9]);
265+
}),
266+
);
267+
268+
it.effect("ends the raw notification stream when the connection scope closes", () =>
269+
Effect.gen(function* () {
270+
const { stdio } = yield* makeInMemoryStdio();
271+
const connection = yield* Scope.make();
272+
const transport = yield* AcpProtocol.makeAcpPatchedProtocol({
273+
stdio,
274+
serverRequestMethods: new Set(),
275+
}).pipe(Scope.provide(connection));
276+
const reader = yield* transport.incoming.pipe(Stream.runCollect, Effect.forkScoped);
277+
278+
yield* Scope.close(connection, Exit.void);
279+
280+
assert.deepEqual(yield* Fiber.join(reader), []);
281+
}),
282+
);
283+
284+
it.effect("keeps the ended raw backlog intact when notifications arrive after termination", () =>
285+
Effect.gen(function* () {
286+
const { stdio, input } = yield* makeInMemoryStdio();
287+
const handled = yield* Queue.unbounded<number>();
288+
const terminated = yield* Deferred.make<void>();
289+
const transport = yield* AcpProtocol.makeAcpPatchedProtocol({
290+
stdio: Stdio.make({
291+
args: stdio.args,
292+
stdin: stdio.stdin,
293+
stdout: () => Sink.forEach(() => Effect.die("stdout failed")),
294+
stderr: () => Sink.drain,
295+
}),
296+
serverRequestMethods: new Set(),
297+
onNotification: (notification) =>
298+
Queue.offer(handled, decodeIndex(notification.params).index).pipe(Effect.asVoid),
299+
onTermination: () => Deferred.succeed(terminated, undefined).pipe(Effect.asVoid),
300+
});
301+
const deliver = (index: number) =>
302+
Queue.offer(
303+
input,
304+
encoder.encode(
305+
`${encodeUnknownJsonString({ jsonrpc: "2.0", method: "x/late", params: { index } })}\n`,
306+
),
307+
).pipe(Effect.andThen(Queue.take(handled)));
308+
309+
for (let index = 0; index < 32; index++) {
310+
assert.equal(yield* deliver(index), index);
311+
}
312+
// A failed write terminates the connection while the reader keeps running.
313+
yield* transport.notify("x/fail", {}).pipe(Effect.exit);
314+
yield* Deferred.await(terminated);
315+
assert.equal(yield* deliver(32), 32);
316+
317+
const retained = yield* transport.incoming.pipe(
318+
Stream.map((notification) => decodeIndex(notification.params).index),
319+
Stream.runCollect,
320+
);
321+
assert.deepEqual(
322+
retained,
323+
Array.from({ length: 32 }, (_, index) => index),
324+
);
325+
}),
326+
);
327+
151328
// clanka once sent a usage_update without `used` and `size`. One bad update
152329
// must not end a session whose later updates are fine.
153330
it.effect("drops a session/update that fails to decode and keeps reading", () =>

0 commit comments

Comments
 (0)