fix(dispatcher): circuit breaker + bounded retry against sustained LLM overload (#1058) - #1073
Conversation
…ained LLM overload (#1058) Under sustained LLM slowness, KoogAgentPlanAdapter paid a full 30s withTimeout stall on every tick with no way to back off, and each timeout printed a duplicate full stack trace from Koog's own internal logger. Add LlmCircuitBreaker (sim-time based, no wall clock) that opens after repeated cycle failures and skips the LLM entirely until a cooldown elapses, then probes recovery once; add one bounded retry for a plain (non-timeout) failure. Silence Koog's duplicate internal error log via the Logback API in Main.kt rather than editing core's shared logback.xml. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
bedaHovorka
left a comment
There was a problem hiding this comment.
code cleanup findigs
There was a problem hiding this comment.
🟡 Changes recommended
Retries can duplicate partially emitted actions, half-open probing is not concurrency-safe, and breaker settings are missing from run identity.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds bounded retries and a simulation-time circuit breaker to reduce repeated LLM stalls during sustained overload.
Changes:
- Adds configurable CLOSED/OPEN/HALF_OPEN breaker behavior.
- Retries non-timeout inference failures once and expands tests.
- Suppresses duplicate Koog exception logging.
File summaries
| File | Description |
|---|---|
dispatcher-agent/.../MeasuringPlanAdapterTest.kt |
Updates measurement expectations for retries. |
dispatcher-agent/.../LlmCircuitBreakerTest.kt |
Tests breaker state transitions. |
dispatcher-agent/.../KoogAgentPlanAdapterTest.kt |
Tests retries, timeouts, and breaker behavior. |
dispatcher-agent/.../DispatcherRunConfigTest.kt |
Tests breaker configuration parsing. |
dispatcher-agent/.../DispatcherDefaultsResourceTest.kt |
Verifies shipped breaker defaults. |
dispatcher-agent/.../dispatcher-defaults.properties |
Defines breaker defaults. |
dispatcher-agent/.../LlmCircuitBreaker.kt |
Implements the circuit breaker. |
dispatcher-agent/.../KoogAgentPlanAdapter.kt |
Integrates retries and fallback handling. |
dispatcher-agent/.../DispatcherRunConfig.kt |
Adds breaker configuration fields. |
dispatcher-agent/.../DispatcherDefaultsResource.kt |
Recognizes the new properties. |
desktop-ui/.../Main.kt |
Suppresses duplicate Koog logs. |
desktop-ui/.../ExampleRegistry.kt |
Wires configured breakers into LLM planners. |
Review details
Suppressed comments (1)
dispatcher-agent/src/main/kotlin/cz/vutbr/fit/interlockSim/dispatcher/planner/LlmCircuitBreaker.kt:90
HALF_OPENallows every caller, not exactly one probe: after one concurrent caller transitionsOPENtoHALF_OPEN, all others enteringshouldAttempttake this branch and hit the overloaded LLM too. Track a single in-flight probe and reject later callers until it records success/failure; also release that state if the probe coroutine is cancelled.
fun shouldAttempt(simTime: Double): Boolean =
when (state) {
State.CLOSED -> true
State.HALF_OPEN -> true
State.OPEN -> {
if (simTime - openedAtSimTime >= cooldownSeconds) {
state = State.HALF_OPEN
true
- Files reviewed: 12/12 changed files
- Comments generated: 4
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ite cooldown for the circuit breaker (#1058) Review round of the breaker PR, resolving every open review thread: - Extract createLlmPlanner: the console and GUI AI examples built byte-identical KoogAgentPlanAdapter instances, including four repeated DispatcherRunConfig scope reads; one private builder now serves both. - A cycle that already emitted an actuator action suppresses BOTH the bounded retry and the rule-based fallback when it later fails — the emissions are already posted, and layering more decisions on top would double-dispatch for the same train. The failure still reaches the breaker, and the tick is reported through the emission classification (LLM_ACTIONS / LLM_NO_OP) instead of RULE_FALLBACK. - HALF_OPEN grants exactly one probe at a time (probeInFlight) and plan() releases the claim in a try/finally (abandonProbe), so a probe cancelled mid-inference cannot wedge the breaker for the rest of the run. - A non-finite cooldownSeconds is rejected at all three layers: the breaker constructor, the DispatcherRunConfig constructor, and the -D parser (which WARNs and falls back to the default, like every other malformed knob). - RunParameters gains defaulted circuitBreakerFailureThreshold and circuitBreakerCooldownSeconds fields, so a run's JSON records the breaker it actually ran with; the live rule-based/LLM recordings and the SweepCell abort snapshot copy them from the same file-tier DispatcherRunConfig the forked child resolves, keeping aborted runs grouped with their completed siblings. - A throwing fallback oracle on the silent-cycle path reports a degraded RULE_FALLBACK tick before the exception propagates, restoring the #927/#999 every-cycle-accounted-once invariant the #1058 restructure had dropped. - The emission-counter reset moves to the top of plan(), so breaker-skip and agent-creation-failure cycles can no longer report a previous cycle's emissions as their own. - MeasuringPlanAdapter's final summary also logs the circuit-breaker status line — skip ticks are scored as ordinary fallback cycles, so the metrics alone cannot show that the breaker skipped the LLM for most of the run. Run-JSON breaker stats are a deliberate follow-up: #1074. Co-Authored-By: Claude Code <noreply@anthropic.com>
|
The review summary also named one suppressed finding with no inline thread: half-open probing is not concurrency-safe. That one is fixed too, in 3f5ca32:
Covered by Full gate green locally (build, detekt, ktlintCheck, test, integrationTest). Run-JSON breaker stats are filed as follow-up #1074. |
There was a problem hiding this comment.
🟡 Changes recommended
Review findings remain in breaker initialization/cycle handling and run-report configuration identity.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (2)
dispatcher-agent/src/main/kotlin/cz/vutbr/fit/interlockSim/dispatcher/planner/KoogAgentPlanAdapter.kt:363
- The breaker is consulted only after
getOrCreateAgent(). If agent creation/assembly fails, this returns to the fallback beforeshouldAttempt()orrecordFailure(), leavingagentnull so every subsequent tick repeats the initialization and warm-up work without ever opening the breaker. Check the breaker before initialization or route creation failures through the same breaker failure path so sustained initialization failures are bounded too.
val a =
try {
getOrCreateAgent()
} catch (e: CancellationException) {
throw e
} catch (e: Exception) {
dispatcher-agent/src/main/kotlin/cz/vutbr/fit/interlockSim/dispatcher/planner/RunParameters.kt:49
- These defaults misrepresent pre-#1058 snapshots: those runs had no circuit breaker, so a 3-failure threshold and 60-second cooldown did not actually apply. Because
RunReportAggregatorgroups on the fullRunParameters, decoding an old JSON with these values can merge it with a current default-breaker run and report them as the same experiment. Preserve an explicit legacy/unknown value (or version the snapshot) rather than claiming these settings were used.
* Defaults to [LlmCircuitBreaker.DEFAULT_FAILURE_THRESHOLD], the threshold every run used before
* this field existed, so decoding a run JSON that predates the field records the value that
* actually applied rather than an unknown one.
- Files reviewed: 18/18 changed files
- Comments generated: 2
- Review effort level: Lite
…ow breaker config in the sweep report (#1073 review round) Two Copilot review threads on PR #1073 pointed at real bugs left from #1058's circuit breaker: - KoogAgentPlanAdapter.plan() returned the fallback on a breaker-OPEN skip without advancing the correlation cycle, so consecutive OPEN-window skips registered their decisions under the same tick and Sp2c21MetricsRecorder's tickIndex drifted from the queue's own counter. - RunParameters' new circuitBreakerFailureThreshold/CooldownSeconds fields already split Parameter Sweep cells apart, but the shared row prefix and both table headers never rendered them, so two cells differing only in breaker config were indistinguishable. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|



Summary
KoogAgentPlanAdapterpaid a full 30swithTimeoutstall on every tick with no backoff, and each timeout printed a duplicate full stack trace from Koog's own internal logger.LlmCircuitBreaker(CLOSED → OPEN → HALF_OPEN), keyed on simulation time rather than wall clock: aftercircuitBreakerFailureThresholdconsecutive cycle failures it opens and skips the LLM entirely forcircuitBreakerCooldownSeconds, then probes recovery once. Both knobs follow the existing-D>dispatcher-defaults.properties> code-constant precedence."Execution exception reported by server!"ERROR log via the Logback API indesktop-ui/Main.kt, deliberately not by editing:core's sharedlogback.xml(kept:coreuntouched entirely for this fix). Tradeoff: this also silences any other Koog-internal error from that package, not just timeouts.plan()was restructured so the outer exception handler only wrapsgetOrCreateAgent(), not the whole cycle — needed to avoid double-reporting a tick when a retried/failed cycle's own fallback throws.Test plan
:dispatcher-agent:test— 1785 tests pass, including 17 new tests (LlmCircuitBreakerTeststate-machine coverage,KoogAgentPlanAdapterTestretry/breaker scenarios,DispatcherRunConfigTest/DispatcherDefaultsResourceTestfor the two new config knobs):dispatcher-agent:integrationTest— passes, including live-Ollama-tagged tests:desktop-ui:test— 807/808 pass (1 pre-existing skip)ktlintCheck,detekt— cleanshadowJarand ran a real headless example end to end to confirm startup and the new logging code path work🤖 Generated with Claude Code