fix(#921): watchdog orphan race + CB dormant self-heal + admin reset - #924
Merged
Conversation
The agent's claude_code.py unregisters its process registry in a `finally` block BEFORE task_execution_service writes `success` to the DB. A single watchdog snapshot taken in that window cannot distinguish a completing execution from a true orphan, so cleanup_service was marking healthy executions FAILED and releasing their slots — triggering the slot saturation → CB-open → CB-dormant cascade described in #921. Recovery now requires two consecutive sightings of (DB-running + agent-missing). Redis sentinel `watchdog:suspected_orphan:{eid}` gates the recovery: the first sighting writes the sentinel and defers; only the second confirms and recovers. The natural-completion race resolves between cycles (DB row becomes terminal) so the second cycle never sees the row in the running query. True-orphan recovery latency goes from 0 to ~5 min — acceptable for the safety-net role. - Added orphans_suspected report field (informational, not in `total`) - Redis client is lazy + async + fail-open: on Redis outage the watchdog reverts to legacy single-cycle behaviour rather than blocking recovery fleet-wide. - Out of scope (separate work in #921): CB dormant-state stuck-forever behaviour, admin reset endpoint. This change prevents the false- positive that trips the CB in the first place. Unit tests: 4 new tests in TestTwoCycleOrphanConfirmation cover first- cycle deferral, second-cycle recovery, the natural-completion race, and agent-flap sentinel clearing. 7 existing tests updated for the new 4-tuple return shape and sentinel mocking. Integration tests: 3 new tests in test_watchdog.py drive the live POST /api/monitoring/cleanup-trigger endpoint against the running backend, with DB/Redis fixture helpers and per-test teardown.
The dormant state was "stop probing — wait for external intervention", which produced the observed 14.5h false-fail outage in #921 when nothing external happened to reset the breaker. Restore baseline recovery: in dormant, allow exactly one probe per CIRCUIT_DORMANT_COOLDOWN_SECONDS (default 1h) under the same probe-lock as open state. Bounds the worst- case dormant outage to ~1h instead of "until a human notices". Implementation: - _ALLOW_REQUEST_LUA: drop the `state == 'dormant' return 'deny'` short- circuit so dormant falls through to the same next_probe_at + probe-lock dance as open. - _RECORD_FAILURE_LUA: when transitioning to or while in dormant, set next_probe_at = now + dormant_cooldown (passed as new ARGV[6]) instead of the open exponential-backoff curve. - Probe success resets to closed via existing _RECORD_SUCCESS_LUA; probe failure increments probe_count and re-arms next_probe_at to another full dormant cooldown. - Updated log message — "manual recovery required" is no longer accurate. Out of scope: force_circuit_dormant (operator-initiated) keeps using CIRCUIT_MAX_COOLDOWN_SECONDS — that's the autonomy-off pause flow from #631, not the failure-cascade dormancy this fix targets. Tests: test_dormant_denies_all_requests reframed as test_dormant_denies_within_cooldown (semantics clarified — still denies during the cooldown window). New test_dormant_probes_after_cooldown proves the post-cooldown probe is admitted and the probe-lock is held. 32/32 CB integration tests pass.
Surface the closed/open → dormant transition as a high-priority operator_queue entry so the Operating Room UI shows "agent silently failing scheduled tasks" without operators having to grep logs. In the incident behind #921 nobody noticed the dormant state for 14.5h because the only signal was a single WARN log buried in backend output. Implementation: - New _emit_dormant_alert(agent_name) module-level helper in agent_client.py. Lazy imports `database.db` to avoid pulling SQLite into the agent_client import chain during early startup. - Called from CircuitState.record_failure on the prior!=dormant → new==dormant transition. The atomic Lua in _RECORD_FAILURE_LUA guarantees exactly one worker observes that transition across the uvicorn pool, so the alert fires once per distinct dormant entry — no de-dupe layer needed. - Failure-tolerant: if the DB write blows up, we log via `logger.exception` and the CB transition itself is unaffected. Tests: test_dormant_transition_emits_operator_queue_alert in integration/test_circuit_breaker.py stubs `database` in sys.modules, drives the breaker to dormant via real Redis transitions, and asserts - exactly one create_operator_queue_item call on transition - item shape: type=circuit_breaker_dormant, priority=high, context has agent_name + transition + dormant_cooldown_seconds - subsequent failures while dormant do NOT fire a second alert (verifies the once-per-entry guarantee) Live verification: drove trinity-system CB to dormant against the running stack; operator_queue row appeared with all expected fields.
Admin escape hatch for the dormant-CB cascade described in #921 — equivalent to the manual `redis-cli DEL agent:circuit:{name}` workaround. The dormant→half-open auto-probe (1h cooldown) already self-heals the breaker, but this endpoint is the first-response tool when an operator already knows the agent is healthy and doesn't want to wait a full cooldown. Implementation: - New route in routers/agents.py. Mounts at the path requested in the ticket so the URL is predictable for runbooks. - Auth: require_role('admin'). Operator-only action; not exposed to agent owners (resetting another tenant's CB isn't theirs to do). - Calls the existing services.agent_client.reset_circuit helper that was already used by /api/monitoring/agents/{name}/check; keeps a single source of truth for the DEL semantics. - Response includes `prior_state` so incident postmortems can read off what state the operator reset out of (closed | open | dormant). Tests: tests/test_circuit_breaker_reset.py — 4 integration tests driving the live stack: - happy path: force dormant → reset → 200 with prior_state=dormant, Redis key deleted - idempotent on closed CB (no Redis key present): 200, prior=closed - unauthenticated → 401/403 - unknown agent → 404 (verifies AuthorizedAgentByName wiring) Live verification: parked trinity-system's CB dormant, hit the new endpoint, received correct prior_state=dormant response, confirmed the Redis hash was deleted.
obasilakis
force-pushed
the
fix/921-cb-dormant-and-watchdog-race
branch
from
May 23, 2026 20:15
db161d6 to
1205dac
Compare
obasilakis
changed the base branch from
fix/913-scheduler-per-agent-timeout
to
dev
May 23, 2026 20:15
…adence test Addresses two informational findings from /review on PR #924: [I1] Operator-queue alert now uses the generic `type: "alert"` so the existing Operating Room UI (QueueCard.vue / QueueItemDetail.vue branch on 'approval|question|alert') renders an Acknowledge control. The narrower CB-specific marker moves to `context.alert_type = "circuit_breaker_dormant"` for callers that need to filter. Without this, operators saw the alert but couldn't ack it via the UI. Same pattern the existing `sync_failing` work should pick up when its UI surface is touched. [I2] New test `test_dormant_probe_failure_rearms_full_dormant_cooldown` in tests/integration/test_circuit_breaker.py locks in the cadence the #921 dormant-self-heal promises: after a dormant probe FAILS, next_probe_at is rearmed to the full DORMANT_COOLDOWN (~1h), NOT the open-state exponential backoff curve. Uses a wide gap between the two cooldown families (0.5s vs 0.001s) so the assertion can distinguish them. Without this guard a future refactor could regress the cadence and operators would lose the predictable hourly probe. 84/84 tests pass across the four #921-touching files.
vybe
approved these changes
May 24, 2026
vybe
left a comment
vybe
left a comment
Contributor
There was a problem hiding this comment.
Approved — clean cascade fix.
Watchdog two-cycle confirmation eliminates the false-positive at the root, the dormant CB now self-heals on a 1h cooldown, the dormant transition fires a one-shot operator-queue alert (atomic via Lua, no de-dupe layer needed), and the admin reset endpoint replaces the redis-cli workaround. Strong test coverage (84/84 across the four #921 files), security clean, focused diff, well-decomposed commits including a /review follow-up commit that addressed the UI-renderability of the alert.
Optional follow-up (not blocking): mirror the new endpoint and dormant self-heal behavior in docs/memory/architecture.md when convenient.
7 tasks
3 tasks
5 tasks
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.
Summary
Closes the #921 false-fail cascade end-to-end. Four commits, each focused:
780c7270— Two-cycle confirmation for watchdog orphan recovery. Closes the race window between the agent's in-finallyregistry.unregisterand the backend's success-write that was producing false-positive orphan recoveries (the cascade's root cause).dab63744— Dormant CB self-heals via 1h cooldown probe. The breaker no longer requires manual intervention — restores baseline recovery behaviour and bounds the worst-case outage from "14.5h until someone notices" to ~1h.5b658a98— Operator-queue alert on CB dormant transition. The incident behind bug: circuit breaker dormant state silently blocks scheduled tasks for hours with no auto-recovery or notification #921 went unnoticed for hours because the only signal was a single WARN log. Now the transition surfaces in the Operating Room.1205dacc—POST /api/agents/{name}/circuit-breaker/resetadmin endpoint. Replaces theredis-cli DEL agent:circuit:{name}operator workaround.Together: the watchdog stops producing the false-positive that opens the CB; if the CB does genuinely open and go dormant, it self-heals; if an operator wants to short-circuit the self-heal they have an admin endpoint; and the dormant state is visible in the UI either way.
Reproduction (pre-fix)
Inserted a
schedule_executionsrow instatus='running'fortrinity-systemwithstarted_at=120sago.POST /api/monitoring/cleanup-triggermarked the rowfailedwith the watchdog error message — exactly the cascade described in #921, because the watchdog can't tell the natural-completion race window apart from a true orphan from a single snapshot.Test plan
tests/test_watchdog_unit.py— 4 new unit tests inTestTwoCycleOrphanConfirmation, existing 7 updated for the widened 4-tuple returntests/test_watchdog.py— 3 new live-stack integration tests inTestTwoCycleOrphanConfirmationdriving the realPOST /api/monitoring/cleanup-triggertests/integration/test_circuit_breaker.py— 1 new test for the dormant→probe transition, 1 new test for the operator-queue alert shape, 1 updated test for the cooldown-deny semanticstests/test_circuit_breaker_reset.py— new file, 4 integration tests for the admin reset endpoint (happy path, idempotent, auth, 404)80 passed, 3 skipped across the four #921-touching test files (
test_watchdog_unit.py,test_watchdog.py,tests/integration/test_circuit_breaker.py,test_circuit_breaker_reset.py).Out of scope
🤖 Generated with Claude Code