fix(kosong): retry a dropped provider stream (terminated) - #1274
Conversation
🦋 Changeset detectedLatest commit: 5e8698c The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
commit: |
![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: b57c798f54
ℹ️ 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".
| const closingPrefix = `${sessionId}${String.fromCharCode(0)}`; | ||
| for (const activeKey of [...this._active.keys()]) { | ||
| if (activeKey.startsWith(closingPrefix)) this._active.delete(activeKey); | ||
| } |
There was a problem hiding this comment.
These deletes only run from ISessionService.onDidClose, but the normal SDK/TUI close path calls Session.close() → rpc.closeSession, and CoreImpl.closeSession removes the core session without firing that service event; SessionService.onDidClose is only fired by archive. In the close/reopen-after-dropped-stream case this patch is meant to fix, _active therefore survives and the next submit can still queue behind the stale prompt, so the cleanup needs to be triggered from the actual closeSession path as well.
Useful? React with 👍 / 👎.
| if ( | ||
| this.host.state.appState.streamingPhase !== 'idle' && | ||
| !this.host.streamingUI.hasActiveTurn() | ||
| ) { | ||
| this.host.setAppState({ streamingPhase: 'idle' }); | ||
| this.host.resetLivePane(); | ||
| const next = this.host.shiftQueuedMessage(); |
There was a problem hiding this comment.
Agent turn failures emit turn.ended and then a separate error event (packages/agent-core/src/agent/turn/index.ts:580-599). If a failed turn has queued input, finalizeTurn shifts one message and schedules it with setTimeout(0); when that new prompt is in waiting but has not received turn.started yet, a delayed error from the previous turn satisfies this condition, resets the new request to idle, and shifts another queued message. Users with multiple queued messages after a failed turn can therefore have prompts dequeued/submitted out of order; this recovery should be gated to errors for the current request rather than any no-active-turn error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11022106d8
ℹ️ 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".
| this.host.setAppState({ streamingPhase: 'idle' }); | ||
| this.host.resetLivePane(); |
There was a problem hiding this comment.
When the first prompt fails before any turn.started, the user can already have typed another message while streamingPhase was waiting. This branch is the only recovery path because no turn.ended will arrive, but it sets the UI back to idle without shifting/sending state.queuedMessages, so the queued message stays stuck until some later action, and a later manual prompt can run ahead of it. Please drain the next queued item on the current pre-turn-error path, with a stale-error guard as needed.
Useful? React with 👍 / 👎.
| { sid, promptId: existing.promptId, agentId: existing.agentId }, | ||
| '[prompt-service] releasing stale active prompt: core reports no turn in flight', | ||
| ); | ||
| this._active.delete(key); |
There was a problem hiding this comment.
If prompt B was queued while prompt A was genuinely active, then A dies without turn.ended and prompt C is submitted, this new stale-active branch deletes A and starts C immediately while B remains in _queued. Since _startNextQueued only runs after C ends, the daemon executes C before the older queued B. When clearing a stale active slot, promote the existing queue first or enqueue the new state behind it so submitted prompts keep FIFO order.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 11022106d8
ℹ️ 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".
| // flight, the slot is stale, so release it and start this prompt instead | ||
| // of queueing behind a phantom forever. This is what keeps "send a | ||
| // message" and "resume" from being permanently blocked. | ||
| if (await this._coreHasActiveTurn(sid)) { |
There was a problem hiding this comment.
When the first prompt is only buffered by manual/SDK compaction, agent-core has no active turn yet (TurnFlow.launch buffers while fullCompaction.isCompacting, and hasActiveTurn is false until a real active turn exists). A second submit during that window now treats the existing prompt as stale, deletes _active, and starts the new prompt instead of queueing it, so the first buffered turn's later turn.started/turn.ended can be attributed to the wrong prompt and the original prompt never gets a prompt lifecycle event.
Useful? React with 👍 / 👎.
| this.host.setAppState({ streamingPhase: 'idle' }); | ||
| this.host.resetLivePane(); |
There was a problem hiding this comment.
If the user submits another message while the current request is still in waiting and the provider then emits an error before any turn.started, this branch correctly notes that no turn.ended will arrive but only marks the UI idle. The queued message remains in queuedMessages because queue draining only happens from finalizeTurn/shell completion, so it can sit there until a later unrelated turn completes and then be sent out of order.
Useful? React with 👍 / 👎.
…ropic path A raw undici `terminated` error — an SSE/HTTP response body cut mid-flight, common on long streaming responses — fell through convertAnthropicError to a generic base ChatProviderError, which isRetryableGenerateError treats as fatal, so it was never retried. Route raw non-SDK errors through the shared classifyBaseApiError heuristic (already used by the OpenAI path) so a dropped stream is classified as a retryable APIConnectionError and retried instead of failing the turn.
1102210 to
5e8698c
Compare
RealKai42
changed the title
1 participant
Problem
A raw undici
TypeError: terminated— raised when an SSE/HTTP response body is dropped mid-flight, common on long streaming responses — fell throughconvertAnthropicErrorto a generic baseChatProviderError.isRetryableGenerateErrortreats that base class as terminal, so the dropped stream was never retried and failed the turn outright. The OpenAI path already recognized this signature and retried; the Anthropic path did not.Fix
Route raw, non-SDK errors in
convertAnthropicErrorthrough a sharedclassifyBaseApiErrorheuristic (promoted from the OpenAI provider intoerrors.tsso both providers classify transport errors the same way). A dropped connection /terminatedbecomes a retryableAPIConnectionError, a timeout becomesAPITimeoutError, and anything else stays a non-retryable baseChatProviderError— so a transient stream drop is retried instead of failing the turn.Tests
convertAnthropicError(new TypeError('terminated'))is asserted to be a retryableAPIConnectionError, both as a unit case and through streamed SSE iteration (mirroring the existing OpenAI-path tests). Fullkosongsuite green (1126 tests), typecheck clean.