-
Notifications
You must be signed in to change notification settings - Fork 350
fix(ai): release Codex websocket on consumer abort #4556
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -339,12 +339,15 @@ function createLazyStream<TApi extends Api>( | |
| limits?: LazyStreamLimits, | ||
| ): (model: Model<TApi>, context: Context, options: OptionsForApi<TApi>) => EventStreamImpl { | ||
| return (model, context, options) => { | ||
| const outer = new EventStreamImpl(); | ||
| let abortTracker: AbortSourceTracker | undefined; | ||
| const outer = new EventStreamImpl(() => | ||
| abortTracker?.abortLocally(new Error("Provider stream consumer stopped before completion")), | ||
| ); | ||
|
Comment on lines
+343
to
+345
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When Useful? React with 👍 / 👎. |
||
| const streamOptions = (options ?? {}) as OptionsForApi<TApi>; | ||
|
|
||
| loadModule() | ||
| .then(module => { | ||
| const abortTracker = createAbortSourceTracker(streamOptions.signal); | ||
| abortTracker = createAbortSourceTracker(streamOptions.signal); | ||
| const providerOptions = { ...streamOptions, signal: abortTracker.requestSignal } as OptionsForApi<TApi>; | ||
| const inner = module.stream(model, context, providerOptions); | ||
| forwardStream(outer, inner, model, streamOptions, abortTracker, limits); | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,8 +35,9 @@ export class EventStream<T, R = T> implements AsyncIterable<T> { | |
| rejectFinalResult!: (err: unknown) => void; | ||
| isComplete: (event: T) => boolean; | ||
| extractResult: (event: T) => R; | ||
| #onConsumerClose?: () => void; | ||
|
|
||
| constructor(isComplete: (event: T) => boolean, extractResult: (event: T) => R) { | ||
| constructor(isComplete: (event: T) => boolean, extractResult: (event: T) => R, onConsumerClose?: () => void) { | ||
| const { promise, resolve, reject } = Promise.withResolvers<R>(); | ||
| // Prevent an unhandled rejection when fail() is called but nobody awaits result(). | ||
| // Callers who do await result() still receive the rejection normally. | ||
|
|
@@ -46,6 +47,7 @@ export class EventStream<T, R = T> implements AsyncIterable<T> { | |
| this.rejectFinalResult = reject; | ||
| this.isComplete = isComplete; | ||
| this.extractResult = extractResult; | ||
| this.#onConsumerClose = onConsumerClose; | ||
| } | ||
|
|
||
| #enqueue(node: QueueNode<T>): void { | ||
|
|
@@ -240,6 +242,7 @@ export class EventStream<T, R = T> implements AsyncIterable<T> { | |
| } finally { | ||
| this.#activeConsumerCount -= 1; | ||
| this.#settleAllConsumerDrains("reject", new Error("Event stream consumer stopped before drain completed")); | ||
| if (!this.done) this.#onConsumerClose?.(); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a consumer calls Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Codex P2 (
— |
||
| } | ||
| } | ||
|
|
||
|
|
@@ -249,7 +252,7 @@ export class EventStream<T, R = T> implements AsyncIterable<T> { | |
| } | ||
|
|
||
| export class AssistantMessageEventStream extends EventStream<AssistantMessageEvent, AssistantMessage> { | ||
| constructor() { | ||
| constructor(onConsumerClose?: () => void) { | ||
| super( | ||
| event => event.type === "done" || event.type === "error", | ||
| event => { | ||
|
|
@@ -260,6 +263,7 @@ export class AssistantMessageEventStream extends EventStream<AssistantMessageEve | |
| } | ||
| throw new Error("Unexpected event type for final result"); | ||
| }, | ||
| onConsumerClose, | ||
| ); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the coding-agent's managed provisional buffer rejects, it closes the iterator returned by
streamSimple, but the main Codex route returns the outer lazy stream fromregister-builtins.ts;forwardStreamowns and continues consuming this inner stream at lines 288-290. Closing the outer iterator therefore never invokes this callback, so the Codex request remains active and the next turn can still encounterwebsocket request already in progress. Propagate outer consumer closure to the inner iterator/request signal, and cover the publicstreamSimplepath rather than calling the provider implementation directly.Useful? React with 👍 / 👎.