Repository navigation
fix(accounts): ensuring a derived role that is already as declared writes nothing - #1259
Merged
Merged
Conversation
…ites nothing
Every reconcile pass converges the fixture's installations, and an
installation that cannot converge yet (a solution whose install step is not
acknowledged, so its teams wait) is retried each pass. The retry resolves the
installation's solution role through EnsureDerivedRole, which upserted the
role and deleted and re-inserted every permission. roles and role_permissions
carry BEFORE triggers that move the organization's authorization revision on
every row they see, an upsert that changes nothing included, so each pass
moved the revision about nine times per role and every capability minted
before it was refused ("authorization revision superseded: minted at
revision 2473, issuer is at 2495").
EnsureDerivedRole now reads the role and its permissions and writes only
what differs: a missing role, a changed description, a permission removed or
added. Idle reconcile passes and idle fixture convergence leave the
revision where it is; a real change still moves it.
The fold of the envelope revision into module installation revisions
(#1257) is not the cause: reconcile passes alone left the revision unchanged.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
crisfiorini-oss
pushed a commit
to crisfiorini-oss/module-saas-starter
that referenced
this pull request
Oct 27, 2026
…le row (codefly-dev#1261) * fix(accounts): racing ensures of one derived role serialize on the role row codefly-dev#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 <noreply@anthropic.com> * 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 <noreply@anthropic.com> --------- Co-authored-by: Antoine Toussaint <312536060+antoinetoussaint-byte@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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.
What we are doing
Every capability is minted at its organization's authorization revision and refused once that moves. On a composed platform run against saas
3c530337, the hop refused a call withauthorization revision superseded: minted at revision 2473, issuer is at 2495: the revision moved 22 times within one request. Nothing long-running could work.Root cause
fixtures/installations.go,Converge) runs after every reconcile pass (work.go,AfterEachPass(installations.Converge)). A declared installation that cannot converge yet is retried on every pass. The typical case is a solution whose install step has not been acknowledged, so its teams cannot be exposed yet.solutionExposureRole→solutionRoleID→EnsureDerivedRole,pkg/infra/postgres_derived_module_access.go:91).EnsureDerivedRoleupserted the role and deleted and re-inserted every permission.rolesandrole_permissionscarryBEFORE INSERT OR UPDATE OR DELETEtriggers that bump the organization's revision for every row they see (roles_bump_authorization_revision,role_permissions_bump_authorization_revision). A BEFORE INSERT trigger fires even when the upsert turns into a no-op, so each pass bumped the revision about nine times per role.The fix
EnsureDerivedRolereads the role and its permissions first, and writes only what differs: a missing role, a changed description, a permission removed or added. Idle passes write nothing, and a real change still moves the revision.Tests
pkg/adapters/host_interface_conformance_test.go, "idle reconcile passes and fixture convergence leave the authorization revision alone": three reconcile passes plus fixture convergence leave Acme's revision unchanged, while a real role change moves it. It fails without the fix. Before the fix, each idle convergence moved the revision by about 25 sequence steps.pkg/infra/derived_role_revision_test.go: ensuring a role already as declared moves no revision; a changed permission set, or a changed description, is written (only the difference).Fix or hack
A fix, at the store method that wrote what it did not need to. Reused: the existing derived-role reconciliation and its callers, unchanged. Not done here: the solution installation that stays pending, and its retries, are a separate finding with their own PR.
Evidence
go vet(both tag sets),go test -race -count=1 -p 1forpkg/infraandpkg/business,go test -tags=pure ./..., and the conformance harness (78 cases) pass locally.🤖 Generated with Claude Code