Skip to content

fix(#8138): log onComplete/onFailure callback errors in stream.ts - #8180

Closed
rafaumeu wants to merge 2 commits into
cheaper-inference:release/v3.8.49from
rafaumeu:fix/stream-silent-catch-8138
Closed

rafaumeu wants to merge 2 commits into
cheaper-inference:release/v3.8.49from
rafaumeu:fix/stream-silent-catch-8138

Conversation

@rafaumeu

Copy link
Copy Markdown
Contributor

Summary

4 empty catch {} blocks around onComplete and onFailure callbacks in stream.ts silently swallowed errors. When callbacks threw, the errors were invisible — stream completion/failure issues were impossible to debug.

Fix

Replaced all 4 empty catches with catch (streamCbErr) that logs via console.debug with context (callback name + error).

Affected callbacks:

  • 2x onComplete (lines ~2403, ~2669)
  • 2x onFailure (lines ~2488, ~2513)

Test

tests/unit/stream-silent-catch-8138.test.ts — verifies 4 catch(streamCbErr) blocks exist with proper logging.

Fixes #8138

@rafaumeu
rafaumeu requested a review from diegosouzapw as a code owner July 22, 2026 16:00
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@diegosouzapw

Copy link
Copy Markdown
Collaborator

Thanks for the fix — the open-sse/utils/stream.ts change itself (4 empty catch {} → catch (streamCbErr) with console.debug logging) is correct, ESLint-clean, and the new test plus the surrounding stream test suite (114 tests) all pass.

One issue with the PR as submitted: the branch is stacked on 6 earlier commits (for #8074, #8072, #8059, #8081, #8056 x2, #8141) that touch 13 other files unrelated to #8138. Those are already tracked separately — some in your own PRs (#8110 for #8081, #8179 for #8141), some in other contributors' open PRs (#8165, #8062, #8058), and some you've already closed as superseded (#8078, #8108, #8109, #8111). Merging this PR as-is would re-land that work a second time and could conflict with those other PRs.

I confirmed the only commit unique to this PR versus its immediate parent (#8179) is the stream.ts one — that's the actual content of #8138's fix.

For the merge, we'll rebase this PR down to just that single commit (stream.ts + the new test) on top of the release branch — no action needed from you there. Two small things worth a follow-up on your side if you want:

  • The 4 new } catch (streamCbErr) { lines lost their indentation (sitting at column 0) — a quick prettier --write open-sse/utils/stream.ts would clean that up.
  • The new test asserts against the source text (regex over the file contents) rather than actually invoking createSSEStream with a throwing callback — it works as a revert-guard, but a behavioral test would be stronger.

Nothing here blocks the fix itself, just the branch hygiene before it lands.

@diegosouzapw
diegosouzapw changed the base branch from main to release/v3.8.49 July 22, 2026 18:27
@rafaumeu
rafaumeu force-pushed the fix/stream-silent-catch-8138 branch 4 times, most recently from 810dfd0 to 7441966 Compare July 22, 2026 22:33
@mergify

mergify Bot commented Jul 22, 2026

Copy link
Copy Markdown

⚠️ The sha of the head commit of this PR conflicts with #8113. Mergify cannot evaluate rules on this PR. Once #8113 is merged or closed, Mergify will resume processing this PR. ⚠️

@rafaumeu
rafaumeu force-pushed the fix/stream-silent-catch-8138 branch 4 times, most recently from a676b20 to a60dbcd Compare July 23, 2026 02:10
rafaumeu added 2 commits July 22, 2026 23:44
…lowing

4 empty catch blocks around onComplete/onFailure callbacks silently
swallowed errors, making stream completion/failure issues invisible
to operators. Replaced with console.debug logging.

Fixes cheaper-inference#8138
@rafaumeu
rafaumeu force-pushed the fix/stream-silent-catch-8138 branch from a60dbcd to 84c3aab Compare July 23, 2026 02:47
@diegosouzapw

Copy link
Copy Markdown
Collaborator

Thanks, @rafaumeu — closing this as subsumed. #8143 (merged 2026-07-23) already logs the exact same 4 onComplete/onFailure callback catch blocks in stream.ts, and its version includes the model context in the message, so the production change here is fully covered. Really appreciate the fix — the same intent landed via #8143. If you spot a callback catch that #8143 missed, a focused follow-up is very welcome. 🙏

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.

fix(stream): silent error swallowing in onComplete/onFailure callbacks in stream.ts

2 participants