-
-
Notifications
You must be signed in to change notification settings - Fork 35
fix(cluster): release GPU leases on worker unregister (fixes #1705) #1726
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -259,8 +259,36 @@ def heartbeat( | |
| pass # No running loop (e.g. in sync tests) — skip gracefully | ||
| return True | ||
|
|
||
| def unregister_worker(self, name: str) -> bool: | ||
| return self._workers.pop(name, None) is not None | ||
| async def unregister_worker(self, name: str) -> bool: | ||
| """Remove a worker and all of its active GPU leases. | ||
|
|
||
| When a worker is explicitly unregistered (admin action), every | ||
| active lease tied to the worker's resources is released so that | ||
| those resource slots become available for new claims immediately. | ||
| This mirrors the lease-release logic in ``_monitor_loop`` for the | ||
| heartbeat-timeout path (taOS #1705). | ||
| """ | ||
| worker = self._workers.get(name) | ||
| if worker is None: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SUGGESTION: Unused local variable The result of if name not in self._workers:
return FalseReply with |
||
| return False | ||
|
|
||
| # Release all active leases for this worker's resources. | ||
| async with self._lease_lock: | ||
| lids: list[str] = [ | ||
| lid for lid, lease in self._leases.items() | ||
| if (parsed := self._parse_resource_id(lease.resource_id)) | ||
| and parsed[0] == name | ||
| ] | ||
| for lid in lids: | ||
| self._leases.pop(lid, None) | ||
| logger.debug( | ||
| "Lease %s released — worker '%s' unregistered", | ||
| lid, name, | ||
| ) | ||
|
|
||
| self._workers.pop(name, None) | ||
| logger.info("Worker '%s' unregistered — %d leases released", name, len(lids)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. WARNING: Worker pop happens outside The
Move Reply with |
||
| return True | ||
|
Comment on lines
+271
to
+291
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
# Locate the relevant file and surrounding symbols.
git ls-files tinyagentos/cluster/manager.py tests/test_cluster.py tests/test_leases.py
printf '\n== manager.py outline ==\n'
ast-grep outline tinyagentos/cluster/manager.py --view expanded || true
printf '\n== unregister_worker implementation ==\n'
sed -n '230,290p' tinyagentos/cluster/manager.py
printf '\n== lease lock usages ==\n'
rg -n "_lease_lock|unregister_worker|claim_lease|_workers\.pop|_workers\.get" tinyagentos/cluster/manager.py
printf '\n== relevant tests ==\n'
sed -n '1,260p' tests/test_cluster.py
printf '\n--- test_leases ---\n'
sed -n '1,260p' tests/test_leases.pyRepository: jaylfc/taOS Length of output: 25349 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '== worker lock / mutations ==\n'
rg -n "_workers\.|_worker_lock|asyncio\.Lock|register_worker|unregister_worker" tinyagentos/cluster/manager.py tinyagentos/cluster -g '*.py'
printf '\n== monitor loop lease-release block ==\n'
sed -n '520,560p' tinyagentos/cluster/manager.py
printf '\n== API routes touching unregister ==\n'
rg -n "unregister_worker|/workers" tinyagentos -g '*.py'Repository: jaylfc/taOS Length of output: 8345 Make 🤖 Prompt for AI Agents |
||
|
|
||
| def get_workers(self) -> list[WorkerInfo]: | ||
| return list(self._workers.values()) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SUGGESTION: Test name claims "atomically" but only verifies lock-blocking, not actual atomicity
The test
test_unregister_releases_leases_atomicallyonly holds_lease_lockfrom the test side and asserts that the unregister coroutine is blocked. It does not actually race a concurrentclaim_leaseagainstunregister_workerto verify end-to-end atomicity (e.g., that a claim started just before unregister is either fully created-and-removed, or never created and not leaked).Either rename the test to
test_unregister_blocks_on_lease_lockfor accuracy, or strengthen it to spawn aclaim_leasetask concurrently withunregister_workerand assert the post-state is consistent (no leaked lease, worker gone).Reply with
@kilocode-bot fix itto have Kilo Code address this issue.