Drive organization deletion to completion - #39
Open
vitramir wants to merge 3 commits into
Open
Conversation
DeleteOrganization performed no permission check at all -- any authenticated caller who knew an organization's UUID could delete it -- and deleted a single row, leaving every org-scoped record in every other service holding an organization_id that resolved to nothing. Deletion is now a lifecycle. Phase 1, in the request: the organization moves to deleting and its member and owner tuples go. Revoking access is not part of deleting the organization, it is what makes deleting its resources safe -- without it a member creates an agent into an organization the cascade has already walked past. The cluster tuple deliberately stays, because cluster admins hold can_manage_organization through admin from cluster and removing it would hide the teardown from the only people who can watch it. The tuples are read back from Authorization rather than derived from the memberships table: an installed app is written a member tuple directly and has no membership row. Phase 2 is a background worker walking the configured teardown order, one step per claim, retrying a failing step with backoff and never advancing past it. Within step 6 Images and LLM precede Secrets, which refuses to delete a secret an image or a subscription still names. Phase 3 removes the remaining tuples, then the memberships and the row together. The slug is held until that commits, so a new organization cannot claim it while live exposures still answer on the old one. UpdateOrganization now checks can_manage_organization rather than owner, which no cluster admin holds on any organization. The refusal to write to a deleting organization is a predicate on the UPDATE rather than a read before it, so a teardown starting mid-request cannot slip through. The deletion lifecycle is SQL, so it is tested against a real database, and CI gains a postgres service so that test runs rather than skipping.
runners.proto imports agynio/api/runner/v1, which the path list does not name. Without --include-imports the generated runners package references a package that was never written.
A DB-backed test that skips is indistinguishable from one that passes in the summary line, which is how tests here have gone red unnoticed. CI now checks the database is up before running, and asserts the lifecycle test reports PASS rather than SKIP.
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.
DeleteOrganizationperformed no permission check at all — any authenticated caller who knew an organization's UUID could delete it — and deleted a single row, leaving every org-scoped record in every other service holding anorganization_idthat resolved to nothing. This makes deletion the lifecycle the spec describes.Phase 1, in the request. The organization moves to
deletingand itsmemberandownertuples go. Revoking access is not part of deleting the organization — it is what makes deleting its resources safe; without it a member creates an agent into an organization the cascade has already walked past.Two details worth review here:
clustertuple deliberately survives. Cluster admins holdcan_manage_organizationthroughadmin from cluster, and removing it would hide the teardown from the only people who can watch it.membershipstable. An installed app is written amembertuple directly and has no membership row, so deriving from the table would leave the app holding access to an organization being emptied.Phase 2 is a background worker walking the configured teardown order, one step per claim, retrying a failing step with backoff and never advancing past it.
ClaimDeletionusesFOR UPDATE SKIP LOCKEDso a second replica works on a different organization rather than blocking.Within step 6, Images and LLM precede Secrets — Secrets keeps its reference check and refuses to delete a secret an image or a subscription still names, so the reverse order stalls the cascade permanently. There is a test asserting that ordering, because it is the kind of thing a later edit silently breaks.
Phase 3 removes the remaining tuples, then the memberships and the row together. The slug is held until that commits, so a new organization cannot claim it while live exposures still answer on the old one.
UpdateOrganizationnow checkscan_manage_organizationrather thanowner, which no cluster admin holds on any organization. The refusal to write to a deleting organization is a predicate on theUPDATErather than a read before it, so a teardown starting mid-request cannot slip through the gap.ListAccessibleOrganizationsandListMyMembershipsfilterdeletingexplicitly. Dropping the tuples is enough for a member, but a cluster admin'scan_add_memberresolves throughadmin from clusterfor every organization on the platform, so state has to be checked rather than inferred from a missing tuple.Tested against a real database. The lifecycle is SQL — an idempotent transition, a claim two replicas must not both take, a slug held by the row's continued existence — and a fake store proves none of it. CI gains a postgres service so the test runs rather than skipping silently, which is how DB-backed tests in this codebase have gone red unnoticed before.
Depends on authorization#44 for
can_manage_organization, and on the twelve service PRs for the RPCs the cascade calls. Chart bumped to 0.2.0.Part of organization settings and deletion.