Skip to content

fix(ai): defer streamed tool argument parsing until read - #9461

Open
petrroll wants to merge 1 commit into
earendil-works:mainfrom
petrroll:investigate/issue-9265
Open

petrroll wants to merge 1 commit into
earendil-works:mainfrom
petrroll:investigate/issue-9265

Conversation

@petrroll

Copy link
Copy Markdown
Contributor

fixes #9265

Stops reparsing the full accumulated tool-call json on every delta. Instead moves reparsing into per-access of .arguments (cached, so only first access on same version re-parses).


It isn't complex code but it's not idiomatic in the codebase and I'm not sure it's worth it. It's done this way not to break anyone existing (mostly at least), but maybe making the system explicit in removing this field access and adding getter method instead (to indicate potential ad-hoc work) s.a. getLastSnapshot() would be better. (It'll allow cleanup of some copy shenanigans which try to copy objects without triggering field reads. ).

I chose not to do it not to break folks but with AI and changelogs and how pi approaches braking changes maybe it'd be better. Let me know or just change my PR / use it as base for your fix. Whichever works.


In any case, this removes unconditional whole-prefix argument parsing from the seven ingestion paths we identified. Reasoning-signature accumulation was already fixed, and plain text/thinking ingestion only appends deltas. It doesn't address frame-encoder catch-up, which can still reparse growing prefixes.

Do note: The current TUI still reads arguments on each update, so it can still incur quadratic parsing, this PR just makes it so other more conscious consumers don't have to.


A few visible differences: the proxy now ignores argument deltas after toolcall_end instead of letting alte data override them. Some other malformed event sequences (s.a. pi-messages argument deltas after tool end) can now error where main tolerated them. Live .arguments is also a getter/setter, so strict descriptor-based validators like isJsonValue() can reject it even though normal field access and JSON serialization work.

@holny

holny commented Sep 11, 2026

Copy link
Copy Markdown

Hit the same #9265 hot path from the issue side — the O(n^2) reparse is real, and the delta-only regression tests here are the right guard.

On the design question you raised: the lazy arguments getter isn't just non-idiomatic, it's invisible work. Any spread or JSON.stringify over a streaming content block (extensions, loggers, anything copying messages mid-stream) silently triggers parseStreamingJson on the partial json, and only this module's own copy() guards against it via the Reflect.ownKeys placeholder. I'd lean towards the explicit accessor for that reason alone. The settle-on-finish() part (plain property afterwards, so session serialization never pays the getter) seems right regardless.

@gaoanze888 gaoanze888 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The delta-only optimization is real, and the submitted focused suites pass (40 tests). I found one terminal-state data-loss case in the OpenAI Responses path.

createSlot() copies item.arguments into block.partialJson, but initializes createPendingToolCall() with arguments: {} and does not call pending.setJson(block.partialJson). If response.output_item.added already contains a complete function-call argument string and the stream then emits response.failed (or otherwise exits before a delta / output_item.done), the finally finalizer materializes {} rather than the provider-supplied arguments.

Minimal sequence:

response.output_item.added(function_call, arguments='{"content":"already present"}')
response.failed(server_error)

On this exact head, processResponsesStream() rejects as expected, but output.content[0].arguments is {}. I added a local regression expecting { content: "already present" }; it fails exactly that way. This also conflicts with the new README statement that terminal messages materialize unread arguments on errors and aborts.

Please initialize the pending state from item.arguments (or call pending.setJson() when creating the function-call slot) and add a regression for the pre-populated-added-item + failed-before-done path. I also tested duplicate/end-late pi-messages sequences; those now error, but the PR explicitly documents that malformed-sequence behavior change, so I am not treating it as a separate blocker.

This comment is AI-generated by /wr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

@earendil-works/pi-ai: O(n²) tool-call argument re-parsing in openai-completions streaming freezes the event loop

3 participants