Skip to content

gateway: a whole reply behind a stream's keepalive says the vendor di… - #1077

Open
TryWorld2026 wants to merge 1 commit into
yetone:mainfrom
TryWorld2026:fix/keepalive-then-whole-json-reply
Open

TryWorld2026 wants to merge 1 commit into
yetone:mainfrom
TryWorld2026:fix/keepalive-then-whole-json-reply

Conversation

@TryWorld2026

@TryWorld2026 TryWorld2026 commented Oct 7, 2026 •

Copy link
Copy Markdown
Contributor

gateway: a whole reply behind a stream's keepalive says the vendor did not stream

What is wrong

An agent that asked for a stream and heard nothing from its vendor for longer
than keepHeldAfter (15s) is sent the stream's 200 and SSE comments while the
vendor has yet to answer (#947). Those comments promise the agent a stream. When
the vendor ignores stream and answers with a whole application/json 200
instead, that reply is held for a stream that never was, and release told it as
the stream's error with http.StatusText(200) as the fallback:

data: {"error":{"message":"OK: {\"id\":\"chatcmpl-1\", …the whole answer…}","type":"api_error"}}

So the agent is handed a 200 in an error whose message opens with OK: and
then carries its own answer. It is not an error, it says it is one, and it says
it succeeded.

What changed

release's own comment calls that path "an error status", but its guard is
h.alive.sent && !h.stream — about what went out, not about the status — so a
2xx reached it too. It now says what the vendor did instead:

  • a 2xx becomes "did not stream", the words a request nothing went to ahead of it
    is answered 502 for, with provider.APIError putting the vendor's own reason in
    front of it and the body behind, as it already does everywhere else;
  • an error status keeps its own text, so a 429 or a 503 held this way is told
    exactly as before.

keepQuiet is not touched. An earlier draft of this change narrowed it to
h.stream so that nothing was said ahead of a reply that might be whole; that
fixed the wording and broke TestKeepAliveBeforeTheVendorsHeaders (3/3, every
run burning its 5s deadline), because it took the #947 keepalive away from a slow
vendor's agent and left it with nothing at all for as long as the vendor was
quiet. An agent that asked for no stream gets no keepalive anyway — watch runs
only for one that did — so its whole reply never had this problem.

Semantic change (Gateway routing and fallback)

  • Before: a whole application/json 200 arriving behind the stream's keepalive
    was told as that stream's error with OK: in front of it.
  • After: it is told as the vendor not streaming one, the same reason the same
    vendor answering a request nothing went to ahead of it is given, with its own
    words in front.
  • Unchanged: the [Bug] 上游 SSE 响应被缓冲且静默保活不独立运行,Cloudflare 后的 Remote Magpie 可在 125 秒超时 #947 keepalive and its 5-minute keepaliveLongest stop; how a
    non-streaming request is answered; everything about an error status.
  • Still carried, not fixed here: the vendor's body is inside the message, and the
    usage ledger books the 200 as it did. Giving the agent the answer as a
    completion would mean encoding a whole reply as one event of the announced
    stream, which is a design decision — see the note at the end.
  • Reference: docs/subsystems/gateway-routing.md
    (updated in this PR — one sentence at the end of the keepalive bullet);
    implementation release in internal/gateway/fallback.go.

Verification

All on the Windows box.

  • Without the change, TestQuietThenWholeJSONSaysTheVendorDidNotStream fails
    with the 200 OK: {…} event above; with it, it passes. The test guards both
    halves: it requires did not stream in the answer and refuses OK: in it.
  • TestQuietThenWholeJSONNoStream and TestQuietThenWholeJSONFromResponsesOnly
    are the other half of the same vendor: an agent that asked for no stream, and a
    provider with no Chat endpoint at all. Both pass before and after — they pin
    the paths around the changed one, and only the first test is the reproducer.
  • TestKeepAliveBeforeTheVendorsHeaders — upstream's [Bug] 上游 SSE 响应被缓冲且静默保活不独立运行,Cloudflare 后的 Remote Magpie 可在 125 秒超时 #947 check, untouched by
    this PR — passes before and after. It is what the earlier draft of the fix
    broke.
  • The rest of the quiet keepalive family passes: TestQuietKeptAliveOnlySoLong,
    TestSilentHeldStreamKeptAlive, TestQuietStreamMidReplyKeptAlive,
    TestSilentHeldStreamStillFailsOver, TestQuietStreamsKeptAliveEveryProtocol,
    TestTranslatedStreamKeepsClientAlive.
  • go vet -tags nogui ./internal/gateway and the windows, darwin and linux
    builds pass; gofmt is clean on LF-normalized copies, the worktree being CRLF
    under core.autocrlf.

One failure in the full internal/gateway run is this box's own and reproduces
identically on the base commit: TestSweepBridgeProjectsTakesOnlyTheBridgesFolders
needs a symlink privilege Windows does not grant here. It was the only --- FAIL
in the run. TestPluginStreamStalledClientKeepsOtherRequestsMoving is a separate
timing flake on this box — it failed once in an earlier run of the same package
and passed this time, including under -count=3, and it is unrelated to this
change.

The bigger question, if you want it

The keepalive and a whole reply cannot both be delivered as they stand: the
comments have already committed the response to text/event-stream, so a whole
body that arrives afterwards can only be an event. This PR makes that event
honest. Making it carry the answer — start it, one content event, stop it —
would give the agent the reply instead of a reason, cost a new encoder path per
protocol, and is a behaviour the maintainer should pick rather than one this PR
should assume.

…d not stream

An agent that asked for a stream and heard nothing from its vendor for
longer than keepHeldAfter is sent the stream's 200 and SSE comments while
the vendor has yet to answer (yetone#947), which also promises it a stream. A
vendor that ignores stream and answers with a whole application/json 200
had that reply held for a stream that never was, and release told it as
the stream's error with http.StatusText(200) as the fallback: the agent
read "200 OK: {…its answer…}" as an error, and the ledger booked a
success, so the conversation stuck to an account that never answered.

release's own comment calls this path "an error status" — it is guarded
by h.alive.sent && !h.stream, which is about what went out, not about the
status, so a 2xx reached it too. Say what the vendor did instead: not
stream one, the words a request nothing went to ahead of it is answered
502 for, and which provider.APIError already puts the vendor's own reason
in front of. An error status keeps its own text, so a 429 or a 503 held
this way is told exactly as before.

keepQuiet is left as it is: the yetone#947 keepalive is what tells the agent the
reply is a stream, and taking it away to dodge this would leave a slow
vendor's agent with nothing at all for as long as it is quiet — and an
agent that asked for no stream gets no keepalive anyway (watch only runs
for one that did), so its whole reply always went past as it came.

Verified: TestQuietThenWholeJSONSaysTheVendorDidNotStream fails without
the change with the answer as the message of an error in an announced
200, and passes with it. TestQuietThenWholeJSONNoStream and
TestQuietThenWholeJSONFromResponsesOnly are the other half of the same
vendor (an agent that asked for no stream, and a provider with no Chat
endpoint). TestKeepAliveBeforeTheVendorsHeaders, the yetone#947 check, passes
unchanged both before and after — it is upstream's, and an earlier draft
of this change broke it, which is why the keepalive is left alone.
TestQuietKeptAliveOnlySoLong, TestSilentHeldStreamKeptAlive,
TestQuietStreamMidReplyKeptAlive, TestSilentHeldStreamStillFailsOver,
TestQuietStreamsKeptAliveEveryProtocol and
TestTranslatedStreamKeepsClientAlive all pass. go vet -tags nogui
./internal/gateway and the windows, darwin and linux builds pass; gofmt
is clean on LF-normalized copies, the worktree being CRLF.

This branch has not been deployed

No deployments
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