Locks are written out at each call site, with four key conventions and no written order #186
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: wiki-cafe/member-console#186
Reference in New Issue
Block a user
The pool row lock is SQL in five places (
internal/entitlements/grant_acts.gotwice,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:hashtextof a bare id (workspace, product, subscription, invoice), sharing one 32-bit space;hashtextextendedof a prefixed string (lifecycle, desired state);hashtextextendedof 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 allowspg_advisory_xact_lockand poolFOR UPDATEonly in the helpers; and the deploy that changes keys runs no old process beside new ones.Part of this landed with the test-kit step of #166:
db.InTxanddb.Beginininternal/dbopen a transaction with a kind and return its begin and commit errors unwrapped.entitlements.Materializingandentitlements.RuleChangetake the rendezvous lock and setlock_timeout; a plain transaction does neither. Every transaction that materializes or changes a rule opens this way.LockPoolsandLockPoolOfGrantininternal/entitlementsare the only Go callers of the two queries that lockcore.resource_pools, andLockPoolstakes several pools in ascending order. No other Go code locks a pool row. The database functionscore.confer,core.end_conferral,core.align_conferral_shape,core.update_conferral_boundsandcore.settle_obligationstill lock it themselves, asdocs/database-locks.mdlists.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.
ReorderPlanLaddershas a smaller case of the same kind: it updates each ladder'ssort_orderin 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:
hashtextextended('entitlement_materialization'), always first (test/mockshot-seed.sqlalso takes it, which the doc does not mention);hashtextextended:lifecycle:<instance>before the site and usage rows, anddesired-state:<connection>:<kind>:<recipient>before the record row;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 order written once.
docs/database-locks.mdstates it, but each caller takes its own locks, so nothing holds a new path to it.The lint rule. Nothing refuses
BeginTxoutsideinternal/db; the only guard is a comment ininternal/entitlements/tx.goasking that the call stay greppable. A forbidigo pattern like the two in.golangci.ymlcan 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.InTxwith 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,RecordLifecycleRequestandRecordFirstLifecycleRequest: a failed commit today returns the answer along with the error.internal/integration/registration.go,RegisterProviders: its provider stamp oncore.resource_keyswaits 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.The list of transactions still opened by hand has changed since the comment above.
RecordLifecycleRequestandRecordFirstLifecycleRequestnow open theirs throughdb.InTx(inOwnTxininternal/integration/lifecycle.go), and so doesRegisterProviders(internal/integration/registration.go:207).SyncProductToStripe's transaction moved with its act intoenqueue(internal/integrations/stripe/catalogsync/catalogsync.go:179). Its comment says why the transaction outlives the function: a no-op or a failure keeps the product's advisory lock untilSynccalls theendit returns.db.Beginreturns an open transaction for exactly that case, and nothing in the comment needsBeginTxitself, so it converts like the others.That leaves 11 calls to
BeginTxoutside tests, the test kit andinternal/db:internal/domains/registry.go:454,WithLockinternal/integrations/discourse/workflows/activities.go:64,inTxinternal/integrations/fedwiki/usage/usage.go:74,BoundWorkspaceinternal/integrations/fedwiki/workflows/reconcile.go:304,sweepLocallyinternal/integrations/fedwiki/workflows/request_activities.go:181,finishAllWithinternal/integrations/stripe/catalogsync/catalogsync.go:179,enqueueinternal/integrations/stripe/workflows/invoice_reconcile.go:411,convergeinternal/server/operator_billing.go:1287,MakeDefaultPriceinternal/server/operator_topology.go:382,ReorderPlanLaddersinternal/server/workspace_partials.go:121,CreateWorkspaceinternal/systemtenant/systemtenant.go:55,EnsureThe three in
internal/serveralso open a transaction inside a handler, which #221 moves out. Opening the moved transactions throughdb.InTxsettles their entries here.One more transaction shape for the lock helpers to settle.
inOwnTxininternal/integration/lifecycle.gorunsRecordLifecycleRequestandRecordFirstLifecycleRequest. It opens a transaction only when it is handed a*sql.DB, and uses any other handle as it is. That is right for a*sql.Tx. It is wrong for a*sql.Conn, which also satisfies the package'sDBTX. Outside a transaction, each statement on a connection commits on its own, so the instance lock (pg_advisory_xact_lockinLockLifecycleInstance) is released as soon as it is taken. The read of the instance's latest generation and the insert after it are then no longer serialized. An older tab's request could be recorded as new after a newer one, which is the race the lock exists to close. Two requests at the same moment would get a unique-index error instead of the busy answer. I have read this, not run it.No caller passes a connection today. The concurrency test in
internal/integration/lifecycle_db_test.gopins a connection to learn its backend id, and it begins a transaction on that connection before it records. A test that recorded on the connection itself would still pass, because that test's two recordings already run one after the other.Either of two changes closes it.
inOwnTxcan ask whether the handle is adb.Beginner(a*sql.DBor a*sql.Conn) instead of a*sql.DB. Or, when the lifecycle lock moves into the shared lock helpers, that helper can take a*sql.Tx, so a connection does not compile.Its body says the usage-row lock is taken under the FedWiki workspace lock. That does not hold: the usage row's lock is taken by the slot gate, the outcome steps, settle, the sweep and the sync, which run in workflow activities and boot passes that hold no workspace lock; it comes after the site rows and before the request rows, as
docs/database-locks.mdnow writes down in its general lock order and for each path.Of the transactions still opened by hand in the list above,
BoundWorkspaceis deleted, andsweepLocallyandfinishAllWithnow open throughdb.InTxat READ COMMITTED, which leaves eightBeginTxcalls outside tests, the test kit andinternal/db.usage.Recountandusage.Holdtake a*sql.Tx, so a handle that is not a transaction does not compile, the shape the comment above suggests for the lifecycle lock.The transaction runner is
db.InTxanddb.Beginininternal/db/tx.go: they open a transaction of a named kind, which states its isolation and what runs first in it, such as the shared lock every transaction that recomputes an organization's limits takes. golangci-lint now refusesBeginTxand(*sql.DB).Beginanywhere outsideinternal/db, tests and the test kit, with a message naming the runner. Tests stay outside the rule, because they open raw transactions to hold a lock while the code under test waits. Five of the transactions listed above now open through the runner: the domain registry's, whose advisory lock becomes the first statement of a kind of its own; the Discourse sweep's and the System tenant's boot step, throughdb.InTx; and the Stripe product sync's and the invoice reconcile's, throughdb.Begin, which keeps their own commit. Nothing they write changes; a failed begin or commit in the first three reads without the label that named the step.MakeDefaultPrice,ReorderPlanLaddersandCreateWorkspacestay as pinned exceptions that name #221, which moves their transactions out of the handlers.The ladder reorder on the topology page now runs at READ COMMITTED, whatever the database server's default, and locks every ladder row in ascending id order before it reads or writes, with
FOR NO KEY UPDATE, the lock its update takes anyway. Two reorders that drag ladders in opposite orders at the same moment no longer deadlock: the second waits, then reads the order the first committed and writes its whole submitted order, where before it could skip a ladder whose stale position happened to match. A conferral that inserts a position on a ladder takes a key-share lock on it through the foreign key, which this lock does not block.What remains for this issue is the advisory keys: one way to build them, the families written once, and the lint rule for advisory and pool locks.
Commit:
6034b033An advisory lock is a Postgres lock on a number of the application's choosing rather than on a row; the console takes them per transaction to serialize work on one object, such as two reconciles of one Stripe subscription. Each kind of object now has a family, and every key is built one way:
hashtextextended('<family>:<id>', 0), a UUID written in canonical form, so two spellings of one id take one lock.db.Lockanddb.LockSharedininternal/db/locks.gobuild the key and take the lock, on a*sql.Txonly, and every advisory lock in the code goes through them. Each family's statement begins with a name line such as-- name: LockSubscription :exec, sopg_stat_activityshows which family a waiting backend waits on, and the statement traces in the tests record that name.The families, in the order a transaction takes them:
materialization(the shared or exclusive lock every transaction that recomputes an organization's limits, or changes a rule, takes first),domains-registry,fedwiki-workspace,lifecycle,stripe-subscription,stripe-invoice,stripe-product-sync,desired-state.docs/database-locks.mdlists them in a table and writes one order every path keeps: that lock first, then the families in the table's order, then rows. The project lint fails when the table and the code's list disagree.The keys of
lifecycleanddesired-stateare unchanged. The other six changed, the materialization lock's included, so a migration changes the key the database's own check of that lock looks for, and the two seeds that take it take the new one. Two processes that build different keys for one object share no lock, so this release must be deployed with every process of the previous one stopped first; the production guide now states that order for every upgrade.The project lint now refuses an advisory lock function named, in any case, in a Go string or in SQL or shell code outside the helpers, the test kit's probe and its own wait tests, the lint rule's own file and tests, the two migrations that name the rendezvous and
internal/db's own tests; a seed that takes any advisory key but the materialization lock's, or takes that one anywhere but first after itsBEGIN;; a row lock oncore.resource_poolsin any query butLockResourcePoolandLockGrantPool, or in a Go string outside the tests; and a call or method value of those two queries outsideinternal/entitlements/tx.go, whereLockPoolsandLockPoolOfGrantcall them. Tests take a family's lock through the same helpers and probe one withdbtest.TryLock, so a test holds the key the code takes.Recording a lifecycle request now comes in two forms:
RecordLifecycleRequestopens its own transaction on a database or a pinned connection, andRecordLifecycleRequestInTxruns in the caller's*sql.Tx. A connection handed to the first is no longer used outside a transaction, which would have released the instance lock as soon as it was taken; handed to the second, it does not compile. The same holds forRecordFirstLifecycleRequest.Commit:
e52fdb37