Repository navigation
fix(canary): one cycle per fleet, not one per uvicorn worker (#1881) - #2047
Conversation
#1881 describes two chained defects and says they must land together, because "shipping the packaging fix alone converts a dormant bug into a live one". Part 1 shipped in #1876 — docker-compose.prod.yml now forwards CANARY_ENABLED and CANARY_SLACK_WEBHOOK_URL. Part 2 did not, so the warned-about state has been live since. Verified on eu2 today. The backend command is `uvicorn main:app ... --workers 2`; the canary cycle log lines interleave at ~2m15s / ~2m47s alternating gaps, which is the signature of two independent 5-minute loops running offset from each other; and canary_violations holds 11,942 rows for the last 24h, double-persisted. Why there was nothing stopping it The FastAPI lifespan calls canary_service.start(), so each worker process starts its own loop. The only mutual exclusion in the service is `self._lock = asyncio.Lock()`, which guards re-entrancy inside ONE process and says nothing about cross-worker exclusion. Reading an asyncio.Lock in a service class as evidence of single-instance-ness is exactly the mistake learnings.md 2026-07-29 records; it is a prompt to ask where the leader lease is, not proof that there is one. What that costs, concretely: R-01 `docker exec`s into every running agent container each cycle, so the fleet is probed twice per 5 min per agent by the subsystem that is supposed to observe it unobtrusively. Every violation is inserted twice. And every shared cross-cycle marker — canary:last_cycle_at, canary:last_cycle_red, E-02's canary:e02:terminal_seen and H-01's canary:h01:suspect_since — has two independent writers. The fix The scheduled loop now runs only when it holds a Redis `canary:leader` lease, mirroring monitoring:leader (#1464) and opqueue:leader (#1632): SET NX to pick a single winner atomically, TTL refresh only when the stored id is our own (so a worker can never steal or clobber a sibling's lease), best-effort release on stop() for an instant handoff. Every worker still runs its loop and re-evaluates leadership each cycle rather than once at startup, so a dead leader's lease expires and a sibling takes over with no restart. Two places it deliberately departs from the precedents, both for the same underlying reason — a duplicate canary cycle is not inert the way a doubled breaker feed (#1464) or an on-conflict create (#1632) is. TTL carries a floor. `interval * 3` is the right scaling for a leader whose work scales with its interval. A canary cycle's cost does not: it is dominated by R-01's container.exec_run sweep across every running agent container, which is bounded by no timeout and scales with FLEET SIZE, not with how often we look. The TTL is refreshed once at the top of a cycle, so it must outlast one worst-case cycle plus the inter-cycle sleep — otherwise the lease lapses mid-cycle, a sibling grabs it, and leadership flaps, restoring the exact concurrent probing the lease exists to remove. The floor is set to what `interval * 3` already yields at the default 300s interval, so the default is unchanged and it only binds if the service is constructed with a shorter one. Fail-open is kept, but not on the precedents' reasoning. monitoring_service can fail open because a doubled record_failure() lands in a breaker that is itself fail-open; operator_queue_service can because duplicate creates dedupe on conflict. Neither argument transfers — here the duplicate re-runs the sweep and double-persists rows. We fail open anyway, because the alternative is strictly worse: this is the one subsystem whose purpose is noticing that something went quiet, H-01 exists because a blind collector reports green, and a canary that stops running is that same silent-green one level up, where no invariant can catch it because invariants only run inside the thing that isn't running. A fail-closed lease would let a Redis blip stop the watcher on every worker at once with nothing saying so. Duplicated probes are noisy and visible; silence is not. A Redis outage is also already a degraded state the harness is built to announce (sources_unavailable; H-01 fires unconfirmed on an unreadable marker), and failing closed would suppress precisely those paths. What the lease does not buy, and what therefore did not change It is best-effort, and the fail-open above re-opens the multi-writer window exactly when Redis is down. H-01's CONFIRMATION_MIN_SECONDS and R-01's DWELL_SECONDS elapsed-wall-clock gates are therefore untouched, and their docstrings — which asserted, correctly at the time, that the service holds no lease — are corrected rather than deleted, with the reason they remain load-bearing spelled out so the next reader does not "simplify" either back to a cycle count. There is a second, more fundamental reason beyond the fail-open window: both gates ride out a REAL-TIME transient (a container finishing teardown, a claude child awaiting its parent's wait()), which is a single-worker property that a cycle count never expressed correctly at any worker count. One knock-on is recorded at R-01's _MAX_OBSERVATION_GAP_SECONDS: a leader failover leaves up to ~1200s (TTL + interval) with nobody cycling, which exceeds that 600s window and restarts the dwell. That is correct — a crashed leader is a genuine observation outage by that constant's own rule — and restarting is the fail-safe direction, so the constant is documented, not widened. run_cycle() is deliberately NOT gated. The lease belongs to the scheduled path, which is the one that runs unattended in every worker. POST /api/canary/run-cycle lands on whichever worker uvicorn routes it to, so gating there would make an explicit admin request return an empty result about half the time under --workers 2 — structurally identical to a green cycle, which is the exact ambiguity the skipped/409 contract was added to remove. Non-leaders log on the leadership transition only; narrating idleness every 5 minutes is how real canary output gets tuned out. Tests go in tests/unit/ (#2037: the 3,700-line canary suite at tests/ root is collected by no CI workflow, so a guard beside it would never go red). 23 tests covering the lease mechanics, failover, both fail-open arms, transition-only logging, that a non-leader never reaches collect_snapshot — with a leader=True positive control, since _loop swallows cycle exceptions and a raise-on-call probe would pass vacuously — that run_cycle stays ungated, and that the two confirmation gates survive. No schema, endpoint, config or compose change.
|
Reviewed the diff rather than the summary. The shape is right — SET NX + own-lease-only refresh, gate placed before Particularly glad the H-01 / R-01 edits are documentation-only and land as "do NOT relax this to a cycle count" with two independent reasons. The lease is best-effort and fails open, so those gates stay load-bearing — and the second reason is the better one: what they ride out is a real-time transient, so cycle count was never the right unit regardless of worker count. Two residuals. Neither blocks, both are worth a decision rather than discovery. 1. The TTL is refreshed only at the top of a cycle, and the cycle has no upper bound. 2. The PR trades zero-gap coverage for no-double-probing, and worker death is not rare on this deployment. Before this change, a worker dying left the sibling cycling with no gap at all. Now: TTL 900s, plus up to Worth considering a sibling takeover keyed on Neither needs fixing in this PR — but residual 2 in particular should be a conscious trade, and it belongs in the docstring next to the fail-open reasoning if it stays as-is. |
dolho
left a comment
dolho
left a comment
There was a problem hiding this comment.
Reviewed the diff against monitoring_service._try_acquire_leadership (#1464) and operator_queue_service (#1632), plus the H-01/R-01 docstring corrections and the 23-test module. CI green on all shards.
The bug is real and measured, not inferred: lifespan starts the service per worker, prod is --workers 2, and self._lock is an asyncio.Lock — in-process re-entrancy only. The eu2 evidence (alternating ~2m15s/~2m47s cycle gaps, 11,942 violation rows in 24h) is the right kind of evidence. The lease mirrors the two precedents closely enough to review by diff, and the two documented departures (TTL floor, fail-open on different reasoning) are both justified — the fail-open argument in particular ("a canary that stops running is silent-green one level up, where no invariant can catch it") is the correct call, since duplicated probes are visible and silence is not.
Four things worth a look, none of them blocking on their own.
1. _release_leadership is a non-atomic GET-then-DELETE
if r is not None and r.get(REDIS_KEY_LEADER) == self._worker_id:
r.delete(REDIS_KEY_LEADER)If our lease expires between the GET and the DELETE and a sibling wins SET NX in that window, we delete the successor's lease. A third worker then acquires while the successor still has _is_leader = True and runs its cycle → two concurrent cycles, i.e. exactly the duplication this PR removes. The window is one round-trip wide at the instant of TTL expiry, so it's rare and self-heals next cycle.
_try_acquire_leadership's get + expire is not exposed the same way (the value must equal our own id, which no sibling can hold), so only release needs it. A Lua compare-and-delete closes it:
if redis.call('get', KEYS[1]) == ARGV[1] then return redis.call('del', KEYS[1]) else return 0 endYou already have ScriptCache in redis_breaker_util. #1464/#1632 share the flaw, so matching them is defensible — but this service is the one where a duplicated cycle has real cost, which is the PR's own argument for departing from the precedent elsewhere.
2. The failover blind window is bought entirely by the TTL, and doesn't have to be
_leader_ttl() = max(interval * 3, 900) = 900s at the default. A crashed leader means nobody cycles for up to TTL + interval ≈ 1200s, which R-01's comment correctly flags as exceeding _MAX_OBSERVATION_GAP_SECONDS and restarting the dwell.
The stated reason the TTL must be that long is that it's refreshed once at the top of the cycle, so it has to cover worst-case cycle + sleep. Refreshing it again after run_cycle returns (or a mid-cycle touch around the R-01 sweep) removes that constraint entirely — the TTL then only has to cover the sleep plus one refresh interval, so it could drop to interval * 2 and failover would land inside ~10 min without any risk of lapsing mid-cycle. That seems strictly better than documenting a 20-minute unwatched window as acceptable, given the PR's own position that an unwatched window is the failure mode H-01 exists to announce.
If you'd rather not, fine — but consider stating the ~1200s worst case in architecture.md alongside the lease, not only in the R-01 constant comment where an operator won't look.
3. Two Redis clients, two failure contracts, in one service
Cycle state uses self._redis() (the slot-service client, raises on failure → caught per call site). The lease uses get_breaker_redis() (returns None → fail-open). Different pools, and more importantly divergent semantics inside one cycle: the lease can fail open to leader while the marker reads raise, or vice versa.
Not wrong — the breaker client is the right leaf choice for a fail-open contract, and decode_responses=True there is what makes r.get(...) == self._worker_id a valid str comparison. But nothing in the code says why there are two, and the next reader will "unify" them onto _redis(), silently converting the lease from fail-open to raise-and-be-caught. One sentence at _try_acquire_leadership naming the reason would prevent that. (If the slot-service client is ever constructed with decode_responses=False, that unification also breaks the comparison silently — worth mentioning in the same sentence.)
4. stop() releases the lease without awaiting the cancelled task
self._task.cancel()
self._task = None
self._release_leadership()stop() is sync, so it can't await — but that means the release can land while a cycle is still unwinding inside the cancelled task, letting a sibling acquire and start a cycle concurrently with our tail. monitoring_service.stop() is async and awaits the cancellation before releasing, which is why it doesn't have this. Given the release is a deliberate optimisation for fast handoff, it's worth one line acknowledging the overlap is possible and bounded, or making stop() async to match the precedent.
5. Question on run_cycle staying ungated
Agreed on the reasoning for the endpoint — a gated on-demand cycle returning empty ~50% of the time is worse than a duplicate. But it is now the one remaining multi-writer path: an admin cycle on a non-leader worker still advances canary:last_cycle_at, writes E-02's terminal_seen and H-01's suspect_since, and persists violations. Does the manual path also drive the alert sink? If it does, a green→red caught by the manual cycle alerts once and the leader's next cycle correctly sees it as continuing-red — fine. If it doesn't, the manual cycle can consume the transition and the Slack alert is lost. Worth confirming in the docstring either way, since the lease makes that the only remaining way for it to happen.
6. Also
- Any cleanup planned for the ~12k historical double-persisted rows on eu2, or does
canary_violationsretention absorb them? Not this PR's job, but a follow-up issue would stop them skewing M7's 7-day counts during the #1766 soak.
What's good
- The TTL floor rationale (cycle cost scales with fleet size, not interval) is exactly the kind of reasoning that's usually missing from a copied lease, and setting the floor to what
interval * 3already yields at the default keeps the change inert. - Correcting the H-01/R-01 docstrings instead of deleting them, and stating the second, lease-independent reason those gates are load-bearing (they ride out a real-time transient — a single-worker property), is the durable version. Those comments were about to become actively misleading.
- Transition-only logging is the right call; a per-cycle "not leader" line is how canary output gets tuned out.
test_non_leader_never_collects_a_snapshotwith aleader=Truepositive control is the test that actually proves the saving, given_loopswallows every exception — nice catch, that's a vacuous-pass trap most guards fall into.
…tomic Review findings (dolho, #2047), plus the mid-cycle residual raised in review. **1. Non-atomic release could delete a SUCCESSOR's lease.** `_release_leadership` was GET-then-DELETE: if our lease expired between the two and a sibling won SET NX in that window, we deleted theirs. A third worker could then acquire while the successor still had `_is_leader = True` — two concurrent cycles, the exact state this PR removes. Both writes are now Lua compare-and-{delete,expire} via the existing `ScriptCache`, so no path can touch a lease that is not ours. #1464/#1632 share the non-atomic shape; we depart for the same reason this service departs on the TTL — a duplicated canary cycle is not inert. **2 + the mid-cycle residual: one TTL was answering two questions.** "How long may a cycle run" wants LARGE (R-01's exec sweep has no timeout and scales with fleet size); "how long before a dead leader is noticed" wants SMALL. The single `max(interval*3, 900)` TTL bought a ~1200s blind failover to pay for (a) — and did not even close (a): a cycle overrunning 900s lapsed its lease mid-flight and a sibling acquired anyway. `_heartbeat_loop` re-arms the lease on its own 60s timer, independent of where the cycle is, so the TTL only has to survive a couple of missed beats: `_LEADER_TTL_SECONDS` is 3x the heartbeat (the precedents' rule), and worst-case failover drops ~1200s → ~780s, owned by `_max_failover_seconds()` so R-01's comment and architecture.md quote one number instead of three prose copies. `_MAX_CYCLE_LEASE_SECONDS` (900s) caps how long the heartbeat will keep a running cycle's lease alive. Without it a WEDGED leader holds the lease forever and nobody cycles — trading "two workers probing" for "no worker watching", the wrong direction here. Past the cap we choose the duplicate over the silence, at ERROR. Same direction as the fail-open. **3.** Documented why two Redis clients coexist and must not be unified — including that `decode_responses=True` on the breaker client is what makes the ownership comparison valid, so a client built without it turns every check silently False. **4.** `stop()` is async and awaits the cancelled task before releasing, matching `monitoring_service`; its one caller in `main.py`'s lifespan already awaits. **5.** Confirmed and documented: the manual `POST /api/canary/run-cycle` path DOES drive the alert sink (`run_cycle` → `_run_cycle_inner` → `CanaryAlerts.emit_transition`), so a transition caught by an admin cycle alerts and the leader's next cycle correctly sees continuing-red. No lost alert. Tests: `FakeRedis` grows a client-aware `register_script`. That is load-bearing, not plumbing — every lease mutation is Lua now, and `_try_acquire_leadership` fails OPEN, so a fake that raised on `register_script` made every non-leader return True (the double-running under test) while the suite still read green. It caught 8 failures here. The two TTL tests asserting the retired single-TTL design are replaced by ones pinning the new split, including that `ttl < interval` is deliberate so nobody "fixes" it back.
|
All five addressed in 1. Non-atomic release — taken. Both writes are Lua compare-and-{delete,expire} through the existing 2 — taken, and merged with the mid-cycle residual I'd raised separately, because they turned out to be one bug. A post-cycle refresh alone doesn't close it: a cycle overrunning the TTL still lapses, and shortening the TTL makes the mid-cycle exposure worse. The root cause is one TTL answering two questions that want opposite values. Added beyond what you asked, because the heartbeat introduces it: 3 — taken. Sentence at 4 — taken. 5 — verified, and the answer is yes. 6 — filed as #2053. Sharper than "retention may absorb it": the doubling stops when eu2 updates past #1881, so if those rows fall inside M7's window the baseline and the soak get measured under different multipliers — which manufactures an apparent drop in violations at exactly the moment pull mode switches on. Cheapest fix is probably to start the baseline clock after the update rather than dedupe, but that constrains scheduling, so it should be a decision. One test note worth your attention. I also confirmed Suite: |
3 participants
Closes #1881.
Note on scope: the issue is a pair, and part 1 already landed as #1876 (
docker-compose.prod.ymlnow forwardsCANARY_ENABLED/CANARY_SLACK_WEBHOOK_URL, guarded bytests/unit/test_canary_env_prod_parity.py). This PR is part 2, so merging it closes the issue for both.That ordering matters, because it is the one the issue warned about: "shipping the packaging fix alone converts a dormant bug into a live one." The harness has been enabled on prod compose without a leader lease since #1876, so this is not a hypothetical.
The live state, measured on eu2
uvicorn main:app ... --workers 2canary_violationsrows in 24h, double-persistedWhy nothing prevented it
The FastAPI lifespan calls
canary_service.start(), so every uvicorn worker starts its own loop. The service's only mutual exclusion isself._lock = asyncio.Lock(), which guards re-entrancy inside one process and says nothing about cross-worker exclusion. Reading anasyncio.Lockin a service class as evidence of single-instance-ness is exactly the mistakelearnings.md2026-07-29 records — it is a prompt to ask where the leader lease is, not proof that there is one.The concrete cost: R-01
docker execs into every running agent container each cycle, so the fleet is probed twice per 5 min per agent by the subsystem meant to observe it unobtrusively; every violation inserts twice; andcanary:last_cycle_at,canary:last_cycle_red, E-02'scanary:e02:terminal_seenand H-01'scanary:h01:suspect_sinceeach have two independent writers.The fix
The scheduled loop runs only while it holds a Redis
canary:leaderlease — mirroringmonitoring:leader(#1464) andopqueue:leader(#1632):SET NXfor a single atomic winner, TTL refresh only when the stored id is our own (so a worker can never steal or clobber a sibling's lease), best-effort release onstop()for instant handoff. Every worker still runs its loop and re-evaluates leadership each cycle rather than once at startup, so a dead leader's lease expires and a sibling takes over with no restart.Two deliberate departures from the precedents, both for the same underlying reason — a duplicate canary cycle is not inert the way a doubled breaker feed (#1464) or an on-conflict create (#1632) is.
1. The TTL carries a floor.
interval * 3is the right scaling for a leader whose work scales with its interval. A canary cycle's cost does not: it is dominated by R-01'scontainer.exec_runsweep across every running agent container, which is bounded by no timeout and scales with fleet size, not with how often we look. The TTL is refreshed once at the top of a cycle, so it must outlast one worst-case cycle plus the inter-cycle sleep — otherwise the lease lapses mid-cycle, a sibling grabs it, leadership flaps, and we are back to the concurrent probing the lease exists to remove. The floor is set to whatinterval * 3already yields at the default 300s interval, so the default is unchanged and it only binds at a shorter one.2. Fail-open is kept, but not on the precedents' reasoning.
monitoring_servicecan fail open because a doubledrecord_failure()lands in a breaker that is itself fail-open;operator_queue_servicecan because duplicate creates dedupe on conflict. Neither argument transfers here. We fail open anyway, because the alternative is strictly worse: this is the one subsystem whose purpose is noticing that something went quiet. H-01 exists because a blind collector reports green — and a canary that stops running is that same silent-green one level up, where no invariant can catch it, because invariants only run inside the thing that isn't running. A fail-closed lease would let a Redis blip stop the watcher on every worker at once with nothing saying so. Duplicated probes are noisy and visible; silence is not. A Redis outage is also already a degraded state the harness announces (sources_unavailable; H-01 fires unconfirmed on an unreadable marker) — failing closed would suppress exactly those paths.What the lease does NOT buy — and what therefore did not change
It is best-effort, and the fail-open above re-opens the multi-writer window precisely when Redis is down. H-01's
CONFIRMATION_MIN_SECONDSand R-01'sDWELL_SECONDSelapsed-wall-clock gates are therefore untouched. Their docstrings asserted — correctly at the time — that the service holds no lease; those are corrected rather than deleted, with the reason they stay load-bearing spelled out so the next reader does not "simplify" either back to a cycle count.There is a second, more fundamental reason than the fail-open window: both gates ride out a real-time transient (a container finishing teardown; a
claudechild awaiting its parent'swait()). That is a single-worker property, so a cycle count was never the right unit at any worker count.One knock-on is recorded at R-01's
_MAX_OBSERVATION_GAP_SECONDS: a leader failover leaves up to ~1200s (TTL + interval) with nobody cycling, exceeding that 600s window and restarting the dwell. Correct — a crashed leader is a genuine observation outage by that constant's own rule — and restarting is the fail-safe direction, so the constant is documented, not widened.run_cycle()is deliberately not gated. The lease belongs to the scheduled path, which is what runs unattended in every worker.POST /api/canary/run-cyclelands on whichever worker uvicorn routes it to, so gating there would make an explicit admin request return an empty result about half the time under--workers 2— structurally identical to a green cycle, the exact ambiguity theskipped/409 contract was added to remove. Non-leaders log on the leadership transition only; narrating idleness every 5 minutes is how real canary output gets tuned out.Acceptance criteria
CANARY_ENABLED/CANARY_SLACK_WEBHOOK_URLforwarded indocker-compose.prod.yml— landed in fix(canary): wire CANARY_ENABLED/_SLACK_WEBHOOK_URL into prod backend env #1876canary_serviceacquires a Redis leader lease before running a cycle —SET NX, TTL a multiple of the interval (with a floor), own-lease-only refresh, fail-open to leader when Redis is downarchitecture.mdBackground Services +requirements/infrastructure.md§31 note the lease, matching how bug: fleet-health monitoring loop runs once per uvicorn worker — duplicate health checks and doubled circuit-breaker failure feed #1464/bug: operator-queue create path has no rate limit or size caps — flooding/operator-fatigue surface (blocking pull default-ON) #1632 are documentedTests
tests/unit/test_1881_canary_leader_lease.py— 23 tests. Undertests/unit/deliberately: the 3,700-line canary suite attests/root is collected by no CI workflow (#2037), so a guard beside it would never go red.Covers the lease mechanics (single winner, refresh keeps leadership, refresh actually re-arms the TTL), release semantics (immediate handoff, own-lease-only,
stop()releases, never raises), failover on TTL expiry, both fail-open arms (RedisNoneand Redis raising), TTL sizing (outlasts a cycle + sleep; floor is interval-independent), transition-only logging,run_cyclestaying ungated, and the key's namespace.Two behavioural guards worth calling out:
collect_snapshot— the saving is the probe, not the bookkeeping. This drives the realrun_cycle/_run_cycle_innerpath with db/Redis/sink stubbed, and carries aleader=Truepositive control:_loopswallows every cycle exception, so a raise-on-call probe would be silently eaten and pass vacuously with or without the gate.Meta-checked: removing the gate from
_loopturnstest_loop_runs_cycle_only_when_leader[False]andtest_non_leader_never_collects_a_snapshot[False]red, and only those.Full unit suite: 7823 passed, 18 skipped (baseline 7801 + these tests).
No schema, endpoint, config or compose change.