Skip to content

fix(sse): give synthesized Responses API keepalive and failure frames a real sequence_number (#14330) - #14572

Merged
diegosouzapw merged 1 commit into
release/v3.8.51from
fix/14330-responses-keepalive-frames
Sep 24, 2026
Merged

diegosouzapw merged 1 commit into
release/v3.8.51from
fix/14330-responses-keepalive-frames

Conversation

@diegosouzapw

Copy link
Copy Markdown
Owner

Closes #14330

⚠️ base-red inherited: #14547

Root cause

Six hand-built SSE frames the /v1/responses route synthesizes (startup/mid-stream keepalive
and five failure paths) violate the documented Responses API event schema: one frame
(OPENAI_RESPONSES_IN_PROGRESS_FRAME) had no sequence_number or response object at all,
and four others (synthResponsesFailure(), OPENAI_RESPONSES_ERROR_FRAME,
buildResponsesErrorDataLine(), formatTranslatedStreamError()'s Responses branch,
toCodexResponseFailedEvent(), failController()'s inline payload) either omitted
sequence_number entirely or hardcoded it to 0 — even though the real per-stream counter
(eventEmitter.ts's ++state.seq, seeded at 0 in responsesTransformer.ts) numbers the
first real event 1. A strict Responses decoder (openai-python SDK, Codex/OpenCode/Grok CLIs)
aborts on a frame missing the field, and a duplicate 0 collides with/precedes the real
stream's own numbering — worst on the failure paths, because the client can die before it
ever sees the real upstream error.

Fix

Six synthesized-frame sites, in four independent modules, have no access to the real
per-stream sequence counter — threading a shared counter across them risked colliding with a
real event's own sequence_number (see the plan-file's Risks section). Per the plan's
guidance, every one of these sites now uses a single shared synthetic seed instead:

  • open-sse/utils/responsesSequence.ts (new): SYNTHETIC_RESPONSES_SEQUENCE_NUMBER = 1 and a
    buildSyntheticResponsesFailedEvent() helper for the two Codex WebSocket sites.
  • open-sse/utils/sseHeartbeat.ts: OPENAI_RESPONSES_IN_PROGRESS_FRAME now carries
    sequence_number and a minimal response object.
  • open-sse/utils/diagnostics.ts: synthResponsesFailure() now includes sequence_number.
  • open-sse/utils/earlyStreamKeepalive.ts: OPENAI_RESPONSES_ERROR_FRAME and
    buildResponsesErrorDataLine() use the shared seed instead of a hardcoded 0.
  • open-sse/utils/streamErrorFormat.ts: formatTranslatedStreamError()'s Responses branch
    uses the shared seed instead of a hardcoded 0.
  • open-sse/executors/codex.ts: toCodexResponseFailedEvent() and the WebSocket
    failController()'s inline payload now build their response.failed event through the
    shared helper (also required trimming both call sites to stay under the frozen file-size
    baseline for codex.ts — see below).

Out of scope, per the plan-file (explicitly deferred by the maintainer): the keepalive frame
shape (comment vs. typed event) — that is PR #13850 / issue #14377's policy call.

Regression test

New file: tests/unit/responses-sse-frame-schema-14330.test.ts (the plan-file's proven repro,
moved into the permanent suite).

RED (unfixed code, git apply -R of this PR's diff):

✖ #14330: OPENAI_RESPONSES_IN_PROGRESS_FRAME ... AssertionError: actual 'undefined' !== expected 'number'
✖ #14330: synthResponsesFailure() ... AssertionError: actual 'undefined' !== expected 'number'
✖ #14330: OPENAI_RESPONSES_ERROR_FRAME ... AssertionError: actual 0 === expected 0 (should not be equal)
✖ #14330: formatTranslatedStreamError() ... AssertionError: actual 0 === expected 0 (should not be equal)
ℹ tests 4 / pass 0 / fail 4

GREEN (fixed code):

✔ #14330: OPENAI_RESPONSES_IN_PROGRESS_FRAME ... (19.6ms)
✔ #14330: synthResponsesFailure() ... (500.9ms)
✔ #14330: OPENAI_RESPONSES_ERROR_FRAME ... (32.6ms)
✔ #14330: formatTranslatedStreamError() ... (9.9ms)
ℹ tests 4 / pass 4 / fail 0

Gates run

  • node --import tsx/esm --test tests/unit/responses-sse-frame-schema-14330.test.ts — RED then GREEN (above)
  • npm run typecheck:core — exit 0
  • node scripts/check/check-open-sse-typecheck.mjs — the only failure is a pre-existing
    open-sse/executors/auggie.ts TS error (TS2769/TS18047), a file this PR does not touch
    (git diff origin/release/v3.8.51 -- open-sse/executors/auggie.ts is empty) — inherited base-red.
  • npx eslint --suppressions-location config/quality/eslint-suppressions.json <every changed file> — exit 0
  • node scripts/check/check-file-size.mjs — clean; codex.ts stayed under its frozen 1570-line
    baseline (1566 after the fix) by routing both WebSocket failure sites through the new shared
    buildSyntheticResponsesFailedEvent() helper instead of duplicating the object inline.
  • node scripts/check/check-complexity-ratchets.mjs --base-ref origin/release/v3.8.51 — OK,
    0 new violations in the 6 touched files (complexity 17/17 base, cognitive 8/8 base)
  • node scripts/check/check-mutation-test-coverage.mjs --strict — fails, but on 8 pre-existing
    entries across accountFallback.ts / src/sse/services/auth.ts / comboStructure.ts /
    quotaScoring.ts — none of the files this PR touches; identical failure reproduced against
    origin/release/v3.8.51's stryker.conf.json (inherited, not caused by this PR)
  • node scripts/check/check-changelog-integrity.mjs — OK
  • npm run check:public-creds — OK

Existing tests

Ran every existing test referencing the touched symbols (OPENAI_RESPONSES_IN_PROGRESS_FRAME,
synthResponsesFailure, OPENAI_RESPONSES_ERROR_FRAME, formatTranslatedStreamError,
toCodexResponseFailedEvent, buildResponsesErrorDataLine) plus executor-codex.test.ts
(the two Codex WebSocket response.failed sites): 130/130 pass.

Three pre-existing tests encoded the OLD (now-corrected) bare
{"type":"response.in_progress"} frame shape and were aligned to the new, schema-valid shape
(added sequence_number/response, never removed or weakened an assertion):

  • tests/unit/early-stream-keepalive.test.ts (2 assertions)
  • tests/unit/sse-heartbeat.test.ts (1 assertion)
  • tests/unit/sseHeartbeat.test.ts (1 assertion)

One test (earlyStreamKeepalive.test.ts: "handler rejection emits error frame") failed only
when run as part of a large concurrent batch and passed cleanly in isolation — an unrelated
timer/ordering flake on this shared devbox, not a regression from this diff.

Plan-file: _tasks/pipeline/bugs/2-implementing/14330-fix-api-synthesized-keepalive-and-failure-frames-on-v1-respons.plan.md

@diegosouzapw
diegosouzapw merged commit db97f0c into release/v3.8.51 Sep 24, 2026
14 of 21 checks passed
diegosouzapw added a commit that referenced this pull request Sep 24, 2026
All three fail on the release tip (fast-path shards 3/4 and 4/4 of #14718, a
PR that touches none of their subjects) and no open base-red PR covers them.
Each follows an intentional product change that merged without its guard:

- responses-handler: #14572 (#14330) gave the synthesized
  response.in_progress keepalive a sequence_number and response object; the
  assertion now pins the full compliant frame instead of the bare one.
- check-vitest-exclusions: #14493 repaired every quarantined suite and
  emptied the inventory; the live-config test still fails on any exclusion the
  inventory does not list, so the non-empty check was the only stale part.
- perf-waterfall-elimination: #14421 routes the Home settings read through
  loadHomeSettings() (defaults to getSettings); the test still requires it in
  the same Promise.all batch, before getMachineId, with no serial await.

28/28 on the idle .113.

Refs #14547
diegosouzapw added a commit that referenced this pull request Sep 24, 2026
…s loader; rebaseline two test files

- responses-handler: the synthesized in_progress frame now carries
  sequence_number 1 and a response object (#14572/#14330) — pin that prefix.
- perf-waterfall A1: Home batches loadHomeSettings() (#14421/#14060) with
  getMachineId(); assert the loader still reads getSettings() by default.
- file-size: documented rebaseline for chatcore-translation-paths (+12) and
  account-fallback-service (+14, 11 of them Prettier reflow of tip lines).

Refs #14496
diegosouzapw added a commit that referenced this pull request Sep 24, 2026
All three fail on the release tip (fast-path shards 3/4 and 4/4 of #14718, a
PR that touches none of their subjects) and no open base-red PR covers them.
Each follows an intentional product change that merged without its guard:

- responses-handler: #14572 (#14330) gave the synthesized
  response.in_progress keepalive a sequence_number and response object; the
  assertion now pins the full compliant frame instead of the bare one.
- check-vitest-exclusions: #14493 repaired every quarantined suite and
  emptied the inventory; the live-config test still fails on any exclusion the
  inventory does not list, so the non-empty check was the only stale part.
- perf-waterfall-elimination: #14421 routes the Home settings read through
  loadHomeSettings() (defaults to getSettings); the test still requires it in
  the same Promise.all batch, before getMachineId, with no serial await.

28/28 on the idle .113.

Refs #14547
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(api): synthesized keepalive and failure frames on /v1/responses are not valid Responses events

1 participant