Skip to content

feat(store): policy and relation deletes keep the row - #1968

Merged
AmanGIT07 merged 5 commits into
mainfrom
soft-delete-policies-relations
Oct 6, 2026
Merged

AmanGIT07 merged 5 commits into
mainfrom
soft-delete-policies-relations

Conversation

@AmanGIT07

Copy link
Copy Markdown
Contributor

What

  • Policy delete, the owner-guarded policy delete, and relation delete set deleted_at instead of removing the row. The SpiceDB tuples are still removed by the services, in the same place as before.
  • The plain unique constraints on policies and relations are replaced by partial unique indexes over live rows, uq_policies_role_resource_principal_live and uq_relations_subject_object_relation_live. Both upserts name those indexes in their conflict target through liveConflictTarget. Migration and query change ship together.
  • A delete that finds no live row reports not found, so a second delete of the same policy or relation no longer writes an audit record or passes silently.
  • The org PATs listing joins live policies only, so a PAT whose policy was removed lists with no scope instead of its old role.

Why

Postgres keeps the history and SpiceDB holds only live access. A permission check on a removed membership fails because no tuple is left, and nothing replays the relations table into SpiceDB. With the old plain constraints, a kept row would block the same tuple forever: a removed member could not rejoin, and a deleted policy could not be granted again. Reads already skip deleted rows on main (#1936).

Behaviour change

  • A removed member can be added again, and a deleted policy can be granted again. The re-add inserts a new row next to the deleted one. A duplicate of a live row still updates it in place.
  • Deleting a policy or relation twice returns not found on the second call. Deleting a relation that has no live row through DeleteRelation or RemovePlatformUser is unchanged: the service finds nothing and returns success, as it does today for a relation that never existed.
  • Policy and relation rows stay in the table after a delete, with deleted_at set.

Rollout

Between the migration running and the new binary starting, the old binary's plain ON CONFLICT no longer matches an index, so policy and relation writes (membership changes, org and project creation) fail for that window with SQL state 42P10. Keeping the old constraint for one release is not an option: with both present, the new target still matches the old constraint, and a re-add would update the deleted row and return it. The down migration fails if a deleted and a live row share a tuple by then.

Tested

  • Repository suites against Postgres 13 in Docker, race detector on, two runs: a re-grant after a soft delete lands on a new row; a live duplicate updates in place; delete keeps the row and marks it; a second delete is not found; the guarded delete keeps the row, and returns not found instead of the guard error when the policy is already deleted; relation delete keeps the row and hides it from Get and GetByFields.
  • The org PATs query renders the live filter inside the policies join and nowhere else.
  • e2e TestRelationAPI: a second DeletePolicy returns not found, the same policy can be created again with a new id, and the user's access returns.
  • Migration applied, rolled back, and re-applied on a scratch Postgres 16: both constraints come back under their names on rollback, both indexes are present after re-apply.
  • With the plain conflict targets against the new indexes, every upsert fails with SQL state 42P10. With the hard DELETE put back, the "row kept" cases fail.
  • Sandbox run against this branch:
Step Result
member removed, then added again new policy row next to the deleted one, access back
PAT scope removed and set again new policy row next to the deleted one, PAT works
PAT deleted its policies marked deleted
org deleted with kept rows present succeeds

SQL Safety

  • Ids and tuple values are bound through goqu.Ex{} and goqu.Record{}; the conflict target and now() are constants.
  • ToSQL() params are forwarded unchanged. The raw guarded-delete statement keeps its $1 to $4 parameters; only the verb changed.
  • No ? inside quoted SQL literals.
  • No new //nolint or #nosec annotations.

The plain unique constraints on policies and relations are replaced by partial unique indexes over rows where deleted_at is null, and both upserts name those indexes in their conflict target. A soft-deleted row no longer blocks the same tuple from being written again, and a duplicate of a live row still updates it in place.
Policy delete, the owner-guarded policy delete, and relation delete set deleted_at instead of removing the row. The SpiceDB tuples are still removed by the services as before. A delete that finds no live row reports not found, so a second delete of the same policy or relation no longer writes an audit record or passes silently.
Policy rows now stay after a delete, so the org PATs listing filters the policies join on deleted_at. A PAT whose policy was removed lists with no scope instead of its old role.
@vercel

vercel Bot commented Oct 5, 2026 •

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated
frontier Ready Ready Preview Oct 6, 2026 7:05am UTC

@coderabbitai

coderabbitai Bot commented Oct 5, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: raystack/frontier/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 7142178f-f367-49e2-b385-5dcf88f8fdda
📥 Commits

Reviewing files that changed from the base of the PR and between de2a3ec and 227a942.

📒 Files selected for processing (1)
  • internal/store/postgres/org_pats_repository.go
💤 Files with no reviewable changes (1)
  • internal/store/postgres/org_pats_repository.go

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.


📝 Summary

Summary by CodeRabbit

  • Bug Fixes
    • Deleted policies and relations are retained as inactive records and remain unavailable in normal lookups.
    • Recreating a deleted policy or relation creates a new record, while updating an active one preserves its existing identity.
    • Repeated attempts to delete an already-deleted policy or relation return a not-found result.
    • PAT listings continue to include PATs even when they have no active policies.

Walkthrough

Policy and relation records now use live-row uniqueness and soft deletion. Upserts can create new rows after deletion, while repeated deletion returns not found. The migration, repository tests, PAT query test, and API regression test cover these changes.

Changes

Live Policy and Relation Lifecycle

Layer / File(s) Summary
Live-row uniqueness and upserts
internal/store/postgres/migrations/20261005100000_policies_relations_live_unique.*.sql, internal/store/postgres/policy_repository.go, internal/store/postgres/policy_repository_test.go, internal/store/postgres/relation_repository.go, internal/store/postgres/relation_repository_test.go
The migration uses partial unique indexes for live policies and relations; the down migration restores full-table unique constraints. Upserts target live rows. Tests cover recreating deleted rows and updating existing live rows.
Soft-delete behavior
internal/store/postgres/policy_repository.go, internal/store/postgres/policy_repository_test.go, internal/store/postgres/relation_repository.go, internal/store/postgres/relation_repository_test.go, test/e2e/regression/api_test.go
Policy and relation deletions mark rows as deleted. Repeated deletion returns not found. Tests cover row visibility, the role guard, and policy recreation through the API.
PAT policy join
internal/store/postgres/org_pats_repository.go, internal/store/postgres/org_pats_repository_test.go
The policies left join filters deleted rows in its ON condition. Tests check that the condition does not become a WHERE clause.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Feature

Suggested reviewers: rohilsurana

Merge Risk: 🟡 Moderate · up to 227a9

The change moves policy and relation deletes to soft deletes. During rollout, writes from the old binary can fail between the migration and the new binary starting. The migration can also block writes while it runs, and rolling it back can fail once policies or relations have been deleted and recreated. Plan the rollout (drain old writers, schedule the migration) and decide how rollback should work before merging.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to de2a3

The new schema can reject grant and membership writes from older running versions, and deletion followed by recreation can prevent the supplied rollback from completing. Existing resource-scoped authorization checks remain in place, but deployment ordering and recovery require an explicit plan.

Retained concerns

  • Medium · reliability · inferred: The up migration drops the full unique constraints that base-version policy and relation upserts require. Older writers using conflict targets without a predicate cannot infer the replacement partial indexes and can fail with PostgreSQL 42P10. If old instances remain active, grant and membership writes can fail across the shared database. Head writers are compatible with the new indexes, but deployment evidence does not establish that old writers are drained before migration.
  • Medium · reliability · inferred: Routine deletion and recreation leave historical and live rows with identical uniqueness columns. The down migration immediately restores full-table uniqueness without reconciling those rows, so that expected state prevents rollback. This limits recovery of the shared authorization store after a faulty rollout; success on data without recreated tuples does not resolve the failure case.
Security review details

Security Blast Radius

  • inferred — Migration compatibility and rollback affect all policy and relation writers sharing the database, rather than only the resource being changed. A normal authorized delete/regrant in one resource can create the duplicate history that blocks shared-schema rollback; this does not itself confer broader tenant privileges.

Security Findings and Attack Paths

  • inferred — Guarded policy deletion has a pre-existing partial-failure path: PostgreSQL deletion commits before SpiceDB cleanup, so cleanup failure can leave authorization tuples granting access. Retrying the guarded repository operation cannot proceed once its live policy lookup returns not-found. Base behavior physically deleted the same row before the same cleanup, so retained history does not establish that this PR introduced or worsened that exposure.

Trust Boundaries and Controls

  • observed — Policy creation authorizes against the requested resource. Policy deletion resolves the stored policy's resource before authorization, with an explicit platform-superuser path for orphan cleanup. The repository's minimum-role guard uses stored resource identity and live-holder locking; the inspected lifecycle change does not replace these controls.

Resilience and Maintainability Implications

  • inferred — For normal relation deletion, authorization cleanup precedes the database update. If that update fails, the live row remains available for a subsequent cleanup attempt while the completed tuple removal denies that grant. This containment differs from the pre-existing PostgreSQL-first guarded policy path.

Hardening Proposals

  • proposed — Define a deployment gate that prevents incompatible old writers after constraint removal, and a recovery procedure that preserves live authorization state while handling duplicate history. Validate that procedure against deletion/recreation data before relying on rollback.
🚥 Pre-merge checks | ✅ 2
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@coveralls

coveralls commented Oct 5, 2026 •

Copy link
Copy Markdown

Coverage Report for CI Build 37427502063

Coverage increased (+0.009%) to 54.353%

Details

  • Coverage increased (+0.009%) from the base build.
  • Patch coverage: 5 uncovered changes across 1 file (12 of 17 lines covered, 70.59%).
  • No coverage regressions found.

Uncovered Changes

File Changed Covered %
internal/store/postgres/policy_repository.go 14 9 64.29%
Total (3 files) 17 12 70.59%

Coverage Regressions

No coverage regressions found.


Coverage Stats

Coverage Status
Relevant Lines: 41387
Covered Lines: 22495
Line Coverage: 54.35%
Coverage Strength: 18.01 hits per line

💛 - Coveralls

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3


ℹ️ Review info
⚙️ Run configuration
  • Configuration used: Repository: raystack/frontier/.coderabbit.yaml
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 6c8a7ed3-6952-419e-8302-fe4b8506b571
📥 Commits

Reviewing files that changed from the base of the PR and between d812a0a and de2a3ec.

📒 Files selected for processing (9)
  • internal/store/postgres/migrations/20261005100000_policies_relations_live_unique.down.sql
  • internal/store/postgres/migrations/20261005100000_policies_relations_live_unique.up.sql
  • internal/store/postgres/org_pats_repository.go
  • internal/store/postgres/org_pats_repository_test.go
  • internal/store/postgres/policy_repository.go
  • internal/store/postgres/policy_repository_test.go
  • internal/store/postgres/relation_repository.go
  • internal/store/postgres/relation_repository_test.go
  • test/e2e/regression/api_test.go

Included review availability: This review used your included allowance. Your plan provides up to 2 included reviews per hour; 1 remain after this review.

Comment thread internal/store/postgres/org_pats_repository.go Outdated
Removed comment about PAT with no live policy listing.
@AmanGIT07
AmanGIT07 enabled auto-merge (squash) October 6, 2026 07:05
@AmanGIT07
AmanGIT07 merged commit 8103c20 into main Oct 6, 2026
8 checks passed
@AmanGIT07
AmanGIT07 deleted the soft-delete-policies-relations branch October 6, 2026 07:09

This branch was successfully deployed

1 active deployment
Preview — 227a9421 Deployed Oct 6, 2026 by vercel[bot]
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants