fix: fleet-restart lock — ownership-checked lease refresh + acquire inside try/finally (#1919) - #1928
Merged
Merged
Conversation
…ire inside try/finally (#1919) The per-iteration refresh was a bare EXPIRE gated on a local flag — after a TTL lapse it extended a concurrent caller's lease while both loops ran. The refresh is now a pre-action ownership gate (GET-compare via the shared redis_breaker_util.lock_token_matches): a foreign token stops the run with honest partial accounting (summary/audit gain processed + stopped_early), an absent token is re-acquired via SETNX so an unraced run completes instead of aborting, EXPIRE→0 routes to the absent path, and refresh Redis errors stay fail-open with one throttled warning per run. list_all_agents_fast() moved inside the try/finally so nothing can ever leak the lock between acquire and release; an abnormal exit audits as stopped_early="error" + exception class name only. TTL 900→2100, sized above the slowest single agent (skill injection alone is bounded at 1800s) so a mid-agent lapse is no longer arithmetically guaranteed. Release stays compare-and-delete, attempted even after detected loss (foreign-safe by construction). +11 unit tests (27 total in the file); live-validated on the local stack (foreign takeover mid-run, absent re-acquire to completion, 409 concurrency, TTL 2098 observed, release verified). Sibling hand-rolled lock sites and the system_seed_service unconditional release → #1920. Fixes #1919 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
obasilakis
approved these changes
Jul 31, 2026
obasilakis
left a comment
Contributor
There was a problem hiding this comment.
Validated via /validate-pr: docs, security, packaging, test adequacy and lock-logic correctness all pass. All 21 CI checks green. Approving.
AndriiPasternak31
added a commit
that referenced
this pull request
Jul 31, 2026
…entries Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
ops:fleet_restartper-iteration lease refresh was a bareEXPIREgated only on a local flag: after a TTL lapse it would extend a concurrent caller's lease while both fleet loops ran — the exact dual-run the lock exists to prevent, silently. The refresh is now an ownership-checked pre-action gate (GET-compare via the new sharedredis_breaker_util.lock_token_matches, same predicate as the release so the two can't drift).summary/audit gainprocessed+stopped_early="lease_lost_foreign"; completed restarts stand; the release compare-and-delete still runs (foreign-safe by construction — skipping it on a false "absent" would self-lockout the fleet for the TTL). Absent token (incl.EXPIRE→0 in the GET→EXPIRE sliver) ⇒ SETNX re-acquire and continue — nothing is racing us, so a destructive half-done run completes instead of aborting;lease_reacquiredis audited. Refresh Redis errors stay fail-open with one throttled warning per run.agents = list_all_agents_fast()moved inside thetry/finally(pre-initialized for thefinally-audit read), so nothing can ever sit between lock acquire and release; an abnormal exit audits asstopped_early="error"+ exception class name only (fix: fleet restart adopts rebuilt base images through the canonical lifecycle path (#1860) #1912 exposure rule), distinguishing an aborted run from an empty fleet.skill_service._INJECT_LOCK_TTL_SECONDS(1800s) — at 900s a mid-agent lapse was arithmetically guaranteed on the slow path, and detection alone would have shipped against a guaranteed trip. Constants are comment-linked so retuning either side re-opens the math visibly.Scope notes
stopped_early/processedare consumed by API/audit readers; the MobileAdmin PWA toast reads only a generic message today — client-side follow-up noted inmobile-admin-pwa.md.system_seed_service's unconditional tokenless release) — consolidation filed as bug: hand-rolled Redis single-flight locks — 7 divergent copies; system_seed_service releases with an unconditional tokenless delete #1920; this PR deliberately stays scoped torestart_fleet+ the first shared helper.Changes
src/backend/routers/ops.py— ownership gate, re-acquire, TTL resize, acquire-inside-try, partial-run honesty fields (responsesummary+ auditdetails)src/backend/redis_breaker_util.py—lock_token_matches(dual bytes/str, documented as belt-and-braces underdecode_responses=True)tests/unit/test_1860_fleet_restart_adoption.py— +11 tests (27 total): 2-agent happy-path lease baseline, foreign stop + release-leaves-foreign, absent re-acquire win/lose, refresh-error scoped fail-open with release still deleting, EXPIRE→0 / EXPIRE-raise, first-iteration loss, bytes token, no-token-values-in-logs, listing-failure releases lock + error auditarchitecture.mdRedis-catalog bullet,agent-lifecycle.md(single-flight section, tests line, changelog row, line refs re-verified),mobile-admin-pwa.md,feature-flows.mdindex row, 2learnings.mdentries (lease-TTL-vs-slowest-iteration; false-detection-vs-self-safe-cleanup)/cso --diffPASS, zero findings — report pair committed underdocs/security-reports/Test Plan
pytest tests/unit/test_1860_fleet_restart_adoption.py— 27/27tests/test_ops.pyagainst the running stack — 41/41test_dispatch_breaker.py+test_69_ephemeral_agents.py— 66/66lease_reacquired:trueaudited), concurrent call → 409, fleet healthy afterFixes #1919
🤖 Generated with Claude Code