🤖 fix: start tool elapsed timers when execute() actually runs#3716
Conversation
Parallel tool calls are serialized by withSequentialExecution, so a queued tool's elapsed timer (anchored at the model-emission timestamp) could exceed its own timeout before it ever ran. - withSequentialExecution now accepts an onExecutionStart callback fired after the execution lock is acquired - StreamManager stamps executionStartedAt on the dynamic-tool part and emits a new tool-call-execution-start event (with a pending map for the race where execute() begins before the fullStream consumer stores the part); replayed tool-call-start events carry executionStartedAt - Frontend aggregator applies the event; ToolMessage passes executionStartedAt as startedAt, and ElapsedTimeDisplay stays hidden until a real execution start exists
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43e657cb3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…s bounds TaskToolCall best-of recovery and WorkflowRunToolCall foreground run discovery used the startedAt prop as a created-after lower bound. Now that startedAt is the execution start (absent on history-replayed parts), fall back to the model-emission timestamp so recovery keeps working for replayed history. Fixes tests/ui/tasks/bestOfProgress.test.ts CI failure.
- replayStream: replay input-available dynamic-tool parts whose executionStartedAt is newer than the reconnect cursor, so the enriched tool-call-start reaches the renderer - StreamingMessageAggregator: merge replayed executionStartedAt into an existing tool part instead of dropping the event as a duplicate
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 350efa5cb3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
StreamingMessageAggregator summed per-tool durations from tool-call-start, so serialized parallel siblings double-counted queue wait (inflating tool time and deflating derived streaming/tok-s stats). tool-call-execution-start now re-anchors pendingToolStarts at the true execution start. Backend SessionTimingService is intentionally unchanged: it measures wall segments (open while any call is outstanding), so sibling queue wait coincides with the running sibling's execution and is not over-counted.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78fa812464
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…eview) Replayed tool-call-start events carrying executionStartedAt now update pendingToolStarts in both aggregator paths: the duplicate-merge branch re-anchors the pre-disconnect emission seed, and fresh replayed parts seed directly from the execution start. Extracted shared reanchorPendingToolStart helper used by handleToolCallExecutionStart too.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a2f2a2647
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… review) tool-call-execution-start is not replay-buffered, so during a reconnect replay it can arrive before the replayed tool-call-start creates the part. Instead of dropping it, stash messageId->toolCallId->timestamp and fold it into handleToolCallStart (part field + pendingToolStarts anchor); cleared in cleanupStreamState.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2397d0d2d1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
handleToolExecutionStart used raw Date.now(), which can be <= the monotonicized tool-call part timestamp in the same millisecond; the reconnect replay repair predicate (executionStartedAt > cursor) would then never fire for clients that disconnected right after tool-call-start. Use nextPartTimestamp() so execution starts are strictly ordered after prior part timestamps.
|
@codex review |
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Summary
Tool-call elapsed timers in the chat UI now start when the tool's
execute()actually begins running instead of when the model emits the tool call, so queued parallel tool calls no longer display elapsed times exceeding their own timeout.Background
Providers may plan several tool calls in parallel, but mux serializes their
execute()handlers viawithSequentialExecution. The UI anchored each tool's elapsed timer on the dynamic-tool parttimestamp(set when thetool-callstream part is processed), so every queued sibling ticked while waiting for the lock — producing confusing displays liketimeout: 25s • 51son tools that had not started running yet.Implementation
withSequentialExecutionaccepts an optionalonExecutionStart(toolCallId)callback invoked immediately after the execution lock is acquired, just before the wrappedexecute()runs.StreamManageruses that callback to stamp a new persistedexecutionStartedAtfield on the dynamic-tool part and emit a newtool-call-execution-startchat event. A pending map handles the race whereexecute()begins before the fullStream consumer stores the part (same pattern as pending workflow-run attachments). Replayedtool-call-startevents carryexecutionStartedAtso reconnects don't reset timers.ToolMessagepassesexecutionStartedAt(instead ofmessage.timestamp) asstartedAt, andElapsedTimeDisplayrenders nothing until a real execution start exists.Resulting UX: queued tools show only their timeout (no ticking counter); the currently-executing tool counts from its true start; completed bash durations are unchanged (
wall_duration_msfrom the tool result).Validation
withSequentialExecutiontest proves a queued sibling's execution start is not reported until the lock is released.execute()winning the race).tool-call-execution-startstampsexecutionStartedAtonto displayed tool rows.Risks
Low. The new event is additive; unknown event types are ignored by older consumers, and
executionStartedAtis optional everywhere. The main behavior change is intentional: executing-timer display now requires the execution-start signal, so pre-existing history rows without it simply show no elapsed ticker while pending (they previously showed a misleading one). Live per-turn tool-time stats (pendingToolStarts/ sessionTimingService) intentionally still measure from tool-call-start and are unaffected.Generated with
mux• Model:anthropic:claude-fable-5• Thinking:xhigh• Cost:$30.52