fix(deploy): reject duplicate codenames before mutating the shared workspace - #130
Conversation
…rkspace (codename, scenario_label) is unique and also names the workspace directory, so a duplicate POST had Workspace.create hand back the existing deployment's directory. The vault password was written there before the commit, so the duplicate overwrote a live deployment's password and then failed with an opaque 500 from the IntegrityError — leaving the original unable to decrypt its vault or unlock its SSH keys. Two changes: check for the clash up front and return a typed 409 naming the existing deployment, and move both workspace-mutating steps (vault password, best-effort proxmox token provisioning) after the row is committed, so a failed create never touches another deployment's files. Reproduced before the fix — second POST raised IntegrityError and the file read back ATTACKER instead of ORIGINAL; after, 409 and the original password intact. Found by Codex review on #129.
![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: 43cc0e874e
ℹ️ 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".
| workspace_path=str(ws.path), | ||
| ) | ||
| session.add(row) | ||
| await session.commit() |
There was a problem hiding this comment.
When a supplied vault password cannot be written—for example because the disk fills, permissions change, or the process exits after this commit—the deployment row remains permanently committed even though the POST fails or disconnects before seeding the required secret. A retry with the same name then receives the new 409 conflict, and there is no API that updates the password or removes the deployment row, leaving the deployment unusable without manual intervention; reserve the unique name within the transaction but commit only after the required file write succeeds, or compensate by removing the row on failure.
Useful? React with 👍 / 👎.
| select(Deployment).where( | ||
| Deployment.codename == payload.codename, | ||
| Deployment.scenario_label == payload.scenario_label, | ||
| ) | ||
| )).scalar_one_or_none() |
There was a problem hiding this comment.
When two POSTs for the same (codename, scenario_label) overlap, both can complete this read before either insert commits, so both proceed and the unique constraint rejects one during session.commit(). That request still leaks an IntegrityError as an opaque 500 instead of the promised DEPLOYMENT_EXISTS 409; handle the constraint failure after rollback or use an atomic insertion/reservation rather than relying solely on this check.
Useful? React with 👍 / 👎.
1 participant
P1 from Codex review on #129, confirmed by reproduction. My change in that PR introduced it.
The corruption path
(codename, scenario_label)is unique and names the workspace directory, so a second POST with the same pair hasWorkspace.createhand back the existing deployment's directory. The vault password was written there beforesession.commit(), so:vault_pass.txtReproduced before the fix:
After:
Fix
DEPLOYMENT_EXISTS) naming the existing deployment, instead of letting the commit fail. The 500 was a bug in its own right — a duplicate codename is a user error, not a server fault.The regression test asserts the 409, that the details name the original deployment, and that the original password reads back unchanged. It fails against the merged version with the raw
IntegrityError.473 passed, ruff clean.