From 66ba7399859f50c73eaedf480f540f2d10292b2a Mon Sep 17 00:00:00 2001 From: abhay-codes07 Date: Fri, 24 Jul 2026 19:58:57 +0530 Subject: [PATCH 1/4] feat(server): add DELETE /v1/hosts/{host_id} to deregister a retired host (#2038) External hosts self-register via the daemon and become rows in the host store, but `server/routes/hosts.py` exposed only GET/POST, so there was no supported way to remove one. A permanently decommissioned machine lingered as an offline entry in every session-creation host picker, and the only workaround was hand-deleting the postgres row. The persistent teardown already exists: `HostStore.delete_host` unbinds any sessions still pointing at the host (nulls their `host_id`) and deletes the row (which also revokes its launch token). This wires it to a route so retiring a host has a first-class API and a clean IaC story. `DELETE /v1/hosts/{host_id}` mirrors `get_host`'s auth: 404 for an unknown host, 403 when the caller is not the owner. It refuses an online host (409) so an active machine is not pulled out from under running work, and refuses a server-managed sandbox host (409) whose lifecycle belongs to the server that created it. On success it returns 204 and the host is gone from the store and the pickers. Tests: removes an offline host (row and picker), 404 unknown, 409 online, 409 managed sandbox, 403 wrong owner, and that a bound session is unbound rather than left dangling at a deleted row. Each DELETE-path test fails against `main` with 405 (no route) and passes with it. Signed-off-by: abhay-codes07 --- omnigent/server/routes/hosts.py | 46 ++++++- tests/server/integration/test_hosts_api.py | 134 +++++++++++++++++++++ 2 files changed, 179 insertions(+), 1 deletion(-) diff --git a/omnigent/server/routes/hosts.py b/omnigent/server/routes/hosts.py index 2a72461113..3a5f2058f6 100644 --- a/omnigent/server/routes/hosts.py +++ b/omnigent/server/routes/hosts.py @@ -22,7 +22,7 @@ import secrets from typing import Any -from fastapi import APIRouter, HTTPException, Query, Request +from fastapi import APIRouter, HTTPException, Query, Request, Response from pydantic import BaseModel from omnigent.db.utils import now_epoch @@ -518,6 +518,50 @@ async def get_host(request: Request, host_id: str) -> dict[str, Any]: "runners": [], } + @router.delete("/hosts/{host_id}", status_code=204) + async def delete_host(request: Request, host_id: str) -> Response: + """Deregister (delete) a host the caller owns. + + Removes a retired self-registered host so it stops appearing in + session-creation pickers forever. Any sessions still bound to it + are unbound by the store (their ``host_id`` is nulled) and the + row is deleted, which also revokes the host's launch token. + + Refuses a host that is currently online (409): retire it after it + goes offline so an active machine is not pulled out from under + running work. Server-managed sandbox hosts are likewise refused + (409) — their lifecycle belongs to the server that created them, + and deleting only the row would orphan the live sandbox. + + :param request: The incoming request (for auth). + :param host_id: Host identifier, e.g. ``"host_a1b2c3d4..."``. + :returns: 204 No Content on success. + :raises HTTPException: 404 if the host does not exist, 403 if the + caller is not the owner, 409 if the host is online or + server-managed. + """ + # Same auth shape as get_host: require_user 401s unauthenticated + # callers when auth is configured; a None user_id means auth is + # disabled entirely (single-user server, reserved "local" owner). + user_id = require_user(request, auth_provider) + host = await asyncio.to_thread(host_store.get_host, host_id) + if host is None: + raise HTTPException(status_code=404, detail="host not found") + if user_id is not None and host.user_id != user_id: + raise HTTPException(status_code=403, detail="not your host") + if host.sandbox_provider is not None: + raise HTTPException( + status_code=409, + detail="server-managed host cannot be deregistered manually", + ) + if host_is_live(host): + raise HTTPException( + status_code=409, + detail="host is online; retire it after it goes offline", + ) + await asyncio.to_thread(host_store.delete_host, host_id) + return Response(status_code=204) + @router.get("/hosts/{host_id}/harnesses/{harness}/model-options") async def get_host_model_options( request: Request, diff --git a/tests/server/integration/test_hosts_api.py b/tests/server/integration/test_hosts_api.py index 46063de2ff..c8b7797ddf 100644 --- a/tests/server/integration/test_hosts_api.py +++ b/tests/server/integration/test_hosts_api.py @@ -310,6 +310,111 @@ async def test_get_host_404( assert resp.status_code == 404 +async def test_delete_host_removes_offline_host( + host_api_app: tuple[FastAPI, HostRegistry, HostStore, SqlAlchemyConversationStore], +) -> None: + """ + DELETE /v1/hosts/{id} removes a retired (offline) host so it stops + polluting the pickers, and the row is gone from the store. + + Regression for #2038: there was no API to deregister a + self-registered host, so a decommissioned machine lingered in every + host picker forever. + """ + app, _reg, host_store, _cs = host_api_app + host_store.upsert_on_connect(_HOST_ID, "retired-vm", "local") + host_store.set_offline(_HOST_ID) + + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + resp = await client.delete(f"/v1/hosts/{_HOST_ID}") + listing = await client.get("/v1/hosts") + + assert resp.status_code == 204, f"expected 204, got {resp.status_code}: {resp.text}" + # Row is actually gone: neither the store nor the picker sees it. + assert host_store.get_host(_HOST_ID) is None + assert listing.json()["hosts"] == [] + + +async def test_delete_host_404_unknown( + host_api_app: tuple[FastAPI, HostRegistry, HostStore, SqlAlchemyConversationStore], +) -> None: + """DELETE returns 404 for a host_id that does not exist.""" + app, _reg, _hs, _cs = host_api_app + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + resp = await client.delete("/v1/hosts/aababcc3941edb738172734a9ab7bb8c") + assert resp.status_code == 404 + + +async def test_delete_host_409_when_online( + host_api_app: tuple[FastAPI, HostRegistry, HostStore, SqlAlchemyConversationStore], +) -> None: + """ + DELETE refuses an online host with 409 and leaves it in place, so an + active machine is not pulled out from under running work. + """ + app, registry, host_store, _cs = host_api_app + _comm = await _connect_host(app, registry) + + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + resp = await client.delete(f"/v1/hosts/{_HOST_ID}") + + assert resp.status_code == 409, f"expected 409 for an online host, got {resp.status_code}" + # Untouched: the live host is still registered. + assert host_store.get_host(_HOST_ID) is not None + + +async def test_delete_host_409_managed_sandbox( + host_api_app: tuple[FastAPI, HostRegistry, HostStore, SqlAlchemyConversationStore], +) -> None: + """ + DELETE refuses a server-managed sandbox host with 409: its lifecycle + belongs to the server that created it, and deleting only the row + would orphan the live sandbox. + """ + app, _reg, host_store, _cs = host_api_app + managed_id = "b8a8862c405a01143b4373e2b155b02a" + host_store.register_managed_host( + host_id=managed_id, + name="sandbox-host", + user_id="local", + token="launch-token-secret", + provider="modal", + sandbox_id="sb-12345", + token_expires_at=int(time.time()) + 3600, + ) + + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + resp = await client.delete(f"/v1/hosts/{managed_id}") + + assert resp.status_code == 409, f"expected 409 for a managed host, got {resp.status_code}" + assert host_store.get_host(managed_id) is not None + + +async def test_delete_host_unbinds_bound_sessions( + host_api_app: tuple[FastAPI, HostRegistry, HostStore, SqlAlchemyConversationStore], +) -> None: + """ + Deleting a host unbinds any session still pointing at it: the + session's ``host_id`` is nulled rather than left dangling at a row + that no longer exists. + """ + app, _reg, host_store, conv_store = host_api_app + host_store.upsert_on_connect(_HOST_ID, "retired-vm", "local") + host_store.set_offline(_HOST_ID) + conv = conv_store.create_conversation(agent_id=None) + # workspace is required alongside host_id by the row check constraint. + conv_store.set_host_id(conv.id, _HOST_ID, workspace="/tmp/ws") + assert conv_store.get_conversation(conv.id).host_id == _HOST_ID + + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + resp = await client.delete(f"/v1/hosts/{_HOST_ID}") + + assert resp.status_code == 204 + unbound = conv_store.get_conversation(conv.id) + assert unbound is not None + assert unbound.host_id is None, "a deleted host must not leave sessions bound to a dead row" + + async def test_list_and_get_host_report_online_from_other_replica( host_api_app: tuple[FastAPI, HostRegistry, HostStore, SqlAlchemyConversationStore], db_uri: str, @@ -755,6 +860,35 @@ async def test_get_host_403_wrong_owner( ) +async def test_delete_host_403_wrong_owner( + multi_user_app: tuple[FastAPI, HostRegistry, HostStore, SqlAlchemyConversationStore], +) -> None: + """ + DELETE /v1/hosts/{id} returns 403 when the caller doesn't own the + host, and the host is left in place. + + If it returned 204, one user could deregister another user's host + out of their picker (and revoke its launch token). + """ + _app, _reg, host_store, _cs = multi_user_app + host_store.upsert_on_connect( + "294391bc835cde1130ef2a02dcd2b7b3", "alice-laptop", "alice@test.com" + ) + host_store.set_offline("294391bc835cde1130ef2a02dcd2b7b3") + + async with AsyncClient(transport=ASGITransport(app=_app), base_url="http://test") as client: + resp = await client.delete( + "/v1/hosts/294391bc835cde1130ef2a02dcd2b7b3", + headers={"x-test-user": "bob@test.com"}, + ) + assert resp.status_code == 403, ( + f"Expected 403 for wrong owner on delete, got {resp.status_code}. " + "Owner check on DELETE /v1/hosts/{{id}} is missing." + ) + # Alice's host survives Bob's attempt. + assert host_store.get_host("294391bc835cde1130ef2a02dcd2b7b3") is not None + + async def test_launch_runner_403_wrong_owner( multi_user_app: tuple[FastAPI, HostRegistry, HostStore, SqlAlchemyConversationStore], ) -> None: From 08ac7f698a45751208b8c785cd7af14876d80194 Mon Sep 17 00:00:00 2001 From: abhay-codes07 Date: Fri, 24 Jul 2026 20:51:06 +0530 Subject: [PATCH 2/4] chore: regenerate openapi.json for the DELETE /v1/hosts/{host_id} route Signed-off-by: abhay-codes07 --- openapi.json | 35 +++++++++++++++++++++++++++++++++++ 1 file changed, 35 insertions(+) diff --git a/openapi.json b/openapi.json index 4d148a906d..a4b2c7df8c 100644 --- a/openapi.json +++ b/openapi.json @@ -6986,6 +6986,41 @@ } }, "/v1/hosts/{host_id}": { + "delete": { + "description": "Deregister (delete) a host the caller owns.\n\nRemoves a retired self-registered host so it stops appearing in\nsession-creation pickers forever. Any sessions still bound to it\nare unbound by the store (their `host_id` is nulled) and the\nrow is deleted, which also revokes the host's launch token.\n\nRefuses a host that is currently online (409): retire it after it\ngoes offline so an active machine is not pulled out from under\nrunning work. Server-managed sandbox hosts are likewise refused\n(409) \u2014 their lifecycle belongs to the server that created them,\nand deleting only the row would orphan the live sandbox.\n\n**Returns:** 204 No Content on success.\n\n**Raises**\n\n- `HTTPException` \u2014 404 if the host does not exist, 403 if the caller is not the owner, 409 if the host is online or server-managed.", + "operationId": "delete_host_v1_hosts__host_id__delete", + "parameters": [ + { + "description": "Host identifier, e.g. `\"host_a1b2c3d4...\"`.", + "in": "path", + "name": "host_id", + "required": true, + "schema": { + "title": "Host Id", + "type": "string" + } + } + ], + "responses": { + "204": { + "description": "Successful Response" + }, + "422": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/HTTPValidationError" + } + } + }, + "description": "Validation Error" + } + }, + "summary": "Delete Host", + "tags": [ + "hosts" + ] + }, "get": { "description": "Get details for a single host.\n\n**Returns:** Host details dict.\n\n**Raises**\n\n- `HTTPException` \u2014 404 if the host does not exist.", "operationId": "get_host_v1_hosts__host_id__get", From f8c40ee4eee8221db4cdaf868fc81d1c86e6a730 Mon Sep 17 00:00:00 2001 From: SabhyaC26 Date: Mon, 27 Jul 2026 23:24:23 +0000 Subject: [PATCH 3/4] fix(server): fully unbind sessions when a host is deleted Deleting a host nulled only omnigent_conversation_metadata.host_id and left runner_id, workspace, and git_branch pointing at a machine that no longer exists. A session in that state is wedged in every direction: it cannot auto-relaunch (that path is gated on host_id), cannot rebind to a new host (the atomic bind matches only runner_id IS NULL, so it returns "session already has a runner bound" forever), and cannot be stopped (Stop requires both host_id and runner_id). There is no API that repairs it, and no foreign key fails loudly to signal the inconsistency. Null all four columns in the same statement instead, matching the full unbind ConversationStore.clear_host_binding already performs for a failed per-session bind. The change lands in the shared primitive rather than behind a flag for the new deregistration route, because it is correct for the only other caller too: managed-sandbox teardown runs after the sandbox has been terminated, so a surviving runner_id there is just as stale. Signed-off-by: SabhyaC26 --- omnigent/stores/host_store.py | 17 ++++++--- tests/server/integration/test_hosts_api.py | 39 +++++++++++++++++++++ tests/stores/test_host_store.py | 40 ++++++++++++++++++++++ 3 files changed, 92 insertions(+), 4 deletions(-) diff --git a/omnigent/stores/host_store.py b/omnigent/stores/host_store.py index b094d108d1..d4d05208ea 100644 --- a/omnigent/stores/host_store.py +++ b/omnigent/stores/host_store.py @@ -774,11 +774,20 @@ def delete_host(self, host_id: str) -> None: Managed-host teardown: removes the host from the picker AND revokes its launch token in one operation (the row IS the - credential). Explicitly nulls ``conversations.host_id`` for any - sessions still bound to this host — the DB no longer cascades - this via FK. No-op when the row does not exist — deletion is + credential). No-op when the row does not exist — deletion is invoked from best-effort cleanup paths that may race. + Fully unbinds every session still pointing at the host, nulling + ``host_id``, ``runner_id``, ``workspace``, and ``git_branch`` + together (the DB no longer cascades this via FK). All four go + because the machine behind them is gone: a row keeping its + ``runner_id`` can never relaunch, rebind (the atomic bind + matches only ``runner_id IS NULL``), or stop (Stop needs both + ``host_id`` and ``runner_id``), so a partial unbind wedges the + session with no API left to repair it. Clearing ``host_id`` and + ``workspace`` in the same statement never violates + ``ck_conversation_metadata_workspace_required_for_host``. + :param host_id: Host identifier, e.g. ``"host_a1b2c3d4..."``. """ with self._session() as session: @@ -788,7 +797,7 @@ def delete_host(self, host_id: str) -> None: SqlConversationMetadata.workspace_id == current_workspace_id(), SqlConversationMetadata.host_id == host_id, ) - .values(host_id=None) + .values(host_id=None, runner_id=None, workspace=None, git_branch=None) ) session.execute( sql_delete(SqlHost).where( diff --git a/tests/server/integration/test_hosts_api.py b/tests/server/integration/test_hosts_api.py index c8b7797ddf..cb608cf781 100644 --- a/tests/server/integration/test_hosts_api.py +++ b/tests/server/integration/test_hosts_api.py @@ -415,6 +415,45 @@ async def test_delete_host_unbinds_bound_sessions( assert unbound.host_id is None, "a deleted host must not leave sessions bound to a dead row" +async def test_delete_host_fully_unbinds_runner_bound_session( + host_api_app: tuple[FastAPI, HostRegistry, HostStore, SqlAlchemyConversationStore], +) -> None: + """ + Deleting a host clears the whole binding of a runner-bound session, + not just ``host_id``. + + The realistic decommission case: the daemon is gone (so the host + reads offline) but the session it launched still carries a + ``runner_id``, ``workspace``, and ``git_branch``. Leaving any of + those set wedges the session for good — it cannot relaunch (gated on + ``host_id``), cannot rebind (the atomic bind matches only + ``runner_id IS NULL``), and cannot be stopped (needs both ids). + """ + app, _reg, host_store, conv_store = host_api_app + host_store.upsert_on_connect(_HOST_ID, "retired-vm", "local") + host_store.set_offline(_HOST_ID) + conv = conv_store.create_conversation(agent_id=None) + conv_store.set_host_id(conv.id, _HOST_ID, workspace="/tmp/ws", git_branch="feature/x") + assert conv_store.set_runner_id(conv.id, "runner_token_deadbeef") + bound = conv_store.get_conversation(conv.id) + assert bound.runner_id == "runner_token_deadbeef" + assert bound.workspace == "/tmp/ws" + assert bound.git_branch == "feature/x" + + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + resp = await client.delete(f"/v1/hosts/{_HOST_ID}") + + assert resp.status_code == 204 + unbound = conv_store.get_conversation(conv.id) + assert unbound.host_id is None + assert unbound.runner_id is None, "a stale runner_id blocks every future rebind" + assert unbound.workspace is None + assert unbound.git_branch is None + # The proof that the unbind is complete: the session can be re-pinned + # to a fresh runner on another host. + assert conv_store.set_runner_id(conv.id, "runner_token_feedface") + + async def test_list_and_get_host_report_online_from_other_replica( host_api_app: tuple[FastAPI, HostRegistry, HostStore, SqlAlchemyConversationStore], db_uri: str, diff --git a/tests/stores/test_host_store.py b/tests/stores/test_host_store.py index 078845caa5..a7722d4d98 100644 --- a/tests/stores/test_host_store.py +++ b/tests/stores/test_host_store.py @@ -855,6 +855,46 @@ def test_delete_host_removes_row_and_revokes_token(db_uri: str) -> None: store.delete_host("dcf4eb5fc0b04985ec45f79cfda95566") +def test_delete_host_fully_unbinds_bound_conversations(db_uri: str) -> None: + """ + ``delete_host`` nulls the whole binding, not just ``host_id``. + + The machine behind the row is gone, so a surviving ``runner_id`` / + ``workspace`` / ``git_branch`` points at nothing. Worse, the leftover + ``runner_id`` is load-bearing: rebinding matches only + ``runner_id IS NULL``, so a partial unbind locks the session out of + every future host for good. + """ + from omnigent.stores.conversation_store.sqlalchemy_store import ( + SqlAlchemyConversationStore, + ) + + store = HostStore(db_uri) + conversations = SqlAlchemyConversationStore(db_uri) + store.upsert_on_connect( + host_id="c3d1b3f4c2a54c8fa1de5f0b9c7e2d10", + name="doomed-vm", + user_id="alice@example.com", + ) + conv = conversations.create_conversation( + host_id="c3d1b3f4c2a54c8fa1de5f0b9c7e2d10", + workspace="/home/alice/proj", + git_branch="feature/x", + runner_id="runner_token_cafebabe", + ) + + store.delete_host("c3d1b3f4c2a54c8fa1de5f0b9c7e2d10") + + unbound = conversations.get_conversation(conv.id) + assert unbound is not None + assert unbound.host_id is None + assert unbound.runner_id is None + assert unbound.workspace is None + assert unbound.git_branch is None + # The row is genuinely reusable, not merely blank. + assert conversations.set_runner_id(conv.id, "runner_token_d00dfeed") + + def test_revoke_launch_token_keeps_row_but_stops_resolution(db_uri: str) -> None: """ ``revoke_launch_token`` is the relaunch-failure cleanup: the From cb3b0eb45846cdd75c0ec32757651e0056c39e3e Mon Sep 17 00:00:00 2001 From: SabhyaC26 Date: Mon, 27 Jul 2026 23:25:06 +0000 Subject: [PATCH 4/4] feat(server): refuse host deregistration while scheduled tasks pin it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A scheduled task pinned to a connected host stores that host_id as a soft reference with no foreign key. Once the host row is gone the task fails on every single fire with host_not_found, and the user cannot repair it in place: the PATCH validator rejects a null host_id, so the only way out is to delete the task and build it again from scratch. Managed hosts were never pinnable, so deregistering a connected host is the first path that can create this orphan. Refuse the delete with 409 while any task still pins the host, naming the tasks so the caller knows what to fix. Silently unpinning would be worse than the 409: an unpinned task resolves the owner's first online host at fire time, so clearing the pin would quietly relocate the work to a different machine. Re-pointing a task at another host is already a single PATCH, which makes the refusal recoverable with the surface that exists. Paused tasks count too — they break identically once resumed. Reading the tasks needs a host-scoped query rather than a filter over the owner's tasks, because single-user deployments store tasks with a null user_id while their hosts are owned by the reserved "local" user, so an owner filter would miss every pinned task there. Also corrects the route docstring, which is published to the public API reference through openapi.json: it promised the host would stay out of the pickers "forever" and claimed the delete revokes a launch token. Neither holds for the hosts this route can delete. Only managed hosts ever carry a token and those are refused, and an external host keeps its host_id locally, so its daemon recreates the row on the next reconnect. Signed-off-by: SabhyaC26 --- omnigent/server/app.py | 1 + omnigent/server/routes/hosts.py | 47 +++++- .../stores/scheduled_task_store/__init__.py | 17 ++ .../scheduled_task_store/sqlalchemy_store.py | 12 ++ openapi.json | 2 +- tests/server/integration/test_hosts_api.py | 155 ++++++++++++++++++ tests/stores/test_scheduled_task_store.py | 37 +++++ 7 files changed, 264 insertions(+), 7 deletions(-) diff --git a/omnigent/server/app.py b/omnigent/server/app.py index 6bf7cc1db6..bdcf5edd02 100644 --- a/omnigent/server/app.py +++ b/omnigent/server/app.py @@ -2700,6 +2700,7 @@ async def _on_hosts_changed(_host_id: str, owner: str | None) -> None: permission_store=permission_store, agent_store=agent_store, agent_cache=agent_cache, + scheduled_task_store=scheduled_task_store, ), prefix="/v1", tags=["hosts"], diff --git a/omnigent/server/routes/hosts.py b/omnigent/server/routes/hosts.py index 3a5f2058f6..33aaf0a197 100644 --- a/omnigent/server/routes/hosts.py +++ b/omnigent/server/routes/hosts.py @@ -50,6 +50,7 @@ from omnigent.stores import AgentStore, ConversationStore from omnigent.stores.host_store import HostStore, host_is_live from omnigent.stores.permission_store import PermissionStore +from omnigent.stores.scheduled_task_store import ScheduledTaskStore _logger = logging.getLogger(__name__) @@ -407,6 +408,7 @@ def create_hosts_router( permission_store: PermissionStore | None = None, agent_store: AgentStore | None = None, agent_cache: AgentCache | None = None, + scheduled_task_store: ScheduledTaskStore | None = None, ) -> APIRouter: """Build the router for host REST endpoints. @@ -427,6 +429,11 @@ def create_hosts_router( :func:`omnigent.server.app.create_app` always supplies it. :param agent_cache: Agent-spec cache used to read the agent's ``os_env.cwd`` boundary. Paired with ``agent_store``. + :param scheduled_task_store: Store backing recurring tasks, read by + host deregistration to refuse deleting a host that tasks still + pin. ``None`` (deployments without the scheduler, and minimal + test wirings) skips that check — no task can be pinned when no + task can exist. :returns: A FastAPI router with host endpoints. """ router = APIRouter() @@ -523,22 +530,35 @@ async def delete_host(request: Request, host_id: str) -> Response: """Deregister (delete) a host the caller owns. Removes a retired self-registered host so it stops appearing in - session-creation pickers forever. Any sessions still bound to it - are unbound by the store (their ``host_id`` is nulled) and the - row is deleted, which also revokes the host's launch token. + session-creation pickers. Any session still bound to it is fully + unbound by the store — ``host_id``, ``runner_id``, ``workspace``, + and ``git_branch`` are nulled together, leaving a session that + can be rebound to another host rather than one wedged against a + row that no longer exists. + + Deregistration is not permanent on its own: an external host + keeps its ``host_id`` in its local config and no server-side + credential is destroyed here (only server-managed hosts ever + hold a launch token, and those are refused below), so a daemon + that dials again simply recreates the row. Stop the daemon + first. Refuses a host that is currently online (409): retire it after it goes offline so an active machine is not pulled out from under running work. Server-managed sandbox hosts are likewise refused (409) — their lifecycle belongs to the server that created them, - and deleting only the row would orphan the live sandbox. + and deleting only the row would orphan the live sandbox. A host + that scheduled tasks pin is refused too (409): the pin is a soft + reference, so deleting the host would leave each task failing on + every fire. Re-point those tasks at another host (or delete + them) first. :param request: The incoming request (for auth). :param host_id: Host identifier, e.g. ``"host_a1b2c3d4..."``. :returns: 204 No Content on success. :raises HTTPException: 404 if the host does not exist, 403 if the - caller is not the owner, 409 if the host is online or - server-managed. + caller is not the owner, 409 if the host is online, + server-managed, or pinned by a scheduled task. """ # Same auth shape as get_host: require_user 401s unauthenticated # callers when auth is configured; a None user_id means auth is @@ -559,6 +579,21 @@ async def delete_host(request: Request, host_id: str) -> Response: status_code=409, detail="host is online; retire it after it goes offline", ) + if scheduled_task_store is not None: + pinned = await asyncio.to_thread(scheduled_task_store.list_by_host_id, host_id) + if pinned: + # Refuse rather than silently unpin: an unpinned task falls + # back to whichever host of the owner's is online at fire + # time, so clearing the pin here would relocate the work + # instead of surfacing the choice. PATCH can re-point a task + # at another host, which is the repair. + raise HTTPException( + status_code=409, + detail=( + f"{len(pinned)} scheduled task(s) are pinned to this host; " + "re-point or delete them first: " + ", ".join(task.id for task in pinned) + ), + ) await asyncio.to_thread(host_store.delete_host, host_id) return Response(status_code=204) diff --git a/omnigent/stores/scheduled_task_store/__init__.py b/omnigent/stores/scheduled_task_store/__init__.py index 2b19730629..b5226059a2 100644 --- a/omnigent/stores/scheduled_task_store/__init__.py +++ b/omnigent/stores/scheduled_task_store/__init__.py @@ -96,6 +96,23 @@ def list(self, *, owner_user_id: str | None = None) -> list[ScheduledTask]: """ ... + @abstractmethod + def list_by_host_id(self, host_id: str) -> list[ScheduledTask]: + """ + List every task pinned to *host_id*, in any state. + + Host deregistration uses this to refuse deleting a host that + tasks still pin: the pin is a soft reference (no FK, Rule R032), + so a deleted host leaves each task failing at fire time with + ``host_not_found``. Paused tasks count — they break the same way + once resumed. + + :param host_id: Host identifier, e.g. ``"host_a1b2c3d4..."``. + :returns: Tasks with ``host_id`` matching, ordered like + :meth:`list`. + """ + ... + @abstractmethod def list_active(self) -> list[ScheduledTask]: """ diff --git a/omnigent/stores/scheduled_task_store/sqlalchemy_store.py b/omnigent/stores/scheduled_task_store/sqlalchemy_store.py index 43106ddf59..40c9850258 100644 --- a/omnigent/stores/scheduled_task_store/sqlalchemy_store.py +++ b/omnigent/stores/scheduled_task_store/sqlalchemy_store.py @@ -177,6 +177,18 @@ def list(self, *, owner_user_id: str | None = None) -> list[ScheduledTask]: rows = session.execute(stmt).scalars().all() return [_to_entity(r) for r in rows] + def list_by_host_id(self, host_id: str) -> list[ScheduledTask]: + """List every task pinned to *host_id*, in any state.""" + with self._session() as session: + stmt = ( + select(SqlScheduledTask) + .where(SqlScheduledTask.workspace_id == current_workspace_id()) + .where(SqlScheduledTask.host_id == host_id) + .order_by(asc(SqlScheduledTask.created_at), asc(SqlScheduledTask.id)) + ) + rows = session.execute(stmt).scalars().all() + return [_to_entity(r) for r in rows] + def list_active(self) -> list[ScheduledTask]: """List active scheduled tasks ordered by ``created_at ASC, id ASC``.""" with self._session() as session: diff --git a/openapi.json b/openapi.json index a4b2c7df8c..d452f6db85 100644 --- a/openapi.json +++ b/openapi.json @@ -6987,7 +6987,7 @@ }, "/v1/hosts/{host_id}": { "delete": { - "description": "Deregister (delete) a host the caller owns.\n\nRemoves a retired self-registered host so it stops appearing in\nsession-creation pickers forever. Any sessions still bound to it\nare unbound by the store (their `host_id` is nulled) and the\nrow is deleted, which also revokes the host's launch token.\n\nRefuses a host that is currently online (409): retire it after it\ngoes offline so an active machine is not pulled out from under\nrunning work. Server-managed sandbox hosts are likewise refused\n(409) \u2014 their lifecycle belongs to the server that created them,\nand deleting only the row would orphan the live sandbox.\n\n**Returns:** 204 No Content on success.\n\n**Raises**\n\n- `HTTPException` \u2014 404 if the host does not exist, 403 if the caller is not the owner, 409 if the host is online or server-managed.", + "description": "Deregister (delete) a host the caller owns.\n\nRemoves a retired self-registered host so it stops appearing in\nsession-creation pickers. Any session still bound to it is fully\nunbound by the store \u2014 `host_id`, `runner_id`, `workspace`,\nand `git_branch` are nulled together, leaving a session that\ncan be rebound to another host rather than one wedged against a\nrow that no longer exists.\n\nDeregistration is not permanent on its own: an external host\nkeeps its `host_id` in its local config and no server-side\ncredential is destroyed here (only server-managed hosts ever\nhold a launch token, and those are refused below), so a daemon\nthat dials again simply recreates the row. Stop the daemon\nfirst.\n\nRefuses a host that is currently online (409): retire it after it\ngoes offline so an active machine is not pulled out from under\nrunning work. Server-managed sandbox hosts are likewise refused\n(409) \u2014 their lifecycle belongs to the server that created them,\nand deleting only the row would orphan the live sandbox. A host\nthat scheduled tasks pin is refused too (409): the pin is a soft\nreference, so deleting the host would leave each task failing on\nevery fire. Re-point those tasks at another host (or delete\nthem) first.\n\n**Returns:** 204 No Content on success.\n\n**Raises**\n\n- `HTTPException` \u2014 404 if the host does not exist, 403 if the caller is not the owner, 409 if the host is online, server-managed, or pinned by a scheduled task.", "operationId": "delete_host_v1_hosts__host_id__delete", "parameters": [ { diff --git a/tests/server/integration/test_hosts_api.py b/tests/server/integration/test_hosts_api.py index cb608cf781..a330041768 100644 --- a/tests/server/integration/test_hosts_api.py +++ b/tests/server/integration/test_hosts_api.py @@ -5,6 +5,7 @@ import asyncio import contextlib import time +import uuid from pathlib import Path import pytest @@ -28,6 +29,9 @@ ) from omnigent.stores.host_store import HostStore from omnigent.stores.permission_store.sqlalchemy_store import SqlAlchemyPermissionStore +from omnigent.stores.scheduled_task_store.sqlalchemy_store import ( + SqlAlchemyScheduledTaskStore, +) pytestmark = pytest.mark.asyncio @@ -100,6 +104,35 @@ def host_api_app( return app, registry, host_store, conv_store +@pytest.fixture() +def host_api_app_with_tasks( + db_uri: str, +) -> tuple[FastAPI, HostStore, SqlAlchemyScheduledTaskStore]: + """Host REST app wired to a scheduled-task store. + + Separate from :func:`host_api_app` because only the deregistration + tests need the scheduler wiring. + + :param db_uri: SQLite URI from the shared fixture. + :returns: Tuple of (app, host_store, scheduled_task_store). + """ + registry = HostRegistry() + host_store = HostStore(db_uri) + conv_store = SqlAlchemyConversationStore(db_uri) + task_store = SqlAlchemyScheduledTaskStore(db_uri) + app = FastAPI() + app.include_router( + create_hosts_router( + registry, + host_store, + conv_store, + scheduled_task_store=task_store, + ), + prefix="/v1", + ) + return app, host_store, task_store + + async def _connect_host( app: FastAPI, registry: HostRegistry, @@ -454,6 +487,128 @@ async def test_delete_host_fully_unbinds_runner_bound_session( assert conv_store.set_runner_id(conv.id, "runner_token_feedface") +def _pin_task( + task_store: SqlAlchemyScheduledTaskStore, + task_id: str, + host_id: str | None, + *, + state: str = "active", +) -> str: + """Create a scheduled task pinned to *host_id*. + + :param task_store: Store to create the task in. + :param task_id: Readable seed for the task's hex id. + :param host_id: Host to pin to, or ``None`` for an unpinned task. + :param state: Lifecycle state, e.g. ``"paused"``. + :returns: The created task's id. + """ + return task_store.create( + scheduled_task_id=uuid.uuid5(uuid.NAMESPACE_DNS, task_id).hex, + name=task_id, + prompt="Triage the inbox", + rrule="FREQ=DAILY;BYHOUR=9;BYMINUTE=0", + user_id="local", + agent_id=uuid.uuid5(uuid.NAMESPACE_DNS, "agent").hex, + timezone="UTC", + workspace="/tmp/ws", + host_id=host_id, + state=state, + ).id + + +async def test_delete_host_409_when_scheduled_task_pinned( + host_api_app_with_tasks: tuple[FastAPI, HostStore, SqlAlchemyScheduledTaskStore], +) -> None: + """ + DELETE refuses a host that scheduled tasks pin, and names them. + + ``scheduled_tasks.host_id`` is a soft reference with no FK, so + deleting the host would leave every fire failing with + ``host_not_found`` — and the PATCH surface cannot null a pin, so the + task could only be recovered by recreating it. + """ + app, host_store, task_store = host_api_app_with_tasks + host_store.upsert_on_connect(_HOST_ID, "retired-vm", "local") + host_store.set_offline(_HOST_ID) + pinned_id = _pin_task(task_store, "st_pinned", _HOST_ID) + + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + resp = await client.delete(f"/v1/hosts/{_HOST_ID}") + + assert resp.status_code == 409, f"expected 409 for a pinned host, got {resp.status_code}" + assert pinned_id in resp.json()["detail"], "the 409 must name the tasks blocking the delete" + # The host survives so the pinned task keeps working. + assert host_store.get_host(_HOST_ID) is not None + + +async def test_delete_host_409_counts_paused_pinned_tasks( + host_api_app_with_tasks: tuple[FastAPI, HostStore, SqlAlchemyScheduledTaskStore], +) -> None: + """ + A paused task still blocks the delete: it breaks the same way the + moment it is resumed, and resuming is a single PATCH away. + """ + app, host_store, task_store = host_api_app_with_tasks + host_store.upsert_on_connect(_HOST_ID, "retired-vm", "local") + host_store.set_offline(_HOST_ID) + _pin_task(task_store, "st_paused", _HOST_ID, state="paused") + + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + resp = await client.delete(f"/v1/hosts/{_HOST_ID}") + + assert resp.status_code == 409 + assert host_store.get_host(_HOST_ID) is not None + + +async def test_delete_host_ignores_tasks_pinned_elsewhere( + host_api_app_with_tasks: tuple[FastAPI, HostStore, SqlAlchemyScheduledTaskStore], +) -> None: + """ + Only tasks pinned to *this* host block it. An unpinned task resolves + its host at fire time and a task pinned to another host is + unaffected, so neither should stop the delete. + """ + app, host_store, task_store = host_api_app_with_tasks + host_store.upsert_on_connect(_HOST_ID, "retired-vm", "local") + host_store.set_offline(_HOST_ID) + other_host = uuid.uuid5(uuid.NAMESPACE_DNS, "other-host").hex + _pin_task(task_store, "st_unpinned", None) + _pin_task(task_store, "st_other", other_host) + + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + resp = await client.delete(f"/v1/hosts/{_HOST_ID}") + + assert resp.status_code == 204, f"expected 204, got {resp.status_code}: {resp.text}" + assert host_store.get_host(_HOST_ID) is None + + +async def test_delete_host_succeeds_after_task_repointed( + host_api_app_with_tasks: tuple[FastAPI, HostStore, SqlAlchemyScheduledTaskStore], +) -> None: + """ + The 409 is recoverable with the existing PATCH surface: re-point the + task at another host and the delete goes through. This is the whole + reason refusing beats silently unpinning — the user chooses where the + task runs next. + """ + app, host_store, task_store = host_api_app_with_tasks + host_store.upsert_on_connect(_HOST_ID, "retired-vm", "local") + host_store.set_offline(_HOST_ID) + replacement = uuid.uuid5(uuid.NAMESPACE_DNS, "replacement-host").hex + host_store.upsert_on_connect(replacement, "new-vm", "local") + task_id = _pin_task(task_store, "st_repoint", _HOST_ID) + + async with AsyncClient(transport=ASGITransport(app=app), base_url="http://test") as client: + blocked = await client.delete(f"/v1/hosts/{_HOST_ID}") + task_store.update(task_id, host_id=replacement) + allowed = await client.delete(f"/v1/hosts/{_HOST_ID}") + + assert blocked.status_code == 409 + assert allowed.status_code == 204, f"expected 204, got {allowed.status_code}: {allowed.text}" + assert host_store.get_host(_HOST_ID) is None + assert task_store.get(task_id).host_id == replacement + + async def test_list_and_get_host_report_online_from_other_replica( host_api_app: tuple[FastAPI, HostRegistry, HostStore, SqlAlchemyConversationStore], db_uri: str, diff --git a/tests/stores/test_scheduled_task_store.py b/tests/stores/test_scheduled_task_store.py index a7ba90378b..90c7e7fb8a 100644 --- a/tests/stores/test_scheduled_task_store.py +++ b/tests/stores/test_scheduled_task_store.py @@ -256,6 +256,43 @@ def test_list_orders_by_created_at_then_id(store: SqlAlchemyScheduledTaskStore) assert ids == [id_a, id_b] +def test_list_by_host_id_returns_pinned_tasks_in_any_state( + store: SqlAlchemyScheduledTaskStore, +) -> None: + """ + ``list_by_host_id`` returns every task pinned to the host regardless + of state, and nothing else. + + Host deregistration reads this to refuse deleting a pinned host, so + an unpinned task (which resolves its host at fire time) and a task + pinned elsewhere must not block the delete — while a paused task, + which breaks the moment it is resumed, must. + """ + host = _uid("host_pinned") + for seed, pinned_to, state in ( + ("st_active", host, "active"), + ("st_paused", host, "paused"), + ("st_elsewhere", _uid("host_other"), "active"), + ("st_unpinned", None, "active"), + ): + store.create( + scheduled_task_id=_uid(seed), + name=seed, + prompt="do a thing", + rrule="FREQ=DAILY", + user_id="alice@example.com", + agent_id=_uid("ag_pinned"), + timezone="UTC", + host_id=pinned_to, + state=state, + ) + + pinned = store.list_by_host_id(host) + + assert {t.id for t in pinned} == {_uid("st_active"), _uid("st_paused")} + assert store.list_by_host_id(_uid("host_with_nothing")) == [] + + def test_list_active_excludes_non_active(store: SqlAlchemyScheduledTaskStore) -> None: """``list_active`` returns only active tasks, excluding paused/deleted.""" store.create(