fix(worker): manifest crash-loop follow-up — availability/liveness decoupling, loaded_models status, unreachable mappings#1870
Conversation
…coupling, loaded_models status, unreachable mappings Fixes jaylfc#1765 (remaining items after hotfix jaylfc#1767): MEDIUM: Emit synthetic backend entries for manifest-declared software types that have no running (probed) counterpart. This decouples availability from liveness — stopped-but-installed backends declared in the worker manifest now advertise their available models to the controller. Previously, manifest entries vanished from heartbeats when their backend wasn't running. Low: Drop kokoro/whisper from SOFTWARE_TO_BACKEND_TYPE. These mappings were unreachable because no probe candidates exist for those backend types. Adding probes would require backend support that isn't implemented; keeping the mappings invites confusion. Nit: Use loaded_models (from /api/ps, in-memory) instead of the full models catalog (from /api/tags, on-disk) when determining whether a manifest-declared model is 'loaded'. Previously, models merely present on disk were reported as 'loaded' rather than 'available'. Tests: 6 new tests (4 for stopped-backend emission, 2 for loaded_models status). All 150 worker+manifest tests pass.
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughWorker backend detection now enriches manifest models using loaded-model residency, synthesizes stopped entries for manifest-only backend types, and preserves fail-soft behavior. Manifest software mappings and related test fixtures are updated accordingly. ChangesManifest backend enrichment
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant WorkerAgent
participant BackendAPI
participant WorkerManifest
WorkerAgent->>BackendAPI: Probe running backends
BackendAPI-->>WorkerAgent: Return models and loaded_models
WorkerAgent->>WorkerManifest: Load manifest entries
WorkerManifest-->>WorkerAgent: Return software and model declarations
WorkerAgent->>WorkerAgent: Mark loaded or available models
WorkerAgent->>WorkerAgent: Add stopped entries for unprobed types
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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 |
|
|
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? |
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| backends.append({ | ||
| "name": bt, | ||
| "type": bt, | ||
| "url": "", |
There was a problem hiding this comment.
WARNING: Synthetic stopped backend uses url: "", which can propagate into an empty advertised worker URL.
build_heartbeat_payload falls back to backends[0]["url"] (agent.py:521) when no advertise_url/TAOS_ADVERTISE_IP is set. When a worker declares manifest models but has no running backend, the synthetic stopped entry is the only element in backends, so backends[0]["url"] is "" and the worker advertises an empty URL to the controller.
This is exactly the manifest-only scenario your new test test_manifest_only_backend_creates_stopped_entry exercises. Consider skipping the url fallback for synthetic/stopped backends (e.g. fall back to get_worker_url() when the selected backend url is falsy) so the worker still advertises a reachable address.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| }) | ||
| if available: | ||
| backends.append({ | ||
| "name": bt, |
There was a problem hiding this comment.
SUGGESTION: Synthetic backend name is just bt (e.g. "llama-cpp"), while probed backends use f"{backend_type}:{port}" (agent.py:150). This gives the same logical backend two different identities depending on liveness.
The scheduler catalog keys on name (scheduler/backend_catalog.py), so a backend that flips between running and stopped could create conflicting/duplicate catalog entries. Consider keeping name consistent with the probed scheme (e.g. include a sentinel port) or documenting the stopped-backend naming convention explicitly.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| "models": [], | ||
| "loaded_models": [], | ||
| "status": "stopped", | ||
| "kv_quant_support": {"k": ["fp16"], "v": ["fp16"], "boundary": False}, |
There was a problem hiding this comment.
SUGGESTION: kv_quant_support is hardcoded to the fp16 default for stopped/synthetic backends. If the declared backend type does not actually support fp16 KV quant, detect_kv_quant_support (agent.py:383) will over-report cluster-wide KV-quant capability because it unions kv_quant_support across all backends.
For a backend that is not currently running, KV-quant support is genuinely unknown. Consider omitting kv_quant_support (or marking it clearly unknown) for synthetic entries rather than asserting a concrete (possibly wrong) capability.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: No New Issues Found | Recommendation: Merge Overview
Files Reviewed (2 files)
Incremental update vs. previous review at Previous Review Summaries (3 snapshots, latest commit 0e52d44)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 0e52d44)Status: 1 Issue Found | Recommendation: Optional follow-up Overview
Issue Details (click to expand)SUGGESTION
Files Reviewed (3 files)
Incremental update vs. previous review at Fix these issues in Kilo Cloud Previous review (commit dc9ab0b)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (1 files)
Incremental update vs. previous review at Fix these issues in Kilo Cloud Previous review (commit 6a511e4)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Reviewed by hy3:free · Input: 78.6K · Output: 2.8K · Cached: 231.9K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tinyagentos/worker/agent.py (1)
184-202: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve loaded status for running llama.cpp backends.
_probe_loaded_models()returns[]for llama.cpp because/v1/modelsalready represents its active model. Using onlyloaded_modelstherefore changes every running llama.cpp manifest model from"loaded"to"available".Proposed fix
- probed_loaded_names = { - m.get("name", "") for m in backend.get("loaded_models", []) - } + residency_models = ( + backend.get("loaded_models", []) + if backend_type in ("rkllama", "ollama") + else backend.get("models", []) + ) + probed_loaded_names = { + m.get("name", "") for m in residency_models + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tinyagentos/worker/agent.py` around lines 184 - 202, Update the model status logic around _probe_loaded_models() so running llama.cpp backends treat the active /v1/models result as loaded, even when loaded_models is empty. Preserve the resident loaded_models behavior for other backend types, and ensure matching manifest model_ids continue to receive "loaded" rather than "available".
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tinyagentos/worker/agent.py`:
- Around line 254-257: Update the synthetic backend construction in the
stopped-manifest fallback so its advertised “url” uses get_worker_url() instead
of an empty string. Preserve the existing backend name and type values, ensuring
downstream worker URL selection does not choose an empty URL.
---
Outside diff comments:
In `@tinyagentos/worker/agent.py`:
- Around line 184-202: Update the model status logic around
_probe_loaded_models() so running llama.cpp backends treat the active /v1/models
result as loaded, even when loaded_models is empty. Preserve the resident
loaded_models behavior for other backend types, and ensure matching manifest
model_ids continue to receive "loaded" rather than "available".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b1567000-4317-47bb-8433-b5f1dcbde39c
📒 Files selected for processing (4)
tests/test_worker.pytests/test_worker_manifest.pytinyagentos/worker/agent.pytinyagentos/worker/worker_manifest.py
💤 Files with no reviewable changes (1)
- tinyagentos/worker/worker_manifest.py
- url: None instead of empty string for synthetic backends (no endpoint)
- kv_quant_support: {} instead of hardcoded fp16 (unknown, not probed)
- Add comments explaining portless name and empty KV quant dict
Resolves Kilo WARNING + 2 SUGGESTION on PR jaylfc#1870
|
Addressed Kilo findings (all 3) in commit dc9ab0b:
All 51 worker+manifest tests pass. CodeRabbit and CLA are green. |
|
Deep-reviewed this at code level @hognek. The availability/liveness decoupling, the
worker_url = self.advertise_url or adv_url or (backends[0]["url"] if backends else self.get_worker_url())
That is exactly issue #1765's item-2 case: a worker whose only backend is installed-but-stopped, no advertise IP. It doesn't crash-loop under systemd anymore, but it converts the crash into a silent registration-failure loop, so the loop moves rather than being eliminated for that config. Heads up: the earlier note that changing Suggested fix - pick the url from live backends only, then fall back: live_url = next((b["url"] for b in backends if b.get("url")), None)
worker_url = self.advertise_url or adv_url or live_url or self.get_worker_url()And a regression test asserting Non-blocking: (1) the synthetic Everything else looks good - fix the registration url and I'll merge. Nice work on the decoupling. |
…c/stopped When detect_backends returns only synthetic stopped backends (all with url=None), the old backends[0]['url'] would produce worker_url=None, causing a silent registration-failure loop. Use next() with a generator to pick the first live backend URL, falling through to get_worker_url() when none found. Also replace em dash with ASCII double-hyphen in worker_manifest.py docstring per review. Regression test: TestRegistrationUrlFallback verifies the payload URL is a real http:// URL, not None.
House convention: no em dashes in repo text. Maintainer polish on @hognek's worker crash-loop follow-up.
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_worker.py (1)
304-305: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
.kwargsfor accessing mock call arguments.Using the
.kwargsattribute is generally more readable and idiomatic in modern Python than using index-based access (call_args[1]).♻️ Proposed refactor
- call_args = mock_client.post.call_args - body = _json.loads(call_args[1]["content"]) + call_args = mock_client.post.call_args + body = _json.loads(call_args.kwargs["content"])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_worker.py` around lines 304 - 305, Update the mock argument access near call_args to use mock_client.post.call_args.kwargs instead of positional indexing through call_args[1], while preserving the existing content extraction and JSON parsing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_worker.py`:
- Around line 304-305: Update the mock argument access near call_args to use
mock_client.post.call_args.kwargs instead of positional indexing through
call_args[1], while preserving the existing content extraction and JSON parsing
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4619532c-7311-4b70-9a4a-c463713bc3ed
📒 Files selected for processing (4)
tests/test_worker.pytests/test_worker_manifest.pytinyagentos/worker/agent.pytinyagentos/worker/worker_manifest.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tinyagentos/worker/worker_manifest.py
- tests/test_worker_manifest.py
- tinyagentos/worker/agent.py
2 participants
Summary
Fixes the remaining items from #1765 (follow-up to merged PR #1762, hotfix #1767 already addressed the crash-loop).
Changes
MEDIUM: Availability/liveness decoupling
detect_backends()now emits synthetic backend entries for manifest-declared software types that have no running (probed) counterpart. Stopped-but-installed backends declared in the worker manifest now advertise their available models to the controller. Previously, manifest entries vanished from heartbeats when their backend wasn't running.Low: Drop unreachable mappings
Removed
kokoroandwhisperfromSOFTWARE_TO_BACKEND_TYPE. These were unreachable because no probe candidates exist for those backend types. Adding probes would require backend support that isn't implemented.Nit: loaded_models for status
Changed
detect_backends()enrichment to useloaded_models(from /api/ps, in-memory) instead of the full models catalog (from /api/tags, on-disk) when determining whether a manifest-declared model is 'loaded'. Previously, models merely present on disk were reported as 'loaded' rather than 'available'.Files changed
tinyagentos/worker/worker_manifest.py— drop kokoro/whisper from SOFTWARE_TO_BACKEND_TYPEtinyagentos/worker/agent.py— loaded_models-based status + synthetic backend entries for stopped backendstests/test_worker_manifest.py— updated mapping teststests/test_worker.py— 6 new tests (4 stopped-backend emission + 2 loaded_models status)Tests
All 150 worker+manifest tests pass (0 failures).
Summary by CodeRabbit
New Features
Bug Fixes
Tests