fix: make CooldownFallbackLLM's event contract legible to a tracing host - #22
Merged
Merged
Conversation
`CooldownFallbackLLM`'s event payloads omitted `leg` on `retry`,
`abandon_stream_retry` and `degrade`, and emitted no `request` event at
all for the fallback leg. A tracing adapter therefore had to infer the
leg, and the only available default ("fallback") mislabelled primary
retries as fallback traffic; calls actually served by the fallback left
no request record.
Every event now carries `leg` — `retry` and `abandon_stream_retry` are
primary-leg events, `degrade` is a fallback-leg event — and both the
cooldown shortcut and the post-exhaustion path announce the fallback
request they are about to make (`mode="cooldown"` / `"degraded"`).
Stream-path degrade events gained the `degrade_from`/`degrade_to` pair
and the `streaming` marker the chat path already had. The contract is
now written down on the class.
No behavioral change to the retry/cooldown state machine itself.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Four follow-ups found while two host products wrote tracing adapters over this class (drafted in ApodexHarness temp/2026-09-03_agentcore-cooldown-fallback-followups.md). - Degrades were invisible in logs. The class logged nothing about its own behaviour, so traffic moving to a weaker model for the whole cooldown window left no operational trace unless a host had wired a telemetry sink and re-implemented the log lines off `event_hook` — which both hosts had started to do, guaranteeing divergent lines for identical behaviour. Every event now also logs: retry / abandoned stream retry / degrade at warning, a failing fallback leg at error, a primary-leg error at debug (the line after it carries the signal). `event_hook` goes back to being purely structured telemetry. - `_model_id` was annotated `-> str` but returned whatever `model_name` / `model` held on a duck-typed client. It coerces now. - `model_name` is a property while `model` is a construction-time snapshot, so a swapped primary reports a stale `model`. That split is forced, not an oversight, and now says so: `LLMClient` declares `model` as a settable attribute and the concrete clients subclass the Protocol and assign it in `__init__`, so the slot is a real descriptor — making it a property here stops the class satisfying `LLMClient` under pyright, and making it a read-only member on the Protocol breaks every concrete client at runtime (both measured, hence the comment rather than a change). `model_name` carries no such constraint and is the label to read when the primary may be swapped. - `components.middleware.llm.base` kept a second copy of the retryable keyword table, with a comment pointing at a file in another repo that no longer has the thing it named. It re-binds to the one table now, which is public as `LEGACY_RETRYABLE_KEYWORDS`. `abandon_stream_retry` also drops the `degrade_from`/`degrade_to` pair added in c6c106e: it is a retry-shaped event, and the `degrade` that follows it is what describes the move — carrying the pair on both invited hosts to write two degrade records for one degradation. The exhausted `degrade` gained `error`, naming what exhausted the primary. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review of the two preceding commits found the new event contract leaking in four places, all on the cooldown path — which carries the bulk of fallback traffic, since one primary failure routes every request for the next `cooldown_seconds` through it. - The cooldown shortcuts called the fallback unguarded, so a fallback that also fails during the window raised out of `CooldownFallbackLLM` with no `error` event, no error log, and a `request` record that never terminated. Both paths now wrap the call the way the exhausted paths do. A "fallback leg also failed" metric built on this contract was blind to precisely the case operators care about. - `degrade` fires once per call, not once per transition, so logging it at warning meant ~1200 WARNING lines per 60s window at 20 req/s, burying the one line marking the actual move. Only the two `exhausted` reasons — the transitions — log at warning now; the cooldown reasons log at debug. - The cooldown degrades carried no `cooldown_seconds`, so the log line fell back to the configured constant and claimed "cooldown 60s" 59 seconds in. They now carry both the configured window and `cooldown_remaining_s`, and the line reports what is left. - `error` was added to the chat exhausted degrade only. A host reading it off a degrade record would `KeyError` on any streaming or cooldown degrade. All four sites carry it now (`""` on the cooldown path), and the contract section documents the full payload. Also rename `abandon_stream_retry`'s `reason` to `abandon_reason`. The contract makes `reason` the discriminator of `degrade`, so a host dispatching on it wrote two degrade records for one stream degradation. Finally, move the retryable-keyword table into the leaf `agent_core.retry_policy`. De-duplicating it added a `components -> providers -> runtime` import edge for a frozenset, which any future `runtime -> components.middleware` import would have turned into a partially-initialised-module ImportError. Both sides import the leaf; both public names stay re-exported where they were. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
There are a few correctness/contract issues to address (notably a logging inconsistency around cooldown window size, plus a docstring mismatch and a small avoidable performance regression in legacy_retryable).
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR tightens the observability contract of CooldownFallbackLLM by making event payloads unambiguous for tracing adapters (explicit leg labeling) and by ensuring the cooldown fallback path emits consistent events/logs, while also breaking an import-cycle risk by relocating the legacy retryable-keyword policy into a leaf module.
Changes:
- Standardizes event payload shape across chat/stream and cooldown/degraded paths (notably
leg, consistent degrade fields, andabandon_reason). - Adds always-on operational logging for key state-machine events and closes cooldown-path failure visibility gaps (fallback failures during cooldown now emit
errorand log). - Moves the legacy retryable policy table into
agent_core.retry_policyand reuses it from bothprovidersandmiddleware.
File summaries
| File | Description |
|---|---|
| tests/test_cooldown_fallback.py | Adds regression coverage for event payload shapes, log levels, and cooldown-path failure visibility. |
| agent_core/retry_policy.py | Introduces a leaf module for shared legacy retryable policy (keywords + predicate). |
| agent_core/providers/fallback.py | Updates CooldownFallbackLLM event contract, adds logging, and guards cooldown fallback calls for traceability. |
| agent_core/components/middleware/llm/base.py | Switches retryable keyword/predicate definitions to import from the new leaf policy module. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 3
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+334
to
+343
| remaining = payload.get("cooldown_remaining_s") | ||
| log( | ||
| "CooldownFallbackLLM: degrading %s -> %s (%s), cooldown %ss", | ||
| payload.get("degrade_from"), | ||
| payload.get("degrade_to"), | ||
| reason, | ||
| payload.get("cooldown_seconds", self.cooldown_seconds) | ||
| if remaining is None | ||
| else f"{remaining:.0f}s remaining of {self.cooldown_seconds}", | ||
| ) |
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
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.
CooldownFallbackLLMemitted events that a tracing adapter could not map without guessing, and the cooldown path — which carries the bulk of fallback traffic, since one primary failure routes every request for the nextcooldown_secondsthrough it — was the least observable part of the state machine.What changed
Every event names the leg it is about. Each payload carries
leg—"primary"or"fallback"— so an adapter never infers it from the event name.retryandabandon_stream_retryare primary-leg events: they describe what happened to the primary, not the leg that serves next.The state machine logs its own transitions. Logging is the always-wired channel; a telemetry sink may not be registered at all, and an operator still has to see that traffic moved to another model. Retries, abandoned stream retries and the two exhausted degrades log at
warning; a failing fallback leg aterror; a primary-leg error atdebug, since theretryordegradeline after it carries the signal.Four cooldown-path gaps closed (found by review of the first two commits):
CooldownFallbackLLMwith noerrorevent, no error log, and arequestrecord that never terminated — so a "fallback leg also failed" metric was blind to precisely the case operators care about. Both paths now wrap the call the way the exhausted paths do.degradefires once per call, not once per transition, so logging it atwarningmeant ~1200 WARNING lines per 60 s window at 20 req/s, burying the one line marking the actual move. Only the twoexhaustedreasons log at warning now.cooldown_seconds, so the log line fell back to the configured constant and claimed "cooldown 60s" 59 seconds in. They now carry the configured window andcooldown_remaining_s, and the line reports what is left.errorwas added to the chat exhausted degrade only, so a host reading it off a degrade record wouldKeyErroron any streaming or cooldown degrade. All four sites carry it (""on the cooldown path), and the docstring documents the full payload.abandon_stream_retry'sreasonis nowabandon_reason. The contract makesreasonthe discriminator ofdegrade; a host dispatching on it wrote two degrade records for one stream degradation.The retryable-keyword table moved to the leaf
agent_core.retry_policy. De-duplicating it had added acomponents -> providers -> runtimeimport edge for a frozenset, which any futureruntime -> components.middlewareimport would have turned into a partially-initialised-moduleImportError. Both sides import the leaf; both public names stay re-exported where they were.Testing
1119 tests pass; ruff and pyright are clean. New regression tests cover the payload shape of all four degrade sites, the cooldown degrade's log level and remaining-window figure, and a failing fallback leg during cooldown on both the
chatandstreampaths. Verified separately that importingcomponents.middleware.llm.basealone no longer pulls in anyagent_core.providersmodule (noranthropic).