From e3f0b97fe9b0c40ac2823e68f5615acc8a33a1af Mon Sep 17 00:00:00 2001 From: Philippe Parage <69145356+pparage@users.noreply.github.com> Date: Thu, 6 Aug 2026 12:37:52 +0200 Subject: [PATCH] fix(deploy): make the vault seed atomic with the deployment row MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit My previous fix traded one failure mode for another: committing before writing the secret meant a failed write (disk full, permissions, crash) left a deployment row whose workspace has no vault password. The obvious retry then hit the new 409 instead of fixing itself, so the deployment was unusable until someone deleted it by hand. Reserve the name with flush() instead of commit(): the INSERT takes the unique constraint at DB level — closing the race the pre-check cannot — while the transaction stays open. The secret is written inside that window and the commit follows. A write failure now rolls the row back and returns a typed VAULT_SEED_FAILED, leaving the caller free to retry. A concurrent duplicate that slips past the pre-check surfaces from flush() as the same 409 rather than an opaque 500. Proxmox token provisioning stays after the commit: it is best-effort, swallows its own errors, and touches Proxmox rather than the workspace. Found by Codex review on the previous fix (43cc0e8). --- app/routes/v1/deployments/crud.py | 52 +++++++++++++++++----- tests/routes/test_deployment_vault_seed.py | 41 +++++++++++++++++ 2 files changed, 82 insertions(+), 11 deletions(-) diff --git a/app/routes/v1/deployments/crud.py b/app/routes/v1/deployments/crud.py index 6146e9d..2ae219c 100644 --- a/app/routes/v1/deployments/crud.py +++ b/app/routes/v1/deployments/crud.py @@ -12,6 +12,7 @@ from fastapi import APIRouter, Depends, status from sqlalchemy import select +from sqlalchemy.exc import IntegrityError from sqlalchemy.ext.asyncio import AsyncSession from app.core.config import settings @@ -91,23 +92,52 @@ async def create_deployment(payload: DeploymentCreate, workspace_path=str(ws.path), ) session.add(row) - await session.commit() - await session.refresh(row) + # Flush, do not commit: this reserves (codename, scenario_label) at the DB + # level — closing the race the pre-check above cannot — while leaving the + # transaction open. The secret is written inside that window, so a failed + # write rolls the row back and the caller can simply retry. Committing + # first would strand a deployment whose workspace has no password, and the + # retry would then hit the 409 rather than fixing itself. + try: + await session.flush() + except IntegrityError as e: + await session.rollback() + raise Range42Error( + error="conflict", + code="DEPLOYMENT_EXISTS", + status=409, + message=( + f"A deployment named {payload.codename}/" + f"{payload.scenario_label} already exists" + ), + details=[{"field": "codename", "reason": "created concurrently"}], + ) from e - # Everything below mutates the workspace, so it runs only once the row is - # persisted — a failed create must never touch another deployment's files. - # # Seed the workspace vault password: deploy_trigger reads # /secrets/vault_pass.txt to set ANSIBLE_VAULT_PASSWORD_FILE and to # unlock the SSH keys for the run, and nothing else writes it. Absent or # empty leaves an operator-seeded file alone. if payload.secrets and payload.secrets.get("vault_password"): - vault_pass_file = ws.path / "secrets" / "vault_pass.txt" - vault_pass_file.parent.mkdir(parents=True, exist_ok=True) - # chmod before the content so the secret is never briefly world-readable. - vault_pass_file.touch(mode=0o600, exist_ok=True) - vault_pass_file.chmod(0o600) - vault_pass_file.write_text(payload.secrets["vault_password"]) + try: + vault_pass_file = ws.path / "secrets" / "vault_pass.txt" + vault_pass_file.parent.mkdir(parents=True, exist_ok=True) + # chmod before the content so the secret is never briefly + # world-readable. + vault_pass_file.touch(mode=0o600, exist_ok=True) + vault_pass_file.chmod(0o600) + vault_pass_file.write_text(payload.secrets["vault_password"]) + except OSError as e: + await session.rollback() + raise Range42Error( + error="workspace_error", + code="VAULT_SEED_FAILED", + status=500, + message="Could not write the workspace vault password", + details=[{"field": "secrets.vault_password", "reason": str(e)}], + ) from e + + await session.commit() + await session.refresh(row) # Best-effort proxmox token provisioning: requires an app-level vault # password file and a proxmox_token secret in the payload. Missing diff --git a/tests/routes/test_deployment_vault_seed.py b/tests/routes/test_deployment_vault_seed.py index 6407d9e..405d136 100644 --- a/tests/routes/test_deployment_vault_seed.py +++ b/tests/routes/test_deployment_vault_seed.py @@ -5,6 +5,7 @@ created through the API could not decrypt anything — the UI collects the password on the deploy form and it had nowhere to go. """ +import pathlib import stat import pytest @@ -159,3 +160,43 @@ async def test_duplicate_codename_is_rejected_without_touching_the_workspace( assert body["code"] == "DEPLOYMENT_EXISTS" assert first.json()["id"] in str(body["details"]) assert vault_pass.read_text() == VAULT_PW, "the live deployment was clobbered" + + +@pytest.mark.asyncio +async def test_failed_seed_leaves_no_deployment_row(tmp_path, monkeypatch): + """A write failure must roll the deployment back, not strand it. + + Committing before the seed left a row whose workspace had no password, + and the obvious retry then hit the 409 instead of fixing itself — the + deployment was unusable until someone deleted it by hand. + """ + app = await _boot(tmp_path, monkeypatch) + + real_write = pathlib.Path.write_text + + def _boom(self, *a, **kw): + if self.name == "vault_pass.txt": + raise OSError(28, "No space left on device") + return real_write(self, *a, **kw) + + monkeypatch.setattr(pathlib.Path, "write_text", _boom) + + async with AsyncClient( + transport=ASGITransport(app=app), base_url="http://t" + ) as c: + failed = await c.post("/v1/deployments/", + json=_payload(secrets={"vault_password": VAULT_PW})) + assert failed.status_code == 500, failed.text + assert failed.json()["code"] == "VAULT_SEED_FAILED" + + listed = await c.get("/v1/deployments/") + assert listed.json()["items"] == [], "the failed create left a row behind" + + # The obvious next move must work rather than 409. + monkeypatch.setattr(pathlib.Path, "write_text", real_write) + retry = await c.post("/v1/deployments/", + json=_payload(secrets={"vault_password": VAULT_PW})) + assert retry.status_code == 201, retry.text + + vault_pass = tmp_path / "ws" / "ALPHA-demo_lab" / "secrets" / "vault_pass.txt" + assert vault_pass.read_text() == VAULT_PW