refactor(deploy): extract the workspace durability contract from the route - #133
Merged
Conversation
…route
Four review rounds found four ways to get one ordering wrong: a
duplicate clobbering a live workspace, a committed row with no password,
a foreign-key error reported as a name clash, and a secret outliving the
row it belonged to. Each fix added another branch to create_deployment,
which by then owned workspace creation, name reservation, secret
seeding, transaction management and token provisioning.
app/core/workspace_secrets now owns the rule those fixes were all
circling: the vault password is durable if and only if the deployment
row is. vault_seed is a context manager, so the caller cannot apply the
seed and forget the revert —
with vault_seed(ws.path, payload.secrets):
await session.commit()
reverts the file exactly when the commit does not happen. Proxmox token
provisioning moves out too, staying deliberately outside that contract:
it talks to Proxmox, not to the workspace's durability.
Behaviour-preserving. The ten route tests written across those four
rounds pass unchanged, and the new unit tests own the contract directly,
including revert-on-BaseException (a cancelled request) and a revert
that itself fails not masking the original error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-through on the note at the end of #132. Behaviour-preserving.
Why
Four review rounds found four ways to get one ordering wrong:
Each fix added another branch to
create_deployment, which by then owned workspace creation, name reservation, secret seeding, transaction management and best-effort token provisioning. A fifth patch was the wrong move.What
app/core/workspace_secretsowns the rule all four fixes were circling:vault_seedis a context manager, so the caller cannot apply the seed and forget the revert:The file goes back to its previous state — removed if there was none, restored if an operator had seeded one — exactly when the commit does not happen. Reverting is best-effort and logs rather than raising, so it can never mask the failure that triggered it.
provision_host_tokenmoves out as well, and stays deliberately outside that contract: it talks to Proxmox rather than to the workspace's durability, and preflight remains the source of truth for whether the credential works.The handler drops from 175 to 136 lines and now reads as: validate → reserve → seed+commit → provision.
Verification
BaseException(a cancelled request must not leave the secret) and a revert that itself fails not masking the original errorfirst=201 duplicate=409 vault=ORIGINAL,deployments persisted: 1, and both bad references reporting404 NOT_FOUND488 passed, ruff clean.