Skip to content

fix(sse): emit valid responses keepalives - #13850

Closed
TheDemonTuan wants to merge 1 commit into
diegosouzapw:release/v3.8.51from
TheDemonTuan:fix/upstream-responses-keepalive
Closed

TheDemonTuan wants to merge 1 commit into
diegosouzapw:release/v3.8.51from
TheDemonTuan:fix/upstream-responses-keepalive

Conversation

@TheDemonTuan

Copy link
Copy Markdown
Contributor

Summary

  • Replace malformed synthetic response.in_progress heartbeat events with SSE comments
  • Preserve real Responses API event sequence numbers and payloads without emitting schema-violating placeholders
  • Update early- and mid-stream regression coverage

Verification

  • node --import tsx/esm --test tests/unit/sseHeartbeat.test.ts tests/unit/sse-heartbeat.test.ts tests/unit/earlyStreamKeepalive.test.ts tests/unit/early-stream-keepalive.test.ts tests/unit/chatcore-streaming-pipeline.test.ts tests/unit/responses-route-early-keepalive-wiring.test.ts (all passed)

@diegosouzapw

Copy link
Copy Markdown
Owner

Re-reviewed this against the fuller bug report that has since landed (#14330), and two things need
changing before it can merge.

First, the new keepalive shape bypasses the opt-out from #10524. The gate at
open-sse/utils/sseHeartbeat.ts:107 keys on shape === HEARTBEAT_SHAPES.COMMENT, and the new
openai-responses-keepalive shape emits a : comment without being that shape — so
OMNIROUTE_SSE_COMMENTS=off no longer suppresses it, which re-breaks the strict clients that flag
was added for. Either reuse the COMMENT shape or widen the gate to any comment-shaped payload.

Second, #14330's third criterion is the invalid failure frames, and this PR doesn't touch them:
six emitters ship response.failed/error events with no sequence_number or a hardcoded 0 while
the translator numbers from 1 (streamHandler.ts:454,464-472, diagnostics.ts:143-159,
codex.ts:515-520 and :982-989, streamErrorFormat.ts:296-307,
earlyStreamKeepalive.ts:83-90,128), and the same invalid keepalive is also emitted mid-stream at
responsesHandler.ts:91-94. Either extend the PR or say in the body that #14330 stays open for
them — and please use Refs #14330 rather than Closes, since our default branch is the release
branch and Closes would auto-close it.

One coordination note: #14377 asks for the keepalive shape to be configurable, which this PR
decides one way by default. Fixing the invalid frames is unconditional and welcome; the shape choice
is probably better behind the env switch that issue asks for. Reds currently on the branch are
inherited from base-red #14496.

@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks @TheDemonTuan for chasing the invalid Responses keepalive frames (#14330). You were right that the synthetic response.in_progress heartbeat did not match the Responses schema.

#14572 (merged 2026-09-24) fixed this on release/v3.8.51 in a different way. The synthesized frames stay as parser-visible events, but they now carry a real sequence_number (SYNTHETIC_RESPONSES_SEQUENCE_NUMBER in open-sse/utils/sseHeartbeat.ts and open-sse/utils/earlyStreamKeepalive.ts). We kept that design over bare : comments because some strict OpenAI-compatible clients need a periodic event frame during the early phase. This branch rewrites the same code the other way, and it also ran into the malformed-JSON admission-release regression in the responses route, so I'm closing it as superseded.

If you hit a client that still rejects the frames the tip sends now, please open an issue with a capture. We'll look at it against the current design.

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