From 54c5d08241e56ad4c27eae4803307767ebba3aba Mon Sep 17 00:00:00 2001 From: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Tue, 27 Oct 2026 08:51:55 -0400 Subject: [PATCH 1/2] fix(accounts): racing ensures of one derived role serialize on the role row #1259 made EnsureDerivedRole read the role and its permissions and write only what differs. The read took no lock under READ COMMITTED, so two ensures of the same cause with different declarations both read before either committed and the role ended at their union, keeping a permission the later declaration dropped; the upsert it replaced serialized them. The role read is now SELECT ... FOR UPDATE: a row lock, which fires no trigger and moves no revision, and which app_tenant may take through its UPDATE grant on the description column. The second ensure waits, reads what the first committed, and ends at its own declaration. The permission insert gets back ON CONFLICT DO NOTHING so an add a racing writer already made cannot fail on the primary key. Tests: a racing pair (the first adds doc:write and stays open, the second declares only solution:use and must wait, ending at solution:use), and the revision test now also covers a pure removal moving the revision and an idle re-ensure after it moving none. The race test fails with the lock removed. Co-Authored-By: Claude Opus 5.5 --- .../pkg/infra/derived_role_revision_test.go | 173 +++++++++++++----- .../infra/postgres_derived_module_access.go | 15 +- 2 files changed, 137 insertions(+), 51 deletions(-) diff --git a/module/services/accounts/code/pkg/infra/derived_role_revision_test.go b/module/services/accounts/code/pkg/infra/derived_role_revision_test.go index fb056cfa8..02f7a5d32 100644 --- a/module/services/accounts/code/pkg/infra/derived_role_revision_test.go +++ b/module/services/accounts/code/pkg/infra/derived_role_revision_test.go @@ -5,6 +5,7 @@ package infra_test import ( "context" "testing" + "time" "github.com/stretchr/testify/require" @@ -13,67 +14,141 @@ import ( "accounts/pkg/infra/storetx" ) +var ( + solutionUse = &gen.Permission{Resource: "solution", Action: "use"} + docRead = &gen.Permission{Resource: "doc", Action: "read"} + docWrite = &gen.Permission{Resource: "doc", Action: "write"} +) + +func derivedRole(orgID, cause, description string, perms ...*gen.Permission) business.DerivedRole { + return business.DerivedRole{OrgID: orgID, DerivedFrom: cause, + Name: "solution role · " + cause, Description: description, Permissions: perms} +} + +// ensureDerivedRole ensures the role in its own committed organization +// transaction and returns its id. +func ensureDerivedRole(t *testing.T, role business.DerivedRole) string { + t.Helper() + var id string + require.NoError(t, testStore.WithOrgTx(testCtx, role.OrgID, func(ctx context.Context) error { + var err error + id, err = testStore.EnsureDerivedRole(ctx, role) + return err + })) + return id +} + +func authorizationRevision(t *testing.T, orgID string) int64 { + t.Helper() + var r int64 + require.NoError(t, testStore.WithControlPlane(testCtx, func(ctx context.Context) error { + return storetx.Tx(ctx).QueryRow(ctx, `SELECT revision FROM public.organization_authorization_revisions WHERE org_id = $1`, orgID).Scan(&r) + })) + return r +} + +func rolePermissions(t *testing.T, roleID string) []string { + t.Helper() + out := []string{} + require.NoError(t, testStore.WithControlPlane(testCtx, func(ctx context.Context) error { + rows, err := storetx.Tx(ctx).Query(ctx, `SELECT resource || ':' || action FROM role_permissions WHERE role_id = $1`, roleID) + if err != nil { + return err + } + defer rows.Close() + for rows.Next() { + var p string + if err := rows.Scan(&p); err != nil { + return err + } + out = append(out, p) + } + return rows.Err() + })) + return out +} + // Ensuring a derived role that is already as declared writes nothing, so it // leaves the organization's authorization revision (what every capability is // minted at) where it is; a changed description or permission set is written, // and only what changed. func TestEnsuringADerivedRoleAsDeclaredMovesNoRevision(t *testing.T) { - userID := seedUser(t) - orgID := seedOrg(t, userID) - revision := func() int64 { - t.Helper() - var r int64 - require.NoError(t, testStore.WithControlPlane(testCtx, func(ctx context.Context) error { - return storetx.Tx(ctx).QueryRow(ctx, `SELECT revision FROM public.organization_authorization_revisions WHERE org_id = $1`, orgID).Scan(&r) - })) - return r - } - permissions := func(roleID string) []string { - t.Helper() - var out []string - require.NoError(t, testStore.WithControlPlane(testCtx, func(ctx context.Context) error { - rows, err := storetx.Tx(ctx).Query(ctx, `SELECT resource || ':' || action FROM role_permissions WHERE role_id = $1`, roleID) - if err != nil { - return err - } - defer rows.Close() - for rows.Next() { - var p string - if err := rows.Scan(&p); err != nil { - return err - } - out = append(out, p) - } - return rows.Err() - })) - return out - } - ensure := func(description string, perms ...*gen.Permission) string { - t.Helper() - var id string - require.NoError(t, testStore.WithOrgTx(testCtx, orgID, func(ctx context.Context) error { - var err error - id, err = testStore.EnsureDerivedRole(ctx, business.DerivedRole{OrgID: orgID, DerivedFrom: "installation=revision-test;role=viewer", - Name: "solution role · revision-test", Description: description, Permissions: perms}) - return err - })) - return id - } - use, read, write := &gen.Permission{Resource: "solution", Action: "use"}, &gen.Permission{Resource: "doc", Action: "read"}, &gen.Permission{Resource: "doc", Action: "write"} + orgID := seedOrg(t, seedUser(t)) + const cause = "installation=revision-test;role=viewer" + revision := func() int64 { return authorizationRevision(t, orgID) } - id := ensure("the viewer role", use, read) + id := ensureDerivedRole(t, derivedRole(orgID, cause, "the viewer role", solutionUse, docRead)) settled := revision() for range 3 { - require.Equal(t, id, ensure("the viewer role", use, read)) + require.Equal(t, id, ensureDerivedRole(t, derivedRole(orgID, cause, "the viewer role", solutionUse, docRead))) } require.Equal(t, settled, revision(), "ensuring a role already as declared wrote it") - require.Equal(t, id, ensure("the viewer role", use, write)) - require.ElementsMatch(t, []string{"solution:use", "doc:write"}, permissions(id)) + require.Equal(t, id, ensureDerivedRole(t, derivedRole(orgID, cause, "the viewer role", solutionUse, docWrite))) + require.ElementsMatch(t, []string{"solution:use", "doc:write"}, rolePermissions(t, id)) moved := revision() require.Greater(t, moved, settled, "a changed permission set is written") - require.Equal(t, id, ensure("the viewer role, renamed", use, write)) - require.Greater(t, revision(), moved, "a changed description is written") - require.ElementsMatch(t, []string{"solution:use", "doc:write"}, permissions(id)) + require.Equal(t, id, ensureDerivedRole(t, derivedRole(orgID, cause, "the viewer role, renamed", solutionUse, docWrite))) + renamed := revision() + require.Greater(t, renamed, moved, "a changed description is written") + require.ElementsMatch(t, []string{"solution:use", "doc:write"}, rolePermissions(t, id)) + + require.Equal(t, id, ensureDerivedRole(t, derivedRole(orgID, cause, "the viewer role, renamed", solutionUse))) + require.ElementsMatch(t, []string{"solution:use"}, rolePermissions(t, id)) + removed := revision() + require.Greater(t, removed, renamed, "a permission no longer declared is removed, and that is written") + + require.Equal(t, id, ensureDerivedRole(t, derivedRole(orgID, cause, "the viewer role, renamed", solutionUse))) + require.Equal(t, removed, revision(), "re-ensuring the narrowed role wrote it") +} + +// Two ensures of the same cause with different declarations serialize on the +// role row: the second waits for the first to commit, reads what it wrote, and +// ends at its own declaration (last writer wins). Without the lock the second +// would read before the first committed and the role would end at the union of +// both, keeping the permission the later declaration dropped. +func TestRacingEnsuresOfADerivedRoleEndAtTheLastDeclaration(t *testing.T) { + orgID := seedOrg(t, seedUser(t)) + const cause = "installation=race-test;role=viewer" + id := ensureDerivedRole(t, derivedRole(orgID, cause, "the viewer role", solutionUse)) + + added, release := make(chan struct{}), make(chan struct{}) + first := make(chan error, 1) + go func() { + first <- testStore.WithOrgTx(testCtx, orgID, func(ctx context.Context) error { + if _, err := testStore.EnsureDerivedRole(ctx, derivedRole(orgID, cause, "the viewer role", solutionUse, docWrite)); err != nil { + return err + } + close(added) + <-release + return nil + }) + }() + select { + case <-added: + case err := <-first: + t.Fatalf("the first ensure ended before it held the role: %v", err) + } + + second := make(chan error, 1) + go func() { + second <- testStore.WithOrgTx(testCtx, orgID, func(ctx context.Context) error { + _, err := testStore.EnsureDerivedRole(ctx, derivedRole(orgID, cause, "the viewer role", solutionUse)) + return err + }) + }() + select { + case err := <-second: + close(release) + <-first + t.Fatalf("the second ensure did not wait for the first to commit (err=%v)", err) + case <-time.After(500 * time.Millisecond): + } + + close(release) + require.NoError(t, <-first) + require.NoError(t, <-second) + require.ElementsMatch(t, []string{"solution:use"}, rolePermissions(t, id), + "the later declaration drops doc:write; the role must not end at the union") } diff --git a/module/services/accounts/code/pkg/infra/postgres_derived_module_access.go b/module/services/accounts/code/pkg/infra/postgres_derived_module_access.go index f03bce5f4..d3848286c 100644 --- a/module/services/accounts/code/pkg/infra/postgres_derived_module_access.go +++ b/module/services/accounts/code/pkg/infra/postgres_derived_module_access.go @@ -101,8 +101,17 @@ func (s *PostgresStore) EnsureDerivedRole(ctx context.Context, role business.Der // refused. A role already as declared is therefore read, never written: // only a role that is missing, or whose description or permissions // differ, is touched, and only by what differs. + // + // The read LOCKS the role row (FOR UPDATE: a row lock, which fires no + // trigger and so moves no revision; `app_tenant` may take it because it + // holds UPDATE on the description column). Two ensures of the same cause + // with different declarations therefore serialize on the row, as the upsert + // they replace did: the second reads the permissions the first committed + // and ends at its own declaration, last writer wins. Without the lock both + // would read before either commits and the role would end at their UNION, + // keeping a permission the later declaration dropped. var roleID, description string - err := executor.QueryRow(ctx, `SELECT id::text, COALESCE(description, '') FROM roles WHERE derived_from = $1`, role.DerivedFrom).Scan(&roleID, &description) + err := executor.QueryRow(ctx, `SELECT id::text, COALESCE(description, '') FROM roles WHERE derived_from = $1 FOR UPDATE`, role.DerivedFrom).Scan(&roleID, &description) switch { case errors.Is(err, pgx.ErrNoRows): // The conflict target is the partial unique index on derived_from, so @@ -163,7 +172,9 @@ func (s *PostgresStore) EnsureDerivedRole(ctx context.Context, role business.Der if held[permission] { continue } - if _, err := executor.Exec(ctx, `INSERT INTO role_permissions (role_id, resource, action) VALUES ($1, $2, $3)`, + // ON CONFLICT DO NOTHING keeps a permission a racing ensure has + // already added from failing this one on the primary key. + if _, err := executor.Exec(ctx, `INSERT INTO role_permissions (role_id, resource, action) VALUES ($1, $2, $3) ON CONFLICT DO NOTHING`, roleID, permission[0], permission[1]); err != nil { return "", w.Wrapf(err, "failed to write the derived role's permission %s:%s", permission[0], permission[1]) } From 795b8728f4bcfd3c497b171544fa51f5df724997 Mon Sep 17 00:00:00 2001 From: antoinetoussaint-byte <312536060+antoinetoussaint-byte@users.noreply.github.com> Date: Tue, 27 Oct 2026 09:23:04 -0400 Subject: [PATCH 2/2] fix(accounts): the derived role lock is FOR NO KEY UPDATE, which no grant of the role waits on FOR UPDATE conflicts with the FOR KEY SHARE every row referencing the role takes through its foreign key. installSolution moves the organization's revision (its agent principal) and then writes the standing grant of the solution role; an ensure of that role with a changed declaration held the role row under FOR UPDATE and then needed the revision row, and the two deadlocked (SQLSTATE 40P01). FOR NO KEY UPDATE is the lock the replaced upsert took: it still serializes two ensures of one role and does not conflict with a reference to it. The permission insert drops ON CONFLICT DO NOTHING again: under the role lock no other ensure can add a permission between the read and the insert, and UpdateRole refuses a derived role, so the clause could only hide a broken lock. MODULE_INSTALLATION.md states that a re-derivation writes only what differs and that two of one cause serialize, last commit wins. Test: an ensure must not deadlock a writer that moved the revision and then references the role; it fails with 40P01 under FOR UPDATE. Co-Authored-By: Claude Opus 5.5 --- module/MODULE_INSTALLATION.md | 5 ++- .../pkg/infra/derived_role_revision_test.go | 45 +++++++++++++++++++ .../infra/postgres_derived_module_access.go | 32 ++++++++----- 3 files changed, 69 insertions(+), 13 deletions(-) diff --git a/module/MODULE_INSTALLATION.md b/module/MODULE_INSTALLATION.md index 7ea74e652..a33043df5 100644 --- a/module/MODULE_INSTALLATION.md +++ b/module/MODULE_INSTALLATION.md @@ -541,4 +541,7 @@ this replaces. applied declaration, which a later generation may move. It is reconciled on the NEXT grant or revoke of its cause: re-deriving replaces the role's permission set, and a module no longer reached stops being re-derived. Until then, an applied -generation that narrows a declaration does not narrow the carried grants. +generation that narrows a declaration does not narrow the carried grants. A +re-derivation writes only what differs, so one that changes nothing leaves the +authorization revision where it is; two re-derivations of one cause serialize on +the role row, and the role ends at the declaration of the one that commits last. diff --git a/module/services/accounts/code/pkg/infra/derived_role_revision_test.go b/module/services/accounts/code/pkg/infra/derived_role_revision_test.go index 02f7a5d32..a3b39833a 100644 --- a/module/services/accounts/code/pkg/infra/derived_role_revision_test.go +++ b/module/services/accounts/code/pkg/infra/derived_role_revision_test.go @@ -152,3 +152,48 @@ func TestRacingEnsuresOfADerivedRoleEndAtTheLastDeclaration(t *testing.T) { require.ElementsMatch(t, []string{"solution:use"}, rolePermissions(t, id), "the later declaration drops doc:write; the role must not end at the union") } + +// The lock an ensure takes on the role row must not conflict with the lock a row +// REFERENCING the role takes through its foreign key (FOR KEY SHARE: scope_grants, +// role_assignments, role_permissions and record_shares all reference roles). +// installSolution moves the organization's revision (its agent principal) and +// THEN writes the standing grant of the solution role; an ensure of that role +// with a changed declaration holds the role row and then needs the revision row. +// FOR UPDATE conflicts with KEY SHARE and the two deadlock; FOR NO KEY UPDATE, +// the lock the upsert it replaced took, does not. +func TestAnEnsureDoesNotDeadlockAWriterThatReferencesTheRole(t *testing.T) { + orgID := seedOrg(t, seedUser(t)) + const cause = "installation=reference-test;role=viewer" + id := ensureDerivedRole(t, derivedRole(orgID, cause, "the viewer role", solutionUse)) + + movedRevision, reference := make(chan struct{}), make(chan struct{}) + referencing := make(chan error, 1) + go func() { + referencing <- testStore.WithOrgTx(testCtx, orgID, func(ctx context.Context) error { + // Any authorization write moves the revision and holds its row. + if _, err := testStore.EnsureDerivedRole(ctx, derivedRole(orgID, "installation=reference-test;role=other", "another role", solutionUse)); err != nil { + return err + } + close(movedRevision) + <-reference + _, err := storetx.Tx(ctx).Exec(ctx, `SELECT 1 FROM roles WHERE id = $1 FOR KEY SHARE`, id) + return err + }) + }() + <-movedRevision + + ensuring := make(chan error, 1) + go func() { + ensuring <- testStore.WithOrgTx(testCtx, orgID, func(ctx context.Context) error { + _, err := testStore.EnsureDerivedRole(ctx, derivedRole(orgID, cause, "the viewer role", solutionUse, docWrite)) + return err + }) + }() + // Let the ensure take the role row and block on the revision row. + time.Sleep(300 * time.Millisecond) + close(reference) + + require.NoError(t, <-referencing, "the referencing writer") + require.NoError(t, <-ensuring, "the ensure") + require.ElementsMatch(t, []string{"solution:use", "doc:write"}, rolePermissions(t, id)) +} diff --git a/module/services/accounts/code/pkg/infra/postgres_derived_module_access.go b/module/services/accounts/code/pkg/infra/postgres_derived_module_access.go index d3848286c..a12e9404d 100644 --- a/module/services/accounts/code/pkg/infra/postgres_derived_module_access.go +++ b/module/services/accounts/code/pkg/infra/postgres_derived_module_access.go @@ -102,16 +102,23 @@ func (s *PostgresStore) EnsureDerivedRole(ctx context.Context, role business.Der // only a role that is missing, or whose description or permissions // differ, is touched, and only by what differs. // - // The read LOCKS the role row (FOR UPDATE: a row lock, which fires no - // trigger and so moves no revision; `app_tenant` may take it because it - // holds UPDATE on the description column). Two ensures of the same cause - // with different declarations therefore serialize on the row, as the upsert - // they replace did: the second reads the permissions the first committed - // and ends at its own declaration, last writer wins. Without the lock both - // would read before either commits and the role would end at their UNION, - // keeping a permission the later declaration dropped. + // The read LOCKS the role row (a row lock, which fires no trigger and so + // moves no revision; `app_tenant` may take it because it holds UPDATE on + // the description column). Two ensures of the same cause with different + // declarations therefore serialize on the row, as the upsert they replace + // did: the second reads the permissions the first committed and ends at + // its own declaration, last writer wins. Without the lock both would read + // before either commits and the role would end at their UNION, keeping a + // permission the later declaration dropped. + // + // FOR NO KEY UPDATE, the lock that upsert took, and not FOR UPDATE: every + // row that references the role (a grant of it, its permissions) takes FOR + // KEY SHARE through its foreign key, which FOR UPDATE conflicts with. + // installSolution moves the revision and then writes the standing grant of + // the solution role; an ensure of that role holding FOR UPDATE and then + // needing the revision row would deadlock with it. var roleID, description string - err := executor.QueryRow(ctx, `SELECT id::text, COALESCE(description, '') FROM roles WHERE derived_from = $1 FOR UPDATE`, role.DerivedFrom).Scan(&roleID, &description) + err := executor.QueryRow(ctx, `SELECT id::text, COALESCE(description, '') FROM roles WHERE derived_from = $1 FOR NO KEY UPDATE`, role.DerivedFrom).Scan(&roleID, &description) switch { case errors.Is(err, pgx.ErrNoRows): // The conflict target is the partial unique index on derived_from, so @@ -172,9 +179,10 @@ func (s *PostgresStore) EnsureDerivedRole(ctx context.Context, role business.Der if held[permission] { continue } - // ON CONFLICT DO NOTHING keeps a permission a racing ensure has - // already added from failing this one on the primary key. - if _, err := executor.Exec(ctx, `INSERT INTO role_permissions (role_id, resource, action) VALUES ($1, $2, $3) ON CONFLICT DO NOTHING`, + // No ON CONFLICT: under the role lock no other ensure can add a + // permission between the read above and this insert, and no other + // path writes a derived role's permissions (UpdateRole refuses it). + if _, err := executor.Exec(ctx, `INSERT INTO role_permissions (role_id, resource, action) VALUES ($1, $2, $3)`, roleID, permission[0], permission[1]); err != nil { return "", w.Wrapf(err, "failed to write the derived role's permission %s:%s", permission[0], permission[1]) }