From 83d279487c5ccba6b5b0c6521b82b7bb913d0b90 Mon Sep 17 00:00:00 2001 From: Philippe Parage <69145356+pparage@users.noreply.github.com> Date: Mon, 10 Aug 2026 12:40:28 +0200 Subject: [PATCH 1/2] fix(proxmox): make host registration idempotent on name The deploy bundle POSTs /v1/proxmox/hosts on every scenario run, and nothing stopped a second row under an existing name: create_host never checked for a clash and proxmox_hosts.name had no constraint, so the documented "idempotent via 409" was never reachable. Each re-run added a host, and the UI picker filled up with duplicates of the same hypervisor. Delete-and-recreate was not an option for the fix: deployments.target_host_id is a FK to proxmox_hosts.id, so a new row per re-run strands every earlier deployment on a host nobody updates. POST now upserts on name and keeps the id, returning 200 instead of 201. Credentials are refreshed too, so a rotated PVE token reaches the backend on the next deploy instead of leaving it authenticating with a stale one -- there is no update route to fix it by hand. Migration 0002 collapses duplicates already in the field before adding the constraint: for each name it keeps the earliest added_at row, repoints any deployments at it, deletes the rest, and logs every collapse. --- .../versions/0002_proxmox_host_unique_name.py | 70 ++++++++++ app/core/models.py | 5 + app/routes/v1/proxmox/hosts.py | 58 +++++++-- openapi.json | 11 ++ tests/routes/test_proxmox_hosts.py | 119 +++++++++++++++++ tests/test_migration_dedupe_proxmox_hosts.py | 121 ++++++++++++++++++ 6 files changed, 376 insertions(+), 8 deletions(-) create mode 100644 alembic/versions/0002_proxmox_host_unique_name.py create mode 100644 tests/test_migration_dedupe_proxmox_hosts.py diff --git a/alembic/versions/0002_proxmox_host_unique_name.py b/alembic/versions/0002_proxmox_host_unique_name.py new file mode 100644 index 0000000..4a85812 --- /dev/null +++ b/alembic/versions/0002_proxmox_host_unique_name.py @@ -0,0 +1,70 @@ +"""dedupe proxmox_hosts, then enforce a unique name + +Until now nothing stopped a second host row under an existing name, and the +deploy bundle re-POSTs the same name on every scenario run — so field +databases carry one duplicate per re-run. The duplicates are not inert: +``deployments.target_host_id`` points at whichever one existed when the +deployment was created. + +Collapsing keys on ``added_at``: the earliest row under a name is the one the +first deploy registered, so it is the row most likely to be referenced and the +one whose id callers may have recorded. Deployments on the later duplicates are +repointed at it before those rows are deleted, so nothing is left dangling. + +Revision ID: 0002_proxmox_host_unique_name +Revises: 0001_v1_initial +Create Date: 2026-08-10 +""" +import logging + +import sqlalchemy as sa +from alembic import op + +revision = '0002_proxmox_host_unique_name' +down_revision = '0001_v1_initial' +branch_labels = None +depends_on = None + +log = logging.getLogger("alembic.runtime.migration") + + +def upgrade() -> None: + conn = op.get_bind() + + # added_at first, id as a deterministic tie-break for rows registered + # within the same clock tick. + rows = conn.execute( + sa.text("SELECT id, name FROM proxmox_hosts ORDER BY name, added_at, id") + ).fetchall() + + keepers: dict[str, str] = {} + for host_id, name in rows: + keeper_id = keepers.setdefault(name, host_id) + if keeper_id == host_id: + continue + + moved = conn.execute( + sa.text( + "UPDATE deployments SET target_host_id = :keeper" + " WHERE target_host_id = :loser" + ), + {"keeper": keeper_id, "loser": host_id}, + ).rowcount + conn.execute( + sa.text("DELETE FROM proxmox_hosts WHERE id = :loser"), + {"loser": host_id}, + ) + log.info( + "proxmox_hosts: collapsed duplicate %r (%s) into %s, " + "repointed %d deployment(s)", + name, host_id, keeper_id, moved, + ) + + with op.batch_alter_table("proxmox_hosts") as batch: + batch.create_unique_constraint("uq_proxmox_host_name", ["name"]) + + +def downgrade() -> None: + # Only the constraint is reversible; the collapsed rows are gone for good. + with op.batch_alter_table("proxmox_hosts") as batch: + batch.drop_constraint("uq_proxmox_host_name", type_="unique") diff --git a/app/core/models.py b/app/core/models.py index da1ce88..ad3f0b6 100644 --- a/app/core/models.py +++ b/app/core/models.py @@ -57,6 +57,11 @@ class ProxmoxHost(Base): protected_vmids_override_json: Mapped[str | None] = mapped_column(Text) added_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), default=_utcnow) last_health_check_json: Mapped[str | None] = mapped_column(Text) + # A host is identified by its name: the deploy bundle re-POSTs the same + # name on every scenario run, and deployments.target_host_id is a FK here, + # so a second row per re-run would strand earlier deployments on a host + # nobody updates. Uniqueness turns those re-runs into in-place updates. + __table_args__ = (UniqueConstraint("name", name="uq_proxmox_host_name"),) class Project(Base): diff --git a/app/routes/v1/proxmox/hosts.py b/app/routes/v1/proxmox/hosts.py index 2d34b51..cb1081f 100644 --- a/app/routes/v1/proxmox/hosts.py +++ b/app/routes/v1/proxmox/hosts.py @@ -7,7 +7,7 @@ from datetime import datetime, timezone import httpx -from fastapi import APIRouter, Depends, status +from fastapi import APIRouter, Depends, Response, status from sqlalchemy import select from sqlalchemy.ext.asyncio import AsyncSession @@ -68,11 +68,57 @@ async def list_hosts( @router.post( - "/hosts", response_model=HostOut, status_code=status.HTTP_201_CREATED + "/hosts", + response_model=HostOut, + status_code=status.HTTP_201_CREATED, + responses={ + 200: { + "model": HostOut, + "description": "Host already registered under this name; updated in place.", + } + }, ) async def create_host( - payload: HostIn, session: AsyncSession = Depends(_session) + payload: HostIn, + response: Response, + session: AsyncSession = Depends(_session), ): + """Register a Proxmox host, or refresh the one already under that name. + + The deploy bundle POSTs this on every scenario run, so it has to be + idempotent. Re-registering keeps the existing row's id — ``deployments`` + reference it by FK — and returns 200 instead of 201. Credentials are part + of what gets refreshed: a rotated PVE token reaches the backend on the next + deploy rather than leaving it authenticating with a stale one. + """ + overrides_json = ( + json.dumps(payload.protected_vmids_override) + if payload.protected_vmids_override + else None + ) + + existing = ( + await session.execute( + select(ProxmoxHost).where(ProxmoxHost.name == payload.name) + ) + ).scalar_one_or_none() + if existing is not None: + existing.api_url = str(payload.api_url) + existing.node_name = payload.node_name + existing.token_ref = payload.token_ref + existing.token_scope = payload.token_scope + existing.default_bridge = payload.default_bridge + existing.protected_vmids_override_json = overrides_json + # added_at deliberately untouched: it records when this host was first + # registered, and the dedupe migration keys on it. + await session.commit() + await session.refresh(existing) + log.info( + "proxmox_host_reregistered", host_id=existing.id, name=existing.name + ) + response.status_code = status.HTTP_200_OK + return _row_to_out(existing) + row = ProxmoxHost( id=uuid.uuid4().hex[:16], name=payload.name, @@ -81,11 +127,7 @@ async def create_host( token_ref=payload.token_ref, token_scope=payload.token_scope, default_bridge=payload.default_bridge, - protected_vmids_override_json=( - json.dumps(payload.protected_vmids_override) - if payload.protected_vmids_override - else None - ), + protected_vmids_override_json=overrides_json, ) session.add(row) await session.commit() diff --git a/openapi.json b/openapi.json index ee455fd..1191214 100644 --- a/openapi.json +++ b/openapi.json @@ -3507,6 +3507,7 @@ "v1 proxmox" ], "summary": "Create Host", + "description": "Register a Proxmox host, or refresh the one already under that name.\n\nThe deploy bundle POSTs this on every scenario run, so it has to be\nidempotent. Re-registering keeps the existing row's id \u2014 ``deployments``\nreference it by FK \u2014 and returns 200 instead of 201. Credentials are part\nof what gets refreshed: a rotated PVE token reaches the backend on the next\ndeploy rather than leaving it authenticating with a stale one.", "operationId": "create_host_v1_proxmox_hosts_post", "requestBody": { "required": true, @@ -3538,6 +3539,16 @@ } } } + }, + "200": { + "description": "Host already registered under this name; updated in place.", + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/HostOut" + } + } + } } } } diff --git a/tests/routes/test_proxmox_hosts.py b/tests/routes/test_proxmox_hosts.py index 79b1c88..2ac1b54 100644 --- a/tests/routes/test_proxmox_hosts.py +++ b/tests/routes/test_proxmox_hosts.py @@ -120,3 +120,122 @@ async def test_health_unreachable_host_returns_unreachable_status( assert body["status"] == "unreachable" finally: await dbmod.dispose_engine() + + +@pytest.mark.asyncio +async def test_reseeding_the_same_host_updates_it_in_place(tmp_path, monkeypatch): + """A scenario re-run POSTs the same name; it must not pile up duplicates. + + The id has to survive: deployments.target_host_id is a FK to it, so a new + row per re-run would strand every earlier deployment on a stale host. + """ + app, dbmod = await _boot(tmp_path, monkeypatch) + try: + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://t" + ) as c: + first = await c.post( + "/v1/proxmox/hosts", + json={ + "name": "pve01", + "api_url": "https://pve01:8006", + "node_name": "pve01", + "token_ref": "r42@pam!tok=abc", + }, + ) + assert first.status_code == 201, first.text + + second = await c.post( + "/v1/proxmox/hosts", + json={ + "name": "pve01", + "api_url": "https://pve01.lan:8006", + "node_name": "pve-node-2", + "token_ref": "r42@pam!tok=rotated", + }, + ) + assert second.status_code == 200, second.text + assert second.json()["id"] == first.json()["id"] + # pydantic HttpUrl normalises a bare-host URL with a trailing slash + assert second.json()["api_url"] == "https://pve01.lan:8006/" + assert second.json()["node_name"] == "pve-node-2" + + listing = await c.get("/v1/proxmox/hosts") + assert listing.json()["total"] == 1 + finally: + await dbmod.dispose_engine() + + +@pytest.mark.asyncio +async def test_reseeding_refreshes_a_rotated_token(tmp_path, monkeypatch): + """The stored PVE token must follow the re-seed, or deploys 401 forever.""" + app, dbmod = await _boot(tmp_path, monkeypatch) + try: + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://t" + ) as c: + await c.post( + "/v1/proxmox/hosts", + json={ + "name": "pve01", + "api_url": "https://pve01:8006", + "node_name": "pve01", + "token_ref": "r42@pam!tok=stale", + }, + ) + await c.post( + "/v1/proxmox/hosts", + json={ + "name": "pve01", + "api_url": "https://pve01:8006", + "node_name": "pve01", + "token_ref": "r42@pam!tok=fresh", + }, + ) + + from sqlalchemy import select + from app.core.models import ProxmoxHost + async with dbmod.get_session_factory()() as session: + stored = ( + await session.execute(select(ProxmoxHost.token_ref)) + ).scalars().all() + assert stored == ["r42@pam!tok=fresh"] + finally: + await dbmod.dispose_engine() + + +@pytest.mark.asyncio +async def test_a_second_host_under_a_different_name_is_still_created( + tmp_path, monkeypatch +): + """Upsert keys on name only — a genuinely new host must still register.""" + app, dbmod = await _boot(tmp_path, monkeypatch) + try: + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://t" + ) as c: + a = await c.post( + "/v1/proxmox/hosts", + json={ + "name": "pve01", + "api_url": "https://pve01:8006", + "node_name": "pve01", + "token_ref": "r42@pam!tok=a", + }, + ) + b = await c.post( + "/v1/proxmox/hosts", + json={ + "name": "pve02", + "api_url": "https://pve02:8006", + "node_name": "pve02", + "token_ref": "r42@pam!tok=b", + }, + ) + assert b.status_code == 201, b.text + assert b.json()["id"] != a.json()["id"] + + listing = await c.get("/v1/proxmox/hosts") + assert listing.json()["total"] == 2 + finally: + await dbmod.dispose_engine() diff --git a/tests/test_migration_dedupe_proxmox_hosts.py b/tests/test_migration_dedupe_proxmox_hosts.py new file mode 100644 index 0000000..3ef1374 --- /dev/null +++ b/tests/test_migration_dedupe_proxmox_hosts.py @@ -0,0 +1,121 @@ +"""Migration 0002: collapse duplicate proxmox_hosts before enforcing uniqueness. + +Every scenario re-run POSTed the same host name and got a brand-new row, so +databases in the field carry duplicates. The unique constraint cannot be added +on top of them, and the extra rows are not inert: deployments hold a FK to +whichever duplicate happened to exist at create time. +""" +import sqlite3 +from importlib import reload + +import pytest + + +def _alembic_config(): + from alembic.config import Config + + return Config("alembic.ini") + + +@pytest.fixture +def db_at_0001(tmp_path, monkeypatch): + """A database migrated to the revision that predates the constraint.""" + from alembic import command + + db = tmp_path / "m.db" + monkeypatch.setenv("RANGE42_DB_URL", f"sqlite+aiosqlite:///{db}") + monkeypatch.setenv("RANGE42_WORKSPACE_ROOT", str(tmp_path)) + from app.core import config as cfg + + reload(cfg) + command.upgrade(_alembic_config(), "0001_v1_initial") + return db + + +def _insert_host(conn, host_id, name, added_at, token): + conn.execute( + "INSERT INTO proxmox_hosts (id, name, api_url, node_name, token_ref," + " default_bridge, added_at) VALUES (?, ?, ?, ?, ?, ?, ?)", + (host_id, name, "https://pve:8006", "pve", token, "vmbr0", added_at), + ) + + +def _insert_deployment(conn, dep_id, codename, target_host_id): + conn.execute( + "INSERT INTO deployments (id, codename, scenario_label, project_id," + " target_host_id, team_count, state, workspace_path, created_at," + " updated_at) VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)", + ( + dep_id, + codename, + "demo_lab", + "proj-1", + target_host_id, + 1, + "planned", + f"/tmp/{codename}", + "2026-07-01 00:00:00", + "2026-07-01 00:00:00", + ), + ) + + +def test_upgrade_keeps_the_oldest_duplicate_and_repoints_deployments(db_at_0001): + from alembic import command + + with sqlite3.connect(db_at_0001) as conn: + _insert_host(conn, "oldest", "pve01", "2026-06-01 00:00:00", "tok-1") + _insert_host(conn, "middle", "pve01", "2026-07-02 00:00:00", "tok-2") + _insert_host(conn, "newest", "pve01", "2026-08-01 00:00:00", "tok-3") + _insert_host(conn, "other", "pve02", "2026-06-15 00:00:00", "tok-4") + # Deployments created against the later duplicates must not be stranded. + _insert_deployment(conn, "dep-a", "alpha", "middle") + _insert_deployment(conn, "dep-b", "bravo", "newest") + _insert_deployment(conn, "dep-c", "charlie", "other") + conn.commit() + + command.upgrade(_alembic_config(), "head") + + with sqlite3.connect(db_at_0001) as conn: + hosts = dict(conn.execute("SELECT name, id FROM proxmox_hosts").fetchall()) + deployments = dict( + conn.execute("SELECT id, target_host_id FROM deployments").fetchall() + ) + + assert hosts == {"pve01": "oldest", "pve02": "other"} + assert deployments == {"dep-a": "oldest", "dep-b": "oldest", "dep-c": "other"} + + +def test_upgrade_rejects_a_second_host_under_an_existing_name(db_at_0001): + from alembic import command + + with sqlite3.connect(db_at_0001) as conn: + _insert_host(conn, "h1", "pve01", "2026-06-01 00:00:00", "tok-1") + conn.commit() + + command.upgrade(_alembic_config(), "head") + + with sqlite3.connect(db_at_0001) as conn: + with pytest.raises(sqlite3.IntegrityError): + _insert_host(conn, "h2", "pve01", "2026-06-02 00:00:00", "tok-2") + + +def test_upgrade_is_a_no_op_on_a_database_with_no_duplicates(db_at_0001): + from alembic import command + + with sqlite3.connect(db_at_0001) as conn: + _insert_host(conn, "h1", "pve01", "2026-06-01 00:00:00", "tok-1") + _insert_host(conn, "h2", "pve02", "2026-06-02 00:00:00", "tok-2") + _insert_deployment(conn, "dep-a", "alpha", "h1") + conn.commit() + + command.upgrade(_alembic_config(), "head") + + with sqlite3.connect(db_at_0001) as conn: + hosts = dict(conn.execute("SELECT name, id FROM proxmox_hosts").fetchall()) + deployments = dict( + conn.execute("SELECT id, target_host_id FROM deployments").fetchall() + ) + + assert hosts == {"pve01": "h1", "pve02": "h2"} + assert deployments == {"dep-a": "h1"} From db105a35fd5c2e85a43081beda0027d363d166cd Mon Sep 17 00:00:00 2001 From: Philippe Parage <69145356+pparage@users.noreply.github.com> Date: Mon, 10 Aug 2026 12:53:08 +0200 Subject: [PATCH 2/2] fix(proxmox): carry the newest credentials through dedupe, recover from a lost race Two problems found in review of the previous commit. The migration kept the oldest duplicate wholesale. Duplicates accumulated one per scenario re-run, so the newest row holds the credentials in force -- and keeping the first row resurrected a token that may have been rotated away, with every deployment repointed at it. The keeper now takes the newest duplicate's api_url / node / token / bridge / overrides while keeping its own id and added_at. The earlier test asserted on ids only, which is why it did not catch this. The upsert's duplicate check and its commit are separated by an await, so two concurrent registrations of the same new name could both take the insert path -- WEB_CONCURRENCY=1 bounds this to one process but not to one task. The loser now rolls back, re-reads the winner and applies its update, matching how create_deployment already handles the same shape. --- .../versions/0002_proxmox_host_unique_name.py | 59 +++++++++++++----- app/routes/v1/proxmox/hosts.py | 61 ++++++++++++------ tests/routes/test_proxmox_hosts.py | 62 +++++++++++++++++++ tests/test_migration_dedupe_proxmox_hosts.py | 44 ++++++++++++- 4 files changed, 192 insertions(+), 34 deletions(-) diff --git a/alembic/versions/0002_proxmox_host_unique_name.py b/alembic/versions/0002_proxmox_host_unique_name.py index 4a85812..d8cd291 100644 --- a/alembic/versions/0002_proxmox_host_unique_name.py +++ b/alembic/versions/0002_proxmox_host_unique_name.py @@ -37,27 +37,58 @@ def upgrade() -> None: sa.text("SELECT id, name FROM proxmox_hosts ORDER BY name, added_at, id") ).fetchall() - keepers: dict[str, str] = {} + groups: dict[str, list[str]] = {} for host_id, name in rows: - keeper_id = keepers.setdefault(name, host_id) - if keeper_id == host_id: + groups.setdefault(name, []).append(host_id) + + for name, ids in groups.items(): + if len(ids) == 1: continue + keeper_id, *loser_ids = ids - moved = conn.execute( + # The id is the oldest row's, because deployments reference it. The + # connection details are the NEWEST row's: duplicates accumulated one + # per scenario re-run, so the last registration holds the credentials + # in force. Keeping the first row wholesale would resurrect a token + # that may since have been rotated away, and every deployment + # repointed at it would start failing auth the moment this ran. + conn.execute( sa.text( - "UPDATE deployments SET target_host_id = :keeper" - " WHERE target_host_id = :loser" + "UPDATE proxmox_hosts SET" + " api_url = latest.api_url," + " node_name = latest.node_name," + " token_ref = latest.token_ref," + " token_scope = latest.token_scope," + " default_bridge = latest.default_bridge," + " protected_vmids_override_json =" + " latest.protected_vmids_override_json," + " last_health_check_json = latest.last_health_check_json" + " FROM (SELECT * FROM proxmox_hosts WHERE id = :newest) AS latest" + " WHERE proxmox_hosts.id = :keeper" ), - {"keeper": keeper_id, "loser": host_id}, - ).rowcount - conn.execute( - sa.text("DELETE FROM proxmox_hosts WHERE id = :loser"), - {"loser": host_id}, + {"newest": loser_ids[-1], "keeper": keeper_id}, ) + + for loser_id in loser_ids: + moved = conn.execute( + sa.text( + "UPDATE deployments SET target_host_id = :keeper" + " WHERE target_host_id = :loser" + ), + {"keeper": keeper_id, "loser": loser_id}, + ).rowcount + conn.execute( + sa.text("DELETE FROM proxmox_hosts WHERE id = :loser"), + {"loser": loser_id}, + ) + log.info( + "proxmox_hosts: collapsed duplicate %r (%s) into %s, " + "repointed %d deployment(s)", + name, loser_id, keeper_id, moved, + ) log.info( - "proxmox_hosts: collapsed duplicate %r (%s) into %s, " - "repointed %d deployment(s)", - name, host_id, keeper_id, moved, + "proxmox_hosts: %r kept id %s with the connection details from %s", + name, keeper_id, loser_ids[-1], ) with op.batch_alter_table("proxmox_hosts") as batch: diff --git a/app/routes/v1/proxmox/hosts.py b/app/routes/v1/proxmox/hosts.py index cb1081f..71819ba 100644 --- a/app/routes/v1/proxmox/hosts.py +++ b/app/routes/v1/proxmox/hosts.py @@ -9,6 +9,7 @@ import httpx from fastapi import APIRouter, Depends, Response, status from sqlalchemy import select +from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession from app.core.errors import AuthFailedError, Range42Error @@ -47,6 +48,26 @@ def _row_to_out(row: ProxmoxHost) -> HostOut: ) +async def _find_host_by_name( + session: AsyncSession, name: str +) -> ProxmoxHost | None: + return ( + await session.execute( + select(ProxmoxHost).where(ProxmoxHost.name == name) + ) + ).scalar_one_or_none() + + +def _refresh_host(row: ProxmoxHost, payload: HostIn, overrides_json: str | None) -> None: + """Carry a re-registration onto an existing row, id and added_at intact.""" + row.api_url = str(payload.api_url) + row.node_name = payload.node_name + row.token_ref = payload.token_ref + row.token_scope = payload.token_scope + row.default_bridge = payload.default_bridge + row.protected_vmids_override_json = overrides_json + + @router.get("/hosts", response_model=Page[HostOut]) async def list_hosts( session: AsyncSession = Depends(_session), @@ -97,27 +118,19 @@ async def create_host( else None ) - existing = ( - await session.execute( - select(ProxmoxHost).where(ProxmoxHost.name == payload.name) - ) - ).scalar_one_or_none() - if existing is not None: - existing.api_url = str(payload.api_url) - existing.node_name = payload.node_name - existing.token_ref = payload.token_ref - existing.token_scope = payload.token_scope - existing.default_bridge = payload.default_bridge - existing.protected_vmids_override_json = overrides_json + async def _update_in_place(row: ProxmoxHost) -> HostOut: # added_at deliberately untouched: it records when this host was first # registered, and the dedupe migration keys on it. + _refresh_host(row, payload, overrides_json) await session.commit() - await session.refresh(existing) - log.info( - "proxmox_host_reregistered", host_id=existing.id, name=existing.name - ) + await session.refresh(row) + log.info("proxmox_host_reregistered", host_id=row.id, name=row.name) response.status_code = status.HTTP_200_OK - return _row_to_out(existing) + return _row_to_out(row) + + existing = await _find_host_by_name(session, payload.name) + if existing is not None: + return await _update_in_place(existing) row = ProxmoxHost( id=uuid.uuid4().hex[:16], @@ -130,7 +143,19 @@ async def create_host( protected_vmids_override_json=overrides_json, ) session.add(row) - await session.commit() + try: + await session.commit() + except IntegrityError: + # Lost the race: another request registered this name between the + # lookup above and this commit. Recover into the update path instead + # of surfacing a 500 — the caller asked for a registration and one + # now exists, which is the outcome they wanted. + await session.rollback() + winner = await _find_host_by_name(session, payload.name) + if winner is None: + raise + log.info("proxmox_host_register_race", name=payload.name, host_id=winner.id) + return await _update_in_place(winner) await session.refresh(row) return _row_to_out(row) diff --git a/tests/routes/test_proxmox_hosts.py b/tests/routes/test_proxmox_hosts.py index 2ac1b54..9ce09da 100644 --- a/tests/routes/test_proxmox_hosts.py +++ b/tests/routes/test_proxmox_hosts.py @@ -239,3 +239,65 @@ async def test_a_second_host_under_a_different_name_is_still_created( assert listing.json()["total"] == 2 finally: await dbmod.dispose_engine() + + +@pytest.mark.asyncio +async def test_losing_the_race_on_a_new_name_still_upserts(tmp_path, monkeypatch): + """Two concurrent registrations of the same new name must not 500. + + WEB_CONCURRENCY=1 keeps this to one process, but asyncio still interleaves + across the await between the duplicate check and the commit: both requests + can find nothing and both take the insert path. The loser of that race hits + uq_proxmox_host_name and has to recover into the update path rather than + surfacing an IntegrityError. + """ + app, dbmod = await _boot(tmp_path, monkeypatch) + try: + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://t" + ) as c: + first = await c.post( + "/v1/proxmox/hosts", + json={ + "name": "pve01", + "api_url": "https://pve01:8006", + "node_name": "pve01", + "token_ref": "r42@pam!tok=winner", + }, + ) + assert first.status_code == 201, first.text + + # Blind ONLY the pre-check, and only once: that is exactly what the + # racing request sees. The recovery path re-reads and must find the + # row the winner committed. + import app.routes.v1.proxmox.hosts as hosts_mod + + real_lookup = hosts_mod._find_host_by_name + seen = {"calls": 0} + + async def _blind_first_lookup(session, name): + seen["calls"] += 1 + if seen["calls"] == 1: + return None + return await real_lookup(session, name) + + monkeypatch.setattr( + hosts_mod, "_find_host_by_name", _blind_first_lookup + ) + + second = await c.post( + "/v1/proxmox/hosts", + json={ + "name": "pve01", + "api_url": "https://pve01:8006", + "node_name": "pve01", + "token_ref": "r42@pam!tok=loser", + }, + ) + assert second.status_code == 200, second.text + assert second.json()["id"] == first.json()["id"] + + listing = await c.get("/v1/proxmox/hosts") + assert listing.json()["total"] == 1 + finally: + await dbmod.dispose_engine() diff --git a/tests/test_migration_dedupe_proxmox_hosts.py b/tests/test_migration_dedupe_proxmox_hosts.py index 3ef1374..1fb455f 100644 --- a/tests/test_migration_dedupe_proxmox_hosts.py +++ b/tests/test_migration_dedupe_proxmox_hosts.py @@ -32,11 +32,13 @@ def db_at_0001(tmp_path, monkeypatch): return db -def _insert_host(conn, host_id, name, added_at, token): +def _insert_host( + conn, host_id, name, added_at, token, api_url="https://pve:8006", node="pve" +): conn.execute( "INSERT INTO proxmox_hosts (id, name, api_url, node_name, token_ref," " default_bridge, added_at) VALUES (?, ?, ?, ?, ?, ?, ?)", - (host_id, name, "https://pve:8006", "pve", token, "vmbr0", added_at), + (host_id, name, api_url, node, token, "vmbr0", added_at), ) @@ -119,3 +121,41 @@ def test_upgrade_is_a_no_op_on_a_database_with_no_duplicates(db_at_0001): assert hosts == {"pve01": "h1", "pve02": "h2"} assert deployments == {"dep-a": "h1"} + + +def test_upgrade_carries_the_newest_credentials_onto_the_surviving_row(db_at_0001): + """Keep the oldest id, but not the oldest credentials. + + Duplicates accumulated one per scenario re-run, so the LAST row holds the + token the operator is actually using. Keeping the first row wholesale would + resurrect a token that may have been rotated away, and every deployment + repointed at it would start failing auth the moment the migration ran. + """ + from alembic import command + + with sqlite3.connect(db_at_0001) as conn: + _insert_host( + conn, "oldest", "pve01", "2026-06-01 00:00:00", "tok-revoked", + api_url="https://old-pve:8006", node="old-node", + ) + _insert_host( + conn, "newest", "pve01", "2026-08-01 00:00:00", "tok-current", + api_url="https://pve01.lan:8006", node="pve-node-2", + ) + conn.commit() + + command.upgrade(_alembic_config(), "head") + + with sqlite3.connect(db_at_0001) as conn: + row = conn.execute( + "SELECT id, added_at, api_url, node_name, token_ref FROM proxmox_hosts" + ).fetchone() + + host_id, added_at, api_url, node_name, token = row + # identity and registration date stay with the row deployments point at + assert host_id == "oldest" + assert added_at.startswith("2026-06-01") + # ...but the connection details come from the most recent registration + assert api_url == "https://pve01.lan:8006" + assert node_name == "pve-node-2" + assert token == "tok-current"