From e1b0713e7cc158119a2eee892c857397cdec39e5 Mon Sep 17 00:00:00 2001 From: Paulo Date: Tue, 7 Jul 2026 01:00:57 +0200 Subject: [PATCH 1/4] ENG-524: renewable-lease lifecycle + keepalive renew endpoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every host is now a renewable lease. POST /hosts without expires_at stores now + LEASE_DEFAULT_TTL (new setting, default 86400s) instead of null, so an abandoned host lapses and the janitor reaps it; an explicit expires_at: null stays supported as the deliberate opt-in to a permanent host. POST /hosts/{id}/renew is the keepalive: it bumps expires_at to the given future instant, or by LEASE_DEFAULT_TTL from now on an empty body. Only active and bootstrapping hosts renew; unclaimed warm-pool members refuse with 409 (pool maintenance owns them) and a missing host is 404. Pool claims now always stamp the caller's resolved lease onto the claimed host — the default lease, an explicit window, or null for a deliberate permanent — replacing the warm-pool max-age TTL. Warming itself still uses POOL_HOST_MAX_AGE_HOURS. Co-Authored-By: Claude Fable 5 --- AGENTS.md | 8 ++ docs/architecture.md | 9 ++ docs/deploy.md | 1 + src/core/settings.py | 6 + src/hosts/api.py | 21 +++- src/hosts/schemas.py | 29 +++-- src/hosts/service.py | 52 ++++++-- src/hosts/tests/test_api.py | 217 ++++++++++++++++++++++++++++++++ src/hosts/tests/test_janitor.py | 25 +++- src/hosts/tests/test_pool.py | 24 +++- 10 files changed, 363 insertions(+), 29 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index a094f20..e3a604f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -131,6 +131,14 @@ The effective host image is captured on the host record at creation time. If `POST /hosts` omits `image`, store `EXE_DEFAULT_IMAGE`; provisioning must use the stored `host.image`, not the current runtime default. +Every host is a renewable lease. `POST /hosts` without `expires_at` stores +`now + LEASE_DEFAULT_TTL`; an explicit `expires_at: null` is the deliberate +opt-in to a permanent host. `POST /hosts/{id}/renew` bumps `expires_at` — to +the given future instant, or by `LEASE_DEFAULT_TTL` from now on an empty body. +Only `active` and `bootstrapping` hosts renew, and unclaimed pool members +refuse with `409` (pool maintenance owns them). The janitor reaps hosts whose +`expires_at` has passed. + ### Delete Semantics `DELETE /hosts/{id}` is service-token only. diff --git a/docs/architecture.md b/docs/architecture.md index 9091e57..646caf9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -93,6 +93,15 @@ successful key returns the original host instead of a duplicate. Caller `env` is stored for provisioning and never returned by the API; keys in `hosts.schemas.RESERVED_HOST_ENV_KEYS` are rejected. +Every host is a renewable lease. A create without `expires_at` gets +`now + LEASE_DEFAULT_TTL`, so a host whose owner disappears lapses and +self-reaps instead of leaking VM cost; an explicit `expires_at: null` +is the deliberate opt-in to a permanent host. `POST /hosts/{id}/renew` +is the keepalive: it bumps `expires_at` to the requested instant, or by +`LEASE_DEFAULT_TTL` from now when the body is empty. Only caller-owned +hosts renew — unclaimed warm-pool members belong to pool maintenance +and refuse with `409`. + Two maintenance commands run as cron jobs from the same image: `hosts.janitor` reaps expired and orphaned hosts, `hosts.pool` keeps a warm pool of pre-provisioned hosts per provider (`POOL_SIZES`, with diff --git a/docs/deploy.md b/docs/deploy.md index 824a3a8..402f231 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -139,6 +139,7 @@ Core, optional: | `SERVICE_LABEL` | `drukbox` | Label stamped onto provider resources (VM tags, SG tags). | | `UVICORN_HOST` | `0.0.0.0` | API bind address. Set `127.0.0.1` to restrict to loopback. | | `PROVISIONING_GRACE_SECONDS` | `600` | Safety TTL on in-flight hosts so the janitor reaps row + VM if the client disconnects mid-provision. Must exceed the worst-case provision duration. | +| `LEASE_DEFAULT_TTL` | `86400` | Lease TTL in seconds for hosts created without an explicit `expires_at`, and the extension applied by an empty `POST /hosts/{id}/renew`. An explicit `expires_at: null` at create time opts out of expiry entirely. | | `IDEMPOTENCY_KEY_TTL_HOURS` | `24` | Retention period for successful `Idempotency-Key` mappings. | | `POOL_SIZES` | `{}` | Warm hosts to keep ready per provider, as JSON (e.g. `{"exe": 2, "hetzner": 1}`). Overrides `POOL_SIZE` for the providers it names. | | `POOL_SIZE` | `0` | Warm hosts to keep ready for the default provider. `0` disables its pool. | diff --git a/src/core/settings.py b/src/core/settings.py index 072b1f9..03c40b2 100644 --- a/src/core/settings.py +++ b/src/core/settings.py @@ -70,6 +70,12 @@ class Settings(BaseSettings): validation_alias="PROVISIONING_GRACE_SECONDS", description="Safety TTL on the host row while provisioning is in flight.", ) + lease_default_ttl: int = Field( + default=86400, + gt=0, + validation_alias="LEASE_DEFAULT_TTL", + description="Lease TTL in seconds for hosts created without an explicit expires_at.", + ) pool_sizes: dict[str, Annotated[int, Field(ge=0)]] = Field( default_factory=dict, validation_alias="POOL_SIZES", diff --git a/src/hosts/api.py b/src/hosts/api.py index 18acd51..a2547ae 100644 --- a/src/hosts/api.py +++ b/src/hosts/api.py @@ -9,7 +9,7 @@ from hosts.deps import get_host_service from hosts.exceptions import HostTeardownError from hosts.models import Host -from hosts.schemas import HostCreate, HostOut +from hosts.schemas import HostCreate, HostOut, HostRenew from hosts.service import HostService from networking.tailscale import NetworkError from providers.exceptions import ProviderError, UnknownProviderError @@ -46,12 +46,15 @@ async def create_host( ] = None, ) -> Host: host_create = payload or HostCreate() + # An omitted expires_at gets the default lease; an explicit null is the + # caller's deliberate opt-in to a permanent host. + expires_at = host_create.expires_at if "expires_at" in host_create.model_fields_set else ... try: return await service.get_or_create_host( env=host_create.env, image=host_create.image, - expires_at=host_create.expires_at, + expires_at=expires_at, idempotency_key=idempotency_key, provider=host_create.provider, ) @@ -85,6 +88,20 @@ async def get_host(host_id: uuid.UUID, service: HostServiceDep) -> Host: raise HTTPException(status_code=404, detail="host not found") +@router.post( + "/{host_id}/renew", + response_model=HostOut, + dependencies=[Depends(require_service_auth)], +) +async def renew_host( + host_id: uuid.UUID, + service: HostServiceDep, + payload: HostRenew | None = None, +) -> Host: + host_renew = payload or HostRenew() + return await service.renew_host(host_id, expires_at=host_renew.expires_at) + + @router.delete( "/{host_id}", status_code=status.HTTP_204_NO_CONTENT, diff --git a/src/hosts/schemas.py b/src/hosts/schemas.py index 55f51cb..199d93a 100644 --- a/src/hosts/schemas.py +++ b/src/hosts/schemas.py @@ -8,6 +8,16 @@ _ENV_KEY_RE = re.compile(r"[A-Za-z_][A-Za-z0-9_]*") +def _expires_at_must_be_future_and_tz_aware(expires_at: datetime | None) -> datetime | None: + if expires_at is None: + return None + if expires_at.tzinfo is None: + raise ValueError("expires_at must include a timezone offset") + if expires_at <= datetime.now(UTC): + raise ValueError("expires_at must be in the future") + return expires_at + + class HostCreate(BaseModel): image: str | None = None env: dict[str, str] = Field(default_factory=dict) @@ -40,16 +50,15 @@ def reject_reserved_env_keys(cls, env: dict[str, str]) -> dict[str, str]: raise ValueError(f"reserved env keys are not allowed: {', '.join(reserved_keys)}") return env - @field_validator("expires_at") - @classmethod - def expires_at_must_be_future_and_tz_aware(cls, expires_at: datetime | None) -> datetime | None: - if expires_at is None: - return None - if expires_at.tzinfo is None: - raise ValueError("expires_at must include a timezone offset") - if expires_at <= datetime.now(UTC): - raise ValueError("expires_at must be in the future") - return expires_at + _validate_expires_at = field_validator("expires_at")(_expires_at_must_be_future_and_tz_aware) + + +class HostRenew(BaseModel): + # Omitted (or null) means "extend by LEASE_DEFAULT_TTL from now"; renewal + # never makes a host permanent — that is a create-time choice. + expires_at: datetime | None = None + + _validate_expires_at = field_validator("expires_at")(_expires_at_must_be_future_and_tz_aware) class HostOut(BaseModel): diff --git a/src/hosts/service.py b/src/hosts/service.py index 184331e..c3b3e0b 100644 --- a/src/hosts/service.py +++ b/src/hosts/service.py @@ -4,6 +4,7 @@ import time import uuid from datetime import UTC, datetime, timedelta +from types import EllipsisType from sqlalchemy import delete, or_, select, update from sqlalchemy.exc import IntegrityError @@ -47,6 +48,12 @@ HostStatus.ERROR.value, } ) +RENEWABLE_STATUSES = frozenset( + { + HostStatus.BOOTSTRAPPING.value, + HostStatus.ACTIVE.value, + } +) def utc_now() -> datetime: @@ -75,15 +82,23 @@ def __init__( else: self.tailscale = None + def _default_lease_expires_at(self) -> datetime: + return utc_now() + timedelta(seconds=self.settings.lease_default_ttl) + async def get_or_create_host( self, *, env: dict[str, str], image: str | None, - expires_at: datetime | None = None, + expires_at: datetime | None | EllipsisType = ..., idempotency_key: str | None = None, provider: str | None = None, ) -> Host: + # ``...`` (omitted) means "default lease"; an explicit None is the + # caller's deliberate opt-in to a permanent, never-reaped host. + if expires_at is ...: + expires_at = self._default_lease_expires_at() + if provider: registered = get_provider_names() if provider not in registered: @@ -148,19 +163,14 @@ async def _try_claim_pool_host( if candidate_id is None: return None - # Only override the TTL when the caller asked for one. The pool - # maintainer set ``expires_at = created_at + pool_host_max_age_hours`` - # at create time; preserving that means a leak-protection TTL exists - # for claimed hosts whose caller forgets to DELETE. - values: dict[str, datetime] = {"claimed_at": now, "updated_at": now} - if expires_at is not None: - values["expires_at"] = expires_at - + # The claim replaces the warm-pool max-age TTL with the caller's lease: + # a concrete window (explicit or the default), or None for a caller + # who deliberately opted into a permanent host. result = await self.session.execute( update(Host) .where(Host.id == candidate_id) .where(Host.claimed_at.is_(None)) - .values(**values) + .values(claimed_at=now, updated_at=now, expires_at=expires_at) .returning(Host) ) host = result.scalar_one_or_none() @@ -176,13 +186,15 @@ async def create_host( *, env: dict[str, str], image: str | None, - expires_at: datetime | None = None, + expires_at: datetime | None | EllipsisType = ..., provider: str | None = None, pool_member: bool = False, ) -> Host: # Always provisions a brand-new VM; the pool maintainer calls this # directly (with pool_member=True) so it never recursively claims its # own pool members. + if expires_at is ...: + expires_at = self._default_lease_expires_at() vm = get_vm_provider(provider) uid = uuid7() name = Host.build_name(uid) @@ -309,6 +321,24 @@ async def list_hosts(self) -> list[Host]: result = await self.session.execute(select(Host).order_by(Host.created_at.desc())) return list(result.scalars()) + async def renew_host(self, host_id: uuid.UUID, *, expires_at: datetime | None = None) -> Host: + host = await self.get_host_for_update(host_id) + + if host is None: + raise ResourceNotFoundError("host not found") + + if host.pool_member and not host.claimed_at: + raise HostStateError("unclaimed pool host is managed by pool maintenance") + + if host.status not in RENEWABLE_STATUSES: + raise HostStateError(f"cannot renew a host in status {host.status}") + + host.expires_at = expires_at or self._default_lease_expires_at() + host.updated_at = utc_now() + await self.session.commit() + await self.session.refresh(host) + return host + async def delete_host( self, host_id: uuid.UUID, *, force: bool = False, pool_shed: bool = False ) -> None: diff --git a/src/hosts/tests/test_api.py b/src/hosts/tests/test_api.py index 7d90dc1..2818ed9 100644 --- a/src/hosts/tests/test_api.py +++ b/src/hosts/tests/test_api.py @@ -1,4 +1,5 @@ import uuid +from datetime import datetime, timedelta from unittest.mock import AsyncMock from uuid6 import uuid7 @@ -207,6 +208,41 @@ async def test_create_host_rejects_past_expires_at(client): assert "in the future" in detail[0]["msg"] +async def test_create_host_defaults_expires_at_to_lease_ttl(client, monkeypatch): + # Safe-by-default lifecycle: omitting expires_at yields a renewable lease, + # not a permanent host, so an abandoned host self-reaps. + monkeypatch.setattr("hosts.service.HostService.provision", AsyncMock()) + ttl = timedelta(seconds=get_settings().lease_default_ttl) + + before = utc_now() + response = await client.post("/hosts", headers={"Authorization": "Bearer service-token"}) + after = utc_now() + + assert response.status_code == 201 + expires_at = datetime.fromisoformat(response.json()["expires_at"]) + assert before + ttl <= expires_at <= after + ttl + + +async def test_create_host_explicit_null_expires_at_creates_permanent_host(client, monkeypatch): + # expires_at: null is the caller's deliberate opt-in to a host with no + # expiry — permanence must be a choice, never an accident. + monkeypatch.setattr("hosts.service.HostService.provision", AsyncMock()) + + response = await client.post( + "/hosts", + headers={"Authorization": "Bearer service-token"}, + json={"expires_at": None}, + ) + + assert response.status_code == 201 + assert response.json()["expires_at"] is None + + async with async_session_factory() as session: + host = await session.get(Host, uuid.UUID(response.json()["id"])) + assert host is not None + assert host.expires_at is None + + async def test_create_host_rejects_oversized_idempotency_key(client): response = await client.post( "/hosts", @@ -832,6 +868,9 @@ async def create_host_record( env: dict[str, str] | None = None, image: str | None = None, tailscale_device_id: str | None = None, + expires_at: datetime | None = None, + claimed_at: datetime | None = None, + pool_member: bool = False, ) -> Host: now = utc_now() settings = get_settings() @@ -852,6 +891,9 @@ async def create_host_record( env=env or {}, created_at=now, updated_at=now, + expires_at=expires_at, + claimed_at=claimed_at, + pool_member=pool_member, ) async with async_session_factory() as session: session.add(host) @@ -860,6 +902,181 @@ async def create_host_record( return host +async def test_renew_host_with_empty_body_extends_by_default_ttl(client): + # Keepalive: an empty renew body bumps the lease to now + LEASE_DEFAULT_TTL. + # The host here is demand-provisioned (pool_member=False, claimed_at NULL), + # pinning that unclaimed non-pool hosts stay renewable by their caller. + host = await create_host_record( + name="lb-renew-default", + status=HostStatus.ACTIVE.value, + expires_at=utc_now() + timedelta(minutes=5), + ) + ttl = timedelta(seconds=get_settings().lease_default_ttl) + + before = utc_now() + response = await client.post( + f"/hosts/{host.id}/renew", + headers={"Authorization": "Bearer service-token"}, + ) + after = utc_now() + + assert response.status_code == 200 + assert response.json()["id"] == str(host.id) + expires_at = datetime.fromisoformat(response.json()["expires_at"]) + assert before + ttl <= expires_at <= after + ttl + + +async def test_renew_host_accepts_explicit_expires_at(client): + host = await create_host_record( + name="lb-renew-explicit", + status=HostStatus.ACTIVE.value, + expires_at=utc_now() + timedelta(hours=1), + ) + requested = utc_now() + timedelta(days=30) + + response = await client.post( + f"/hosts/{host.id}/renew", + headers={"Authorization": "Bearer service-token"}, + json={"expires_at": requested.isoformat()}, + ) + + assert response.status_code == 200 + renewed = datetime.fromisoformat(response.json()["expires_at"]) + assert abs((renewed - requested).total_seconds()) < 1 + + +async def test_renew_host_renews_bootstrapping_host(client): + # A host still waiting on device discovery / keyscan is already caller-owned; + # its keepalive must not 409 just because activation hasn't finished. + host = await create_host_record( + name="lb-renew-boot", + status=HostStatus.BOOTSTRAPPING.value, + expires_at=utc_now() + timedelta(minutes=5), + ) + + response = await client.post( + f"/hosts/{host.id}/renew", + headers={"Authorization": "Bearer service-token"}, + ) + + assert response.status_code == 200 + assert datetime.fromisoformat(response.json()["expires_at"]) > utc_now() + + +async def test_renew_host_rejects_non_renewable_statuses(client): + for index, host_status in enumerate( + ( + HostStatus.PROVISIONING.value, + HostStatus.CREATING_NETWORK.value, + HostStatus.CREATING_VM.value, + HostStatus.ERROR.value, + ), + start=1, + ): + host = await create_host_record(name=f"lb-renew-blocked-{index}", status=host_status) + + response = await client.post( + f"/hosts/{host.id}/renew", + headers={"Authorization": "Bearer service-token"}, + ) + + assert response.status_code == 409 + assert response.json()["error_code"] == "HOST_STATE" + + +async def test_renew_host_rejects_unclaimed_pool_host(client): + # Warm pool members are managed by pool maintenance; a caller may only + # renew a host it owns (claimed, or demand-provisioned). + host = await create_host_record( + name="lb-renew-pool", + status=HostStatus.ACTIVE.value, + pool_member=True, + expires_at=utc_now() + timedelta(hours=4), + ) + + response = await client.post( + f"/hosts/{host.id}/renew", + headers={"Authorization": "Bearer service-token"}, + ) + + assert response.status_code == 409 + assert response.json()["error_code"] == "HOST_STATE" + + async with async_session_factory() as session: + persisted = await session.get(Host, host.id) + assert persisted is not None + assert persisted.expires_at == host.expires_at + + +async def test_renew_host_allows_claimed_pool_host(client): + host = await create_host_record( + name="lb-renew-claimed", + status=HostStatus.ACTIVE.value, + pool_member=True, + claimed_at=utc_now(), + expires_at=utc_now() + timedelta(hours=4), + ) + + response = await client.post( + f"/hosts/{host.id}/renew", + headers={"Authorization": "Bearer service-token"}, + ) + + assert response.status_code == 200 + + +async def test_renew_host_returns_404_for_missing_host(client): + response = await client.post( + f"/hosts/{uuid.uuid4()}/renew", + headers={"Authorization": "Bearer service-token"}, + ) + + assert response.status_code == 404 + assert response.json()["detail"] == "host not found" + + +async def test_renew_host_rejects_past_expires_at(client): + host = await create_host_record(name="lb-renew-past", status=HostStatus.ACTIVE.value) + + response = await client.post( + f"/hosts/{host.id}/renew", + headers={"Authorization": "Bearer service-token"}, + json={"expires_at": "2020-01-01T12:00:00+00:00"}, + ) + + assert response.status_code == 422 + detail = response.json()["detail"] + assert detail[0]["loc"] == ["body", "expires_at"] + assert "in the future" in detail[0]["msg"] + + +async def test_renew_host_rejects_naive_expires_at(client): + host = await create_host_record(name="lb-renew-naive", status=HostStatus.ACTIVE.value) + + response = await client.post( + f"/hosts/{host.id}/renew", + headers={"Authorization": "Bearer service-token"}, + json={"expires_at": "2099-01-01T12:00:00"}, + ) + + assert response.status_code == 422 + detail = response.json()["detail"] + assert detail[0]["loc"] == ["body", "expires_at"] + + +async def test_renew_host_rejects_missing_or_bad_service_auth(client): + host_id = uuid.uuid4() + + missing_response = await client.post(f"/hosts/{host_id}/renew") + bad_response = await client.post( + f"/hosts/{host_id}/renew", + headers={"Authorization": "Bearer wrong-token"}, + ) + + assert missing_response.status_code == 401 + assert bad_response.status_code == 403 + + async def test_delete_host_maps_database_failure_to_503(client, monkeypatch): # A SQLAlchemyError during teardown must surface as a controlled 503, like # create_host's DB-failure path, not an unhandled 500. diff --git a/src/hosts/tests/test_janitor.py b/src/hosts/tests/test_janitor.py index 5d6c6fc..936d9fa 100644 --- a/src/hosts/tests/test_janitor.py +++ b/src/hosts/tests/test_janitor.py @@ -6,7 +6,7 @@ from core.database import async_session_factory from hosts.janitor import reap_expired_hosts from hosts.models import Host, HostStatus -from hosts.service import utc_now +from hosts.service import HostService, utc_now from providers.exe.settings import ExeSettings @@ -100,6 +100,29 @@ async def test_janitor_ignores_hosts_without_expires_at(monkeypatch): assert await session.get(Host, host.id) is not None +async def test_janitor_spares_a_renewed_host(monkeypatch): + # The keepalive contract: a lapsed-but-not-yet-reaped host whose owner + # renews in time survives the next janitor pass. + monkeypatch.setattr("networking.tailscale.Tailscale.release_device", AsyncMock()) + delete_vm = AsyncMock() + monkeypatch.setattr("providers.exe.provider.ExeProvider.delete_vm", delete_vm) + host = await _create_host( + name="lb-sandbox-renewed", + status=HostStatus.ACTIVE.value, + expires_at=datetime.now(UTC) - timedelta(minutes=1), + ) + + async with async_session_factory() as session: + await HostService(session).renew_host(host.id) + + reaped = await reap_expired_hosts() + + assert reaped == [] + delete_vm.assert_not_awaited() + async with async_session_factory() as session: + assert await session.get(Host, host.id) is not None + + async def test_janitor_force_reaps_abandoned_early_state_host(monkeypatch): # An expired safety TTL on an early-state row means provisioning was # abandoned (a live provision's TTL is still in the future). The janitor diff --git a/src/hosts/tests/test_pool.py b/src/hosts/tests/test_pool.py index 56387e0..4800e7a 100644 --- a/src/hosts/tests/test_pool.py +++ b/src/hosts/tests/test_pool.py @@ -238,12 +238,12 @@ async def test_pool_claim_overrides_pool_max_age_with_caller_expires_at( assert abs((claimed.expires_at - caller_expires_at).total_seconds()) < 1 -async def test_pool_claim_preserves_pool_ttl_when_caller_omits_expires_at( +async def test_pool_claim_stamps_default_lease_when_caller_omits_expires_at( pooled_settings, monkeypatch ): - # Regression test for findings #2/#3: a claimed host without caller TTL - # used to have its expires_at wiped to NULL; the pool's max-age safety - # net must survive the claim so a forgotten DELETE doesn't leak forever. + # A claim without a caller expires_at carries the default lease, replacing + # the warm-pool max-age TTL — so a claimed host never leaks forever and + # its owner can keep it alive through renewal. pool_expires_at = datetime.now(UTC) + timedelta(hours=4) await _seed_pool_host(name="lb-pool-1", expires_at=pool_expires_at) monkeypatch.setattr("hosts.service.HostService.provision", AsyncMock()) @@ -252,8 +252,22 @@ async def test_pool_claim_preserves_pool_ttl_when_caller_omits_expires_at( service = HostService(session, settings=pooled_settings) claimed = await service.get_or_create_host(env={}, image=None) # no expires_at + lease_end = utc_now() + timedelta(seconds=pooled_settings.lease_default_ttl) assert claimed.expires_at is not None - assert abs((claimed.expires_at - pool_expires_at).total_seconds()) < 1 + assert abs((claimed.expires_at - lease_end).total_seconds()) < 5 + + +async def test_pool_claim_honors_explicit_permanent_lease(pooled_settings, monkeypatch): + # An explicit expires_at=None is the caller's deliberate opt-in to a + # permanent host; the claim must clear the warm-pool max-age TTL. + await _seed_pool_host(name="lb-pool-1", expires_at=datetime.now(UTC) + timedelta(hours=4)) + monkeypatch.setattr("hosts.service.HostService.provision", AsyncMock()) + + async with async_session_factory() as session: + service = HostService(session, settings=pooled_settings) + claimed = await service.get_or_create_host(env={}, image=None, expires_at=None) + + assert claimed.expires_at is None async def test_concurrent_claims_dont_grab_the_same_pool_host(pooled_settings, monkeypatch): From f2566c772677e4b25667a6ac940ae9794f99e042 Mon Sep 17 00:00:00 2001 From: Paulo Date: Tue, 7 Jul 2026 01:07:53 +0200 Subject: [PATCH 2/4] ENG-524: janitor re-validates the lease under the row lock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The janitor snapshots expired host ids, then deletes each one later under its own row lock — a renewal committing in that window returned 200 while the same janitor pass still tore the VM down. delete_host now takes expired_only (janitor-only, like pool_shed): if the locked read shows a live lease again, the host is spared. Co-Authored-By: Claude Fable 5 --- src/hosts/janitor.py | 16 ++++++++++------ src/hosts/service.py | 12 +++++++++++- src/hosts/tests/test_janitor.py | 23 +++++++++++++++++++++++ 3 files changed, 44 insertions(+), 7 deletions(-) diff --git a/src/hosts/janitor.py b/src/hosts/janitor.py index 42bba7a..a175e81 100644 --- a/src/hosts/janitor.py +++ b/src/hosts/janitor.py @@ -17,11 +17,13 @@ async def reap_expired_hosts() -> list[uuid.UUID]: """Delete hosts whose ``expires_at`` has passed. Iterates candidate ids without holding row locks; each delete acquires its - own per-row lock through HostService.delete_host. Rows still in an early - provisioning state are force-reaped: their expires_at is the safety TTL - (created + provisioning_grace_seconds), so an expired one means provisioning - was abandoned — a live provision's TTL is still in the future. The force - path tears down any VM partial provisioning may have created. + own per-row lock through HostService.delete_host and re-validates the lease + there (expired_only), so a renewal that lands after selection spares the + host. Rows still in an early provisioning state are force-reaped: their + expires_at is the safety TTL (created + provisioning_grace_seconds), so an + expired one means provisioning was abandoned — a live provision's TTL is + still in the future. The force path tears down any VM partial provisioning + may have created. """ async with async_session_factory() as session: now = utc_now() @@ -39,7 +41,9 @@ async def reap_expired_hosts() -> list[uuid.UUID]: for host_id in expired_ids: try: async with async_session_factory() as session: - await HostService(session, tailscale=tailscale).delete_host(host_id, force=True) + await HostService(session, tailscale=tailscale).delete_host( + host_id, force=True, expired_only=True + ) except ResourceNotFoundError: # Another janitor cycle, or an explicit DELETE, already removed it. continue diff --git a/src/hosts/service.py b/src/hosts/service.py index c3b3e0b..ca82e6c 100644 --- a/src/hosts/service.py +++ b/src/hosts/service.py @@ -340,7 +340,12 @@ async def renew_host(self, host_id: uuid.UUID, *, expires_at: datetime | None = return host async def delete_host( - self, host_id: uuid.UUID, *, force: bool = False, pool_shed: bool = False + self, + host_id: uuid.UUID, + *, + force: bool = False, + pool_shed: bool = False, + expired_only: bool = False, ) -> None: host = await self.get_host_for_update(host_id) @@ -352,6 +357,11 @@ async def delete_host( # excess and this locked read — leave it for its owner, don't reap it. return + if expired_only and (not host.expires_at or host.expires_at > utc_now()): + # The owner renewed this host between the janitor selecting it as + # expired and this locked read — the lease is live again, spare it. + return + if not force and host.status in DELETE_BLOCKED_STATUSES: raise HostStateError("host is still provisioning") diff --git a/src/hosts/tests/test_janitor.py b/src/hosts/tests/test_janitor.py index 936d9fa..36b86df 100644 --- a/src/hosts/tests/test_janitor.py +++ b/src/hosts/tests/test_janitor.py @@ -123,6 +123,29 @@ async def test_janitor_spares_a_renewed_host(monkeypatch): assert await session.get(Host, host.id) is not None +async def test_janitor_delete_skips_a_host_renewed_in_the_race(monkeypatch): + # A host renewed between the janitor's expired-id selection and the locked + # per-row delete must be spared — a 200 renewal is a keepalive promise. + delete_vm = AsyncMock() + monkeypatch.setattr("providers.exe.provider.ExeProvider.delete_vm", delete_vm) + host = await _create_host( + name="lb-sandbox-raced", + status=HostStatus.ACTIVE.value, + expires_at=datetime.now(UTC) - timedelta(minutes=1), + ) + # The janitor already selected this id as expired; the owner's renewal + # commits before the janitor's delete acquires the row lock. + async with async_session_factory() as session: + await HostService(session).renew_host(host.id) + + async with async_session_factory() as session: + await HostService(session).delete_host(host.id, force=True, expired_only=True) + + delete_vm.assert_not_awaited() + async with async_session_factory() as session: + assert await session.get(Host, host.id) is not None + + async def test_janitor_force_reaps_abandoned_early_state_host(monkeypatch): # An expired safety TTL on an early-state row means provisioning was # abandoned (a live provision's TTL is still in the future). The janitor From d07ef5866dada3a643a9d4ffe149cf503d5aeca9 Mon Sep 17 00:00:00 2001 From: Paulo Date: Tue, 7 Jul 2026 02:16:57 +0200 Subject: [PATCH 3/4] ENG-524: keep grace TTL in flight; report only real reaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review findings. First, resolving the default lease up front put the full lease on the in-flight provisioning row, so an abandoned provision would leak until the lease instead of reaping after PROVISIONING_GRACE_SECONDS. The omitted-lease sentinel now travels to the points where a lease is actually stamped — the pool claim, or the post-provision TTL rewrite — so a default create keeps just the safety TTL while provisioning and the lease starts when the host is usable. Second, delete_host now returns whether it actually deleted the row; the janitor no longer logs or returns a renewal-race-spared host as reaped, and pool shedding no longer counts a claim-race-spared host as removed. Co-Authored-By: Claude Fable 5 --- src/hosts/janitor.py | 5 ++++- src/hosts/pool.py | 4 ++-- src/hosts/service.py | 34 +++++++++++++++++++++------------ src/hosts/tests/test_api.py | 26 +++++++++++++++++++++++++ src/hosts/tests/test_janitor.py | 32 ++++++++++++++++++++++++++++++- src/hosts/tests/test_pool.py | 3 ++- 6 files changed, 87 insertions(+), 17 deletions(-) diff --git a/src/hosts/janitor.py b/src/hosts/janitor.py index a175e81..8af560c 100644 --- a/src/hosts/janitor.py +++ b/src/hosts/janitor.py @@ -41,7 +41,7 @@ async def reap_expired_hosts() -> list[uuid.UUID]: for host_id in expired_ids: try: async with async_session_factory() as session: - await HostService(session, tailscale=tailscale).delete_host( + deleted = await HostService(session, tailscale=tailscale).delete_host( host_id, force=True, expired_only=True ) except ResourceNotFoundError: @@ -51,6 +51,9 @@ async def reap_expired_hosts() -> list[uuid.UUID]: log.exception("janitor: failed to reap expired host: host_id=%s", host_id) continue else: + if not deleted: + # Renewed between selection and the locked delete — spared. + continue log.info("janitor: reaped expired host: host_id=%s", host_id) reaped.append(host_id) finally: diff --git a/src/hosts/pool.py b/src/hosts/pool.py index b4efe45..451bb93 100644 --- a/src/hosts/pool.py +++ b/src/hosts/pool.py @@ -141,8 +141,8 @@ async def _maintain( try: async with async_session_factory() as session: service = HostService(session, settings=settings, tailscale=tailscale) - await service.delete_host(host_id, pool_shed=True) - removed_excess += 1 + if await service.delete_host(host_id, pool_shed=True): + removed_excess += 1 except Exception: log.exception("pool: failed to shed excess pool host_id=%s", host_id) diff --git a/src/hosts/service.py b/src/hosts/service.py index ca82e6c..99072f7 100644 --- a/src/hosts/service.py +++ b/src/hosts/service.py @@ -95,10 +95,10 @@ async def get_or_create_host( provider: str | None = None, ) -> Host: # ``...`` (omitted) means "default lease"; an explicit None is the - # caller's deliberate opt-in to a permanent, never-reaped host. - if expires_at is ...: - expires_at = self._default_lease_expires_at() - + # caller's deliberate opt-in to a permanent, never-reaped host. The + # sentinel travels to the point where a lease is actually stamped + # (pool claim, or the post-provision rewrite) so the in-flight row + # keeps the short provisioning safety TTL. if provider: registered = get_provider_names() if provider not in registered: @@ -139,7 +139,7 @@ async def get_or_create_host( return host async def _try_claim_pool_host( - self, *, provider: str, expires_at: datetime | None + self, *, provider: str, expires_at: datetime | None | EllipsisType ) -> Host | None: # Pick a candidate, then atomically claim it with UPDATE ... WHERE # claimed_at IS NULL ... RETURNING. The WHERE predicate is the actual @@ -163,6 +163,8 @@ async def _try_claim_pool_host( if candidate_id is None: return None + if expires_at is ...: + expires_at = self._default_lease_expires_at() # The claim replaces the warm-pool max-age TTL with the caller's lease: # a concrete window (explicit or the default), or None for a caller # who deliberately opted into a permanent host. @@ -193,8 +195,6 @@ async def create_host( # Always provisions a brand-new VM; the pool maintainer calls this # directly (with pool_member=True) so it never recursively claims its # own pool members. - if expires_at is ...: - expires_at = self._default_lease_expires_at() vm = get_vm_provider(provider) uid = uuid7() name = Host.build_name(uid) @@ -202,9 +202,15 @@ async def create_host( host_image = image or vm.default_image # Safety TTL covers the strand window: if the client disconnects # mid-provision, this is what makes the janitor reap the row + VM. - # Replaced with the caller's value after provisioning succeeds. + # Replaced with the caller's value after provisioning succeeds. A + # default-lease create keeps just the safety TTL in flight — the + # lease is stamped only once the host is usable. safety_expires_at = now + timedelta(seconds=self.settings.provisioning_grace_seconds) - initial_expires_at = max(expires_at, safety_expires_at) if expires_at else safety_expires_at + initial_expires_at = ( + max(expires_at, safety_expires_at) + if isinstance(expires_at, datetime) + else safety_expires_at + ) host = Host( id=uid, env=env, @@ -231,6 +237,8 @@ async def create_host( # in a dedicated session so we don't extend ``self.session``'s # transaction (which can perturb advisory-lock-bearing callers like # the pool maintainer). + if expires_at is ...: + expires_at = self._default_lease_expires_at() async with async_session_factory() as ttl_session: fresh = await ttl_session.get(Host, host.id) if fresh is not None: @@ -346,7 +354,8 @@ async def delete_host( force: bool = False, pool_shed: bool = False, expired_only: bool = False, - ) -> None: + ) -> bool: + """Delete the host; return False when a maintenance guard spared it.""" host = await self.get_host_for_update(host_id) if host is None: @@ -355,12 +364,12 @@ async def delete_host( if pool_shed and host.claimed_at: # A caller claimed this host between the maintainer selecting it as # excess and this locked read — leave it for its owner, don't reap it. - return + return False if expired_only and (not host.expires_at or host.expires_at > utc_now()): # The owner renewed this host between the janitor selecting it as # expired and this locked read — the lease is live again, spare it. - return + return False if not force and host.status in DELETE_BLOCKED_STATUSES: raise HostStateError("host is still provisioning") @@ -395,6 +404,7 @@ async def delete_host( ) await self.session.delete(host) await self.session.commit() + return True async def provision(self, host_id: str) -> None: host = await self.get_host(uuid.UUID(host_id)) diff --git a/src/hosts/tests/test_api.py b/src/hosts/tests/test_api.py index 2818ed9..ba13fdf 100644 --- a/src/hosts/tests/test_api.py +++ b/src/hosts/tests/test_api.py @@ -223,6 +223,32 @@ async def test_create_host_defaults_expires_at_to_lease_ttl(client, monkeypatch) assert before + ttl <= expires_at <= after + ttl +async def test_create_host_keeps_safety_ttl_while_provisioning(monkeypatch): + # The in-flight row carries the short provisioning-grace TTL, not the full + # default lease, so an abandoned provision reaps after the grace window; + # the lease itself is stamped once the host is usable. + inflight = {} + + async def capture_inflight_expiry(self, host_id): + async with async_session_factory() as session: + row = await session.get(Host, uuid.UUID(host_id)) + assert row is not None + inflight["expires_at"] = row.expires_at + + monkeypatch.setattr("hosts.service.HostService.provision", capture_inflight_expiry) + grace = timedelta(seconds=get_settings().provisioning_grace_seconds) + ttl = timedelta(seconds=get_settings().lease_default_ttl) + + async with async_session_factory() as session: + before = utc_now() + host = await HostService(session).create_host(env={}, image=None) + after = utc_now() + + assert before + grace <= inflight["expires_at"] <= after + grace + assert host.expires_at is not None + assert before + ttl <= host.expires_at <= after + ttl + + async def test_create_host_explicit_null_expires_at_creates_permanent_host(client, monkeypatch): # expires_at: null is the caller's deliberate opt-in to a host with no # expiry — permanence must be a choice, never an accident. diff --git a/src/hosts/tests/test_janitor.py b/src/hosts/tests/test_janitor.py index 36b86df..6507df1 100644 --- a/src/hosts/tests/test_janitor.py +++ b/src/hosts/tests/test_janitor.py @@ -139,8 +139,38 @@ async def test_janitor_delete_skips_a_host_renewed_in_the_race(monkeypatch): await HostService(session).renew_host(host.id) async with async_session_factory() as session: - await HostService(session).delete_host(host.id, force=True, expired_only=True) + deleted = await HostService(session).delete_host(host.id, force=True, expired_only=True) + assert not deleted + delete_vm.assert_not_awaited() + async with async_session_factory() as session: + assert await session.get(Host, host.id) is not None + + +async def test_reap_does_not_report_a_host_renewed_in_the_race(monkeypatch): + # End-to-end reporting under the renewal race: the janitor selected the id + # as expired, the owner's renewal lands before the locked delete — the + # host survives and must not be logged or returned as reaped. + delete_vm = AsyncMock() + monkeypatch.setattr("providers.exe.provider.ExeProvider.delete_vm", delete_vm) + + real_delete_host = HostService.delete_host + + async def renew_then_delete(self, host_id, **kwargs): + async with async_session_factory() as session: + await HostService(session).renew_host(host_id) + return await real_delete_host(self, host_id, **kwargs) + + monkeypatch.setattr("hosts.service.HostService.delete_host", renew_then_delete) + host = await _create_host( + name="lb-sandbox-race-report", + status=HostStatus.ACTIVE.value, + expires_at=datetime.now(UTC) - timedelta(minutes=1), + ) + + reaped = await reap_expired_hosts() + + assert reaped == [] delete_vm.assert_not_awaited() async with async_session_factory() as session: assert await session.get(Host, host.id) is not None diff --git a/src/hosts/tests/test_pool.py b/src/hosts/tests/test_pool.py index 4800e7a..7463471 100644 --- a/src/hosts/tests/test_pool.py +++ b/src/hosts/tests/test_pool.py @@ -509,8 +509,9 @@ async def test_pool_shed_skips_a_host_claimed_in_the_race(pooled_settings, monke async with async_session_factory() as session: service = HostService(session, settings=pooled_settings) - await service.delete_host(claimed.id, pool_shed=True) + deleted = await service.delete_host(claimed.id, pool_shed=True) + assert not deleted delete_vm.assert_not_awaited() async with async_session_factory() as session: assert await session.get(Host, claimed.id) is not None From fe60209e18183a7660992e4f45eabe580644b3a8 Mon Sep 17 00:00:00 2001 From: Paulo Date: Tue, 7 Jul 2026 02:42:26 +0200 Subject: [PATCH 4/4] ENG-524: create finalization must not clobber a renewal; black-box surface Two review findings. First, the post-provision TTL rewrite stamped the create-time lease unconditionally, discarding a renewal made while the host was still bootstrapping (a client whose POST timed out can find the row and renew it). The rewrite is now a guarded UPDATE on the in-flight safety value, so newer intent wins. Second, the renew route joins the black-box OpenAPI surface: full-api.spec.js expects POST /hosts/{host_id}/renew and exercises it against the live created host (auth, extend, 404). Co-Authored-By: Claude Fable 5 --- api-tests/tests/full-api.spec.js | 15 +++++++++++++++ src/hosts/service.py | 16 ++++++++++------ src/hosts/tests/test_api.py | 26 ++++++++++++++++++++++++++ 3 files changed, 51 insertions(+), 6 deletions(-) diff --git a/api-tests/tests/full-api.spec.js b/api-tests/tests/full-api.spec.js index 2937f10..093d10b 100644 --- a/api-tests/tests/full-api.spec.js +++ b/api-tests/tests/full-api.spec.js @@ -16,6 +16,7 @@ const EXPECTED_OPENAPI_OPERATIONS = [ "POST /http-proxies", "POST /http-proxies/{name}/hosts/{host_id}", "POST /hosts", + "POST /hosts/{host_id}/renew", ]; const HOST_KEYS = [ @@ -248,6 +249,20 @@ test.describe("Drukbox API", () => { createdProxyName = null; }); + test("POST /hosts/{host_id}/renew extends the host lease", async () => { + await expectStatus(await publicApi.post(`/hosts/${createdHost.id}/renew`), 401); + await expectStatus(await badTokenApi.post(`/hosts/${createdHost.id}/renew`), 403); + + const renewed = await expectJson(await api.post(`/hosts/${createdHost.id}/renew`), 200); + expectHost(renewed); + expect(renewed.id).toBe(createdHost.id); + expect(Date.parse(renewed.expires_at)).toBeGreaterThan(Date.now()); + + const missingId = "00000000-0000-0000-0000-000000000000"; + const missing = await expectJson(await api.post(`/hosts/${missingId}/renew`), 404); + expect(missing.detail).toBe("host not found"); + }); + test("DELETE /hosts/{host_id} tears down the created host", async () => { await expectStatus(await publicApi.delete(`/hosts/${createdHost.id}`), 401); await expectStatus(await badTokenApi.delete(`/hosts/${createdHost.id}`), 403); diff --git a/src/hosts/service.py b/src/hosts/service.py index 99072f7..b6c9837 100644 --- a/src/hosts/service.py +++ b/src/hosts/service.py @@ -236,15 +236,19 @@ async def create_host( # Provisioning won: replace the safety TTL with the caller's intent # in a dedicated session so we don't extend ``self.session``'s # transaction (which can perturb advisory-lock-bearing callers like - # the pool maintainer). + # the pool maintainer). Guarded on the in-flight value: a renewal + # that landed while the host was bootstrapping is newer intent and + # must not be clobbered. if expires_at is ...: expires_at = self._default_lease_expires_at() async with async_session_factory() as ttl_session: - fresh = await ttl_session.get(Host, host.id) - if fresh is not None: - fresh.expires_at = expires_at - fresh.updated_at = utc_now() - await ttl_session.commit() + await ttl_session.execute( + update(Host) + .where(Host.id == host.id) + .where(Host.expires_at == initial_expires_at) + .values(expires_at=expires_at, updated_at=utc_now()) + ) + await ttl_session.commit() await self.session.refresh(host) return host diff --git a/src/hosts/tests/test_api.py b/src/hosts/tests/test_api.py index ba13fdf..143b4a5 100644 --- a/src/hosts/tests/test_api.py +++ b/src/hosts/tests/test_api.py @@ -249,6 +249,32 @@ async def capture_inflight_expiry(self, host_id): assert before + ttl <= host.expires_at <= after + ttl +async def test_create_host_preserves_renewal_made_during_provisioning(monkeypatch): + # A client whose POST timed out can find the bootstrapping row and renew + # it; the create's post-provision TTL rewrite must not clobber that newer + # lease when provisioning finishes. + renewal = {} + + async def renew_mid_provision(self, host_id): + async with async_session_factory() as session: + row = await session.get(Host, uuid.UUID(host_id)) + assert row is not None + row.status = HostStatus.BOOTSTRAPPING.value + await session.commit() + async with async_session_factory() as session: + renewed = await HostService(session).renew_host( + uuid.UUID(host_id), expires_at=utc_now() + timedelta(days=7) + ) + renewal["expires_at"] = renewed.expires_at + + monkeypatch.setattr("hosts.service.HostService.provision", renew_mid_provision) + + async with async_session_factory() as session: + host = await HostService(session).create_host(env={}, image=None) + + assert host.expires_at == renewal["expires_at"] + + async def test_create_host_explicit_null_expires_at_creates_permanent_host(client, monkeypatch): # expires_at: null is the caller's deliberate opt-in to a host with no # expiry — permanence must be a choice, never an accident.