Conversation
src/cloudflare/internal/ai-api.ts
Outdated
| options, | ||
| }); |
There was a problem hiding this comment.
Bug: this JSON body serializes the full options object which still contains signal. Unlike #generateFetch (which receives cleanedOptions) and #generateStreamFetch (which destructures signal out), the websocket path sends "signal": {} to the server.
The fix has two parts:
- In
run(), passcleanedOptionsinstead ofoptions:
res = await this.#generateWebsocketFetch(inputs, cleanedOptions, model);- Here, read signal from
this.#options(same pattern as#generateFetch) instead ofoptions:
| options, | |
| }); | |
| const fetchOptions: RequestInit = { |
And below, change options.signal to this.#options.signal.
|
I'm Bonk, and I've done a quick review of your PR. Summary: Adds Issues found (1):
Everything else looks clean — the type definitions are consistent across all snapshot files, the |
cf6ddb5 to
2dbda91
Compare
| prefix, | ||
| extraHeaders, | ||
| sessionOptions, | ||
| signal, |
There was a problem hiding this comment.
@G4brym there's a weird inconsistency where cleanedOptions is only passed to #generateFetch(), not to #generateWebsocketFetch nor #generateStreamFetch.
My coding agent seems to have solved this by adding filtering code directly inside #generateWebsocketFetch and #generateStreamFetch. But would it be more appropriate to pass the cleanedOptions to these functions instead? Or is it intentional that they want prefix / extraHeaders / sessionOptions to be embedded in their request query params / body?
There was a problem hiding this comment.
the missing cleanedOptions in these 2 functions should be a mistake, because both generatestream and generatewebsocket use this.#options
yes, we should probably move to pass the cleanedOptions and remove the filtering code from the individual functions
There was a problem hiding this comment.
Yes, I can second that, we can safely pass cleanedOptions to both #generateStreamFetch() and #generateWebsocketFetch() for clarity
| returnRawResponse?: boolean; | ||
| prefix?: string; | ||
| extraHeaders?: object; | ||
| signal?: AbortSignal; |
There was a problem hiding this comment.
How would this actually work in practice with a JSRPC binding?
Considering the AI binding is being actively migrated to JSRPC I'm concerned that this exposes an implementation detail that will force us to stay with fetch invocations for a very long time even when no longer necessary.
Especially since we so far could not make the AbortSignal pass over JSRPC work in production even after you did your best to help @kentonv
There was a problem hiding this comment.
if signal were to work over jsrpc, i like the idea
the plan we have in mind for the .run() method, is to move to jsrpc all invocations that are websocket !== true
only websocket requests will stay over fetch (mostly text to audio or audio to text)
There was a problem hiding this comment.
I honestly didn't know that AbortSignal over RPC still wasn't working. We'll have to fix that.
But we cannot block landing this PR on that. The inability to cancel requests is a huge pain point in gadgets and presumably for many other Workers AI users.
G4brym
left a comment
There was a problem hiding this comment.
lgtm, this will also require upstream work, so that if a request is canceled by the user's signal, the AI inference is also canceled
Adds another option `signal: AbortSignal`, which is passed to the underlying `fetch()`. This allows an application to cancel an AI request.
2dbda91 to
7bf8379
Compare
|
(just a rebase) |
`cleanedOptions` was being created but passed only into `#generateFetch`, not `#generateWebsocketFetch` nor `#generateStreamFetch`. They should all be using the same pattern. (Written by ChatGPT 5.4.)
Adds another option
signal: AbortSignal, which is passed to the underlyingfetch().This allows an application to cancel an AI request.