fix(reliability): clear per-agent Redis runtime state across the agent lifecycle (#1560)#1568
Merged
Merged
Conversation
…t lifecycle (#1560) `agent:circuit:{name}` (transport breaker) is keyed by agent NAME, not container identity, and carries no TTL. Nothing in the lifecycle cleared it, so a container replaced under the same name inherited its predecessor's `dormant` verdict and fast-failed every execution with "Agent circuit breaker open — agent is unhealthy" — without the backend ever contacting the agent. Adds `services/agent_runtime_state.py` as the single enumeration point for every name-keyed per-agent Redis keyspace (the Redis-side twin of the `AGENT_REFS` registry in `db/agent_cleanup.py`), with two entry points whose blast radii differ by whether a container is running: clear_agent_breakers heartbeat + transport circuit + dispatch breaker (safe on a live container) clear_agent_runtime_state the above + execution slots (teardown paths only — force_clear_slots would drop capacity accounting for an in-flight #1083 execution) Wired into six lifecycle points. The issue's acceptance criteria named delete, rename and create; none is the reachable path: * create is unreachable — `is_agent_name_reserved` sees soft-deleted rows and 409s, locking the name for the whole retention window; * the name only unlocks at the retention purge, which cleared no Redis state; * the reachable path is `start_agent_internal`, whose `needs_recreation` branch replaces the container on any config drift (subscription switch, resource change, auth-token rotation), so one fleet-wide rotation resurrects every stale verdict. So start/recreate, purge, and the `trinity-system` bootstrap were added beyond the written criteria. The start-path clear runs before the recreate, since `containers_run(detach=True)` brings the replacement up, and is guarded on `needs_recreation or not was_already_running` so a no-op start cannot reset a breaker protecting a wedged agent. Tests: a bidirectional parity guard fails CI when a new `agent:*` keyspace ships unregistered; wiring tests pin all six call sites and that lifecycle.py never clears slots; an integration test exercises the real Lua (fakeredis has no EVALSHA) plus an opt-in leg driving the full recreate path over HTTP. Both guards are mutation-tested. Complementary to #1561, which removes the source of breaker poisoning; this removes the inheritance. Both are needed. Fixes #1560 Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The `lint (sys.modules pollution check)` CI gate flagged 7 new violations. They were real, not a lint technicality: the integration test replaced `sys.modules["services"]` with a bare stub module and never restored it, so every later test in the same session that imported the real `services` package would have seen the stub — the exact cross-file pollution class #762 introduced this gate for. - Unit files: drop the `sys.modules[mod_name] = module` registration in `_load` entirely. `agent_runtime_state.py` is a stdlib-only leaf with no `@dataclass` needing `sys.modules[cls.__module__]` to resolve annotations, so the registration bought nothing and risked pollution. Per-test stubs already go through `monkeypatch.setitem`, which restores itself. - Integration file: keep the bindings (they are what makes the production lazy imports resolve to the real modules against real Redis — monkeypatch cannot reach an import performed inside the function under test) and add the sanctioned `_STUBBED_MODULE_NAMES` + autouse `_restore_sys_modules` snapshot/restore pair, per tests/unit/test_telegram_webhook_backfill.py. Verified: `python tests/lint_sys_modules.py` clean; running the integration file followed by a suite that imports the real `services` package passes. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vybe
approved these changes
Jul 10, 2026
vybe
left a comment
Contributor
There was a problem hiding this comment.
/validate-pr ✅ — Fixes #1560. Single-enumeration agent_runtime_state.py registry + parity test + recreate-inheritance integration test; clears both breakers/heartbeat/slots across 6 lifecycle points, clears before containers_run(detach=True), guards slot-clear against in-flight #1083 capacity. Docs (architecture/feature-flows/learnings) updated. No secrets/packaging gaps.
This was referenced Jul 10, 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
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.
Summary
agent:circuit:{name}(transport breaker) is keyed by agent name, not container identity, and carries no TTL. Nothing in the agent lifecycle cleared it, so any container replaced under the same name inherited its predecessor'sdormantverdict and fast-failed every execution with "Agent circuit breaker open — agent is unhealthy" — without the backend ever contacting the agent.services/agent_runtime_state.pyas the single enumeration point for every name-keyed per-agent Redis keyspace — the Redis-side twin of the existingAGENT_REFSregistry indb/agent_cleanup.py— and wires it into six lifecycle points.agent:*keyspace ships unregistered, so the next key added cannot be silently forgotten (AC fix: add missing logging_config.py to backend Dockerfile #4).Root cause, and why the acceptance criteria were incomplete
The issue names delete / rename / create as the fix sites. Investigation showed none of them is the reachable path:
is_agent_name_reserved()(db/agents.py:127) sees soft-deleted rows andcrud.py409s, so a name is locked for the whole retention window.purge_agent_ownership()has exactly one caller — the retention sweep atcleanup_service.py— and it cleared no Redis state.start_agent_internal: itsneeds_recreationbranch replaces the container on any config drift (subscription switch, resource change, auth-token rotation, guardrails edit). A single fleet-wide subscription rotation recreates every agent at once and resurrects every stale verdict.Three sites were therefore added beyond the written ACs: start/recreate, purge, and the
trinity-systembootstrap (system_agent_service.ensure_system_agent()recreates a fixed, permanently-recycled name via its owncontainers_run).Design: two entry points, different blast radii
clear_agent_breakers(sync)clear_agent_runtime_state(async)Slots are cleared only where the container is provably gone or stopped.
force_clear_slotswholesale-DELsagent:slots:{name}; on a running agent that would drop capacity accounting for an in-flight fire-and-forget execution (#1083).The start-path clear runs before the recreate, not after:
recreate_container_with_updated_configbrings the replacement up viacontainers_run(detach=True), so clearing afterwards leaves a window in which a concurrent dispatch reads the predecessor's verdict against an already-running container.It is guarded on
needs_recreation or not was_already_running, so a no-opstartof an already-running agent cannot be used to reset a breaker protecting a genuinely wedged agent.Evidence from a live instance
Before the fix, a dev instance carried 10 orphaned
agent:circuit:*keys from long-deleted agents — every onestate=dormant,TTL=-1, and every one still inside its deny window:They never decay into a harmless state: the #1561 poller keeps re-arming
next_probe_at(last_failure_tswas 964s old on an agent deleted a week earlier). Deleting the keys by hand is futile while #1561 is unfixed — they were rebuilt tofailures=9, state=openwithin 20 minutes.Reproduced and then verified end-to-end against a real agent:
Relationship to #1561 (PR #1565)
Complementary, both needed, no file overlap. #1561 removes the source (the sync-health poller hammering soft-deleted agents and poisoning their breakers); this PR removes the inheritance. Note that until #1565 lands, the delete-time clear is undone within 60s by that poller — correctness here rests on the purge / create / start clears, not the delete one.
Tests
tests/unit/test_1560_agent_redis_key_parity.py— bidirectional registry parity; fails CI on an unregisteredagent:*keyspace. Mutation-tested (adding a fake keyspace fails it).tests/unit/test_1560_breaker_cleared_on_lifecycle.py— per-subsystem fail-open behaviour + the six call sites, incl. thatlifecycle.pynever calls the slot-clearing variant. Mutation-tested (removing the start-path clear fails it).tests/integration/test_1560_breaker_lifecycle.py— against real Redis (real Lua; fakeredis has noEVALSHA): adormantcircuit denies, and after the sweepallow_request()allows — the exact gatetask_execution_servicereads. Plus an opt-in leg (TRINITY_LIFECYCLE_TEST_AGENT) that drives the full AC Feature/process engine #6 path: plant verdict → task fast-fails → real recreate → task returns 200.150 passed, 1 skippedacross the new tests plus the heartbeat / circuit-breaker / dispatch-wiring / soft-delete / cleanup-sweep / correlated-pause suites.Deliberately not in scope
task_execution_service.py:1016) is unchanged. AC Setup improvements #5 is satisfied by its "either the state is cleared" branch; the wording is the explicitly-named secondary defect of bug: Disabling agent autonomy forces circuit breaker DORMANT and blocks all inbound user responses #1557, which is where it will change, so the two branches don't collide on the same line.circuit_breaker_dormantoperator-queue entry naming a deleted agent (mentioned in the issue's Technical Notes) is untouched — it reaches into a different subsystem. Worth a follow-up.Known interaction (authorization-consistency note, not a security issue)
force_circuit_dormant(reason="autonomy_disabled")(agent_service/autonomy.py, #631) is an owner-only side effect, and the explicitPOST /circuit-breaker/resetis admin-only. ButPOST /agents/{name}/startisAuthorizedAgentByName(owner, shared user, or admin), so after this change a shared non-owner can clear the breakers by stopping and starting the agent, and a restart clears theautonomy_disableddormant marker while the autonomy flag stays off.This was reviewed adversarially and is not a security finding. The clear touches only
agent:*:{that-name}Redis keys;agent_ownership.autonomy_enabledand the scheduleenabledflags live in SQLite and are untouched, so no autonomous execution is re-enabled — only background health-probing resumes. A shared user already holds chat/dispatch rights to that exact agent, so no new access, data, tool, or tenant is reachable. Thetrinity-systempath is closed:clear_agent_breakers(SYSTEM_AGENT_NAME)is reachable only via adminreinitializeor astartthatcan_user_access_agent('trinity-system')denies with a uniform 404.Worth an operator's eye nonetheless: the breaker-clearing side effect on
start(shared) is broader than the dedicated reset endpoint (admin). #1557 removes the autonomy↔breaker conflation entirely, at which point the marker interaction disappears.Two residual reliability notes, both self-healing and out of scope: a microsecond TOCTOU between
purge_agent_ownership(name)and the sweep could drop one slot-ZSET entry for a name recreated inside that window (reclaimed by the slot TTL reaper); and the stalecircuit_breaker_dormantoperator-queue entry is untouched (see above).Fixes #1560