feat(cluster): worker registry persistence, split-brain protection, circuit breaker - #1928
feat(cluster): worker registry persistence, split-brain protection, circuit breaker#1928hognek wants to merge 2 commits into
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
![kilo-code-bot[bot]](https://avatars.github.com/in/2193792?s=60&v=4){kind=small}
| # taOS #640: split-brain protection — reject heartbeat from a worker | ||
| # that is echoing a different generation (another active controller). | ||
| # Legacy workers that don't send generation get a pass (None). | ||
| if generation is not None and generation != self._generation: |
There was a problem hiding this comment.
WARNING: Split-brain protection is bypassed for any worker that omits generation (legacy / not-yet-upgraded agents).
if generation is not None and generation != self._generation: means a worker that simply does not send generation (or sends None) is always accepted, regardless of which controller it last talked to. In a true split-brain scenario where a stale controller is still alive and its workers never adopt the new generation, those workers would be happily accepted here — defeating the protection for the entire legacy fleet. Consider rejecting workers that should be on a generation once the fleet is expected to send one, or at least logging/metrics on how many None-generation heartbeats are accepted so the protection isn't silently inert.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| logger.info(f"Worker registered: {info.name} ({info.platform}, {len(info.capabilities)} capabilities)") | ||
|
|
||
| # taOS #640: persist worker to SQLite so the registry survives restarts. | ||
| if self._registry_store is not None: |
There was a problem hiding this comment.
WARNING: register_worker performs no generation check, so split-brain protection only covers heartbeat, not the re-enroll path.
The new generation guard lives only in heartbeat() (line 248). A worker that re-registers via register_worker (the normal re-enroll flow on worker boot) is accepted unconditionally and persisted, even if it was last talking to a now-stale controller. When a controller restarts, its generation increments, but a worker that re-registers (instead of heartbeating) is admitted regardless — the exact split-brain case this PR targets. Either pass/verify generation through register_worker too, or document why re-registration is exempt and why that can't race with a live stale controller.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| # taOS #640: persist updated worker state to SQLite. | ||
| if self._registry_store is not None: | ||
| try: | ||
| asyncio.get_running_loop().create_task( |
There was a problem hiding this comment.
WARNING: Fire-and-forget create_task(self._persist_worker(worker)) has no error handling and is never awaited.
If _persist_worker raises inside the spawned task (e.g. DB locked, disk full, serialization error), the exception becomes an unretrieved task exception logged as a generic asyncio warning, and the worker's persisted state silently goes stale with no logger.exception like the other call sites have. It is also not awaited before stop(), so a controller shutdown can drop the final heartbeat's persistence. Wrap the task body in try/except (mirroring the register_worker/unregister_worker handling) and/or track it in self._background_tasks so it can be awaited/cleaned up on stop.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| first_failure_at: float | ||
|
|
||
|
|
||
| # Sensible defaults: 5 failures in 60 seconds trips the breaker. |
There was a problem hiding this comment.
SUGGESTION: Comment says "5 failures in 60 seconds" but DEFAULT_FAILURE_THRESHOLD = 3.
This is misleading to readers/maintainers; the actual default trips after 3 failures. Either update the comment to match the constant or change the constant if 5 was intended.
| # Sensible defaults: 5 failures in 60 seconds trips the breaker. | |
| # Sensible defaults: 3 failures in 60 seconds trips the breaker. |
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| if self._db is None: | ||
| raise RuntimeError("WorkerRegistryStore not initialised") | ||
| await self._db.execute( | ||
| "UPDATE cluster_generation SET value = value + 1 WHERE id = 1" |
There was a problem hiding this comment.
SUGGESTION: increment_generation uses a separate UPDATE then SELECT, which is two round-trips and a non-atomic read of the value.
Use SQLite's RETURNING to get the new value in a single atomic statement, e.g. UPDATE cluster_generation SET value = value + 1 WHERE id = 1 RETURNING value. This removes the second query, avoids any window where another writer could shift the value between the UPDATE and the SELECT, and is supported by aiosqlite/SQLite 3.35+ (the same baseline the rest of the stores already rely on).
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit 9f1034e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 9f1034e)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (6 files)
Reviewed by hy3:free · Input: 41.8K · Output: 3.4K · Cached: 255.2K |
9f1034e to
46aa5a6
Compare
…ircuit breaker Fixes jaylfc#640. Four interconnected changes: 1. **Worker registry persistence** — New WorkerRegistryStore (BaseStore + SQLite) persists worker registrations so the in-memory dict survives controller restarts. On startup, loaded workers are marked 'stale' until they re-heartbeat; local worker is skipped during load. 2. **Split-brain protection via generation counter** — A generation counter (stored in the same SQLite table) is incremented on each controller start. Workers echo it in heartbeats; stale generations are rejected. Legacy workers that don't send generation get a pass (None). 3. **Circuit breaker for failing workers** — New FailureTracker with sliding window (default: 3 failures in 60s). TaskRouter skips tripped workers instead of serially blocking 60s * N_workers. Success clears the circuit. 4. **In-flight routing recovery** — Addressed by Fix 1: persisted workers survive controller restart; stale-until-heartbeat prevents routing to workers that haven't reconnected. Files: - NEW: tinyagentos/cluster/worker_registry_store.py - NEW: tinyagentos/cluster/failure_tracker.py - MODIFIED: tinyagentos/cluster/manager.py (init, start, heartbeat, register_worker, unregister_worker, _persist_worker, _load_persisted_workers) - MODIFIED: tinyagentos/cluster/router.py (circuit breaker skip/record) - MODIFIED: tinyagentos/app.py (wire new stores) - NEW: tests/test_cluster_persistence.py (24 tests: store, tracker, manager persistence, router circuit breaker) Tests: 79/79 pass (55 existing + 24 new cluster tests)
- failure_tracker: fix comment (3 failures, not 5) - manager: add generation check to register_worker (mirrors heartbeat guard) - manager: add debug log for None-generation heartbeats from legacy workers - manager: wrap heartbeat-persist fire-and-forget in try/except with background task tracking - worker_registry_store: use RETURNING for atomic increment_generation - routes: wire generation through WorkerRegister and HeartbeatBody models Rebased onto origin/dev (8b97eb5a). All 96 cluster/persistence/worker-protocol tests pass.
46aa5a6 to
64e753e
Compare
|
All 5 Kilo findings addressed + rebased onto origin/dev (8b97eb5a): WARNING (3 fixed):
SUGGESTION (2 fixed): Routes wired: 96/96 cluster/persistence/worker-protocol tests pass. |
1 participant
Closes #640.
Four interconnected changes for multi-node reliability:
1. Worker registry persistence — New
WorkerRegistryStore(SQLite-backed viaBaseStore) persists worker registrations so the in-memoryClusterManager._workersdict survives controller restarts. On startup, loaded workers are marked"stale"until they re-heartbeat. The"local"worker is skipped during load (it re-registers on every boot).2. Split-brain protection via generation counter — A generation counter (stored in the same SQLite table) is incremented on each controller
start(). Workers echo it in heartbeats; stale generations are rejected. Legacy workers that don't send generation get a pass (None).3. Circuit breaker for failing workers — New
FailureTrackerwith sliding window (default: 3 failures in 60s).TaskRouterskips tripped workers instead of serially blocking60s * N_workers. Success clears the circuit; failures recorded per-worker.4. In-flight routing recovery — Addressed by Fix 1: persisted workers survive controller restart; stale-until-heartbeat prevents routing to workers that haven't reconnected.
Files changed
tinyagentos/cluster/worker_registry_store.py— SQLite store for worker records + generation countertinyagentos/cluster/failure_tracker.py— Sliding-window circuit breakertinyagentos/cluster/manager.py— init/start/heartbeat/register/unregister with persistence, generation counter, failure_tracker wiringtinyagentos/cluster/router.py— Circuit breaker skip + record on failure/successtinyagentos/app.py— Wire new stores into ClusterManager creationtests/test_cluster_persistence.py— 24 tests covering store CRUD, generation counter, stale-on-load, heartbeat revival, split-brain rejection, circuit breaker tripping/skipping/clearingTesting
Backward compatibility
ClusterManager.__init__accepts new optionalworker_registry_storeandfailure_trackerparams — defaults to Noneheartbeat()accepts new optionalgenerationparam — None = legacy worker, acceptedstore.init()failure_tracker is None(no circuit breaker)