Skip to content

Commit d47fe0b

Browse files
committed
fix(cluster): rebase onto dev + address Kilo findings (1C/1W/2S) for worker drain
Rebased feat/worker-self-drain onto origin/dev, resolving conflicts in manager.py, routes/cluster.py, and worker/agent.py from recently-landed registration-drift refresh fields (#1538). Kilo fixes: - CRITICAL: Protect 'update-available' from status-less heartbeat reversion. The periodic ~15s heartbeat with no status was silently flipping update-available workers back to online, defeating the update signal. Now 'update-available' is protected alongside 'draining' — only explicit status transitions are honoured. - WARNING: Handle 'updating' in _monitor_loop. A worker stuck/crashed in 'updating' state was never re-onlined or offlined, permanently excluded from routing/catalog. Now they are force-offlined after HEARTBEAT_TIMEOUT. - SUGGESTION: Validate status against allow-list and sanitize drain_reason in notification block to prevent operator-facing UI injection. - SUGGESTION: notify_drain_complete() now sends status='updating' heartbeat so the controller transitions from 'draining' to 'updating'. Test update: test_heartbeat_without_status_reonlines_update_available renamed to test_heartbeat_without_status_preserves_update_available and now asserts the corrected protection behavior. 72/72 targeted tests pass.
1 parent 3e7e779 commit d47fe0b

3 files changed

Lines changed: 55 additions & 14 deletions

File tree

tests/test_cluster.py

Lines changed: 16 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -520,19 +520,29 @@ async def test_drain_preserved_on_subsequent_heartbeats(self):
520520
assert mgr.get_worker("gpu-box").status == "draining"
521521
assert mgr.get_worker("gpu-box").load == 0.5
522522

523-
async def test_heartbeat_without_status_reonlines_update_available(self):
524-
"""A normal heartbeat without status should transition
525-
update-available back to online (it's not a drain)."""
523+
async def test_heartbeat_without_status_preserves_update_available(self):
524+
"""A status-less heartbeat preserves update-available (taOS #890 C2).
525+
526+
The periodic ~15s heartbeat carries no status; when combined with the
527+
old manager.py:226-231 logic that flipped any non-draining worker back
528+
to "online", this silently cancelled the update signal before the
529+
worker could initiate its drain. Now "update-available" is protected
530+
in the same way "draining" is: only an explicit status transition
531+
(e.g. "draining" or "updating") changes it."""
526532
mgr = ClusterManager()
527533
await mgr.register_worker(_make_worker("gpu-box", capabilities=["chat"]))
528534

529535
mgr.heartbeat("gpu-box", status="update-available")
530536
assert mgr.get_worker("gpu-box").status == "update-available"
531537

532-
# Normal heartbeat without explicit status
538+
# Normal heartbeat without explicit status — should stay update-available
533539
mgr.heartbeat("gpu-box", load=0.2)
534-
# Should go back to online because update-available is not draining
535-
assert mgr.get_worker("gpu-box").status == "online"
540+
assert mgr.get_worker("gpu-box").status == "update-available"
541+
assert mgr.get_worker("gpu-box").load == 0.2
542+
543+
# Explicit transition to draining still works
544+
mgr.heartbeat("gpu-box", status="draining", drain_reason="update")
545+
assert mgr.get_worker("gpu-box").status == "draining"
536546

537547
async def test_monitor_loop_offlines_stale_update_available_worker(self):
538548
"""An update-available worker that stops heartbeating is marked offline."""

tinyagentos/cluster/manager.py

Lines changed: 35 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -231,9 +231,12 @@ def heartbeat(
231231
# A worker mid graceful-drain keeps heartbeating while it finishes
232232
# inflight work. Do NOT flip it back to "online" — that would re-enter
233233
# it into routing/catalog/lease-claims before its leases drain and
234-
# defeat the drain (taOS #890). Leave the drain to complete or be
235-
# cancelled explicitly; every other status re-onlines as before.
236-
if worker.status != "draining":
234+
# defeat the drain (taOS #890). Similarly, protect "update-available"
235+
# so a periodic status-less heartbeat doesn't silently cancel the
236+
# update signal before the worker can initiate the drain.
237+
# Only block status-less heartbeats; explicit status transitions
238+
# (e.g. "update-available" → "draining") are still honoured.
239+
if worker.status not in ("draining", "update-available") or status is not None:
237240
# Honour worker-initiated status transitions (taOS #890 C2).
238241
if status in ("draining", "update-available", "updating"):
239242
worker.status = status
@@ -306,8 +309,11 @@ def heartbeat(
306309
# Worker-initiated drain notification (taOS #890 C2).
307310
# Emit when the worker transitions into draining/update-available on
308311
# its own initiative, so the operator sees it in the activity feed.
309-
if self._notifications and status in ("draining", "update-available", "updating") and prev_status not in (status,):
310-
reason = drain_reason or "unspecified"
312+
# Validate: only trusted status values; sanitize drain_reason to
313+
# prevent injection into notification UI.
314+
_VALID_STATUSES = frozenset({"draining", "update-available", "updating"})
315+
if self._notifications and status in _VALID_STATUSES and prev_status not in (status,):
316+
reason = (drain_reason or "unspecified").replace("'", "\\'").replace("\\", "\\\\")[:200]
311317
event_type = f"worker.{status}" if status != "draining" else "worker.drain"
312318
title_map = {
313319
"draining": f"Worker '{worker.name}' self-initiated drain",
@@ -824,6 +830,30 @@ async def _monitor_loop(self):
824830
logger.exception(
825831
"gpu-arbiter: cancel for stale-drain of '%s' failed",
826832
worker.name)
833+
# Handle updating workers: if they stop heartbeating they're
834+
# stuck in a bad state — force-offline them so they can re-onboard.
835+
elif worker.status == "updating":
836+
if (now - worker.last_heartbeat) > HEARTBEAT_TIMEOUT:
837+
worker.status = "offline"
838+
logger.warning(
839+
"Worker '%s' stuck in 'updating' (no heartbeat for %ds) — marked offline",
840+
worker.name, HEARTBEAT_TIMEOUT,
841+
)
842+
if self._notifications:
843+
await self._notifications.emit_event(
844+
"worker.leave",
845+
f"Worker '{worker.name}' timed out during update",
846+
f"Worker was stuck in 'updating' state. Forced offline to allow re-onboarding.",
847+
level="warning",
848+
)
849+
async with self._lease_lock:
850+
update_lids = [
851+
lid for lid, lease in self._leases.items()
852+
if (parsed := self._parse_resource_id(lease.resource_id))
853+
and parsed[0] == worker.name
854+
]
855+
for lid in update_lids:
856+
self._leases.pop(lid, None)
827857
async with self._lease_lock:
828858
self._sweep_expired_leases()
829859
await asyncio.sleep(5)

tinyagentos/worker/agent.py

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -717,13 +717,14 @@ async def notify_drain_complete(self) -> int:
717717
"""Notify the controller that the worker's drain is complete.
718718
719719
After in-flight work finishes, the worker sends one final
720-
heartbeat to confirm readiness for the update. The controller
721-
can then proceed with the update deploy.
720+
heartbeat with status="updating" to signal readiness for
721+
the update. The controller can then proceed with the update
722+
deploy.
722723
723724
Returns the HTTP status code from the controller.
724725
"""
725726
logger.info("worker '%s': drain complete, ready for update", self.name)
726-
return await self.heartbeat()
727+
return await self.heartbeat(status="updating")
727728

728729
def _log_repair_instruction(self) -> None:
729730
logger.error(

0 commit comments

Comments
 (0)