feat(store): role delete keeps the row and refuses while a live policy uses it

core/role/service.go

@@ -299,16 +299,16 @@ func (s Service) Delete(ctx context.Context, id string) error {
 		return err
 	}
 
-	// Delete the row first. The policies.role_id foreign key rejects this while
-	// any policy still references the role, surfaced as ErrRoleInUse — so a role that
-	// is still in use is left fully intact (no SpiceDB tuples are touched below).
+	// Mark the row deleted first. The repository refuses with ErrRoleInUse while
+	// any live policy still references the role, so a role that is still in use
+	// is left fully intact (no SpiceDB tuples are touched below).
 	// This is intentionally a guard, not a cascade: removing a role would revoke
 	// access for everyone granted it, so the caller must drop those policies first.
 	if err := s.repository.Delete(ctx, roleToDelete.ID); err != nil {
 		return err
 	}
 
-	// row is gone → remove the role's permission tuples (app/role:<id>#<perm>@...)
+	// row is marked deleted → remove the role's permission tuples (app/role:<id>#<perm>@...)
 	return s.relationService.Delete(ctx, relation.Relation{Object: relation.Object{
 		ID:        roleToDelete.ID,
 		Namespace: schema.RoleNamespace,

core/role/service_test.go

@@ -79,7 +79,7 @@ func Test_Delete(t *testing.T) {
 
 		mockID := uuid.New().String()
 		mockRepository.On("Get", mock.Anything, mockID).Return(role.Role{ID: "role-1"}, nil).Once()
-		// the policies.role_id FK rejects the row delete -> ErrRoleInUse
+		// a live policy still references the role -> ErrRoleInUse
 		mockRepository.On("Delete", mock.Anything, "role-1").Return(role.ErrRoleInUse).Once()
 
 		svc := role.NewService(mockRepository, mockRelationSvc, mockPermissionSvc, mockAuditRecordRepo, nil)

internal/store/postgres/role_repository.go

@@ -2,6 +2,7 @@ package postgres
 
 import (
 	"context"
+	"database/sql"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -14,9 +15,9 @@ import (
 
 	"github.com/google/uuid"
 
-	"database/sql"
-
 	"github.com/doug-martin/goqu/v9"
+	"github.com/doug-martin/goqu/v9/exp"
+	"github.com/jmoiron/sqlx"
 	"github.com/raystack/frontier/core/namespace"
 	"github.com/raystack/frontier/core/role"
 	"github.com/raystack/frontier/pkg/db"
@@ -259,27 +260,62 @@ func (r RoleRepository) Update(ctx context.Context, rl role.Role) (role.Role, er
 }
 
 func (r RoleRepository) Delete(ctx context.Context, id string) error {
-	query, params, err := dialect.Delete(TABLE_ROLES).Where(
-		goqu.Ex{
-			"id": id,
-		},
-	).Returning(&Role{}).ToSQL()
+	lockQuery, lockParams, err := fromLive(TABLE_ROLES).
+		Select("id").
+		Where(goqu.Ex{"id": id}).
+		ForUpdate(exp.Wait).
+		ToSQL()
 	if err != nil {
 		return fmt.Errorf("%w: %s", errQuery, err)
 	}
 
-	var roleModel Role
-	if err = r.dbc.WithTimeout(ctx, TABLE_ROLES, "Delete", func(ctx context.Context) error {
-		return r.dbc.QueryRowxContext(ctx, query, params...).StructScan(&roleModel)
+	livePolicy := fromLive(TABLE_POLICIES).
+		Select(goqu.L("1")).
+		Where(goqu.I(TABLE_POLICIES + ".role_id").Eq(goqu.I(TABLE_ROLES + ".id")))
+
+	deleteQuery, deleteParams, err := softDelete(TABLE_ROLES).Where(
+		goqu.Ex{"id": id},
+		goqu.L("NOT EXISTS ?", livePolicy),
+	).ToSQL()
+	if err != nil {
+		return fmt.Errorf("%w: %s", errQuery, err)
+	}
+
+	// One transaction, two statements. The first locks the live role row the
+	// way a hard DELETE would. The second marks it deleted, unless a live
+	// policy still points at it.
+	if err = r.dbc.WithTxn(ctx, sql.TxOptions{}, func(tx *sqlx.Tx) error {
+		return r.dbc.WithTimeout(ctx, TABLE_ROLES, "Delete", func(ctx context.Context) error {
+			// Lock the role row first. A policy insert holds a lock on the role
+			// row until it commits, so this waits for it and the check below sees that policy.
+			var lockedID string
+			if err := tx.QueryRowContext(ctx, lockQuery, lockParams...).Scan(&lockedID); err != nil {
+				return err
+			}
+
+			result, err := tx.ExecContext(ctx, deleteQuery, deleteParams...)
+			if err != nil {
+				return err
+			}
+			deleted, err := result.RowsAffected()
+			if err != nil {
+				return err
+			}
+			if deleted == 0 {
+				return role.ErrRoleInUse
+			}
+			return nil
+		})
 	}); err != nil {
+		// WithTxn wraps every error it rolled back on as "rollback: ...". Here
+		// the rollback is the normal path, so return the plain sentinel.
+		if errors.Is(err, role.ErrRoleInUse) {
+			return role.ErrRoleInUse
+		}
 		err = checkPostgresError(err)
 		switch {
 		case errors.Is(err, sql.ErrNoRows):
 			return role.ErrNotExist
-		case errors.Is(err, ErrForeignKeyViolation):
-			// policies.role_id references roles(id) with no ON DELETE rule, so a
-			// role still bound to any policy cannot be deleted.
-			return role.ErrRoleInUse
 		default:
 			return err
 		}

internal/store/postgres/role_repository_test.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"testing"
+	"time"
 
 	"github.com/raystack/frontier/internal/bootstrap/schema"
 
@@ -422,6 +423,71 @@ func (s *RoleRepositoryTestSuite) TestDelete() {
 			}
 		})
 	}
+
+	s.Run("should keep the row and mark it deleted", func() {
+		target := s.roles[1]
+		s.Require().NoError(s.repository.Delete(s.ctx, target.ID))
+
+		var deleted bool
+		err := s.client.QueryRowxContext(s.ctx, "SELECT deleted_at IS NOT NULL FROM roles WHERE id = $1", target.ID).Scan(&deleted)
+		s.Require().NoError(err)
+		s.Assert().True(deleted)
+	})
+
+	s.Run("should return not found when the role is already deleted", func() {
+		target := s.roles[2]
+		_, err := s.client.ExecContext(s.ctx, "UPDATE roles SET deleted_at = now() WHERE id = $1", target.ID)
+		s.Require().NoError(err)
+
+		s.Assert().ErrorIs(s.repository.Delete(s.ctx, target.ID), role.ErrNotExist)
+	})
+
+	s.Run("should return in use and keep the role when a live policy uses it", func() {
+		target := s.roles[3]
+		_, err := bootstrapPolicy(s.client, s.orgID, target, uuid.NewString())
+		s.Require().NoError(err)
+
+		s.Assert().ErrorIs(s.repository.Delete(s.ctx, target.ID), role.ErrRoleInUse)
+		_, err = s.repository.Get(s.ctx, target.ID)
+		s.Assert().NoError(err)
+	})
+
+	s.Run("should delete the role when only deleted policies use it", func() {
+		target := s.roles[4]
+		_, err := bootstrapPolicy(s.client, s.orgID, target, uuid.NewString())
+		s.Require().NoError(err)
+		_, err = s.client.ExecContext(s.ctx, "UPDATE policies SET deleted_at = now() WHERE role_id = $1", target.ID)
+		s.Require().NoError(err)
+
+		s.Assert().NoError(s.repository.Delete(s.ctx, target.ID))
+	})
+
+	s.Run("should wait for a policy insert that has not committed and return in use", func() {
+		target, err := s.repository.Upsert(s.ctx, role.Role{
+			Name:     "role with a running policy insert",
+			OrgID:    s.orgID,
+			Metadata: metadata.Metadata{},
+		})
+		s.Require().NoError(err)
+		tx, err := s.client.BeginTxx(s.ctx, nil)
+		s.Require().NoError(err)
+		defer tx.Rollback() // nolint
+		_, err = tx.ExecContext(s.ctx, "INSERT INTO policies (role_id, resource_id, resource_type, principal_id, principal_type) VALUES ($1, $2, 'ns1', $3, 'app/user')",
+			target.ID, s.orgID, uuid.NewString())
+		s.Require().NoError(err)
+
+		done := make(chan error, 1)
+		go func() { done <- s.repository.Delete(s.ctx, target.ID) }()
+
+		s.Require().Eventually(func() bool {
+			var waiting int
+			err := s.client.QueryRowxContext(s.ctx, "SELECT count(*) FROM pg_stat_activity WHERE datname = current_database() AND wait_event_type = 'Lock'").Scan(&waiting)
+			return err == nil && waiting == 1
+		}, time.Second, 10*time.Millisecond, "the delete did not wait for the policy insert")
+
+		s.Require().NoError(tx.Commit())
+		s.Assert().ErrorIs(<-done, role.ErrRoleInUse)
+	})
 }
 
 func (s *RoleRepositoryTestSuite) TestGetByName() {

test/e2e/regression/api_test.go

@@ -3092,6 +3092,39 @@ func (s *APIRegressionTestSuite) TestOrganizationRoleDeleteInUse() {
 
 	// after a successful delete, no role->permission tuples should linger
 	s.Assert().False(roleHasPermTuples())
+
+	// the deleted role is hidden from reads and cannot be deleted twice
+	_, err = s.testBench.Client.GetOrganizationRole(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.GetOrganizationRoleRequest{
+		OrgId: orgID,
+		Id:    roleID,
+	}))
+	s.Require().Error(err)
+	s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err))
+	_, err = s.testBench.Client.DeleteOrganizationRole(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteOrganizationRoleRequest{
+		OrgId: orgID,
+		Id:    roleID,
+	}))
+	s.Require().Error(err)
+	s.Assert().Equal(connect.CodeNotFound, connect.CodeOf(err))
+
+	// the name is free again and lands on a new row
+	recreateResp, err := s.testBench.Client.CreateOrganizationRole(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.CreateOrganizationRoleRequest{
+		OrgId: orgID,
+		Body: &frontierv1beta1.RoleRequestBody{
+			Title:       "in use role",
+			Name:        "in_use_role",
+			Scopes:      []string{"app/organization"},
+			Permissions: []string{"app.organization.grouplist"},
+		},
+	}))
+	s.Require().NoError(err)
+	s.Assert().NotEqual(roleID, recreateResp.Msg.GetRole().GetId())
+
+	// the org now holds a live role and a deleted one; the delete still goes through
+	_, err = s.testBench.Client.DeleteOrganization(ctxOrgAdminAuth, connect.NewRequest(&frontierv1beta1.DeleteOrganizationRequest{
+		Id: orgID,
+	}))
+	s.Require().NoError(err)
 }
 
 func TestEndToEndAPIRegressionTestSuite(t *testing.T) {