Circuit route propagation overwrites etcd from stale snapshots; replace the in-process lock with fenced conditional writes
- Dominant language
- Python
- Stars
- 670
- Forks
- 183
- Avg merge
- 17h 7m
- Merged PRs (30d)
- 358
Description
BA-5499 introduced a per-circuit asyncio.Lock in CircuitManager to stop a stale in-memory circuit.route_info snapshot from overwriting etcd service keys. The lock does not prevent that race. This issue replaces it with a fencing-token based conditional etcd write.
Backport target: 26.4. origin/26.4 carries the same code shape (5 _circuit_locks references in coordinator/types.py, atomic_replace_prefixes in common/etcd.py), so the fix is cherry-pickable. All parts ship as ONE PR so the backport stays a single cherry-pick; splitting them would force three ordered cherry-picks (migration first).
== Why the lock does not work ==
1. The critical section is drawn in the wrong place. This is a read-modify-write whose read sits outside the lock. services/endpoint.py:93 calls repository.update_routes(items), which commits and closes the DB transaction before returning the result.circuit snapshot; the lock is only taken afterwards inside update_circuit_routes_bulk (types.py:210-212). Even in a single process:
T1: update_routes commits -> routes=[R1], snapshot S1 (no lock held)
T2: register_routes commits -> routes=[R1,R2], snapshot S2 (no lock held)
T2: acquires lock -> etcd = [R1,R2] -> releases
T1: acquires lock ~~> etcd = [R1] <~~ R2 lost
DB says [R1,R2], etcd says [R1] - exactly the state BA-5499 reported. on_reconcile_traefik_routes is worse: server.py:582-584 reads every circuit at once and writes them one by one, so later circuits use an even older snapshot.
2. The lock scope does not match the shared resource. The shared resource is etcd (global); the lock is process-local. The coordinator is designed for multiple instances: leader election at server.py:378-408 (key leader:appproxy:coordinator), a consumer group at defs.py:14 with dispatcher.consume at server.py:604-612, and a PgAdvisoryLock factory. The etcd writers live in different processes: REST handlers (api/endpoint.py:137-211 -> services/endpoint.py:112,176,240), api/proxy.py:196 initialize_circuits, api/circuit_v2.py:78,112 unload_circuits all run on whichever instance the load balancer picked, while on_reconcile_traefik_routes runs every 30s on an arbitrary member of the consumer group.
3. Two etcd write paths were never covered by the lock at all: initialize_traefik_circuits (types.py:130) and reconcile_traefik_etcd_state (types.py:302). The latter also deletes stale prefixes, and because it works from a pre-read DB snapshot it can delete keys for a circuit created after that snapshot.
Side defect removed by this change: _release_circuit_lock (types.py:115-116, 268) pops the lock regardless of waiters, so a woken waiter and a newly arriving caller end up holding two different asyncio.Lock objects and both enter the critical section.
Note that what actually limits the damage today is the 30s reconcile loop (e0a2668df, added six days AFTER the lock), not the lock. But reconcile is itself a stale-snapshot writer, so wrong routing can persist for a cycle or two.
== Design: fencing token ==
mod_revision-based CAS is not available: etcd-client-py 0.5.1 exposes only get/get_prefix/keys_prefix/put/delete/txn on Communicator, with no revision accessor on read responses. Extending the binding would mean a separate repo and release cycle.
Instead use a fencing token with the DB as the authority. Conditional etcd v3 transactions already have a precedent in this codebase: AsyncEtcd.replace() (common/etcd.py:678-706) uses .when([Compare.value(...)]) plus result.succeeded(). Compare and CompareOp are already imported at common/etcd.py:45-55; the binding provides value / version / create_revision / mod_revision / lease / with_prefix / with_range and EQUAL / GREATER / LESS / NOT_EQUAL (verified at runtime).
1. Add a monotonically increasing integer Circuit.route_revision (default 0, non-null), bumped inside the same DB transaction that changes route_info. updated_at is not usable: it is a Python-side datetime.now(UTC) (models/circuit.py:247) and is exposed to clock skew across instances.
2. When writing a circuit subtree to etcd, also record its route_revision. Keep the revision key OUTSIDE the prefix Traefik reads (e.g. bai_meta/{circuit_id}/route_revision) so the Traefik etcd provider never parses an unknown key.
3. Guard the write transaction with Compare.value(revision_key, CompareOp.LESS, ). Fixed-width zero padding makes bytewise lexicographic comparison agree with numeric comparison. A missing key compares as empty, so the first write passes.
4. If succeeded() is False, a newer writer already applied its state: skip without retrying. Stale writers stand down on their own, so there is no retry loop and no livelock.
== Scope ==
1. common/etcd.py - add an optional compare-condition parameter to atomic_replace_prefixes (497-543) and return whether the transaction committed. Today it calls EtcdTransactionAction().and_then(actions).or_else([]) with an empty .when([...]), so it is atomic but unconditional. Calls that pass no condition must keep the current behaviour, because manager/services/manager_admin/service.py:134 relies on it.
2. coordinator/models/circuit.py plus a new alembic revision under coordinator/models/alembic/versions/ - add the route_revision column. The migration must be idempotent on both main and 26.4, following src/ai/backend/manager/models/alembic/README.md.
3. coordinator/repositories/endpoint.py - bump route_revision in the same DB transaction on every path that changes route_info, and reflect it on the returned circuit object: sync_endpoints (162), update_routes (213), register_routes (307), unregister_routes (417).
4. coordinator/types.py - delete _circuit_locks / _get_lock / _release_circuit_lock / circuit_lock (108-121) and their call sites (212, 260, 268), and guard etcd writes with the conditional transaction plus route_revision. Include the two currently unguarded paths, initialize_traefik_circuits (130) and reconcile_traefik_etcd_state (302).
5. Tests - test_circuit_locking.py assumes the lock exists and is replaced by fencing coverage; remove the lock monkeypatch at test_reconcile_traefik_routes.py:90.
Story points are capped at 2 by convention, but the real size is about 3-4 days. The work is kept in one issue so the 26.4 backport remains a single cherry-pick.
== Success Criteria ==
Conditional etcd write
- [ ] No condition passed: commits as before and returns True
- [ ] Condition passed and satisfied: commits, returns True, subtree replaced
- [ ] Condition passed and not satisfied: does not commit, returns False, existing keys untouched
- [ ] Condition key absent: CompareOp.LESS passes so the first write succeeds
- [ ] Announcement update in manager_admin/service.py:134 behaves unchanged
route_revision
- [ ] Migration applied: existing circuit rows get route_revision = 0
- [ ] Downgrade drops the column
- [ ] Migration is idempotent on both main and 26.4 (re-running does not fail)
- [ ] route_revision increases by exactly 1 after each of update_routes, register_routes, unregister_routes, sync_endpoints
- [ ] A no-op call that does not actually change route_info does not increase it
Lock removal and fencing
- [ ] No asyncio.Lock reference remains in coordinator/types.py
- [ ] Write with a stale revision: etcd transaction rejected, existing keys kept, skip logged
- [ ] Write with the newest revision: subtree replaced successfully
- [ ] Two concurrent updates propagated in reversed order: final etcd state matches the higher revision (reproduction scenario)
- [ ] unload_circuits also removes the revision key
- [ ] reconcile_traefik_etcd_state does not delete keys of a circuit created after its snapshot
- [ ] initialize_traefik_circuits also goes through the conditional write
- [ ] The revision key lives outside the Traefik-read prefix and does not affect Traefik config parsing
Common
- [ ] test_circuit_locking.py replaced by fencing coverage and the lock monkeypatch at test_reconcile_traefik_routes.py:90 removed
- [ ] pants test passes for affected packages
- [ ] The 26.4 backport branch passes the same tests
JIRA Issue: BA-7121
Contributor guide
Research direction
Start with common/etcd.py:497-543 and its AsyncEtcd.replace() precedent, then inspect the route_revision paths in coordinator/models/circuit.py, coordinator/repositories/endpoint.py, and coordinator/types.py. Replace the lock-based coverage with conditional writes, add the idempotent migration, and use the listed fencing and reconciliation tests to verify stale writers are rejected and newer state is preserved.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend, databases, distributed-systems
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Clearly specified
- Newbie friendliness
- 48/100