refactor(#921): close watchdog race agent-side; -336 LOC#934
Merged
Conversation
obasilakis
added a commit
that referenced
this pull request
May 25, 2026
…ery gap /review on PR #934 found that the simplification fixed the periodic watchdog (`_reconcile_orphaned_executions`) but missed the startup recovery (`recover_orphaned_executions`). The startup pass read only the `executions` field from `/api/executions/running`, ignoring the new `recently_completed_ids` — so a backend restart that coincided with an in-flight completion could still false-orphan that row. Fix: - Hoist the response parsing into a single module-level helper `_extract_agent_known_ids(payload)` that returns the union of currently-running execution_ids + recently_completed_ids. Defensive against missing fields (older agent images degrade silently) and malformed entries (no execution_id key). - Use the helper from BOTH call sites: the periodic watchdog and the startup recovery. They can no longer drift out of sync. Tests: - New `TestExtractAgentKnownIds` (5 tests) covering: union, missing recently_completed_ids, empty response, null fields, and malformed entries without execution_id. - Existing `TestGetAgentRunningIdsUnionsRecentlyCompleted` still passes unchanged — it now exercises the helper indirectly via the watchdog method. - 28/28 startup-recovery tests (test_orphaned_execution_recovery.py, test_redis_slot_recovery.py, test_startup_recovery_race.py) still pass — the helper substitution is behaviour-preserving for the pre-#921 response shape.
…end code The original #921 fix closed the watchdog false-positive race in the backend via Redis sentinels and two-cycle confirmation. Eugene flagged the result as overly complicated. The actual race lives on the agent side — `claude_code.py` calls `registry.unregister()` in a `finally` block BEFORE the backend's success-write — so fix it there and the backend can revert to a simpler, single-cycle reconciler. Agent side (~37 lines added): - `process_registry` now keeps a `_recently_completed: dict[id, float]` buffer populated by `unregister()`, with a 5-min TTL (above the worst observed in-flight delay in the #921 incident, ~55s). - New `list_recently_completed_ids()` returns IDs within the window, evicting expired entries lazily. - `GET /api/executions/running` adds a `recently_completed_ids` field alongside the existing `executions` list. Backend side (~150 lines removed): - `_get_agent_running_ids` now unions `executions` with `recently_completed_ids` from the agent's response. The agent's recently-completed window absorbs the success-write race directly, so the watchdog never sees the false "missing" sighting. - Deleted: `redis.asyncio` import, `ORPHAN_SENTINEL_PREFIX` / `ORPHAN_SENTINEL_TTL_SECONDS` constants, `self._redis` field, `_get_redis` / `_mark_suspected_orphan` / `_is_suspected_orphan` / `_clear_suspected_orphan` helpers, `orphans_suspected` report field, the two-cycle branch in `_reconcile_orphaned_executions`. Return signature reverts from 4-tuple to 3-tuple. Backwards compatibility: older agent images that haven't shipped the buffer return only `executions` — `_get_agent_running_ids` tolerates the missing field and degrades to pre-#921 behaviour. No coupled deploy required; the simplification works as soon as the backend ships. Tests: - Deleted `TestTwoCycleOrphanConfirmation` from both unit and live-stack test files. Reverted existing tests to 3-tuple semantics — orphan recovery is single-cycle again. - Added `TestGetAgentRunningIdsUnionsRecentlyCompleted` (unit): 3 tests covering the union, the missing-field tolerance, and the empty case. - Added `tests/unit/test_recently_completed_buffer.py`: 6 tests for the agent-side primitive (register/unregister lifecycle, TTL eviction, TTL sized above incident gap). - Live verification: one cycle marks a true orphan failed, no `orphans_suspected` field in the report. Net: 182 added, 518 deleted (-336 lines). Same observable behaviour for the natural-completion case (no false positive); true orphans recover on a single cycle instead of two (~5 min faster).
…ery gap /review on PR #934 found that the simplification fixed the periodic watchdog (`_reconcile_orphaned_executions`) but missed the startup recovery (`recover_orphaned_executions`). The startup pass read only the `executions` field from `/api/executions/running`, ignoring the new `recently_completed_ids` — so a backend restart that coincided with an in-flight completion could still false-orphan that row. Fix: - Hoist the response parsing into a single module-level helper `_extract_agent_known_ids(payload)` that returns the union of currently-running execution_ids + recently_completed_ids. Defensive against missing fields (older agent images degrade silently) and malformed entries (no execution_id key). - Use the helper from BOTH call sites: the periodic watchdog and the startup recovery. They can no longer drift out of sync. Tests: - New `TestExtractAgentKnownIds` (5 tests) covering: union, missing recently_completed_ids, empty response, null fields, and malformed entries without execution_id. - Existing `TestGetAgentRunningIdsUnionsRecentlyCompleted` still passes unchanged — it now exercises the helper indirectly via the watchdog method. - 28/28 startup-recovery tests (test_orphaned_execution_recovery.py, test_redis_slot_recovery.py, test_startup_recovery_race.py) still pass — the helper substitution is behaviour-preserving for the pre-#921 response shape.
CI's `lint sys.modules pollution check` flagged the agent-server import shim's bare `sys.modules[name] = stub` writes. They worked locally but risked leaking the namespace stubs into other tests in the same pytest session. Adopt the snapshot/restore pattern the lint recognises (precedent: tests/unit/test_telegram_webhook_backfill.py): - Top-level `_STUBBED_MODULE_NAMES` list of the four agent_server.* namespaces this file injects. - Autouse `_restore_sys_modules` fixture snapshots sys.modules entries for those names before each test and restores them after — so the stubs never leak. - Move the namespace stubbing into a lazy `_import_process_registry()` helper invoked per-test, which the autouse fixture's snapshot/restore cleanly wraps. Local lint now passes (0 new violations vs baseline). 6/6 buffer tests still pass.
vybe
force-pushed
the
refactor/921-agent-side-completion-buffer
branch
from
May 25, 2026 16:56
a1d1df1 to
29b0ed7
Compare
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
The original #921 fix worked around a watchdog race using a backend-side Redis sentinel and two-cycle confirmation. The race actually lives on the agent side —
claude_code.py'sfinally: registry.unregister()runs before the backend writessuccess. Closing it there lets the backend revert to a simple, single-cycle reconciler.Net: 182 added, 518 deleted (-336 lines).
Agent side (~37 lines added)
process_registrynow keeps a_recently_completed: dict[id, float]buffer populated byunregister(), with a 5-min TTL (well above the worst observed in-flight delay in the bug: circuit breaker dormant state silently blocks scheduled tasks for hours with no auto-recovery or notification #921 incident, ~55s).list_recently_completed_ids()returns IDs within the window, lazily evicting expired entries.GET /api/executions/runningadds arecently_completed_idsfield alongsideexecutions.Backend side (~150 lines removed)
_extract_agent_known_ids(payload)is the single source of truth for parsing/api/executions/running. It returns the union of currently-running execution_ids +recently_completed_ids. Used by both the periodic watchdog (_get_agent_running_ids) and the startup recovery (recover_orphaned_executions) so they can't drift out of sync.redis.asyncioimport,ORPHAN_SENTINEL_PREFIX/ORPHAN_SENTINEL_TTL_SECONDSconstants,self._redisfield,_get_redis/_mark_suspected_orphan/_is_suspected_orphan/_clear_suspected_orphanhelpers,orphans_suspectedreport field, two-cycle branch in_reconcile_orphaned_executions. Return signature reverts from 4-tuple to 3-tuple.Backwards compatibility
Older agent images that haven't shipped the buffer return only
executions—_extract_agent_known_idstolerates the missing field and degrades to pre-#921 behaviour. No coupled deploy required; the backend simplification works as soon as it ships. Agents pick up the buffer on the next base-image rebuild + container recreate.Behaviour delta
Test plan
tests/test_watchdog_unit.py—TestTwoCycleOrphanConfirmationdeleted; existing tests reverted to 3-tuple unpacking; newTestExtractAgentKnownIds(5 tests) covers the union, missing field, empty response, null fields, and malformed entries withoutexecution_id;TestGetAgentRunningIdsUnionsRecentlyCompleted(3 tests) exercises the watchdog method end-to-end.tests/test_watchdog.py—TestTwoCycleOrphanConfirmationreplaced with a singletest_true_orphan_recovered_on_single_cycleagainst the live backend.tests/unit/test_recently_completed_buffer.py(new) — 6 tests for the agent-side primitive: register/unregister lifecycle, lazy TTL eviction, TTL sized above the observed bug: circuit breaker dormant state silently blocks scheduled tasks for hours with no auto-recovery or notification #921 incident gap, unknown-id and multi-id handling. Uses the_STUBBED_MODULE_NAMES+_restore_sys_modulesautouse pattern so the agent-server namespace stubs can't leak.tests/integration/test_circuit_breaker.py— 34 tests unchanged, all pass. CB work from fix(#921): watchdog orphan race + CB dormant self-heal + admin reset #924 untouched.tests/test_circuit_breaker_reset.py— 4 tests unchanged, all pass.test_orphaned_execution_recovery.py,test_redis_slot_recovery.py,test_startup_recovery_race.py) — 28/28 still pass; the shared helper is behaviour-preserving for the pre-bug: circuit breaker dormant state silently blocks scheduled tasks for hours with no auto-recovery or notification #921 response shape.runningrow fortrinity-system, triggered cleanup, observedorphaned_executions=1in one cycle with row markedfailed. Noorphans_suspectedfield in the report.Out of scope (deferred from earlier triage)
_emit_dormant_alertout ofagent_clientintomonitoring_service(decouple low-level CB code from the operator queue DB write).RECENTLY_COMPLETED_TTL_SECONDS— current 5min default is well above the observed incident gap.Each stands on its own and doesn't depend on this PR.
🤖 Generated with Claude Code