Locks are written out at each call site, with four key conventions and no written order #186

New Issue

2026-10-06T21:25:45Z

cgalo5758 commented

2026-10-06 21:25:45 +00:00

The pool row lock is SQL in five places (internal/entitlements/grant_acts.go twice, grant_resumption.go, internal/server/operator_org_types.go, operator_plan_ladders.go) beside two generated queries doing the same. Advisory keys are built four ways: hashtext of a bare id (workspace, product, subscription, invoice), sharing one 32-bit space; hashtextextended of a prefixed string (lifecycle, desired state); hashtextextended of a bare name (materialization); a constant (domains registry). The entitlements spec fixes part of the order; the rest lives in call-site comments, and the plan-ending paths take the pool lock at different points (#180). #165 adds a usage-row lock under the workspace lock with nothing to check it against.

Done when: lock functions live in one place; every advisory key is built from a family prefix and an id; the families and their order are written once under docs/; the project lint allows pg_advisory_xact_lock and pool FOR UPDATE only in the helpers; and the deploy that changes keys runs no old process beside new ones.

The pool row lock is SQL in five places (`internal/entitlements/grant_acts.go` twice, `grant_resumption.go`, `internal/server/operator_org_types.go`, `operator_plan_ladders.go`) beside two generated queries doing the same. Advisory keys are built four ways: `hashtext` of a bare id (workspace, product, subscription, invoice), sharing one 32-bit space; `hashtextextended` of a prefixed string (lifecycle, desired state); `hashtextextended` of a bare name (materialization); a constant (domains registry). The entitlements spec fixes part of the order; the rest lives in call-site comments, and the plan-ending paths take the pool lock at different points (#180). #165 adds a usage-row lock under the workspace lock with nothing to check it against. Done when: lock functions live in one place; every advisory key is built from a family prefix and an id; the families and their order are written once under `docs/`; the project lint allows `pg_advisory_xact_lock` and pool `FOR UPDATE` only in the helpers; and the deploy that changes keys runs no old process beside new ones.

cgalo5758 added this to the Public launch milestone 2026-10-06 21:25:45 +00:00

cgalo5758 added the

kind

debt

area/entitlements area/billing area/integrations labels 2026-10-06 21:25:45 +00:00

cgalo5758 referenced this issue

2026-10-06 23:00:20 +00:00

Nothing looks for dead code, duplication, tangled functions or a value written from too many places #166

cgalo5758 referenced this issue

2026-10-07 08:00:29 +00:00

Adding a tier locks pools in creation order, while other paths sort them by pool id #194

cgalo5758 referenced this issue from a commit

2026-10-09 03:30:42 +00:00

Document the test kit, the transaction runner and the surface constructors

cgalo5758 commented

2026-10-09 03:44:20 +00:00

Part of this landed with the test-kit step of #166:

The transaction runner. db.InTx and db.Begin in internal/db open a transaction with a kind and return its begin and commit errors unwrapped. entitlements.Materializing and entitlements.RuleChange take the rendezvous lock and set lock_timeout; a plain transaction does neither. Every transaction that materializes or changes a rule opens this way.
The pool-lock helper. LockPools and LockPoolOfGrant in internal/entitlements are the only Go callers of the two queries that lock core.resource_pools, and LockPools takes several pools in ascending order. No other Go code locks a pool row. The database functions core.confer, core.end_conferral, core.align_conferral_shape, core.update_conferral_bounds and core.settle_obligation still lock it themselves, as docs/database-locks.md lists.

What remains:

One lock order is inverted (#196): recording a failed drain locks the obligation row before the pool row, while the drain locks the pool first, so the two can deadlock. ReorderPlanLadders has a smaller case of the same kind: it updates each ladder's sort_order in the order the drag submitted, so two opposite reorders at the same moment can deadlock.

The advisory key families are each taken by their own callers:

the rendezvous, hashtextextended('entitlement_materialization'), always first (test/mockshot-seed.sql also takes it, which the doc does not mention);
prefixed keys through hashtextextended: lifecycle:<instance> before the site and usage rows, and desired-state:<connection>:<kind>:<recipient> before the record row;
bare ids through hashtext: the Stripe subscription (after the rendezvous, before the pool rows), and the Stripe invoice, the product and the FedWiki workspace, each taken first;
the domain registry's literal key, taken first.

The order written once. docs/database-locks.md states it, but each caller takes its own locks, so nothing holds a new path to it.

The lint rule. Nothing refuses BeginTx outside internal/db; the only guard is a comment in internal/entitlements/tx.go asking that the call stay greppable. A forbidigo pattern like the two in .golangci.yml can hold it once the sites below convert. Tests open 73 more transactions by hand across 33 files, which the rule would also meet unless it excludes tests.

Transactions still opened by hand (14, outside tests). None of them materializes. Converting each to db.InTx with no kind changes only how its begin and commit errors read and that rollback runs through the runner, except where noted:

internal/domains/registry.go, WithLock: the registry's advisory lock moves into the function or a kind of its own.
internal/integration/lifecycle.go, RecordLifecycleRequest and RecordFirstLifecycleRequest: a failed commit today returns the answer along with the error.
internal/integration/registration.go, RegisterProviders: its provider stamp on core.resource_keys waits behind materializers' foreign-key locks with no timeout; under a rendezvous kind the wait would time out instead.
internal/integrations/discourse/workflows/activities.go, inTx: holds the transaction across Discourse HTTP calls, and converting leaves that as it is.
internal/integrations/fedwiki/usage/usage.go, BoundWorkspace; internal/integrations/fedwiki/workflows/reconcile.go, sweepLocally; internal/integrations/fedwiki/workflows/request_activities.go, finishAllWith: a return that writes nothing would commit instead of rolling back, with the same effect, and a failed commit today returns a result along with the error.
internal/integrations/stripe/workflows/invoice_reconcile.go, converge: its commit error is classified for Temporal's retry; converted, it would read like the function's own already-classified errors.
internal/server/operator_billing.go, MakeDefaultPrice: rendering moves after the transaction, and the per-step error labels collapse into one.
internal/server/operator_billing.go, SyncProductToStripe: renders its answers while holding the product lock and commits at two points, so converting needs an outcome returned from the transaction, one commit, and rendering after it.
internal/server/operator_topology.go, ReorderPlanLadders: rendering moves after the transaction.
internal/server/workspace_partials.go, CreateWorkspace: its pool-assignment insert waits behind a pool lock with no timeout; under a rendezvous kind the wait would time out instead.
internal/systemtenant/systemtenant.go, Ensure: two commit labels collapse into one.

Part of this landed with the test-kit step of #166: - **The transaction runner.** `db.InTx` and `db.Begin` in `internal/db` open a transaction with a kind and return its begin and commit errors unwrapped. `entitlements.Materializing` and `entitlements.RuleChange` take the rendezvous lock and set `lock_timeout`; a plain transaction does neither. Every transaction that materializes or changes a rule opens this way. - **The pool-lock helper.** `LockPools` and `LockPoolOfGrant` in `internal/entitlements` are the only Go callers of the two queries that lock `core.resource_pools`, and `LockPools` takes several pools in ascending order. No other Go code locks a pool row. The database functions `core.confer`, `core.end_conferral`, `core.align_conferral_shape`, `core.update_conferral_bounds` and `core.settle_obligation` still lock it themselves, as `docs/database-locks.md` lists. What remains: **One lock order is inverted** (#196): recording a failed drain locks the obligation row before the pool row, while the drain locks the pool first, so the two can deadlock. `ReorderPlanLadders` has a smaller case of the same kind: it updates each ladder's `sort_order` in the order the drag submitted, so two opposite reorders at the same moment can deadlock. **The advisory key families** are each taken by their own callers: - the rendezvous, `hashtextextended('entitlement_materialization')`, always first (`test/mockshot-seed.sql` also takes it, which the doc does not mention); - prefixed keys through `hashtextextended`: `lifecycle:<instance>` before the site and usage rows, and `desired-state:<connection>:<kind>:<recipient>` before the record row; - bare ids through `hashtext`: the Stripe subscription (after the rendezvous, before the pool rows), and the Stripe invoice, the product and the FedWiki workspace, each taken first; - the domain registry's literal key, taken first. **The order written once.** `docs/database-locks.md` states it, but each caller takes its own locks, so nothing holds a new path to it. **The lint rule.** Nothing refuses `BeginTx` outside `internal/db`; the only guard is a comment in `internal/entitlements/tx.go` asking that the call stay greppable. A forbidigo pattern like the two in `.golangci.yml` can hold it once the sites below convert. Tests open 73 more transactions by hand across 33 files, which the rule would also meet unless it excludes tests. **Transactions still opened by hand** (14, outside tests). None of them materializes. Converting each to `db.InTx` with no kind changes only how its begin and commit errors read and that rollback runs through the runner, except where noted: - `internal/domains/registry.go`, `WithLock`: the registry's advisory lock moves into the function or a kind of its own. - `internal/integration/lifecycle.go`, `RecordLifecycleRequest` and `RecordFirstLifecycleRequest`: a failed commit today returns the answer along with the error. - `internal/integration/registration.go`, `RegisterProviders`: its provider stamp on `core.resource_keys` waits behind materializers' foreign-key locks with no timeout; under a rendezvous kind the wait would time out instead. - `internal/integrations/discourse/workflows/activities.go`, `inTx`: holds the transaction across Discourse HTTP calls, and converting leaves that as it is. - `internal/integrations/fedwiki/usage/usage.go`, `BoundWorkspace`; `internal/integrations/fedwiki/workflows/reconcile.go`, `sweepLocally`; `internal/integrations/fedwiki/workflows/request_activities.go`, `finishAllWith`: a return that writes nothing would commit instead of rolling back, with the same effect, and a failed commit today returns a result along with the error. - `internal/integrations/stripe/workflows/invoice_reconcile.go`, `converge`: its commit error is classified for Temporal's retry; converted, it would read like the function's own already-classified errors. - `internal/server/operator_billing.go`, `MakeDefaultPrice`: rendering moves after the transaction, and the per-step error labels collapse into one. - `internal/server/operator_billing.go`, `SyncProductToStripe`: renders its answers while holding the product lock and commits at two points, so converting needs an outcome returned from the transaction, one commit, and rendering after it. - `internal/server/operator_topology.go`, `ReorderPlanLadders`: rendering moves after the transaction. - `internal/server/workspace_partials.go`, `CreateWorkspace`: its pool-assignment insert waits behind a pool lock with no timeout; under a rendezvous kind the wait would time out instead. - `internal/systemtenant/systemtenant.go`, `Ensure`: two commit labels collapse into one.

cgalo5758 referenced this issue

2026-10-09 03:50:02 +00:00

Recording a failed drain locks the obligation before the pool, while the drain locks the pool first #196

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: wiki-cafe/member-console#186