diff --git a/omnigent/server/app.py b/omnigent/server/app.py index 82d45c7bd5..17f5922f56 100644 --- a/omnigent/server/app.py +++ b/omnigent/server/app.py @@ -2337,6 +2337,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 60c84041b5..715e80734f 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 @@ -56,6 +56,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__) @@ -529,6 +530,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. @@ -549,6 +551,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() @@ -652,6 +659,78 @@ 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. 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. 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, + 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 + # 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", + ) + 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) + @router.get("/hosts/{host_id}/harnesses/{harness}/model-options") async def get_host_model_options( request: Request, diff --git a/omnigent/stores/host_store.py b/omnigent/stores/host_store.py index 2b914fc935..d75a56b403 100644 --- a/omnigent/stores/host_store.py +++ b/omnigent/stores/host_store.py @@ -787,11 +787,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("delete_host") as session: @@ -801,7 +810,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/omnigent/stores/scheduled_task_store/__init__.py b/omnigent/stores/scheduled_task_store/__init__.py index 4a753206f5..a0d099b909 100644 --- a/omnigent/stores/scheduled_task_store/__init__.py +++ b/omnigent/stores/scheduled_task_store/__init__.py @@ -97,6 +97,23 @@ def list(self, *, owner_user_id: str | None = None) -> list[ScheduledTask]: """ ... + @abstractmethod + def list_by_host_id(self, host_id: str) -> builtins.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) -> builtins.list[ScheduledTask]: """ diff --git a/omnigent/stores/scheduled_task_store/sqlalchemy_store.py b/omnigent/stores/scheduled_task_store/sqlalchemy_store.py index f286bdb146..8939b11cbe 100644 --- a/omnigent/stores/scheduled_task_store/sqlalchemy_store.py +++ b/omnigent/stores/scheduled_task_store/sqlalchemy_store.py @@ -181,6 +181,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) -> builtins.list[ScheduledTask]: + """List every task pinned to *host_id*, in any state.""" + with self._session("list_tasks_by_host_id") 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) -> builtins.list[ScheduledTask]: """List active scheduled tasks ordered by ``created_at ASC, id ASC``.""" with self._session("list_active_tasks") as session: diff --git a/openapi.json b/openapi.json index 72a90b91bc..f64a7ae4eb 100644 --- a/openapi.json +++ b/openapi.json @@ -7258,6 +7258,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. 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": [ + { + "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 \u2014 the `list_hosts` fields (including `gateway_inference`, `None` when unreported) plus `runners`.\n\n**Raises**\n\n- `HTTPException` \u2014 404 if the host does not exist.", "operationId": "get_host_v1_hosts__host_id__get", diff --git a/tests/server/integration/test_hosts_api.py b/tests/server/integration/test_hosts_api.py index e227caa46e..0a299f61cf 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 @@ -120,6 +124,35 @@ def _build_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, @@ -453,6 +486,272 @@ 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_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") + + +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, @@ -898,6 +1197,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: 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 diff --git a/tests/stores/test_scheduled_task_store.py b/tests/stores/test_scheduled_task_store.py index 0337dd3de1..5bc5603bad 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(