Skip to content

fix(grok-web): stop streaming errors from reporting false success - #12458

Merged
diegosouzapw merged 5 commits into
release/v3.8.51from
fix/v3851-grok-web-stream-error-boundary
Sep 3, 2026
Merged

diegosouzapw merged 5 commits into
release/v3.8.51from
fix/v3851-grok-web-stream-error-boundary

Conversation

@diegosouzapw

@diegosouzapw diegosouzapw commented Sep 2, 2026 •

Copy link
Copy Markdown
Owner

Summary

Grok Web could translate an upstream NDJSON error into normal assistant content, then emit a
successful stop + [DONE] sequence under HTTP 200. This change makes the producer preserve the
actual failure boundary without depending on the shared stream hardening work.

Scenario Before After
Error before legitimate output HTTP 200 assistant role/content with normal completion Readiness rejects the stream as sanitized HTTP 502, returning success:false to the outer fallback contract
Error after legitimate content Raw upstream detail rendered as [Error: ...], followed by stop Partial content is preserved, then a fixed public stream error reaches the existing pipeline finalizer; no normal stop
Reader/transport failure after content Sanitized text was still emitted as a successful assistant turn Stream errors with the fixed public message and persists a 502 failure
Client cancellation / early terminal tool call Upstream NDJSON reader could remain live or blocked Cancellation is requested exactly once, fire-and-forget, without awaiting a stuck provider cancel promise

The assistant role is now delayed until real content, reasoning, or a tool call exists. Existing
streaming tool-call behavior remains intact.

Security and lifecycle properties

  • Raw upstream error messages, paths, bearer-like values, and stack text never reach the client.
  • Post-content failures invoke the existing onStreamFailure/pipeline persistence path exactly
    once and persist the fixed public message with status 502.
  • Client cancellation emits no later stop, [DONE], or error log and propagates to the upstream
    reader exactly once, even when the upstream cancel() promise never resolves.
  • The parent unit test performs no repository imports and does not mutate its environment,
    globals, database, or call-log writer. This is required because the fast unit suite can run with
    --test-isolation=none.
  • All stateful coverage runs in a child process with an allowlisted environment only: runtime
    path/module/locale/temp values, NODE_ENV=test, a synthetic test-only API_KEY_SECRET, and
    backup/color test flags. No provider credential or token is inherited.
  • The child creates fresh DATA_DIR and OMNIROUTE_PLUGINS_DIR, blocks ambient fetch, stubs
    every Grok event stream, drains and closes call-log resources, resets the DB, closes the shared
    logger, and only then removes its temporary directory.

Evidence

Gate Result
Original false-success RED on base 7802f6ea163f18f349348dbfd8710a159ac32e15 RED 0/1 - error-only upstream returned HTTP 200 instead of expected 502
Hostile downstream-cancel regression before resource fix RED 0/1 - upstream cancel count was 0, expected 1
Shared-process isolation repro before fixture split RED 7/9 - call-log drain and Grok post-content persistence collided through top-level DATA_DIR/DB state
Process-isolated boundary fixture PASS 7/7
Parent subprocess wrapper PASS 1/1 - also asserts the child reported 7 tests, 7 passes, and 0 failures
Combined --test-isolation=none wrapper + call-log drain repro PASS 3/3
Grok Web + Cloudflare classification + executor split regressions PASS 88/88 behaviors - 81 direct regressions plus 7 child boundary tests; outer runner PASS 82/82 (81 + wrapper)
Open-SSE official typecheck gate PASS - openSseTypecheckErrors=0
ESLint with config/quality/eslint-suppressions.json PASS
Prettier check PASS
Changelog integrity + git diff --check PASS
Independent P1/HIGH/material-P2 production review PASS - zero material findings after cancellation remediation
Latest release reconciliation PASS - merged e243b04de22da2d78900c27fde5f83f4fbc3d7f9 without conflicts; focused 82/82 tests and gates rerun on candidate 93b1f7d1256d388bbb547dc8a172ad3a7d74abca

Final candidate was reconciled with live release SHA
e243b04de22da2d78900c27fde5f83f4fbc3d7f9; exact head is
93b1f7d1256d388bbb547dc8a172ad3a7d74abca.

Scope boundary / follow-up

This PR deliberately fixes the streaming path only. The adjacent non-streaming Grok path still
uses the upstream chunk.error when building its JSON 502 response; that needs a separate,
focused sanitization PR and is not hidden by this change.

@diegosouzapw
diegosouzapw marked this pull request as ready for review September 3, 2026 23:59
@diegosouzapw
diegosouzapw merged commit ca2edfd into release/v3.8.51 Sep 3, 2026
21 checks passed
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…egosouzapw#12458)

Validado em lote numa worktree combinada com os 14 PRs desta campanha de error-boundary sobre o tip de `release/v3.8.51`: `typecheck:core` limpo e **120/120** nos 23 arquivos de teste que os PRs trazem.

Um ponto que só apareceu no tree combinado: **diegosouzapw#12465 e diegosouzapw#12466 criam o mesmo arquivo novo** `open-sse/utils/streamReadiness.ts` (que não existe no tip) com desenhos divergentes de cancelamento — `cancelled` + `releaseLock` imediato num, `readInFlight`/`cancelRequested` com `cancelReader` fire-and-forget no outro. Adotei a versão do diegosouzapw#12466, que difere e defere o release do lock para quando a leitura em voo termina, e validei a escolha rodando as suítes dos **dois** PRs contra ela: 21/21 no readiness compartilhado e 22/22 incluindo o boundary do Perplexity.
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.

1 participant