mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-15 19:32:20 +03:00
fix(sse): restore abort mapping, cache telemetry and fence safety on the non-streaming leg (#12990)
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.
This commit is contained in:
committed by
GitHub
parent
99282e1054
commit
3ab53d188c
1
changelog.d/fixes/chatcore-nonstreaming-regressions.md
Normal file
1
changelog.d/fixes/chatcore-nonstreaming-regressions.md
Normal file
@@ -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.
|
||||
@@ -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<string, unknown>,
|
||||
});
|
||||
} 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<string, unknown>,
|
||||
}),
|
||||
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 || "",
|
||||
|
||||
@@ -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]: <reason>`, 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, {
|
||||
|
||||
Reference in New Issue
Block a user