fix(a2a): stop running an agent nobody waits for, and say why it failed (CRM-236) - #52
Merged
Merged
Conversation
…ed (CRM-236)
Two defects left over from the degraded-provider incident, both on the
processor side.
1. The run outlived the caller. bot-runtime gives up at its own ceiling and
closes the socket, but the processor never noticed:
04:12:47 bot-runtime -> processor
04:13:17 bot-runtime gives up, socket closed
04:18:26 processor: "Agent execution completed successfully"
Five minutes of model calls with nowhere to go — burning the very quota
whose exhaustion caused the timeout. run_unless_client_disconnects() now
races the run against the ASGI disconnect and cancels it. Verified on the
live stack: client aborted at 12:38:38.789, cancelled at 12:38:38.755,
and no "completed successfully" followed.
Detection deliberately uses receive(), NOT is_disconnected(). Polling
is_disconnected() never fires under uvicorn once the body has been read:
pause_reading() takes the socket off the selector, so connection_lost()
is never called. The first version of this fix did exactly that, passed
its unit tests, and still let the agent run 27s past the client abort on
the live stack. Awaiting receive() calls resume_reading() and the
disconnect arrives as an event.
A2A_CANCEL_ON_DISCONNECT=false restores the old behaviour.
2. Every failure was a 500. standard_runner wraps everything in
InternalServerError(str(e)), so a quota refusal and a NameError produced
byte-identical responses and the only way to tell them apart was reading
container logs. classify_provider_error() reads the cause chain and maps
the recognised conditions to 429/503/502/413 with a distinct JSON-RPC
code, leaving genuine bugs on 500.
map_status_to_error_code() had no 429/413/499 entries, so without the
catalog additions the envelope would have kept saying INTERNAL_ERROR.
Recognition is conservative in both directions: our own InternalServerError
name and a bare "timeout" are NOT markers — matching them would classify
every bug we write as a provider outage, inverting the bug being fixed.
Credentials are redacted and the detail is capped, since provider errors
echo request URLs containing the key.
Live verification, both paths, against the running stack:
- invalid provider key -> 502 / EXTERNAL_SERVICE_ERROR / -32004, through
the real double-wrapped runner chain (was 500 / INTERNAL_ERROR / -32603)
- the incident's verbatim quota string -> 429 / RATE_LIMIT_EXCEEDED / -32001
- NameError still classifies as None (stays a 500)
Retry parity: bot-runtime already retries 429/500/502/503/504, so moving off
500 changes nothing there. 499 and 413 are correctly not retryable.
Tests: 15 provider_errors + 12 client_disconnect. Negative proof for both —
with cancellation disabled the disconnect test fails AND the work runs the
full 30s; without the catalog entries map_status_to_error_code(429) returns
INTERNAL_ERROR. Baseline unchanged (80 passed / 3 pre-existing failures /
22 pre-existing collection errors from missing local deps).
Reviewer's GuideAdds client-disconnect-aware execution for A2A agent runs and introduces structured classification of provider errors so quota/availability/auth/context issues are surfaced with appropriate HTTP and JSON-RPC codes instead of generic 500s. Sequence diagram for disconnect-cancelled A2A agent executionsequenceDiagram
participant Client
participant A2A as A2A_Route
participant Runner as run_unless_client_disconnects
participant Agent as run_agent
Client->>A2A: handle_message_send()
A2A->>Runner: run_unless_client_disconnects(request, run_agent(...))
Runner->>Agent: Start agent execution
par Agent work
Agent-->>Runner: Agent result
and ASGI disconnect detection
Client--xRunner: http.disconnect via receive()
Runner->>Agent: cancel()
Runner-->>A2A: ClientGoneAway
end
A2A-->>Client: 499 CLIENT_CLOSED_REQUEST
Flow diagram for provider error classificationflowchart TD
E[Agent exception] --> C[classify_provider_error]
C --> S{Provider condition recognized?}
S -->|No| I[Generic 500 INTERNAL_ERROR]
S -->|Yes| M[ProviderFailure with redacted detail]
M --> R[map_status_to_error_code]
R --> O[Structured HTTP and JSON-RPC error response]
M --> L[log_provider_failure]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 3 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/utils/provider_errors.py" line_range="182-207" />
<code_context>
+ haystack = f"{type(link).__name__} {link}".lower()
+ status = _status_code_of(link)
+
+ if status == 429 or _matches(haystack, _RATE_LIMIT_MARKERS):
+ return _build(
+ "rate_limit",
+ 429,
+ -32001,
+ "The model provider refused the request: rate limit or quota exhausted.",
+ link,
+ )
+
+ if status in (502, 503, 504) or _matches(haystack, _UNAVAILABLE_MARKERS):
</code_context>
<issue_to_address>
**issue (bug_risk):** A numeric status does not actually take precedence over text markers. An exception with `status_code = 401` and text containing `quota exceeded` is classified as a 429 rate-limit failure because the rate-limit branch checks text before the authentication branch, returning the wrong status, message, and JSON-RPC code.
**Triggers:** When a provider uses an HTTP 401/403 response whose message also mentions quota or rate limiting.
**Suggested fix:** When a recognized numeric status is present, classify from that status before applying text markers, or prevent text markers from overriding an incompatible status.
</issue_to_address>
### Comment 2
<location path="src/utils/provider_errors.py" line_range="209" />
<code_context>
+ link,
+ )
+
+ if _matches(haystack, _CONTEXT_MARKERS):
+ return _build(
+ "context_length",
+ 413,
+ -32005,
+ "The request exceeded the model's context window.",
+ link,
+ )
+
+ return None
</code_context>
<issue_to_address>
**issue (bug_risk):** A provider exception carrying `status_code = 413` is never classified: 413 is not handled by any status branch, and the context-length branch requires one of its text markers. Such failures fall through to `None` and are returned as the generic 500 instead of the intended 413 / payload-too-large response.
**Triggers:** When the provider SDK reports an oversized request using a numeric 413 status without one of the exact context-length phrases.
**Suggested fix:** Treat status 413 as `context_length` before text matching.
```suggestion
if status == 413 or _matches(haystack, _CONTEXT_MARKERS):
```
</issue_to_address>
### Comment 3
<location path="src/utils/provider_errors.py" line_range="226-227" />
<code_context>
+) -> ProviderFailure:
+ detail = redact_secrets(f"{type(link).__name__}: {link}")
+ # Provider errors are verbose (full request echoes); keep the response bounded.
+ if len(detail) > 600:
+ detail = detail[:600] + "…"
+ return ProviderFailure(
+ kind=kind,
</code_context>
<issue_to_address>
**nitpick (bug_risk):** The detail bound is 601 characters, not 600: the code keeps the first 600 characters and then appends an ellipsis. Responses and logs therefore exceed the documented 600-character limit.
**Triggers:** When a redacted provider error exceeds 600 characters.
**Suggested fix:** Reserve one character for the ellipsis, or omit the ellipsis when enforcing a strict 600-character maximum.
```suggestion
if len(detail) > 600:
detail = detail[:599] + "…"
```
</issue_to_address>Sourcery assessment
Needs a human reviewer. 2 findings to address first, and if disconnect detection is wrong, an in-progress agent can be cancelled after it has already performed partial tool or database side effects, leaving work that a revert cannot undo. The provider-error path also changes externally returned failure details, though reverting restores the old behavior for future requests.
Blocking findings: src/utils/provider_errors.py:207, src/utils/provider_errors.py:209
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+182
to
+207
| if status == 429 or _matches(haystack, _RATE_LIMIT_MARKERS): | ||
| return _build( | ||
| "rate_limit", | ||
| 429, | ||
| -32001, | ||
| "The model provider refused the request: rate limit or quota exhausted.", | ||
| link, | ||
| ) | ||
|
|
||
| if status in (502, 503, 504) or _matches(haystack, _UNAVAILABLE_MARKERS): | ||
| return _build( | ||
| "unavailable", | ||
| 503, | ||
| -32002, | ||
| "The model provider is unavailable or overloaded.", | ||
| link, | ||
| ) | ||
|
|
||
| if status in (401, 403) or _matches(haystack, _AUTH_MARKERS): | ||
| return _build( | ||
| "auth", | ||
| 502, | ||
| -32004, | ||
| "The model provider rejected our credentials.", | ||
| link, | ||
| ) |
There was a problem hiding this comment.
issue (bug_risk): A numeric status does not actually take precedence over text markers. An exception with status_code = 401 and text containing quota exceeded is classified as a 429 rate-limit failure because the rate-limit branch checks text before the authentication branch, returning the wrong status, message, and JSON-RPC code.
Triggers: When a provider uses an HTTP 401/403 response whose message also mentions quota or rate limiting.
Suggested fix: When a recognized numeric status is present, classify from that status before applying text markers, or prevent text markers from overriding an incompatible status.
| link, | ||
| ) | ||
|
|
||
| if _matches(haystack, _CONTEXT_MARKERS): |
There was a problem hiding this comment.
issue (bug_risk): A provider exception carrying status_code = 413 is never classified: 413 is not handled by any status branch, and the context-length branch requires one of its text markers. Such failures fall through to None and are returned as the generic 500 instead of the intended 413 / payload-too-large response.
Triggers: When the provider SDK reports an oversized request using a numeric 413 status without one of the exact context-length phrases.
Suggested fix: Treat status 413 as context_length before text matching.
Suggested change
| if _matches(haystack, _CONTEXT_MARKERS): | |
| if status == 413 or _matches(haystack, _CONTEXT_MARKERS): |
Comment on lines
+226
to
+227
| if len(detail) > 600: | ||
| detail = detail[:600] + "…" |
There was a problem hiding this comment.
nitpick (bug_risk): The detail bound is 601 characters, not 600: the code keeps the first 600 characters and then appends an ellipsis. Responses and logs therefore exceed the documented 600-character limit.
Triggers: When a redacted provider error exceeds 600 characters.
Suggested fix: Reserve one character for the ellipsis, or omit the ellipsis when enforcing a strict 600-character maximum.
Suggested change
| if len(detail) > 600: | |
| detail = detail[:600] + "…" | |
| if len(detail) > 600: | |
| detail = detail[:599] + "…" |
…th A2A codes (CRM-236 review)
Addresses the two CRITICAL findings on the processor side.
1. Our own infrastructure was reported as an LLM provider outage.
_status_code_of walked the whole cause chain and matched a status with no
provider anchor, and status took precedence over text. The runner calls
raise_for_status() against internal services (standard_runner.py:222 memory,
:311 evo-kb-service), so their httpx errors entered the chain:
evo-kb-service down (503) -> "The model provider is unavailable"
wrong internal token (401) -> "The model provider rejected our credentials"
That is the exact inversion of the bug this module exists to fix. The care
had gone into the text markers and none into the status match.
Classification now requires positive evidence that the exception came from a
provider (module, class name, or text fingerprint). Chose an anchor over a
blacklist of internal hosts: a blacklist rots — every new internal service
has to be remembered, and forgetting one silently reintroduces the inversion.
Refined once while testing: wording that ONLY a provider produces
("resource_exhausted", "maximum context length", "api key not valid",
"model is overloaded") anchors on its own, because requiring an SDK
fingerprint on top of it lost legitimate cases. Ambiguous markers ("rate
limit", "too many requests", a bare 429/503) still need separate evidence —
those are exactly what an internal service produces too.
2. The new JSON-RPC codes collided with this repo's own A2A catalogue.
src/schemas/a2a_types.py already owns -32001 TaskNotFound, -32002
TaskNotCancelable, -32003 PushNotificationNotSupported, -32004
UnsupportedOperation and -32005 ContentTypeNotSupported, and a2a_routes.py
emits them (:958, :2213, :2307). Reasoning about the reserved RANGE was not
enough: an exhausted quota went on the wire as "Task not found" to any
conforming A2A client — the opposite of this card's requirement.
Moved to -32010..-32013, leaving -32006..-32009 free for that catalogue to
grow. A test now derives the taken codes from a2a_types and asserts no
intersection, so the next addition cannot collide silently.
Also (MEDIUM 5): the 500 fallback is the COMMON path, because classification is
conservative on purpose — so it is the path most likely to carry a credential.
It emitted str(e) raw into the log and into data.error, leaking `?key=AIza…`.
redact_secrets already existed in this PR; it is now applied there too.
Also (MEDIUM 8): A2A_CANCEL_ON_DISCONNECT and A2A_DISCONNECT_POLL_SECONDS are
documented in .env.example — the operator's escape hatches were undiscoverable.
Tests: 39 (was 27). New coverage for internal-service failures at 503/502/401/
403/429, an internal auth failure, provider SDK status without text markers, and
the code-collision guard.
One existing test was REWRITTEN rather than kept: it asserted that any exception
carrying status_code = 429 classified as a rate limit, which is precisely the
defect. It now pins that the status wins over text WITHIN a provider exception,
with a counterpart asserting that a bare status on an unknown exception
classifies nothing.
…iew 7) message/stream reported every failure as a generic -32603 and echoed str(e) raw, so a quota exhaustion was as opaque there as it used to be on message/send — and the raw text carries `?key=AIza…`. On the other half of that finding I reached a different conclusion, and it changes what the fix should be. The review says both defects survive on this route. The error one does. The disconnect one does NOT, and adding the guard there would have introduced a bug: sse-starlette runs _listen_for_disconnect inside cancel_on_finish, which cancels the entire task group — including _stream_response, the consumer of this generator — as soon as http.disconnect arrives. Cancellation already works here, natively. Worse, run_unless_client_disconnects awaits receive() itself. Putting it on this route would leave TWO consumers on the same receive channel, and whichever won the race would swallow the disconnect message the other was waiting for. The mechanism that makes the guard correct on message/send is exactly what makes it wrong here. Verified by reading the installed sse-starlette 3.0.2, not from memory. So this commit applies only the error half: provider classification plus redaction on the fallback.
… 14) Incident timelines, before/after tables and the review's own reasoning belong in the PR body, where they already are. Here they were a 20-line module docstring above eight imports, a 19-line block above a tuple of strings and a 13-line comment above one dataclass field. Kept is what the code cannot say: why polling is_disconnected() fails under uvicorn, why the codes sit at -3201x, why an anchor is required instead of a blacklist, and why the streaming route must not get the disconnect guard. 132 added comment lines to 88, no behaviour touched.
gomessguii
approved these changes
Aug 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fecha os dois defeitos do lado do processor que faltavam no CRM-236. O PR do bot-runtime (timeout + aviso ao cliente) e evolution-foundation/evo-bot-runtime#9 — os dois sao do mesmo card e devem subir juntos.
1. A execucao sobrevivia a quem esperava por ela
O bot-runtime desiste no proprio teto e fecha o socket. O processor nunca percebia:
Cinco minutos de chamadas de modelo sem destino. Nao e so computacao desperdicada: o turno continua consumindo a mesma cota cuja exaustao causou o timeout, ou seja, o processor pilha carga justamente quando menos pode.
run_unless_client_disconnects()corre a execucao contra o sinal de desconexao do ASGI e cancela quando o cliente vai embora.O detalhe que quase passou
A deteccao usa
receive(), naois_disconnected(). Polling deis_disconnected()nunca dispara sob uvicorn depois que o corpo foi lido: opause_reading()tira o socket do selector, ninguem observa mais o descritor, o FIN/RST do peer nao e notado econnection_lost()nunca e chamado.A primeira versao deste fix fazia exatamente isso. Passou nos testes unitarios e, na stack rodando, ainda deixou o agente correr 27s alem do abort do cliente (cliente cortou 12:35:53, processor terminou 12:36:20). So o teste ao vivo pegou. Awaiting
receive()chamaresume_reading()e a desconexao chega como evento, sem intervalo de polling.A2A_CANCEL_ON_DISCONNECT=falserestaura o comportamento antigo.2. Todo erro era 500
standard_runnerembrulha tudo emInternalServerError(str(e)), entao uma recusa por cota e umNameErrorproduziam respostas identicas byte a byte — e o unico jeito de distinguir era abrir o log do container.classify_provider_error()le a cadeia de causas e mapeia as condicoes reconhecidas para 429/503/502/413 com codigo JSON-RPC proprio, deixando bugs nossos no 500.map_status_to_error_code()nao tinha entrada para 429/413/499 — sem as adicoes no catalogo o envelope continuaria dizendoINTERNAL_ERRORmesmo com o status certo.Conservador nas duas direcoes
O nome da nossa propria
InternalServerErrore um"timeout"solto nao sao marcadores. Casar com eles classificaria todo bug que escrevermos como queda de provedor — a inversao exata do bug sendo corrigido. Ha teste para os dois casos.Credenciais sao redigidas e o detalhe e limitado a 600 chars: erros de provedor ecoam a URL da requisicao com a chave na query string.
Verificacao ao vivo (stack rodando, nao so unit test)
500 / INTERNAL_ERROR / -32603502 / EXTERNAL_SERVICE_ERROR / -32004+ "The model provider rejected our credentials."500 / INTERNAL_ERROR429 / RATE_LIMIT_EXCEEDED / -32001NameError(bug nosso)classifydevolveNone)O teste da chave invalida passou pela cadeia real de duplo embrulho do runner, nao por excecao sintetica.
Paridade de retry
isRetryableStatusno bot-runtime ja aceita 429/500/502/503/504, entao sair do 500 nao muda o comportamento de retry. 499 e 413 corretamente nao sao retentaveis (nos e que desistimos; e contexto grande demais nao melhora repetindo).Testes
tests/unit/test_provider_errors.py— 15tests/unit/test_client_disconnect.py— 12 (cobrem o caminho ASGIreceivee o fallback de polling)Prova negativa feita nos dois:
map_status_to_error_code(429)devolveINTERNAL_ERRORBaseline:
git stashem develop limpo da os mesmos 22 erros de coleta e as mesmas 3 falhas emtest_vault_header_resolution.py(deps ausentes no venv local, nada a ver com este PR). Nenhuma falha nova.Trade-offs assumidos
receive. Seguro aqui porque o handler ja consumiu o corpo viaawait request.json()antes; nenhuma mensagemhttp.requestpode ser roubada.Fora deste PR, mas necessario para o card funcionar
Os composes fixavam
AI_CALL_TIMEOUT_SECONDS=30, anulando o default de 90 — corrigido em PR separado no repo raiz..env.examplecarrega o mesmo 30 e e arquivo protegido aqui; precisa de um mantenedor para atualizar, senao o fix do teto nao tem efeito em stack nova.Summary by Sourcery
Stop orphaned agent executions on client disconnects and return accurate, sanitized error responses for recognized model-provider failures.
New Features:
Bug Fixes:
Enhancements:
Tests:
Ordem de merge
Este PR tem base em
develope é independente da cadeia #49→#50→#51 — pode ser mergeadoa qualquer momento, antes ou depois dela.
O que não é independente: os outros dois PRs do CRM-236.
evo-bot-runtimeevo-crm-community#9 e #175 devem subir juntos: sem o #175 os composes continuam fixando
AI_CALL_TIMEOUT_SECONDS=30e o novo default de 90 do #9 nunca entra em vigor.Review — críticos 1 e 4, e o item 7 (onde discordo)
🔴 1 — nossa própria infra era reportada como queda do provedor
Reproduzido contra o módulo real, com o duplo embrulho do runner:
O diagnóstico estava certo: o cuidado foi todo nos marcadores de texto, nenhum no casamento por status.
Decisão de design: exigir âncora de provedor em vez de excluir serviços internos. Uma lista de exclusão apodrece — todo serviço interno novo precisa ser lembrado, e esquecer um reintroduz a inversão em silêncio. Exigir evidência positiva falha na direção segura a que o módulo já se comprometeu.
Refinado uma vez durante o teste, e o ajuste importa: exigir impressão digital de SDK em cima de frases inequívocas perdia casos legítimos. Então frases que só um provedor produz (
resource_exhausted,maximum context length,api key not valid,model is overloaded) ancoram sozinhas; marcadores ambíguos (rate limit,too many requests, 429/503 pelado) continuam exigindo evidência separada — são exatamente os que um serviço interno também produz.🔴 4 — colisão com o catálogo A2A do próprio repo
Confirmado:
-32001éTaskNotFounde já é emitido ema2a_routes.py:958, 2213, 2307. Raciocinar sobre a faixa reservada não bastou — eu não olhei o que o repo já usava dela.Movidos para
-32010..-32013, deixando-32006..-32009livres. Agora há teste que deriva os códigos ocupados dea2a_typese exige interseção vazia, para a próxima adição não colidir em silêncio.🟡 5 — redação no fallback
O fallback 500 é o caminho comum (a classificação é conservadora de propósito), então é onde a chave mais vaza.
redact_secretsaplicado no log e nodata.error.🟡 7 — aqui cheguei a outra conclusão, e ela muda o fix
O defeito de erro sobrevive no streaming — corrigido (classificação + redação).
O de desconexão não, e adicionar a guarda lá teria introduzido um bug. Lendo o
sse-starlette 3.0.2instalado:cancel_on_finishcancela o task group inteiro — incluindo_stream_response, que consome o nosso gerador — assim quehttp.disconnectchega. O cancelamento já funciona ali, nativamente.E pior:
run_unless_client_disconnectstambém espera emreceive(). Colocá-la nessa rota deixaria dois consumidores no mesmo canal, e quem vencesse a corrida engoliria a mensagem que o outro aguardava. O mecanismo que torna a guarda correta nomessage/sendé exatamente o que a torna errada no stream.Testes
39 (eram 27). Cobrem falhas de serviço interno em 503/502/401/403/429, falha de auth interna, status de SDK sem marcador de texto, e o guard de colisão de código.
Um teste existente foi reescrito, não mantido: ele fixava que qualquer exceção com
status_code = 429classificava — que é precisamente o defeito do item 1.Em aberto, dito por mim
10 (teste de fiação nas rotas/handlers) não foi feito. É o mais relevante do que sobrou: os módulos têm boa cobertura isolada, mas ninguém exercita
handle_message_sendde ponta a ponta.