Skip to content

fix(ai-gateway): close terminal stream errors - #4900

Merged
chrarnoldus merged 2 commits into
mainfrom
investigate-missing-logging-event-after-client-dis
Jul 30, 2026
Merged

fix(ai-gateway): close terminal stream errors#4900
chrarnoldus merged 2 commits into
mainfrom
investigate-missing-logging-event-after-client-dis

Conversation

@chrarnoldus

Copy link
Copy Markdown
Contributor

Problem

Provider-sent SSE error events were forwarded to clients but were not treated as terminal events. When the client disconnected in response, logs showed the cancellation without the terminal event that triggered it, and the stream stayed open until that cancellation or upstream EOF.

Fix

  • treat Chat Completions {error: ...}, Messages type: "error", and Responses type: "error" events as terminal
  • log them through the existing received terminal stream event logger
  • close the downstream stream and cancel upstream immediately after forwarding the error
  • add regression coverage for logging, closure, cancellation, and raw response capture across all three API kinds

Testing

CI will validate the change.

@chrarnoldus chrarnoldus self-assigned this Jul 30, 2026
Comment thread apps/web/src/lib/rewriteModelResponse.ts Outdated
@kilo-code-bot

kilo-code-bot Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Executive Summary

Incremental review of b30adde4c confirms the Chat Completions terminal-error check now requires a non-null error value, resolving the only previously open finding, with regression coverage added and no new issues in the changed lines.

Verified in this pass
  • rewriteModelResponse.ts:457 uses 'error' in json && json.error != null, matching the safer processUsage.responses.ts:157 pattern, so a provider emitting "error": null on normal chunks no longer truncates the stream.
  • The assignment cannot reset a previously set terminal flag: the onEvent guard returns early once terminalEventReceived is true.
  • New test does not treat a null error field as terminal asserts continued streaming and the original [DONE] passthrough.
  • Messages (type === 'error') and Responses terminal detection are unaffected by the null-value concern.
Files Reviewed (2 files)
  • apps/web/src/lib/rewriteModelResponse.ts
  • apps/web/src/lib/rewriteModelResponse.test.ts
Previous Review Summary (commit 8f04e16)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit 8f04e16)

Status: 1 Issue Found | Recommendation: Address before merge

Executive Summary

The terminal-error handling is correct and well covered by tests; the only concern is that the Chat Completions detection uses a key-presence check ('error' in json), which would terminate a healthy stream if a provider emits "error": null on normal chunks.

Overview

Severity Count
CRITICAL 0
WARNING 0
SUGGESTION 1
Issue Details (click to expand)

SUGGESTION

File Line Issue
apps/web/src/lib/rewriteModelResponse.ts 457 'error' in json treats a null-valued error field as terminal, truncating the client stream; processUsage.responses.ts:157 uses the safer 'error' in json && json.error form
Verified as correct
  • Interval from createStreamProgressLogger is still cleared on the new early-return path (onFinally runs in rewriteSseStream's finally), so no timer leak is introduced.
  • Suppressing data: [DONE] after an error event matches the existing read-error path behavior, which already closes without [DONE].
  • Upstream cancellation does not starve usage accounting: accountForMicrodollarUsage reads a response.clone() tee branch, which keeps draining after this branch cancels.
  • MessagesApiError union member preserves existing message_start/message_delta narrowing.
Files Reviewed (2 files)
  • apps/web/src/lib/rewriteModelResponse.ts - 1 issue
  • apps/web/src/lib/rewriteModelResponse.test.ts - 0 issues

Fix these issues in Kilo Cloud


Reviewed by claude-opus-5 · Input: 34 · Output: 4.4K · Cached: 643.5K

Review guidance: REVIEW.md from base branch main

@chrarnoldus
chrarnoldus enabled auto-merge (squash) July 30, 2026 18:14
@chrarnoldus
chrarnoldus merged commit 2a08ce1 into main Jul 30, 2026
16 checks passed
@chrarnoldus
chrarnoldus deleted the investigate-missing-logging-event-after-client-dis branch July 30, 2026 18:20
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.

2 participants