fix(deploy): make the vault seed atomic with the deployment row - #131
Conversation
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).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e3f0b97fe9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try: | ||
| await session.flush() | ||
| except IntegrityError as e: | ||
| await session.rollback() | ||
| raise Range42Error( |
There was a problem hiding this comment.
When project_id or target_host_id references a nonexistent row, the foreign-key checks enabled in app/core/db.py make this flush raise the same IntegrityError, so the API now incorrectly returns 409 DEPLOYMENT_EXISTS even though no deployment with this name exists. Inspect the violated constraint before translating the exception, and reserve this response for uq_deployment_workspace.
Useful? React with 👍 / 👎.
| details=[{"field": "secrets.vault_password", "reason": str(e)}], | ||
| ) from e | ||
|
|
||
| await session.commit() |
There was a problem hiding this comment.
If the SQLite commit fails after the password write—for example because of an I/O or disk-full error—the request exits with no committed deployment while vault_pass.txt remains in the reused workspace. A later create without a password skips the seed branch and silently adopts that stale secret, and the failed request may also leave its secret on disk indefinitely; preserve/restore the prior file or remove the newly seeded file when this commit fails.
Useful? React with 👍 / 👎.
1 participant
Second P1 from Codex, this time on my own fix in #130. It is correct — I traded a clobber for an orphan.
What #130 left behind
#130 moved the secret write after
commit()so a failed create could not touch another deployment's files. But then a failed write (disk full, permissions, process death) leaves a committed deployment row whose workspace has no vault password. The obvious retry hits the 409 that PR added, so the deployment is stuck — recoverable only viaDELETE /v1/deployments/{id}, which nobody would think to reach for.Both orderings were wrong because the row and the secret were being made durable in separate steps.
Fix: reserve, write, then commit
session.flush()instead ofcommit()issues the INSERT — taking the(codename, scenario_label)unique constraint at DB level — while leaving the transaction open. The secret is written inside that window; the commit follows.rollback(), no row, typedVAULT_SEED_FAILED, retry works cleanlyflush()as the same 409 instead of an opaque 500. The pre-check alone was a TOCTOU race; this closes it.Proxmox token provisioning stays after the commit deliberately: it is best-effort, swallows its own errors, and touches Proxmox rather than the workspace, so it has no bearing on whether the row should exist.
Test
Patches
Path.write_textto raiseENOSPCforvault_pass.txt, then asserts: 500 withVAULT_SEED_FAILED, no deployment row persisted, and that an immediate retry succeeds with the password on disk. It errors against the previous implementation, where the OSError propagated uncaught and the row survived.474 passed, ruff clean.