From 3ab53d188c31efbc2954c90709770a29df7bcd11 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Tue, 8 Sep 2026 09:10:03 -0300 Subject: [PATCH] fix(sse): restore abort mapping, cache telemetry and fence safety on the non-streaming leg (#12990) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Consertadas 5 das 7 regressões que o #12867 introduziu em `tests/unit/chatcore-translation-paths.test.ts` — arquivo que ele não toca, e por isso fora da minha validação focada quando o mergeei. Medido: **74/74** em `ce49d96` (antes), **67/74** em `d6f3150` (depois), **72/74** agora. **Abort de cliente perdeu o mapeamento (3 testes).** O leg classificava por `error.name === "AbortError"`, mas `abort(reason)` pode rejeitar com string crua sem `name` — essa forma caía em 502 em vez de 499, o que o #7907 fixou. E a mensagem passava por `formatProviderError`, entregando `[499]: request aborted by client` ao cliente. O `chatCore` sempre usou `isLocalStreamLifecycleError` e o literal `"Request aborted"`; espelhado. **`clientResponse` sintético em abort (1 teste).** O caminho antigo omitia o campo porque o cliente já tinha desconectado — esse corpo é o que teríamos enviado, e o dashboard lê o campo como "o que o cliente recebeu". O caminho novo gravava sempre. **Telemetria de prompt cache sumiu do call log (1 teste).** `claudePromptCacheLogMeta` só era construído dentro do `executeProviderRequest`; o leg virou dono do primeiro send e a variável ficou `null`, então `_omniroute.claudePromptCache` desapareceu **em silêncio** de todo call log desse caminho. Não é teste chato: é observabilidade perdida em produção. **Corpo não canonicalizável derrubava a request (1 teste).** `derivePostInjectionRequestIdentity` era chamado antes de qualquer checagem de flag; ele canonicaliza o corpo e o `canonicalStringify` rejeita `Date`, `Map` e instâncias de classe por desenho. Um corpo com essas formas lançava `TypeError` em **toda** request não-streaming, inclusive com `SERVER_OWNED_TOOL_LOOP_ENABLED` desligada, que é o default. Agora deriva só quando o loop pode rodar e falha fechada. Evidência: 74 testes do arquivo 72/74; 138 nas 5 suítes vizinhas com 136 passando; `typecheck:core` limpo; `check-api-typecheck` OK 289; ESLint 0. **As 2 restantes ficam abertas de propósito** — `refreshes GitHub credentials after 401` e `locks per-model quota failures`. Mesma causa: o leg encerra num não-2xx sem passar pela classificação de falha do `chatCore`. `nonStreamingProviderLeg.ts` não tem uma ocorrência de `lockModel`, `refreshCredentials` ou `markAccountUnavailable`; o `chatCore` tem ~170 linhas disso mais o bloco de refresh 401. Em produção: token Copilot não renova no 401, e 402/429 por quota não trava o modelo naquela conexão. Não consertei porque devolver a `Response` ao `chatCore` é impossível (já consumida por `.text()`) e reimplementar a classificação no leg é decisão de desenho do refactor — @HouMinXi tem o contexto. --- .../chatcore-nonstreaming-regressions.md | 1 + open-sse/handlers/chatCore.ts | 54 ++++++++++++++++--- .../chatCore/nonStreamingProviderLeg.ts | 26 +++++---- 3 files changed, 64 insertions(+), 17 deletions(-) create mode 100644 changelog.d/fixes/chatcore-nonstreaming-regressions.md diff --git a/changelog.d/fixes/chatcore-nonstreaming-regressions.md b/changelog.d/fixes/chatcore-nonstreaming-regressions.md new file mode 100644 index 0000000000..b11fdc89e8 --- /dev/null +++ b/changelog.d/fixes/chatcore-nonstreaming-regressions.md @@ -0,0 +1 @@ +- Restore four non-streaming behaviours the server-owned tool loop refactor dropped: client aborts map to 499 with the fixed `Request aborted` message, an aborted request no longer logs a synthetic `clientResponse`, Claude prompt-cache telemetry is recorded again, and a body that cannot be canonicalized no longer throws when the tool loop is off. diff --git a/open-sse/handlers/chatCore.ts b/open-sse/handlers/chatCore.ts index 96ff6a260d..446cb84a9f 100644 --- a/open-sse/handlers/chatCore.ts +++ b/open-sse/handlers/chatCore.ts @@ -4927,7 +4927,14 @@ export async function handleChatCore({ error: err.error || "Provider request failed", providerRequest: finalBody || translatedBody, providerResponse: isNetworkThrow ? undefined : err.response, - clientResponse: buildErrorBody(err.status, err.error || "Provider request failed"), + // On a client abort the client already disconnected before we got here, so this + // body is what we WOULD have sent, not what was delivered. The dashboard reads + // `clientResponse` as "what the client received", so logging it misleads — + // `error` above already records the reason. The pre-#12867 path omitted it here; + // the leg-based path must keep doing so. + clientResponse: isLocalStreamLifecycleError(err.originalError) + ? undefined + : buildErrorBody(err.status, err.error || "Provider request failed"), cacheSource: "upstream", }); persistFailureUsage(err.status, err.errorCode || `upstream_${err.status}`); @@ -4939,8 +4946,34 @@ export async function handleChatCore({ const expectedConn = managedLease ? String(getCurrentConnectionId() || connectionId || "") || undefined : undefined; + // The identity is the tool loop's execution fence key, and deriveToolRequestIdentity + // canonicalizes the body — which by design rejects Dates, Maps and class instances. + // It was computed eagerly, so a body carrying any of those threw on EVERY + // non-streaming request even with SERVER_OWNED_TOOL_LOOP_ENABLED off (the default). + // Derive it only when the loop can run, and fail closed rather than crash: no + // identity means no fence, and without a fence the loop must not run. + let toolLoopEnabled = isServerOwnedToolLoopEnabled(); + let postInjectionRequestIdentity = ""; + if (toolLoopEnabled) { + try { + postInjectionRequestIdentity = derivePostInjectionRequestIdentity({ + apiKeyId: memoryOwnerId || "local", + headers: clientRawRequest?.headers ?? null, + skillRequestId, + postInjectionBody: (body || {}) as Record, + }); + } catch (identityError) { + log?.warn?.( + "SERVER_OWNED_TOOL_LOOP", + `request body is not canonicalizable, skipping the loop: ${ + identityError instanceof Error ? identityError.message : "unknown" + }` + ); + toolLoopEnabled = false; + } + } const loopApply = await applyServerOwnedToolLoopIfNeeded({ - enabled: isServerOwnedToolLoopEnabled(), + enabled: toolLoopEnabled, stream, isResponsesEndpoint, sourceFormat, @@ -4951,12 +4984,7 @@ export async function handleChatCore({ apiKeyId: memoryOwnerId || "local", sessionId: pipelineSessionId, requestId: skillRequestId, - requestIdentity: derivePostInjectionRequestIdentity({ - apiKeyId: memoryOwnerId || "local", - headers: clientRawRequest?.headers ?? null, - skillRequestId, - postInjectionBody: (body || {}) as Record, - }), + requestIdentity: postInjectionRequestIdentity, builtinToolNames: injectionResult.builtinToolNames, injectedCustomSkillNames: injectionResult.injectedCustomSkillNames, customSkillExecutionEnabled: @@ -5066,6 +5094,16 @@ export async function handleChatCore({ providerHeaders = normalizeHeaders(okLeg.headers); } finalBody = providerRequestCapture.body(okLeg.providerRequest || translatedBody); + // Built inside executeProviderRequest on the pre-#12867 path. The leg now owns the + // first non-streaming send, so that assignment never runs here and the meta stayed + // null — `_omniroute.claudePromptCache` silently vanished from every call log on + // this path. Same inputs, same helper, at the point where they are available. + claudePromptCacheLogMeta = buildClaudePromptCacheLogMeta( + targetFormat, + finalBody, + providerHeaders, + clientRawRequest?.headers + ); const capturedOk = providerRequestCapture.latest?.(); reqLogger.logTargetRequest( okLeg.requestUrl || capturedOk?.url || "", diff --git a/open-sse/handlers/chatCore/nonStreamingProviderLeg.ts b/open-sse/handlers/chatCore/nonStreamingProviderLeg.ts index 0e8f2e5969..7656939533 100644 --- a/open-sse/handlers/chatCore/nonStreamingProviderLeg.ts +++ b/open-sse/handlers/chatCore/nonStreamingProviderLeg.ts @@ -23,6 +23,7 @@ import { restoreNonStreamingToolNames } from "./passthroughToolNames.ts"; import { extractUsageFromResponse } from "../usageExtractor.ts"; import { sanitizeUsagePayloadForRequest } from "../../utils/usageTracking.ts"; import { createErrorResult, formatProviderError } from "../../utils/error.ts"; +import { isLocalStreamLifecycleError } from "@/shared/utils/circuitBreaker"; import { unwrapClinepassEnvelope } from "../../utils/clinepassEnvelope.ts"; import { unwrapClineNonStreamingEnvelope } from "./clineResponseEnvelope.ts"; import { @@ -232,7 +233,6 @@ function parseRetryAfterMs(response: Response): number | null { return null; } - function finishOk( input: ProviderLegInput, params: { @@ -462,14 +462,22 @@ export async function runNonStreamingProviderLeg( ) { throw error; } - const failureStatus = - error instanceof Error && error.name === "AbortError" - ? 499 - : error instanceof Error && error.name === "TimeoutError" - ? 504 - : 502; - const failureMessage = - error instanceof Error + // `abort(reason)` can reject with a raw string that has no `name`/`status`, so + // `error.name === "AbortError"` is too narrow — that shape fell through to the 502 + // provider-failure default (#7907). chatCore classified this through + // isLocalStreamLifecycleError before this leg took over the first send; mirror it. + const isRequestAborted = isLocalStreamLifecycleError(error); + const failureStatus = isRequestAborted + ? 499 + : error instanceof Error && error.name === "TimeoutError" + ? 504 + : 502; + // A client abort is not a provider failure: formatProviderError would stamp the raw + // upstream text as `[499]: `, leaking it to the client. chatCore has always + // normalized this to the fixed "Request aborted". + const failureMessage = isRequestAborted + ? "Request aborted" + : error instanceof Error ? formatProviderError(error, provider, currentModel, failureStatus) : "Provider request failed"; const receipt = buildReceipt(input, {