fix(stream): add logging to empty catch blocks in stream error handling - #8143
Conversation
- targetExhaustion: skip remaining same-provider models on 401 auth failure (prevents opencode-zen noauth cascade wasting retry attempts) (#8133) - modelFamilyFallback: skip unsupported models in T5 fallback chain (prevents GitHub provider trying deprecated claude-opus-4.8/4.7) (#8134) - sqljsAdapter: split package.json resolve string to suppress Next.js Can't resolve warning at build time (#8135)
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
- stream.ts: Log errors in onComplete/onFailure callbacks (lines 929, 1112, 2451, 2536, 2561, 2717) - streamHandler.ts: Log errors in stall watchdog and trackPendingRequest (lines 249, 334, 657, 663, 667) - cursor.ts: Add comments to intentional H2 lifecycle catches, log KV/exec errors - next.config.mjs: Externalize sql.js to suppress build warnings Closes #8138, #8139, #8140, #8141, #8142
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8be93402f
ℹ️ 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".
| !providerExhausted && | ||
| result.status === 401 && | ||
| Boolean(provider && provider !== "unknown") && | ||
| !hasPerModelQuota(provider, rawModel); | ||
|
|
||
| if (providerExhausted || authExhausted) { |
There was a problem hiding this comment.
Scope 401 exhaustion to the failed connection
When a combo has multiple active accounts for the same non-per-model provider, a single expired or invalid account can return 401 here and exhaustedProviders.add(provider) will cause every later target for that provider to be skipped for the rest of the request, including targets with different connectionIds that still have valid credentials. The comment says this is for “all accounts” being unavailable, but the predicate only checks the status/provider; scope the 401 to exhaustedConnections when a connection is known, or only provider-exhaust after proving all accounts are unavailable.
AGENTS.md reference: open-sse/services/AGENTS.md:L11-L13
Useful? React with 👍 / 👎.
…ustion/fallback hunks Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
…n-growth Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
|
Thanks, @chirag127! 🙏 Merged the logging-hygiene scope (console.debug on the stream-subsystem empty catch blocks + sql.js externalize). Dropped the two undisclosed behavioral hunks that were out of scope — the #8133 401 auth-exhaustion branch and the #8134 family-fallback filter (which regressed kiro-claude-sonnet-5-2267). Rebaselined stream.ts file-size for the legitimate logging own-growth. If you'd like the #8133/#8134 changes, please resubmit them as separately-tested PRs. |
…ng (diegosouzapw#8143) * fix(combo,model-fallback,sqljs): three stream-reliability fixes - targetExhaustion: skip remaining same-provider models on 401 auth failure (prevents opencode-zen noauth cascade wasting retry attempts) (diegosouzapw#8133) - modelFamilyFallback: skip unsupported models in T5 fallback chain (prevents GitHub provider trying deprecated claude-opus-4.8/4.7) (diegosouzapw#8134) - sqljsAdapter: split package.json resolve string to suppress Next.js Can't resolve warning at build time (diegosouzapw#8135) * fix(stream): add logging to empty catch blocks in stream error handling - stream.ts: Log errors in onComplete/onFailure callbacks (lines 929, 1112, 2451, 2536, 2561, 2717) - streamHandler.ts: Log errors in stall watchdog and trackPendingRequest (lines 249, 334, 657, 663, 667) - cursor.ts: Add comments to intentional H2 lifecycle catches, log KV/exec errors - next.config.mjs: Externalize sql.js to suppress build warnings Closes diegosouzapw#8138, diegosouzapw#8139, diegosouzapw#8140, diegosouzapw#8141, diegosouzapw#8142 * refactor(stream): scope PR to logging hygiene, drop out-of-scope exhaustion/fallback hunks Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> * chore(quality): rebaseline stream.ts for diegosouzapw#8143 empty-catch logging own-growth Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --------- Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Co-authored-by: rafaumeu <rafael.zendron22@gmail.com> Co-authored-by: chirag127 <chirag127@users.noreply.github.com>
…ng (diegosouzapw#8143) * fix(combo,model-fallback,sqljs): three stream-reliability fixes - targetExhaustion: skip remaining same-provider models on 401 auth failure (prevents opencode-zen noauth cascade wasting retry attempts) (diegosouzapw#8133) - modelFamilyFallback: skip unsupported models in T5 fallback chain (prevents GitHub provider trying deprecated claude-opus-4.8/4.7) (diegosouzapw#8134) - sqljsAdapter: split package.json resolve string to suppress Next.js Can't resolve warning at build time (diegosouzapw#8135) * fix(stream): add logging to empty catch blocks in stream error handling - stream.ts: Log errors in onComplete/onFailure callbacks (lines 929, 1112, 2451, 2536, 2561, 2717) - streamHandler.ts: Log errors in stall watchdog and trackPendingRequest (lines 249, 334, 657, 663, 667) - cursor.ts: Add comments to intentional H2 lifecycle catches, log KV/exec errors - next.config.mjs: Externalize sql.js to suppress build warnings Closes diegosouzapw#8138, diegosouzapw#8139, diegosouzapw#8140, diegosouzapw#8141, diegosouzapw#8142 * refactor(stream): scope PR to logging hygiene, drop out-of-scope exhaustion/fallback hunks Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> * chore(quality): rebaseline stream.ts for diegosouzapw#8143 empty-catch logging own-growth Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> --------- Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com> Co-authored-by: rafaumeu <rafael.zendron22@gmail.com> Co-authored-by: chirag127 <chirag127@users.noreply.github.com>
Summary
Replaces silent empty catch blocks with contextual logging across the stream subsystem, and externalizes sql.js in next.config.mjs to suppress build warnings.
Scope note (maintainer edit): this PR is scoped to logging hygiene only. Two unrelated hunks that had ended up on this branch (a
targetExhaustion.ts401 auth-exhaustion change and amodelFamilyFallback.tscandidate-skip change, tracked separately as #8133 and #8134) have been reverted back to the release baseline here so this PR stays focused and doesn't carry a regression the other two changes introduced againsttests/unit/kiro-claude-sonnet-5-2267.test.ts. Those two changes should be proposed in their own PR(s).Changes
open-sse/utils/stream.ts
onCompletecallbacks (lines 929, 1112, 2451, 2567, 2723)onFailurecallbacks (lines 929, 1112, 2542)console.debugwith[STREAM]prefix and model contextopen-sse/utils/streamHandler.ts
trackPendingRequestdecrement failures (line 249)onErrorcallback failures (line 336)controller.close()catches (lines 537, 546, 558)open-sse/executors/cursor.ts
next.config.mjs
sql.jstoserverExternalPackagesto suppress 20+ build warningssrc/lib/db/adapters/sqljsAdapter.ts
"sql.js" + "/package.json"expression instead of a static resolve string, so Next's static analysis stops flagging an unresolvablesql.js/package.jsonbuild warning (fix(dependencies): sqljsAdapter build warning "Can't resolve sql.js/package.json" #8135)Issues Fixed
Testing
npm run typecheck:corepassestests/unit/stream-handler-catch-logging-8143.test.ts: regression guard proving the stall-watchdog'shandleErrorcatch now logs viaconsole.debuginstead of silently swallowingtests/unit/kiro-claude-sonnet-5-2267.test.tspasses (was regressed by the now-revertedmodelFamilyFallback.tshunk; confirmed green after the revert)