feat(state): deploy state lives in the stage's Branch (ADR-0033)#113
Conversation
commit: |
1e41f0e to
7dbe618
Compare
| * describes, and the Branch delete is refused while the database is still a | ||
| * live member. | ||
| */ | ||
| export async function deleteStageStateDatabase( |
There was a problem hiding this comment.
This is a layering violation. Framework may not be aware of prisma cloud
There was a problem hiding this comment.
Fixed in 5f10c4d. You were right, and it was hiding under an exception scoped to something else: architecture.config.json says the framework domain imports nothing, and this only passed lint:deps via the crossDomainExceptions entry whose reason covers Project/Branch resolution — not state. The CLI no longer knows a state database exists: core's ExtensionDescriptor gains a teardown hook mirroring preflight, and the prismaCloud extension implements it. git grep lowering/state packages/0-framework/ returns nothing, and this file's net diff against main is now zero. The remaining Project/Branch leak (preflight's input shape, ensure-containers, run-alchemy) predates this PR and is recorded as the CLI/Prisma-Cloud decoupling successor project in the plan.
There was a problem hiding this comment.
Same fix, 5f10c4d — the destroy tail now loops extension.teardown?.({ projectId, branchId, stage }) exactly like the existing preflight loop, and the throw-vs-warn asymmetry moved into the extension with the deletion (its reasons are Prisma Cloud's: a Branch delete refuses while a database is attached; production's removal is cleanup and must not fail the command).
There was a problem hiding this comment.
Looks like you took all the code out of yhere and pulled it into the framework
There was a problem hiding this comment.
It reads that way from the shrink, but the code went sideways, not up: this file went 409 → 270 lines while discovery.ts (+158) and delete.ts (+63) appeared in the same package — lowering as a whole grew. The deletion here is the retired workspace-project discovery (resolveStateProject, candidate scanning, bareWorkspaceId); the extraction exists because bootstrap and delete must resolve the branch and list candidates identically, or a delete could remove something a bootstrap would have adopted. What had crossed into the framework was a ~30-line wrapper — the violation you flagged on ensure-containers.ts, gone in 5f10c4d.
| deleteStateDatabaseWith(target, verifyOwnership); | ||
|
|
||
| /** | ||
| * Test seam: identical to {@link deleteStateDatabase} but with the ownership |
There was a problem hiding this comment.
Wtf is a "test seam"? Is this test code? Why is it in production code?
There was a problem hiding this comment.
Label gone in 5f10c4d. It was production code with an injectable parameter, not test code — the doc now says plainly: identical to deleteStateDatabase, except the ownership check is a parameter so tests can supply a fake instead of opening a real Postgres connection. Folding the parameter away entirely (providing the verifier through the Effect context like ManagementClient) is deliberately left for the decoupling work rather than done in this PR.
| * cannot be read back afterwards, which is why every run mints a fresh | ||
| * connection instead of reusing one. | ||
| */ | ||
| export const mintConnection = ( |
There was a problem hiding this comment.
Does one "mint" a connection?
There was a problem hiding this comment.
No. createConnection as of 5f10c4d, prose included. (mintConnection predated this PR, but we'd reworded its doc and kept the verb, so it was ours to fix.)
| /** The stage the state store was being resolved for: a project id, or `projectId/branchId` for a named stage. */ | ||
| readonly target: string; |
There was a problem hiding this comment.
wtf is this? target already has a name and it's neither branch nor stage
There was a problem hiding this comment.
Renamed to container in 5f10c4d — the word deploy-cli.md already uses for "the Project, plus the Branch for a named stage". target was doubly wrong: it collided with the deploy target (ADR-0011) and named nothing real. The value is the project id, or projectId/branchId for a named stage.
| @@ -1,3 +1,5 @@ | |||
| export { deleteStateDatabase } from './delete.ts'; | |||
| export type { StateTarget } from './discovery.ts'; | |||
There was a problem hiding this comment.
Nothing, as of 5f10c4d — deleted. It was a byte-for-byte duplicate of ResolvedContainer (container.ts:23, same package) under a name that reads as "the deploy target for state". ResolvedContainer is used everywhere it appeared.
… lands Review response (PR #113). The destroy tail put a Prisma Cloud concept — the state database — inside the framework domain, hiding under a cross-domain exception whose reason string covers only Project/Branch resolution. The CLI now knows nothing about state: core's ExtensionDescriptor gains a `teardown` hook mirroring `preflight` (same input shape, same call site discipline), the prismaCloud extension implements it in teardown.ts, and the throw-vs-warn asymmetry moves with it into the domain that owns its reasons — a named stage's Branch delete would refuse while a database is attached, so failing early names the cause; production's removal is cleanup and must not fail the command. The hook still carries projectId/branchId, as preflight already does; removing that vocabulary from core entirely is the CLI/Prisma Cloud decoupling, tracked as its own follow-up project. The review's four naming findings, all applied: - StateTarget is deleted. It was a byte-for-byte duplicate of ResolvedContainer under a name that collides with the deploy target (ADR-0011); the existing type is used everywhere instead. - HostedStateBootstrapError's `target` field is now `container`, the word deploy-cli.md already uses for "the Project, plus the Branch for a named stage". - mintConnection is createConnection. One creates a connection; "mint" was decoration. - The "Test seam:" labels are gone; the `…With` variants' docs now say plainly that the ownership check is a parameter so tests can supply a fake instead of opening a real Postgres connection. The teardown tests stub the ownership check everywhere, including the production warn path, which previously fell back to the real verifier and would have opened a connection to a fake DSN. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
|
Review response, all five comments addressed in 54fdff0 + 0517a69:
All gates green locally (16/16 turbo tasks, casts delta 0, |
… lands Review response (PR #113). The destroy tail put a Prisma Cloud concept — the state database — inside the framework domain, hiding under a cross-domain exception whose reason string covers only Project/Branch resolution. The CLI now knows nothing about state: core's ExtensionDescriptor gains a `teardown` hook mirroring `preflight` (same input shape, same call site discipline), the prismaCloud extension implements it in teardown.ts, and the throw-vs-warn asymmetry moves with it into the domain that owns its reasons — a named stage's Branch delete would refuse while a database is attached, so failing early names the cause; production's removal is cleanup and must not fail the command. The hook still carries projectId/branchId, as preflight already does; removing that vocabulary from core entirely is the CLI/Prisma Cloud decoupling, tracked as its own follow-up project. The review's four naming findings, all applied: - StateTarget is deleted. It was a byte-for-byte duplicate of ResolvedContainer under a name that collides with the deploy target (ADR-0011); the existing type is used everywhere instead. - HostedStateBootstrapError's `target` field is now `container`, the word deploy-cli.md already uses for "the Project, plus the Branch for a named stage". - mintConnection is createConnection. One creates a connection; "mint" was decoration. - The "Test seam:" labels are gone; the `…With` variants' docs now say plainly that the ownership check is a parameter so tests can supply a fake instead of opening a real Postgres connection. The teardown tests stub the ownership check everywhere, including the production warn path, which previously fell back to the real verifier and would have opened a connection to a fake DSN. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
0517a69 to
1e78ccc
Compare
… lands Review response (PR #113). The destroy tail put a Prisma Cloud concept — the state database — inside the framework domain, hiding under a cross-domain exception whose reason string covers only Project/Branch resolution. The CLI now knows nothing about state: core's ExtensionDescriptor gains a `teardown` hook mirroring `preflight` (same input shape, same call site discipline), the prismaCloud extension implements it in teardown.ts, and the throw-vs-warn asymmetry moves with it into the domain that owns its reasons — a named stage's Branch delete would refuse while a database is attached, so failing early names the cause; production's removal is cleanup and must not fail the command. The hook still carries projectId/branchId, as preflight already does; removing that vocabulary from core entirely is the CLI/Prisma Cloud decoupling, tracked as its own follow-up project. The review's four naming findings, all applied: - StateTarget is deleted. It was a byte-for-byte duplicate of ResolvedContainer under a name that collides with the deploy target (ADR-0011); the existing type is used everywhere instead. - HostedStateBootstrapError's `target` field is now `container`, the word deploy-cli.md already uses for "the Project, plus the Branch for a named stage". - mintConnection is createConnection. One creates a connection; "mint" was decoration. - The "Test seam:" labels are gone; the `…With` variants' docs now say plainly that the ownership check is a parameter so tests can supply a fake instead of opening a real Postgres connection. The teardown tests stub the ownership check everywhere, including the production warn path, which previously fell back to the real verifier and would have opened a connection to a fake DSN. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
1e78ccc to
04cb231
Compare
Deploy state moves from the workspace-level prisma-composer-state project
into a framework-owned database of the same name inside each stage's own
Branch; production's rides the Project's implicit default Branch. State then
has exactly the lifetime of the environment it describes: deleting a Branch
or Project — from the CLI, the Console, or any future git integration —
takes its state with it, with no knowledge of the framework on the platform
side.
ADR-0009 rejected this as circular ("the project is created and destroyed by
the deploys whose state it would hold"). ADR-0023/0024 dissolved that: the
CLI creates the containers via the Management API before Alchemy runs and
deletes them after destroy, outside state entirely. The remaining objection
(fragmentation of workspace-level questions) answers to the platform — those
are Project and Branch listings, not state queries.
ADR-0009 is marked superseded and annotated in the index; ADR-0010 records
that the lock now lives in a per-stage store (redundant key kept) and that
deleting the store severs the lease, killing an in-flight deploy of a stage
being torn down. ADR-0024 gains the distinction the PDP team confirmed: "the
default stage lives at the Project level" describes addressing, not physics
— the platform attaches every resource to a Branch, defaulting to the
Project's implicit one.
The sweep for prose describing the workspace store as current also reached
ADR-0011, ADR-0023, ADR-0030 (service-key values), layering.md's
provisioning-state spectrum, and alchemy-lowering.md. The deploying guide
gains the destroy ordering and a manual cutover procedure — there is no
automated migration.
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
Two slices: this one (ADR-0033 and the documentation corrections) and the implementation of the per-branch bootstrap plus the destroy ordering the ADR commits us to. design-notes.md carries the implementation-grade design the second slice executes, including the platform facts it rests on, verified against pdp-control-plane rather than inferred. The plan also records the successor project — the Compute GitHub App integration for Composer repos — which this project is a prerequisite for: containment makes webhook-driven branch teardown correct, and project-scoped state is what lets a build hold a project-scoped credential instead of a workspace-scoped one. assets/pdp-asks.md is the standalone write-up of the two open questions for the PDP team (per-stage database quota/billing; the dependency contract of project deletion). Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Both questions this project was going to ask the PDP team are answerable from their source, so they are answered rather than asked. The plan limit on databases is workspace-wide — counted across every project — so each stage's state store takes one of the workspace's 50 (free) or 1000 (paid) slots. Nothing bills per database: `createDatabase` carries a limit and never a price, and Postgres bills storage in GiB-hours and queries per cycle, both of which round to nothing for a few rows of JSON read only at deploy. The case that can bite is a free-plan workspace running many preview stages against the same 50 slots, so the ADR says that rather than "one extra small database, no big deal". The second answer corrects something I had wrong. Project deletion is blocked only by *active compute deployments*: deleteProject deletes every one of the project's database tenants itself, and deleteWithGuard refuses only on a live ComputeVersion, then cascades. Neither Branches nor databases ever block it — the schema comment on App.branchId says so outright. So the production state database was never going to wedge deleteAppProject the way I claimed; that step stays because production's state should not outlive production and hold a slot when another stage keeps the Project alive. The named-stage step is unaffected and genuinely is a precondition: DELETE /v1/branches does refuse a Branch with live members. The design notes now also point at the pdp-control-plane checkout in this worktree rather than the older one in ~/Projects — they had already diverged. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Implements ADR-0033. Bootstrap no longer hunts the workspace for a project named prisma-composer-state; it resolves the state database inside the Branch the CLI already resolved for the deploy, and destroy removes that database before the container it lives in. `prismaState()` now reads PRISMA_PROJECT_ID and (named stages only) PRISMA_BRANCH_ID — the ids run-alchemy already sets on the child process — so the store is addressed directly instead of discovered by listing every project in the workspace and inspecting same-named candidates. `PRISMA_WORKSPACE_ID` leaves the state layer entirely. Production passes no branch and the store resolves the project's default Branch read-only, by its isDefault flag: the platform guarantees every live project owns one, and creating it is the platform's business, not ours. The store's schema, its lock, its service, and the transient store are untouched, as is resource lowering. `discovery.ts` exists because bootstrap and delete must resolve the branch and list candidates identically — two copies would drift, and drifting apart is exactly how a delete removes something a bootstrap would have adopted. Ownership is verified by marker before adopting a database and again before deleting one. Deleting by name alone would take a user's database that happens to share the name, which is the guessing ADR-0005 forbids; delete.test.ts pins this with a stub that throws rather than a stub that returns. Destroy's two failure modes are deliberately different. A named stage throws: the Branch delete would fail anyway while a database is attached, so failing at the state step names the real cause. Production warns and continues, inheriting deleteAppProject's stance that failing a command over a cleanup step is worse than leaving a shell behind. Verified live against a real workspace, not only in tests: a named stage's state database lands on that stage's Branch and the round trip leaves no database, no Branch, and no Project; production's lands on the implicit default Branch and is removed before the Project; a stage whose Branch is torn down by the platform's cascading path loses its state with it and redeploys from a genuinely fresh store. No workspace-level state project is created by any path. That run also corrected the ADR. The platform has two Branch-delete paths, not one: the cascading teardown (git integration, idle-preview reclaim) removes the state database along with every other child, while the guarded delete the Console uses refuses outright while a database is attached. The consequences now say so instead of claiming every platform surface cascades. skills/prisma-composer/SKILL.md said state was workspace-hosted and shared by every deployer. It is neither, and it is a file agents read. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
The design notes now carry what actually happened rather than what I first claimed: the flat database endpoint is a server-side create-then-attach and narrows the stray-database window rather than closing it; project deletion is blocked only by active compute deployments; there are two platform Branch-delete paths and only the cascading one cleans state up. Each was found by the implementer or the reviewer reading source I had asserted from a schema. Also recorded: the residual we accepted cannot be regression-tested with an honest fake, a failed deploy strands its containers because ensureContainers runs before preflight, and the follow-ups this slice deliberately left alone. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
The state bootstrap takes 6.3 seconds on a stage's first deploy, not the minute-plus I claimed. pn-widgets runs 89s here against 92s on main — no measurable regression. Two e2e runs timed out at 3m22s and I read "52 seconds" as the state database provisioning. It was time from job start: checkout, install, build — work both branches do identically, before the deploy command runs at all. The real cause was a platform window that killed three branches between 12:18 and 12:26 (spi-inversion, this one, streams-minted-key) and cleared by 12:37, after which every run passed including this one. The commit that carried the invented cost into ADR-0033's consequences and raised the CI budget to accommodate it is dropped rather than reverted, so the claim never enters the record. The budget stays at 3 minutes. That the budget is marginal is true and worth its own change, argued from the degradation-window data rather than from a misdiagnosis. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
… lands Review response (PR #113). The destroy tail put a Prisma Cloud concept — the state database — inside the framework domain, hiding under a cross-domain exception whose reason string covers only Project/Branch resolution. The CLI now knows nothing about state: core's ExtensionDescriptor gains a `teardown` hook mirroring `preflight` (same input shape, same call site discipline), the prismaCloud extension implements it in teardown.ts, and the throw-vs-warn asymmetry moves with it into the domain that owns its reasons — a named stage's Branch delete would refuse while a database is attached, so failing early names the cause; production's removal is cleanup and must not fail the command. The hook still carries projectId/branchId, as preflight already does; removing that vocabulary from core entirely is the CLI/Prisma Cloud decoupling, tracked as its own follow-up project. The review's four naming findings, all applied: - StateTarget is deleted. It was a byte-for-byte duplicate of ResolvedContainer under a name that collides with the deploy target (ADR-0011); the existing type is used everywhere instead. - HostedStateBootstrapError's `target` field is now `container`, the word deploy-cli.md already uses for "the Project, plus the Branch for a named stage". - mintConnection is createConnection. One creates a connection; "mint" was decoration. - The "Test seam:" labels are gone; the `…With` variants' docs now say plainly that the ownership check is a parameter so tests can supply a fake instead of opening a real Postgres connection. The teardown tests stub the ownership check everywhere, including the production warn path, which previously fell back to the real verifier and would have opened a connection to a fake DSN. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
…roject Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Main's new architecture-coverage check requires every source file to carry a
{domain, layer, plane}; teardown.ts is preflight.ts's sibling and gets the
same classification: prisma-cloud / extensions / control.
Signed-off-by: willbot <w.a.madden+machine@gmail.com>
Signed-off-by: Will Madden <madden@prisma.io>
Main's spi-inversion merge also minted an ADR-0033, so the state ADR renumbers to ADR-0034 — file, index entry, and every cross-reference in docs, code comments, and the project workspace. Main's entry keeps 0033; references to it are untouched. The same merge left main's own CI red: generate-stack.ts imports @prisma/composer/report, and the merge introduced both that import and the architecture-coverage check that rejects unaliased workspace specifiers, without adding the alias. One line in tsconfig.depcruise.json fixes it here and fixes main when this merges. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
04cb231 to
0ad59f4
Compare
Main merged the same @prisma/composer/report alias this branch added, so the rebase left the key twice; the file now matches main's. Signed-off-by: willbot <w.a.madden+machine@gmail.com> Signed-off-by: Will Madden <madden@prisma.io>
Deploy state moves from the workspace-level
prisma-composer-stateprojectinto a framework-owned database of the same name inside each stage's own
Branch; production's rides the Project's implicit default Branch. State then
has exactly the lifetime of the environment it describes.
ADR-0033 and its implementation, together — the ADR half was reviewed and
approved earlier in this PR; the code has landed since, so the approval predates
it and I've re-requested review.
Changes
The decision — ADR-0033, superseding ADR-0009. Carries the two opposite ordering rules (the CLI deletes state last, because destroy reads it; the platform's cascading teardown deletes it in any order, and doing so early severs the deploy lock and kills an in-flight deploy of a stage being torn down). ADR-0009 marked superseded and annotated; ADR-0010 records the lock now living in a per-stage store; ADR-0024 gains the distinction the PDP team confirmed — "the default stage lives at the Project level" is addressing, not physics. Staleness sweep reached ADR-0011, ADR-0023, ADR-0030,
layering.md,alchemy-lowering.md,deploy-cli.md,deploying.md(destroy ordering + a manual cutover procedure; there is no automated migration), andSKILL.md.The implementation —
bootstrap.tsresolves the state database inside the Branch the CLI already resolved, instead of listing every project in the workspace and inspecting same-named candidates.prismaState()readsPRISMA_PROJECT_ID/PRISMA_BRANCH_ID(already set on the alchemy child);PRISMA_WORKSPACE_IDleaves the state layer. NewdeleteStateDatabase, and a destroy tail that removes state before the Branch (throws — the Branch delete would fail anyway) or before the Project (warns — cleanup shouldn't fail the command).discovery.tsis shared by bootstrap and delete deliberately: two copies of "which branch, which candidates" would drift, and drifting apart is how a delete removes something a bootstrap would have adopted. Ownership is verified by marker before adopting and before deleting — deleting by name alone would take a user's database that happens to share the name.Untouched, and the review should check the absence as much as the presence: the store's schema, its lock, its service, the transient store, resource lowering, and the
ensure-containersdeploy path.Verified
Unit tests can't prove "deleting the thing takes its state with it", so this ran live against a real workspace. All four conditions passed and the workspace was left clean: a named stage's state database lands on that stage's Branch and the round trip leaves no database, no Branch, no Project; production's lands on the implicit default Branch and is removed before the Project; a stage whose Branch is torn down by the platform loses its state with it and redeploys from a genuinely fresh store; no workspace-level state project is created by any path.
Three things I had wrong, and who caught them
Worth reading, because the corrections are the substance here:
GET/POST /v1/projects/{projectId}/databasesdoesn't take branch parameters at all — I'd read the flat/v1/databasesendpoint's contract and attributed it to the project-scoped one.branchIdis born on the project's default Branch (createPrismaPostgresDatabasecallsgetOrCreateDefaultBranchin its create transaction), which made the create-then-PATCHtwo-step my design offered actively unsafe: on a named stage it would put the state database on production's Branch first, and a failed move strands it where production's bootstrap looks.destroy --stage. The consequences now say that instead of claiming every platform surface cascades.The pattern in all four: I asserted platform behaviour from an endpoint's schema instead of reading its implementation.
Known and accepted
postgres/Database.tscarries the same two-step (and a wider window) for user databases. Out of scope — resource lowering isn't this slice — but it's a real latent bug and is filed.Closes TML-3049.
🤖 Generated with Claude Code