fix(deploy): report the real error, and undo a seed whose commit failed - #132
Conversation
Two more from Codex review of the atomicity fix. Foreign keys are enforced at DB level (PRAGMA foreign_keys=ON), so an unknown project_id or target_host_id reached the flush and raised the same IntegrityError as a name clash — the API answered 'A deployment named BRAVO/demo_lab already exists' for a project that does not exist. Validate both references up front with a proper 404, and translate only a uniqueness violation into 409; anything else keeps its own detail. If the commit failed after the password was written, the row was gone but vault_pass.txt remained. A later create that supplies no password skips the seed branch entirely, so it would silently adopt a secret belonging to a deployment that never existed. Snapshot the prior file and restore it — or remove it when there was none — when the commit fails. All three new tests fail against the previous commit.
![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: c4f723fc6f
ℹ️ 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".
| if vault_pass_file.is_file(): | ||
| prior_secret = vault_pass_file.read_text() |
There was a problem hiding this comment.
When an operator-seeded password file contains non-UTF-8 bytes, read_text() raises UnicodeDecodeError (which is not caught by the OSError handler), so a request supplying a replacement password fails with an unhandled 500 before the existing file can be overwritten. Text-mode snapshotting can also normalize line endings when the old secret is restored after a commit failure. Snapshotting and restoring the file with read_bytes()/write_bytes() preserves arbitrary password-file contents and permits replacement of non-UTF-8 secrets.
Useful? React with 👍 / 👎.
1 participant
Two P2s from Codex on the atomicity fix (#131). Both reproduce.
1. A bad reference was reported as "already exists"
PRAGMA foreign_keys=ON(app/core/db.py:24) means an unknownproject_idortarget_host_idraisesIntegrityErrorfrom the sameflush()as a name clash — and got translated into 409DEPLOYMENT_EXISTS:Now:
Both references are validated up front (matching how
patch_projecthandles a missing project), and theIntegrityErrorhandler now translates only a uniqueness violation into 409 — anything else keeps its own detail instead of being dressed up as a name clash.2. A failed commit left the secret behind
If
commit()failed after the password was written, the row was gone butvault_pass.txtremained in the workspace. The next create for that name without a password skips the seed branch entirely — so it would silently adopt a secret belonging to a deployment that never existed, and the orphaned secret would otherwise sit on disk indefinitely.The prior file is now snapshotted before writing and restored on commit failure — removed when there was none, put back when an operator had seeded one. Restoration is best-effort and logs rather than raising, so it cannot mask the original failure.
Tests
Three, all failing against the previous commit:
477 passed, ruff clean.
This is the fourth round on this one handler. Each round found something real, but that is a signal in itself: create-deployment now carries workspace creation, name reservation, secret seeding and best-effort token provisioning. Worth extracting the workspace-side effects behind a single unit with its own tests rather than continuing to patch the route — happy to open an issue if you agree.