From 6034b033ad9dcb1c19b2a8085d2ab8d3cea891c5 Mon Sep 17 00:00:00 2001 From: Christian Galo Date: Sat, 31 Oct 2026 02:24:08 -0500 Subject: [PATCH] Open every transaction through the runner, and lock the ladder reorder's rows in order First of two changes for #186. golangci-lint now refuses BeginTx and (*sql.DB).Begin outside internal/db, tests and the test kit, with a message naming the runner (db.InTx, db.Begin). Tests stay outside the rule because they open raw transactions to hold a lock while the code under test waits; go-static-analysis records that exclusion. Five hand-opened transactions 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, through db.InTx; and the Stripe product sync's and the invoice reconcile's, through db.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 its step. MakeDefaultPrice, ReorderPlanLadders and CreateWorkspace stay pinned exceptions that name #221, which moves their transactions out of the handlers. The ladder reorder now runs at READ COMMITTED, whatever the server's default, and locks every ladder row in ascending id order with FOR NO KEY UPDATE (LockPlanLadderOrder) before it reads or writes. Two reorders in opposite orders at once no longer deadlock: the second waits, 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's foreign-key share lock on a ladder is not blocked. The tests run the reorder at the default and at a REPEATABLE READ default. docs/database-locks.md, docs/go-conventions.md and docs/testing.md say what the code does. No user-visible string changes; the deploy changes no data. Break list: 12 records, 12 CAUGHT. Named without a record: the error labels three conversions drop (log text no test or caller compares), the two conversions that change no behaviour, and one comment. Full suite on reset databases: 5656 passed, 16 skipped, none failed. --- .golangci.yml | 26 ++ docs/database-locks.md | 25 +- docs/go-conventions.md | 6 +- docs/testing.md | 3 +- internal/billing/plan_ladders.sql.go | 42 +++ internal/billing/querier.go | 8 + internal/billing/queries/plan_ladders.sql | 13 + internal/domains/registry.go | 37 +-- internal/domains/registry_db_test.go | 26 ++ internal/entitlements/tx.go | 14 +- .../discourse/workflows/activities.go | 18 +- .../stripe/catalogsync/catalogsync.go | 2 +- .../stripe/workflows/invoice_reconcile.go | 3 +- internal/server/operator_topology.go | 36 ++- .../operator_topology_reorder_db_test.go | 276 ++++++++++++++++++ internal/systemtenant/systemtenant.go | 30 +- .../.openspec.yaml | 2 + .../design.md | 139 +++++++++ .../proposal.md | 45 +++ .../specs/go-static-analysis/spec.md | 56 ++++ .../specs/operator-topology-overview/spec.md | 64 ++++ .../tasks.md | 244 ++++++++++++++++ 22 files changed, 1044 insertions(+), 71 deletions(-) create mode 100644 internal/server/operator_topology_reorder_db_test.go create mode 100644 openspec/changes/transactions-open-through-the-runner/.openspec.yaml create mode 100644 openspec/changes/transactions-open-through-the-runner/design.md create mode 100644 openspec/changes/transactions-open-through-the-runner/proposal.md create mode 100644 openspec/changes/transactions-open-through-the-runner/specs/go-static-analysis/spec.md create mode 100644 openspec/changes/transactions-open-through-the-runner/specs/operator-topology-overview/spec.md create mode 100644 openspec/changes/transactions-open-through-the-runner/tasks.md diff --git a/.golangci.yml b/.golangci.yml index 10add8ff..de056fdd 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -27,6 +27,11 @@ linters: - pattern: '^http\.Request\.FormValue$' pkg: '^net/http$' msg: read a posted field from r.PostForm; r.FormValue falls back to the URL query (docs/operator-ux-conventions.md) + - pattern: '\.BeginTx$' + msg: open the transaction through db.InTx or db.Begin with a kind (docs/go-conventions.md) + - pattern: '^sql\.DB\.Begin$' + pkg: '^database/sql$' + msg: open the transaction through db.InTx or db.Begin with a kind (docs/go-conventions.md) exclusions: generated: strict warn-unused: true @@ -36,6 +41,14 @@ linters: - path: '^internal/(integrations/stripe|stripetest)/' linters: [forbidigo] text: 'take the key mode from the injected configuration' + # The runner opens every transaction (docs/go-conventions.md); tests and + # the test kit open raw ones to hold a lock or to roll back a fixture. + - path: '^internal/db/' + linters: [forbidigo] + text: 'open the transaction through db\.InTx or db\.Begin' + - path: '(_test\.go$|^internal/testkit/)' + linters: [forbidigo] + text: 'open the transaction through db\.InTx or db\.Begin' # A lookup that fails is the probe's answer: the check reports # ProbeStateError, the state that says the record could not be checked. - path: '^internal/workflows/domains/verify\.go$' @@ -69,6 +82,19 @@ linters: - path: '^internal/integrations/fedwiki/web/requests\.go$' linters: [forbidigo] source: 'parseAddressValues\(r\.FormValue\("instance"\), r\.FormValue\("generation"\)\)' # #205 + # #221: each handler opens its own transaction until its act moves to the package that owns its tables. + - path: '^internal/server/operator_billing\.go$' + linters: [forbidigo] + text: 'open the transaction through db\.InTx or db\.Begin' + source: 'tx, err := h\.Database\.BeginTx\(ctx, nil\)' # #221 + - path: '^internal/server/operator_topology\.go$' + linters: [forbidigo] + text: 'open the transaction through db\.InTx or db\.Begin' + source: 'tx, err := h\.Database\.BeginTx\(ctx, &sql\.TxOptions\{Isolation: sql\.LevelReadCommitted\}\)' # #221 + - path: '^internal/server/workspace_partials\.go$' + linters: [forbidigo] + text: 'open the transaction through db\.InTx or db\.Begin' + source: 'tx, err := h\.Database\.BeginTx\(r\.Context\(\), nil\)' # #221 issues: max-issues-per-linter: 0 max-same-issues: 0 diff --git a/docs/database-locks.md b/docs/database-locks.md index 62fd449a..de9b331e 100644 --- a/docs/database-locks.md +++ b/docs/database-locks.md @@ -51,6 +51,7 @@ flag-only writes change neither claims nor allocation counts. | Plan act obligation row | `core.plan_act_obligations` row of one (act, pool) | `FOR UPDATE` by a move, a listing batch's supersession and Retry's requeue; `UPDATE` by the failure recorder | the `LockPlanActObligation` query (a move, after the pool row), which calls `core.lock_plan_act_obligation`; the function takes the row `FOR UPDATE`, `core.write_plan_act_obligations` (the open obligations of earlier acts on the batch's pools), `core.requeue_plan_act_obligations` (the retried act's failed obligations of the types it still owns) and `core.record_plan_act_obligation_failure` | Two passes settling one obligation, and a settlement against a supersession or a requeue. A batch and a Retry take several rows in ascending `pool_id` order and touch disjoint rows: a batch supersedes the open rows of earlier acts on the types its act owns, a Retry requeues the failed rows of the types its act still owns, and both hold the shared rendezvous, so no default change can move the ownership while either runs. | | Product row | `core.products` row of one product | `FOR SHARE` by the tier trigger; `FOR NO KEY UPDATE` by the product edit's `UPDATE` | `core.refuse_tier_product_without_set`, the trigger on every insert into `core.plan_ladder_tiers` and every change of a tier's product; `UpdateProduct` | A tier insert against a clear of the product's set, so the two cannot both commit at READ COMMITTED, the isolation both writers run at (product-catalog invariant 13). The writer holds `UPDATE` on the table, so the trigger takes the lock with the caller's rights. The tier add takes the row after its pool rows; the product edit takes it alone. | | Org type row | `core.org_types` row of one type | `FOR NO KEY UPDATE` | the `LockOrgType` query, run by the default change's T0 | The T0 against the demonstration seed and an edit of the type's other columns, which write the row without the rendezvous. Every other writer of the default already waits on the exclusive rendezvous. `FOR NO KEY UPDATE` does not conflict with the `FOR KEY SHARE` an organization insert takes on its type's row. | +| Plan ladder rows | every `core.plan_ladders` row | `FOR NO KEY UPDATE`, ascending `plan_ladder_id` | the `LockPlanLadderOrder` query, run by the topology reorder (`ReorderPlanLadders`) first in its transaction | Two reorders, so the later one waits and writes against the order the earlier committed. It does not conflict with the `FOR KEY SHARE` a tier, a position or an org type's default takes through its foreign key. | | Grant lineage head | the `core.grants` row no other grant extends | `FOR UPDATE OF g` | the `GetGrantLineageHeadForUpdate` query, run by `RevokeGrantTx`, `ExtendWaitingGrantTx` and `ManageGrantTx` | Acts on one lineage, so an extension that committed while another act waited is visible to it. | | Usage row | `core.numeric_entitlement_usage` row of one (pool, resource key) | `FOR UPDATE` | the `LockNumericUsage` query, run by `usage.Recount` and by the slot gate, after the site rows the transaction writes and before the request rows it finishes (the hold rows have no place in the order, see rule 5); the count is a statement of its own after the lock | Two recounts of one pool, so each count reads every change committed before its lock was granted; two slot gates for one pool, so the second counts the first's hold and two creates cannot both take the last slot. | | FedWiki site row | `fedwiki.sites` row of one domain or one site id | `FOR UPDATE`; `FOR SHARE` by `GetSiteForPlacement` | the `LockSiteStatusByDomain` query, run by `sweepLocally` and the sync's status write (`writeStatus`), and the `LockSiteStatusByID` query, run by the slot gate and by `RecordSiteStatusActivity` and `RecordSwapActivity` (which check the recorded workspace and domain under it), and `LockArchivedSiteForPurge`, run by retention; `GetSiteForPlacement`, run by `reconcileSite` and `syncPlacement` inside `Registry.WithLock` immediately before the first claim or placement write | Paths that move one site (a member's request, the downgrade sweep and the farm sync), so each writes the status it finds under the lock and the recount that follows counts the row as it ended; the sync's reading of the site's requests, which therefore reflects every request that committed before the lock was granted; the gate's reading of an activation whose row the sweep already made active; retention's eligibility check through deletion; and a status write or deletion against creation of that site's placement, so the writer's trigger sees the placement. | @@ -360,6 +361,14 @@ transaction timeout before the rendezvous statement. batch's supersession and a Retry's requeue, take them in ascending order and touch disjoint rows. +The ladder reorder on the topology page (`ReorderPlanLadders`) is not a plan +act: it changes only the ladders' display order, which no conferral reads. A +READ COMMITTED transaction takes every ladder row (`LockPlanLadderOrder`), then +updates the `sort_order` of each ladder whose position changed. It opens at +READ COMMITTED whatever the server's default, because the lock read that waited +returns the order the earlier reorder committed, where REPEATABLE READ would +fail it with a serialization error. + ### Invoice reconcile `invoiceReconcile.converge` opens a plain transaction, with no rendezvous, and @@ -573,14 +582,14 @@ An eligible creator waits for that site while holding the registry lock. The retention purge may hold it through its farm call for 30 seconds, delaying other registry operations; skipped sites do not incur this wait. -`domains.Registry.WithLock` opens a plain transaction and takes the registry -lock before anything else. Every composite operation, including the FedWiki -create, release and sync projections, runs on the `*Tx` it passes, so the lock -is taken once. The comment on `registryAllocationLock` states the rule for a -transaction that would touch both the registry and a usage row: registry lock -first. No transaction does both today: the slot gate and the recounts are -workflow activities that run their own transactions, apart from the -transaction that allocates or releases the name. +`domains.Registry.WithLock` opens its transaction through the runner with +`registryKind`, whose first statement is the registry lock. Every composite +operation, including the FedWiki create, release and sync projections, runs on +the `*Tx` it passes, so the lock is taken once. The comment on +`registryAllocationLock` states the rule for a transaction that would touch both +the registry and a usage row: registry lock first. No transaction does both +today: the slot gate and the recounts are workflow activities that run their own +transactions, apart from the transaction that allocates or releases the name. ## Rules every path keeps diff --git a/docs/go-conventions.md b/docs/go-conventions.md index fb1d9358..861c1663 100644 --- a/docs/go-conventions.md +++ b/docs/go-conventions.md @@ -116,8 +116,10 @@ page that shows a choice and the endpoint that enforces it come to disagree. ## Transactions and pool locks New code opens a transaction through `db.InTx` or `db.Begin` -(`internal/db/tx.go`) with a kind; the paths that still call `BeginTx` -directly are listed in #186. `db.InTx(ctx, database, kind, fn)` commits when +(`internal/db/tx.go`) with a kind; golangci-lint refuses `BeginTx` and +`(*sql.DB).Begin` outside `internal/db`, tests and the test kit +(`.golangci.yml`); three handlers #221 moves out of `internal/server` are its +pinned exceptions. `db.InTx(ctx, database, kind, fn)` commits when `fn` returns nil and rolls back on an error or a panic. `db.Begin` returns the transaction for a caller that needs to keep it open across several steps. A kind says how the transaction opens: `db.Plain` and `db.ReadCommitted` open with nothing run diff --git a/docs/testing.md b/docs/testing.md index 9c4c5a63..e851ca3b 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -326,7 +326,8 @@ checks, and `make lint-templates` is a sixth that runs on its own: `rows` and `sql` handling (rowserrcheck, sqlclosecheck), unused parameters (unparam), duplicated code (dupl), function complexity (gocognit, limit 30) and forbidden identifiers - (forbidigo), among them the rod waits the browser tests refuse (type 7). + (forbidigo), among them the rod waits the browser tests refuse (type 7) and + `BeginTx` or `(*sql.DB).Begin` outside `internal/db`, tests and the test kit. - **`scripts/deadcode.sh`** — production functions no program reaches, except those listed in `scripts/deadcode-testonly.txt`. - **`go run ./scripts/sqlc-unused`** — generated queries with no caller outside diff --git a/internal/billing/plan_ladders.sql.go b/internal/billing/plan_ladders.sql.go index ec0a6c2f..23075d90 100644 --- a/internal/billing/plan_ladders.sql.go +++ b/internal/billing/plan_ladders.sql.go @@ -582,6 +582,48 @@ func (q *Queries) ListTiersWithProductsByLadderIDs(ctx context.Context, planLadd return items, nil } +const lockPlanLadderOrder = `-- name: LockPlanLadderOrder :many +SELECT plan_ladder_id, sort_order +FROM core.plan_ladders +ORDER BY plan_ladder_id +FOR NO KEY UPDATE +` + +type LockPlanLadderOrderRow struct { + PlanLadderID string `json:"plan_ladder_id"` + SortOrder int32 `json:"sort_order"` +} + +// Every ladder's display order, each row locked in ascending id order: the +// topology reorder takes them first in its transaction, so two reorders wait +// for each other instead of deadlocking, and the later one compares against +// the order the earlier committed. FOR NO KEY UPDATE is the lock the +// reorder's update of sort_order takes anyway; it does not conflict with the +// FOR KEY SHARE a tier, a position or an org type's default takes on the +// ladder through its foreign key, so no conferral waits on a reorder. +func (q *Queries) LockPlanLadderOrder(ctx context.Context) ([]LockPlanLadderOrderRow, error) { + rows, err := q.db.QueryContext(ctx, lockPlanLadderOrder) + if err != nil { + return nil, err + } + defer rows.Close() + items := []LockPlanLadderOrderRow{} + for rows.Next() { + var i LockPlanLadderOrderRow + if err := rows.Scan(&i.PlanLadderID, &i.SortOrder); err != nil { + return nil, err + } + items = append(items, i) + } + if err := rows.Close(); err != nil { + return nil, err + } + if err := rows.Err(); err != nil { + return nil, err + } + return items, nil +} + const setPlanLadderSortOrder = `-- name: SetPlanLadderSortOrder :exec UPDATE core.plan_ladders SET sort_order = $2 diff --git a/internal/billing/querier.go b/internal/billing/querier.go index ca812541..4002d940 100644 --- a/internal/billing/querier.go +++ b/internal/billing/querier.go @@ -474,6 +474,14 @@ type Querier interface { // ListTiersByLadderWithProducts for several ladders in one read (#10): the // tiers of each ladder as it is now, by rank, with each tier's product. ListTiersWithProductsByLadderIDs(ctx context.Context, planLadderIds []string) ([]ListTiersWithProductsByLadderIDsRow, error) + // Every ladder's display order, each row locked in ascending id order: the + // topology reorder takes them first in its transaction, so two reorders wait + // for each other instead of deadlocking, and the later one compares against + // the order the earlier committed. FOR NO KEY UPDATE is the lock the + // reorder's update of sort_order takes anyway; it does not conflict with the + // FOR KEY SHARE a tier, a position or an org type's default takes on the + // ladder through its foreign key, so no conferral waits on a reorder. + LockPlanLadderOrder(ctx context.Context) ([]LockPlanLadderOrderRow, error) // Second half of the make-default handshake. Scoped to (price_id, product_id) // so a stale/forged price ID from another product cannot be promoted; the // is_active guard keeps a deactivated price from becoming the default. diff --git a/internal/billing/queries/plan_ladders.sql b/internal/billing/queries/plan_ladders.sql index 2df08778..461d80ca 100644 --- a/internal/billing/queries/plan_ladders.sql +++ b/internal/billing/queries/plan_ladders.sql @@ -34,6 +34,19 @@ UPDATE core.plan_ladders SET sort_order = $2 WHERE plan_ladder_id = $1; +-- name: LockPlanLadderOrder :many +-- Every ladder's display order, each row locked in ascending id order: the +-- topology reorder takes them first in its transaction, so two reorders wait +-- for each other instead of deadlocking, and the later one compares against +-- the order the earlier committed. FOR NO KEY UPDATE is the lock the +-- reorder's update of sort_order takes anyway; it does not conflict with the +-- FOR KEY SHARE a tier, a position or an org type's default takes on the +-- ladder through its foreign key, so no conferral waits on a reorder. +SELECT plan_ladder_id, sort_order +FROM core.plan_ladders +ORDER BY plan_ladder_id +FOR NO KEY UPDATE; + -- name: DeletePlanLadder :exec DELETE FROM core.plan_ladders WHERE plan_ladder_id = $1; diff --git a/internal/domains/registry.go b/internal/domains/registry.go index 49c1db65..f0f008ac 100644 --- a/internal/domains/registry.go +++ b/internal/domains/registry.go @@ -17,6 +17,7 @@ import ( "github.com/google/uuid" "github.com/jackc/pgx/v5/pgconn" + "git.coopcloud.tech/wiki-cafe/member-console/internal/db" "git.coopcloud.tech/wiki-cafe/member-console/internal/dnsname" ) @@ -71,6 +72,16 @@ const ( // the usage row, would otherwise grab the two in opposite orders and deadlock. const registryAllocationLock int64 = 0x646f6d61696e73 +// registryKind opens a registry allocation's transaction: the registry's +// advisory lock is its first statement, so nothing in the transaction runs +// before the lock (docs/database-locks.md, Registry). +var registryKind = db.FirstRuns(func(ctx context.Context, tx *sql.Tx) error { + if _, err := tx.ExecContext(ctx, `SELECT pg_advisory_xact_lock($1)`, registryAllocationLock); err != nil { + return fmt.Errorf("domains: registry advisory lock: %w", err) + } + return nil +}) + // Claim policy defaults (design D6). Each is the fallback for the matching // Policy field, so a Registry built without WithPolicy — the provider // activity registries, boot reconciliation, every test — allocates exactly as @@ -462,28 +473,18 @@ func (t *Tx) PlacementByResource(ctx context.Context, provider, resourceRef stri } // WithLock runs fn inside a transaction holding the registry advisory lock -// (released on commit or rollback). Returning an error rolls the whole -// composite back; returning nil commits. See registryAllocationLock for the -// lock-ordering invariant against entitlement usage rows. +// (released on commit or rollback), opened through the runner with +// registryKind. Returning an error rolls the whole composite back; returning +// nil commits. fn's error, a failed begin and a failed commit come back as +// the runner returns them. See registryAllocationLock for the lock-ordering +// invariant against entitlement usage rows. func (r *Registry) WithLock(ctx context.Context, fn func(context.Context, *Tx) error) error { if r.db == nil { return ErrNoDatabase } - tx, err := r.db.BeginTx(ctx, nil) - if err != nil { - return fmt.Errorf("domains: begin tx: %w", err) - } - defer tx.Rollback() - if _, err := tx.ExecContext(ctx, `SELECT pg_advisory_xact_lock($1)`, registryAllocationLock); err != nil { - return fmt.Errorf("domains: registry advisory lock: %w", err) - } - if err := fn(ctx, &Tx{q: New(tx), policy: r.Policy(), executor: tx}); err != nil { - return err - } - if err := tx.Commit(); err != nil { - return fmt.Errorf("domains: commit: %w", err) - } - return nil + return db.InTx(ctx, r.db, registryKind, func(tx *sql.Tx) error { + return fn(ctx, &Tx{q: New(tx), policy: r.Policy(), executor: tx}) + }) } // CheckAvailability answers whether workspaceID could allocate fqdn right diff --git a/internal/domains/registry_db_test.go b/internal/domains/registry_db_test.go index 616cd6d3..77e65a55 100644 --- a/internal/domains/registry_db_test.go +++ b/internal/domains/registry_db_test.go @@ -9,10 +9,13 @@ import ( "errors" "fmt" "net" + "slices" "sync" "testing" "time" + "github.com/google/uuid" + "git.coopcloud.tech/wiki-cafe/member-console/internal/testkit/dbtest" ) @@ -753,3 +756,26 @@ func TestCheckAvailabilityOutsideEveryClaim(t *testing.T) { t.Errorf("malformed name = (%s, %q), want invalid with the validator message", av.Verdict, av.MemberMessage()) } } + +// registryLockStatement is the statement the registry's kind runs first, +// named as dbtest.Counted names a raw statement: its first line. +const registryLockStatement = `SELECT pg_advisory_xact_lock($1)` + +// TestRegistryLockIsTheFirstStatement proves a registry transaction runs the +// registry lock before any statement of its composite: the statements of one +// WithLock begin with the begin and then the lock. +func TestRegistryLockIsTheFirstStatement(t *testing.T) { + counted, statements := dbtest.Counted(t) + err := NewRegistry(counted).WithLock(context.Background(), func(ctx context.Context, tx *Tx) error { + _, err := tx.PlacementByResource(ctx, "test", uuid.NewString()) + return err + }) + if err != nil { + t.Fatalf("a registry composite returned %v", err) + } + got := statements.Names() + want := []string{"begin", registryLockStatement} + if len(got) < len(want) || !slices.Equal(got[:len(want)], want) { + t.Errorf("a registry transaction ran %v, want begin and then the registry lock", got) + } +} diff --git a/internal/entitlements/tx.go b/internal/entitlements/tx.go index 138f22e9..3a040fb5 100644 --- a/internal/entitlements/tx.go +++ b/internal/entitlements/tx.go @@ -35,12 +35,14 @@ const rendezvousLockTimeout = 5 * time.Second const rendezvousMissingMessage = "materialization_rendezvous_missing" // Materializing opens a transaction that may materialize pools: it sets the -// lock timeout and takes the shared rendezvous before anything else. A -// BeginTx in a file that also materializes, confers or reapplies defaults is -// the defect this kind exists to make greppable. It runs at READ COMMITTED, -// the server's default: under snapshot isolation the statement that waits for -// the rendezvous takes the snapshot before the wait ends, so a materializer -// queued behind a rule commit would read the rules as they were before it. +// lock timeout and takes the shared rendezvous before anything else. +// golangci-lint refuses BeginTx outside internal/db, so a transaction that +// materializes opens through this kind, or through a kind without the +// rendezvous, which core.assert_rendezvous then refuses. It runs at READ +// COMMITTED, the server's default: under snapshot isolation the statement that +// waits for the rendezvous takes the snapshot before the wait ends, so a +// materializer queued behind a rule commit would read the rules as they were +// before it. var Materializing = MaterializingWithin(rendezvousLockTimeout, 0) // MaterializingWithin is Materializing with its own lock timeout and diff --git a/internal/integrations/discourse/workflows/activities.go b/internal/integrations/discourse/workflows/activities.go index 0606307d..955a4571 100644 --- a/internal/integrations/discourse/workflows/activities.go +++ b/internal/integrations/discourse/workflows/activities.go @@ -21,6 +21,7 @@ import ( "fmt" "log/slog" + "git.coopcloud.tech/wiki-cafe/member-console/internal/db" "git.coopcloud.tech/wiki-cafe/member-console/internal/integrations/discourse/client" "git.coopcloud.tech/wiki-cafe/member-console/internal/integrations/discourse/linkage" dcmod "git.coopcloud.tech/wiki-cafe/member-console/internal/integrations/discourse/store" @@ -59,17 +60,14 @@ func NewActivities(cfg ActivitiesConfig) *Activities { return &Activities{cfg: cfg} } -// inTx runs fn inside one transaction against the activity's DB. +// inTx runs fn inside one transaction against the activity's DB, opened +// through the runner; fn's error, a failed begin and a failed commit come +// back as the runner returns them. The transaction spans whatever Discourse +// calls fn makes. func (a *Activities) inTx(ctx context.Context, fn func(q *dcmod.Queries) error) error { - tx, err := a.cfg.Database.BeginTx(ctx, nil) - if err != nil { - return fmt.Errorf("begin tx: %w", err) - } - defer tx.Rollback() - if err := fn(dcmod.New(tx)); err != nil { - return err - } - return tx.Commit() + return db.InTx(ctx, a.cfg.Database, db.Plain, func(tx *sql.Tx) error { + return fn(dcmod.New(tx)) + }) } // VerifyAPIKeyActivity fails the sweep up front when the admin API key is diff --git a/internal/integrations/stripe/catalogsync/catalogsync.go b/internal/integrations/stripe/catalogsync/catalogsync.go index 4ae673c1..5720c8ac 100644 --- a/internal/integrations/stripe/catalogsync/catalogsync.go +++ b/internal/integrations/stripe/catalogsync/catalogsync.go @@ -180,7 +180,7 @@ func noEnd() {} // back itself, so a no-op or a failure keeps the lock until the caller // calls end (Sync). func enqueue(ctx context.Context, d DB, productID string, product billing.Product, price billing.Price, keyMode string) (Outcome, func(), error) { - tx, err := d.BeginTx(ctx, nil) + tx, err := db.Begin(ctx, d, db.Plain) if err != nil { return 0, noEnd, &Error{Stage: StageBegin, Err: err} } diff --git a/internal/integrations/stripe/workflows/invoice_reconcile.go b/internal/integrations/stripe/workflows/invoice_reconcile.go index 17a41a0e..32071b97 100644 --- a/internal/integrations/stripe/workflows/invoice_reconcile.go +++ b/internal/integrations/stripe/workflows/invoice_reconcile.go @@ -38,6 +38,7 @@ import ( "go.temporal.io/sdk/temporal" "git.coopcloud.tech/wiki-cafe/member-console/internal/billing" + "git.coopcloud.tech/wiki-cafe/member-console/internal/db" "git.coopcloud.tech/wiki-cafe/member-console/internal/fulfillment" internalstripe "git.coopcloud.tech/wiki-cafe/member-console/internal/integrations/stripe/store" ) @@ -408,7 +409,7 @@ func isIntegrityViolation(pgErr *pgconn.PgError) bool { // converge writes the invoice, its lines and its payments in one // transaction under the per-invoice lock (design D3). func (r *invoiceReconcile) converge(ctx context.Context, customerID string) (InvoiceOutcome, error) { - tx, err := r.db.BeginTx(ctx, nil) + tx, err := db.Begin(ctx, r.db, db.Plain) if err != nil { return "", fmt.Errorf("stripe invoice %s (%s): begin tx: %w", r.id, r.trig.Ref, err) } diff --git a/internal/server/operator_topology.go b/internal/server/operator_topology.go index 88c19f47..06812260 100644 --- a/internal/server/operator_topology.go +++ b/internal/server/operator_topology.go @@ -5,6 +5,7 @@ package server import ( "context" + "database/sql" "log/slog" "net/http" "sort" @@ -352,7 +353,11 @@ func (h *OperatorPartialsHandler) orgTypeProvisioning(ctx context.Context) []Org // contiguous sort_order by position (0..N-1) — so the result is deterministic // even when ladders previously shared the default sort_order = 0 — validating // every ID against the live ladder set and writing the changed rows in one -// transaction. Submitted IDs that don't exist are ignored. +// transaction. Submitted IDs that don't exist are ignored; every ladder row is +// locked in ascending id order before anything is read (LockPlanLadderOrder). +// It runs at READ COMMITTED whatever the server's default: the lock read that +// waited returns the order the earlier reorder committed, where REPEATABLE +// READ would fail it with a serialization error. func (h *OperatorPartialsHandler) ReorderPlanLadders(w http.ResponseWriter, r *http.Request) { if err := r.ParseForm(); err != nil { h.renderPlanTopologyBody(w, r, "", "Invalid reorder request.") @@ -367,19 +372,8 @@ func (h *OperatorPartialsHandler) ReorderPlanLadders(w http.ResponseWriter, r *h } ctx := r.Context() - existing, err := h.BillingQ.ListPlanLadders(ctx) - if err != nil { - h.Logger.Error("failed to list plan ladders for reorder", slog.Any("error", err)) - h.renderPlanTopologyBody(w, r, "", "Failed to reorder ladders.") - return - } - // Map each known ladder to its current sort_order so we can skip no-op writes. - current := make(map[string]int32, len(existing)) - for _, l := range existing { - current[l.PlanLadderID] = l.SortOrder - } - tx, err := h.Database.BeginTx(ctx, nil) + tx, err := h.Database.BeginTx(ctx, &sql.TxOptions{Isolation: sql.LevelReadCommitted}) if err != nil { h.Logger.Error("failed to begin reorder tx", slog.Any("error", err)) h.renderPlanTopologyBody(w, r, "", "Failed to reorder ladders.") @@ -388,6 +382,22 @@ func (h *OperatorPartialsHandler) ReorderPlanLadders(w http.ResponseWriter, r *h defer tx.Rollback() qtx := billing.New(tx) + // Every ladder row, locked in ascending id order before anything is read + // or written, so two reorders wait for each other instead of deadlocking + // and the later one compares against the order the earlier committed + // (LockPlanLadderOrder). + locked, err := qtx.LockPlanLadderOrder(ctx) + if err != nil { + h.Logger.Error("failed to lock plan ladders for reorder", slog.Any("error", err)) + h.renderPlanTopologyBody(w, r, "", "Failed to reorder ladders.") + return + } + // Map each known ladder to its current sort_order so we can skip no-op writes. + current := make(map[string]int32, len(locked)) + for _, l := range locked { + current[l.PlanLadderID] = l.SortOrder + } + // Assign a contiguous sort_order by submitted position, skipping unknown IDs // (defensive against a stale page) and rows already at the right value. pos := int32(0) diff --git a/internal/server/operator_topology_reorder_db_test.go b/internal/server/operator_topology_reorder_db_test.go new file mode 100644 index 00000000..59f59bcd --- /dev/null +++ b/internal/server/operator_topology_reorder_db_test.go @@ -0,0 +1,276 @@ +// SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Commercial +// SPDX-FileCopyrightText: 2025-2026 Christian Galo + +package server + +import ( + "database/sql" + "errors" + "io" + "log/slog" + "net/http" + "net/http/httptest" + "net/url" + "slices" + "strings" + "testing" + "time" + + "github.com/jackc/pgx/v5/pgconn" + + "git.coopcloud.tech/wiki-cafe/member-console/internal/billing" + "git.coopcloud.tech/wiki-cafe/member-console/internal/entitlements" + "git.coopcloud.tech/wiki-cafe/member-console/internal/organization" + "git.coopcloud.tech/wiki-cafe/member-console/internal/testkit/dbtest" + "git.coopcloud.tech/wiki-cafe/member-console/internal/testkit/world" +) + +// reorderWait bounds how long a test waits for a reorder it started in the +// background, well over any wait the test provokes. +const reorderWait = 10 * time.Second + +// reorderEnv is the topology reorder handler over the shared test database, +// with two committed ladders. The handler opens its own transaction on the +// database, so the ladders are committed; the tests compare only their own +// ladders' positions, since other tests leave ladders in the database. +type reorderEnv struct { + t *testing.T + w *world.World + h *OperatorPartialsHandler + low world.Ladder // the ladder with the smaller id of the two + high world.Ladder // the ladder with the larger id +} + +// newReorderEnv builds the handler as operator_plan_ladders_mapfirst_test.go +// does and creates two ladders, their ids sorted into low and high. +func newReorderEnv(t *testing.T) *reorderEnv { + t.Helper() + w := world.New(t, dbtest.Open(t)) + a := w.Ladder("Reorder A") + b := w.Ladder("Reorder B") + if a.ID > b.ID { + a, b = b, a + } + logger := slog.New(slog.NewTextHandler(io.Discard, nil)) + return &reorderEnv{ + t: t, + w: w, + h: &OperatorPartialsHandler{ + BillingQ: billing.New(w.DB), + EntitlementsQ: entitlements.New(w.DB), + OrgQ: organization.New(w.DB), + Database: w.DB, + Logger: logger, + Templates: NewSafeTemplates(OperatorSet(t), slog.Default()), + }, + low: a, + high: b, + } +} + +// reorder posts the ladder ids in order to the reorder handler and returns +// the answer it recorded. +func (e *reorderEnv) reorder(ids ...string) *httptest.ResponseRecorder { + form := url.Values{"ladder": ids} + req := httptest.NewRequest(http.MethodPost, "/partials/operator/plan-ladders/reorder", strings.NewReader(form.Encode())) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + rec := httptest.NewRecorder() + e.h.ReorderPlanLadders(rec, req) + return rec +} + +// reorderInBackground runs reorder on a goroutine and returns a channel that +// receives its answer. +func (e *reorderEnv) reorderInBackground(ids ...string) <-chan *httptest.ResponseRecorder { + done := make(chan *httptest.ResponseRecorder, 1) + go func() { done <- e.reorder(ids...) }() + return done +} + +// finished waits for a reorder started in the background and returns its +// answer. +func (e *reorderEnv) finished(done <-chan *httptest.ResponseRecorder) *httptest.ResponseRecorder { + e.t.Helper() + select { + case rec := <-done: + return rec + case <-time.After(reorderWait): + e.t.Fatalf("the reorder did not finish within %v", reorderWait) + return nil + } +} + +// positions reads each ladder's sort_order, in the order of ids. +func (e *reorderEnv) positions(ids ...string) []int32 { + e.t.Helper() + out := make([]int32, len(ids)) + for i, id := range ids { + if err := e.w.DB.QueryRowContext(e.w.Ctx, + `SELECT sort_order FROM core.plan_ladders WHERE plan_ladder_id = $1`, id).Scan(&out[i]); err != nil { + e.t.Fatalf("read the position of ladder %s: %v", id, err) + } + } + return out +} + +// side opens a transaction on the database with BeginTx (tests are outside the +// runner rule), returns it with its backend pid, and rolls it back when the +// test ends. +func (e *reorderEnv) side() (*sql.Tx, int) { + e.t.Helper() + tx, err := e.w.DB.BeginTx(e.w.Ctx, nil) + if err != nil { + e.t.Fatalf("open a side transaction: %v", err) + } + e.t.Cleanup(func() { tx.Rollback() }) + var pid int + if err := tx.QueryRowContext(e.w.Ctx, `SELECT pg_backend_pid()`).Scan(&pid); err != nil { + e.t.Fatalf("read the side transaction's backend: %v", err) + } + return tx, pid +} + +// lockLadder takes the row lock clause (for example FOR NO KEY UPDATE or +// FOR KEY SHARE NOWAIT) on one ladder in tx and returns the error. +func (e *reorderEnv) lockLadder(tx *sql.Tx, id, clause string) error { + _, err := tx.ExecContext(e.w.Ctx, `SELECT 1 FROM core.plan_ladders WHERE plan_ladder_id = $1 `+clause, id) + return err +} + +// lockUnavailable reports whether err is SQLSTATE 55P03, the answer of a +// NOWAIT lock that another transaction holds. +func lockUnavailable(err error) bool { + var pgErr *pgconn.PgError + return errors.As(err, &pgErr) && pgErr.Code == "55P03" +} + +// toast is the recorder's HX-Trigger header. +func toast(rec *httptest.ResponseRecorder) string { + return rec.Header().Get("HX-Trigger") +} + +// TestReordersTakeLaddersInAscendingIdOrder holds the lower ladder in a side +// transaction, starts a reorder that submits the ladders high first, and asks +// for the higher ladder NOWAIT once the reorder waits: under the right order +// the reorder holds nothing of the higher ladder yet, so the probe succeeds. +func TestReordersTakeLaddersInAscendingIdOrder(t *testing.T) { + e := newReorderEnv(t) + e.w.SortOrder(e.low, 0) + e.w.SortOrder(e.high, 1) + side, sidePID := e.side() + if err := e.lockLadder(side, e.low.ID, "FOR NO KEY UPDATE"); err != nil { + t.Fatalf("the side transaction could not lock ladder %s: %v", e.low.ID, err) + } + done := e.reorderInBackground(e.high.ID, e.low.ID) + dbtest.AwaitLockWait(t, e.w.DB, dbtest.Backend{BlockedBy: sidePID, Abort: func() error { + if len(done) > 0 { + return errors.New("the reorder answered without waiting for the other transaction") + } + return nil + }}) + switch err := e.lockLadder(side, e.high.ID, "FOR NO KEY UPDATE NOWAIT"); { + case lockUnavailable(err): + t.Errorf("the reorder held ladder %s while it waited for ladder %s: it did not take the ladders in ascending id order", e.high.ID, e.low.ID) + case err != nil: + t.Errorf("the probe of ladder %s failed: %v", e.high.ID, err) + } + side.Rollback() + rec := e.finished(done) + if !strings.Contains(toast(rec), "Ladder order updated.") { + t.Errorf("the reorder answered %q, want the toast \"Ladder order updated.\"", toast(rec)) + } + if got, want := e.positions(e.high.ID, e.low.ID), []int32{0, 1}; !slices.Equal(got, want) { + t.Errorf("after the reorder, the ladders read %v, want %v", got, want) + } +} + +// TestAReorderAfterAnotherWritesItsWholeOrder plays an earlier reorder in a +// side transaction that holds two ladders, starts a reorder that waits for it, +// and commits the side: the reorder then compares against the order the side +// committed and writes the whole order it was given. It runs at the server's +// default isolation and at a REPEATABLE READ default, where the reorder's own +// READ COMMITTED keeps the lock read from failing with a serialization error. +func TestAReorderAfterAnotherWritesItsWholeOrder(t *testing.T) { + aReorderAfterAnotherWritesItsWholeOrder(t, newReorderEnv(t), "at the server's default") + e := newReorderEnv(t) + e.h.Database = openRepeatableReadDefault(t) + aReorderAfterAnotherWritesItsWholeOrder(t, e, "at a repeatable-read default") +} + +// aReorderAfterAnotherWritesItsWholeOrder is the body of +// TestAReorderAfterAnotherWritesItsWholeOrder on e; where names the isolation +// default in each failure. +func aReorderAfterAnotherWritesItsWholeOrder(t *testing.T, e *reorderEnv, where string) { + t.Helper() + c := e.w.Ladder("Reorder C") + ladders := []world.Ladder{e.low, e.high, c} + slices.SortFunc(ladders, func(a, b world.Ladder) int { return strings.Compare(a.ID, b.ID) }) + l1, l2, l3 := ladders[0], ladders[1], ladders[2] + e.w.SortOrder(l1, 0) + e.w.SortOrder(l2, 1) + e.w.SortOrder(l3, 2) + side, sidePID := e.side() + sideQ := billing.New(side) + for id, pos := range map[string]int32{l1.ID: 2, l3.ID: 0} { + if err := sideQ.SetPlanLadderSortOrder(e.w.Ctx, billing.SetPlanLadderSortOrderParams{PlanLadderID: id, SortOrder: pos}); err != nil { + t.Fatalf("the side transaction could not place ladder %s: %v", id, err) + } + } + done := e.reorderInBackground(l1.ID, l2.ID, l3.ID) + dbtest.AwaitLockWait(t, e.w.DB, dbtest.Backend{BlockedBy: sidePID, Abort: func() error { + if len(done) > 0 { + return errors.New("the reorder answered without waiting for the other transaction") + } + return nil + }}) + if err := side.Commit(); err != nil { + t.Fatalf("commit the side transaction: %v", err) + } + rec := e.finished(done) + if !strings.Contains(toast(rec), "Ladder order updated.") { + t.Errorf("%s, the reorder answered %q, want the toast \"Ladder order updated.\"", where, toast(rec)) + } + if got, want := e.positions(l1.ID, l2.ID, l3.ID), []int32{0, 1, 2}; !slices.Equal(got, want) { + t.Errorf("%s, after waiting on another reorder, the ladders read %v, want the submitted order %v", where, got, want) + } +} + +// openRepeatableReadDefault opens the test database with REPEATABLE READ as +// its default isolation, as a server can set it. +func openRepeatableReadDefault(t *testing.T) *sql.DB { + t.Helper() + dsn := dbtest.DSN(t) + sep := "?" + if strings.Contains(dsn, "?") { + sep = "&" + } + database := dbtest.Connect(t, dsn+sep+"default_transaction_isolation=repeatable%20read") + if got := dbtest.On(t, database).String(`SHOW default_transaction_isolation`); got != "repeatable read" { + t.Fatalf("default_transaction_isolation = %q, want %q", got, "repeatable read") + } + return database +} + +// TestAReorderLeavesKeyShareFree holds the reorder's lock in a side +// transaction and asks for FOR KEY SHARE NOWAIT on a ladder from a second +// one, the lock a foreign key's check takes: FOR NO KEY UPDATE does not +// conflict with it. +func TestAReorderLeavesKeyShareFree(t *testing.T) { + e := newReorderEnv(t) + side, _ := e.side() + if _, err := billing.New(side).LockPlanLadderOrder(e.w.Ctx); err != nil { + t.Fatalf("the side transaction could not lock the ladders: %v", err) + } + other, _ := e.side() + switch err := e.lockLadder(other, e.low.ID, "FOR KEY SHARE NOWAIT"); { + case lockUnavailable(err): + t.Errorf("a key-share lock on ladder %s waited behind the reorder's lock: %v", e.low.ID, err) + case err != nil: + t.Errorf("the key-share probe of ladder %s failed: %v", e.low.ID, err) + } + other.Rollback() + third, _ := e.side() + if err := e.lockLadder(third, e.low.ID, "FOR NO KEY UPDATE NOWAIT"); !lockUnavailable(err) { + t.Errorf("ladder %s was free of the reorder's lock, so the key-share probe proved nothing: %v", e.low.ID, err) + } +} diff --git a/internal/systemtenant/systemtenant.go b/internal/systemtenant/systemtenant.go index 0d0d9af8..a777cce4 100644 --- a/internal/systemtenant/systemtenant.go +++ b/internal/systemtenant/systemtenant.go @@ -18,6 +18,7 @@ import ( "errors" "fmt" + "git.coopcloud.tech/wiki-cafe/member-console/internal/db" "git.coopcloud.tech/wiki-cafe/member-console/internal/identity" "git.coopcloud.tech/wiki-cafe/member-console/internal/organization" ) @@ -50,14 +51,26 @@ const ( // FedWiki sync schedule is wired. It is safe to call on every boot and under // concurrent boots (each insert is ON CONFLICT DO NOTHING). Any returned error // should be treated by the caller as fatal: the sync's landing workspace must -// exist. -func Ensure(ctx context.Context, db *sql.DB) (workspaceID string, err error) { - tx, err := db.BeginTx(ctx, nil) +// exist. It opens its transaction through the runner; a failed begin or commit +// comes back as the runner returns it. +func Ensure(ctx context.Context, database *sql.DB) (string, error) { + var workspaceID string + err := db.InTx(ctx, database, db.Plain, func(tx *sql.Tx) error { + var err error + workspaceID, err = ensure(ctx, tx) + return err + }) if err != nil { - return "", fmt.Errorf("system tenant: begin tx: %w", err) + return "", err } - defer tx.Rollback() + return workspaceID, nil +} +// ensure is Ensure's steps on its transaction: it adopts the System +// organization's workspace when there is one, and otherwise creates the +// chain user, person, org type, organization, workspace and resolves the +// workspace it made. +func ensure(ctx context.Context, tx *sql.Tx) (string, error) { idQ := identity.New(tx) orgQ := organization.New(tx) @@ -66,13 +79,11 @@ func Ensure(ctx context.Context, db *sql.DB) (workspaceID string, err error) { // differently-named workspace (e.g. one left by an ad-hoc seed) without // creating a second one. if ws, gErr := orgQ.GetSystemOrgWorkspace(ctx); gErr == nil { - if err = tx.Commit(); err != nil { - return "", fmt.Errorf("system tenant: commit (adopt): %w", err) - } return ws.WorkspaceID, nil } else if !errors.Is(gErr, sql.ErrNoRows) { return "", fmt.Errorf("system tenant: resolve workspace: %w", gErr) } + var err error // Create the chain: user -> person -> org_type -> org -> workspace. if err = idQ.EnsureSystemUser(ctx, userOIDCSubject); err != nil { @@ -132,8 +143,5 @@ func Ensure(ctx context.Context, db *sql.DB) (workspaceID string, err error) { return "", fmt.Errorf("system tenant: resolve workspace after create: %w", err) } - if err = tx.Commit(); err != nil { - return "", fmt.Errorf("system tenant: commit: %w", err) - } return ws.WorkspaceID, nil } diff --git a/openspec/changes/transactions-open-through-the-runner/.openspec.yaml b/openspec/changes/transactions-open-through-the-runner/.openspec.yaml new file mode 100644 index 00000000..4bfa6709 --- /dev/null +++ b/openspec/changes/transactions-open-through-the-runner/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-10-30 diff --git a/openspec/changes/transactions-open-through-the-runner/design.md b/openspec/changes/transactions-open-through-the-runner/design.md new file mode 100644 index 00000000..fcc816cc --- /dev/null +++ b/openspec/changes/transactions-open-through-the-runner/design.md @@ -0,0 +1,139 @@ +## Context + +`internal/db/tx.go` holds the transaction runner. `db.InTx(ctx, b, kind, fn)` opens a transaction of `kind` on `b` (a `*sql.DB`, or a `*sql.Conn` a test pins), commits when `fn` returns nil, rolls back on an error or a panic and re-raises the panic; `fn`'s error, a failed begin and a failed commit come back unwrapped. `db.Begin(ctx, b, kind)` opens one and hands it back, for a caller that keeps it open across several steps. A kind is the options and the first statements: `db.Plain` (the server's default isolation, nothing first), `db.ReadCommitted`, `db.ReadOnlyRepeatableRead`, `db.FirstRuns(first)`, and the two rendezvous kinds of `internal/entitlements/tx.go`. + +#186's last comment counts eight calls of `BeginTx` outside tests, the test kit and `internal/db`. At HEAD: + +| Call | Kind today | Locks taken inside | What the transaction does | +|---|---|---|---| +| `domains.Registry.WithLock` (`internal/domains/registry.go:472`) | plain | the registry advisory lock first, then claim and placement rows; FedWiki's creators read one site `FOR SHARE` | every mutating allocation of a name | +| `(*Activities).inTx` (`internal/integrations/discourse/workflows/activities.go:64`) | plain | none of its own | the Discourse group sweep's writes; holds the transaction across Discourse HTTP calls | +| `catalogsync.enqueue` (`internal/integrations/stripe/catalogsync/catalogsync.go:183`) | plain | the product's advisory lock first | the pending mappings and the outbox inserts; a no-op or a failure returns with the transaction open until `Sync` calls `end` | +| `invoiceReconcile.converge` (`internal/integrations/stripe/workflows/invoice_reconcile.go:411`) | plain | the invoice's advisory lock first | the invoice, its lines and its payments; the commit error is classified for Temporal's retry (`r.dbErr("commit", err)`) | +| `systemtenant.Ensure` (`internal/systemtenant/systemtenant.go:55`) | plain | none | the System tenant's rows at boot, committed at one of two points | +| `MakeDefaultPrice` (`internal/server/operator_billing.go:1382`) | plain | the price rows its two updates touch | the product's default-price marker | +| `ReorderPlanLadders` (`internal/server/operator_topology.go:376`) | plain | each changed ladder row, in submitted order | the ladders' `sort_order` | +| `CreateWorkspace` (`internal/server/workspace_partials.go:121`) | plain | the foreign-key locks of its inserts | the workspace and its primary pool assignment | + +`docs/database-locks.md` describes the registry, the product sync and the invoice reconcile; it does not list the ladder reorder. + +## Goals / Non-Goals + +**Goals:** +- Every transaction outside `internal/db`, tests and the test kit opens through the runner, and the lint refuses a new one that does not. +- The ladder reorder takes its rows in one order and writes against the order it locked. + +**Non-Goals:** +- Advisory keys, the lock helpers, the order written once, the lint for advisory and pool row locks: the second change of #186. +- Moving a handler's transaction into the package that owns its tables (#221). +- Tests and the test kit. + +## Decisions + +### D1. What each transaction opens with + +| Call | After | Behaviour change | +|---|---|---| +| `Registry.WithLock` | `db.InTx(ctx, r.db, registryKind, ...)`, where `registryKind` is `db.FirstRuns` of the registry's advisory lock | A failed begin or commit comes back as the runner returns it, without `domains: begin tx:` or `domains: commit:`; a failed lock still reads `domains: registry advisory lock: ...`, because `db.Begin` returns a first statement's error as it is. A panic in `fn` rolls back before it rises instead of during the unwinding. `fn`'s error comes back as it did. | +| Discourse `inTx` | `db.InTx(ctx, a.cfg.Database, db.Plain, ...)` | A failed begin comes back without `begin tx:`. The transaction still spans the Discourse calls `fn` makes. | +| `catalogsync.enqueue` | `db.Begin(ctx, d, db.Plain)` | None: the begin error is still `&Error{Stage: StageBegin}`, the commit and the open transaction on a no-op are the function's own. | +| `invoiceReconcile.converge` | `db.Begin(ctx, r.db, db.Plain)` | None: the commit is still classified through `r.dbErr("commit", err)`. | +| `systemtenant.Ensure` | `db.InTx(ctx, database, db.Plain, ...)` around the unchanged steps, now in `ensure(ctx, tx)` | A failed begin or commit comes back as the runner returns it; its caller, `cmd/bootstate.go`, wraps it as `ensure system tenant: ...`. The two commit labels (`commit (adopt)`, `commit`) collapse into the runner's one commit. The adoption path now commits through the runner, as it committed before. | +| `MakeDefaultPrice`, `ReorderPlanLadders`, `CreateWorkspace` | unchanged, except the reorder's begin, which names READ COMMITTED (D4); pinned for #221 (D2) | none here, apart from the reorder's lock and its isolation (D4) | + +`db.Begin` is for the two whose commit is their own: `enqueue` returns with its transaction open on a no-op, and `converge` classifies its commit error. `db.InTx` is for the three whose commit has no meaning of its own. No kind changes: each opened with the server's default options and still does, so the registry, the Discourse sweep, the System tenant and the two Stripe paths keep their isolation and set no lock timeout. + +The registry's lock becomes the first statement of its kind rather than the first statement of `fn`, as the rendezvous is of its kinds: a transaction opened with `registryKind` cannot run a statement before the lock. + +### D2. The three handlers are pinned for #221 + +#186's second comment gave these three entries to #221, which moves each act into the package that owns its tables and opens it through the runner there; converting them in place would write code #221 replaces. Each keeps its line as a forbidigo exception in `.golangci.yml`, under a `# #221` comment, matching the exact source line, the form `go-static-analysis` requires of held-back findings: a changed line no longer matches, so the lint fails until the pin is updated or the transaction moves. + +### D3. The lint rule + +Two forbidigo patterns beside the three in `.golangci.yml`, with the configuration's `analyze-types: true` and one message: + +```yaml + - pattern: '\.BeginTx$' + msg: open the transaction through db.InTx or db.Begin with a kind (docs/go-conventions.md) + - pattern: '^sql\.DB\.Begin$' + pkg: '^database/sql$' + msg: open the transaction through db.InTx or db.Begin with a kind (docs/go-conventions.md) +``` + +With types analysed, forbidigo matches the method's name qualified by its receiver's package and type (`sql.DB.BeginTx`, `sql.Conn.BeginTx`, `db.Beginner.BeginTx`), so the first pattern ends at `.BeginTx` to catch a call through any of them, an interface that embeds `db.Beginner` included. `(*sql.DB).Begin` takes no context and has no `BeginTx` in its name; the second pattern catches it, so a one-word rewrite of `BeginTx` does not pass. + +Exclusions, by path: `^internal/db/` (the runner itself calls `b.BeginTx`), and `_test\.go$` and `^internal/testkit/` (a test opens a raw transaction to hold a lock while the code under test waits, or to roll back its fixture; a test's transaction never materializes, and converting 107 calls in 53 files, many of which name their database `db` and so shadow the package, buys nothing). Each exclusion matches the rule's message, so it hides no other forbidigo finding. + +`go-static-analysis` says test files are checked "with no exclusion for `_test.go`". The test exclusion here is one rule's, not a linter's, so the requirement is modified to allow exactly that: a forbid rule whose own requirement states that tests are outside it, with the exclusion matching that rule's message alone. Every linter still runs on tests at the same limits. + +The rule is proven by four command breaks (tasks 4.6): a `BeginTx` added in `catalogsync.enqueue` and a `Begin` on a `*sql.DB` in `systemtenant.Ensure` each fail `./scripts/golangci-lint.sh` with the message, a pinned line whose text changes fails it too, and a `FormValue` call in a test that the test exclusion could hide fails it with its own message. + +### D4. The ladder reorder's order + +`ReorderPlanLadders` today reads every ladder's `sort_order` through `h.BillingQ` before its transaction, then, for each submitted id in submitted order, updates the ladders whose position changed. An `UPDATE` of `sort_order`, a column no key covers, takes the row `FOR NO KEY UPDATE`. Two reorders that change the same two ladders in opposite orders each hold one row and wait for the other's. + +After: the first statement inside the transaction is a new query, + +```sql +-- name: LockPlanLadderOrder :many +SELECT plan_ladder_id, sort_order +FROM core.plan_ladders +ORDER BY plan_ladder_id +FOR NO KEY UPDATE; +``` + +and the reorder builds its map of current positions from those rows instead of the read before the transaction. Postgres locks the rows of a locking `SELECT ... ORDER BY` in the order it returns them, so every reorder takes every ladder row in ascending id order before it writes. The second of two reorders waits on the first row the first holds; when the first commits, the second's read returns the committed order, and it writes every ladder whose position differs from that, so its whole submitted order stands. + +Why `FOR NO KEY UPDATE`: it is the lock the `UPDATE` takes anyway. `FOR UPDATE` would also conflict with `FOR KEY SHARE`, the lock a foreign key's check takes on the referenced row; `core.plan_ladder_tiers`, `core.pool_provision_ladders`, `core.pool_provision_transitions` and `core.org_types` all reference `core.plan_ladders`, so a conferral that inserts a position on a ladder would wait on a reorder, under the rendezvous's five-second lock timeout. + +Why every row and not the submitted ones: the positions are a total order over all ladders, a reorder renumbers every submitted ladder from 0, and the query takes no parameter, so a submitted id that is not a UUID is still ignored, as today, instead of failing a cast. The table holds a handful of rows. + +Rejected: sorting the writes by id. It closes the deadlock but keeps the read before the transaction, so the second of two reorders can still skip a write against a stale position. + +The reorder's transaction runs at READ COMMITTED whatever the server's default. Its guarantee is that a locking `SELECT` which waited returns the row version the earlier reorder committed, which READ COMMITTED does by taking a new snapshot per statement; at REPEATABLE READ the same statement fails with a serialization error (SQLSTATE 40001) and the handler answers "Failed to reorder ladders.". `docs/go-conventions.md` asks a transaction whose correctness needs each statement to take its own snapshot to open with `db.ReadCommitted`, and the product edit does the same (`tiers-need-an-entitlement-set`). The begin is `h.Database.BeginTx(ctx, &sql.TxOptions{Isolation: sql.LevelReadCommitted})`, `db.ReadCommitted`'s own options, so the line stays a #221 pin (D2) with its source text changed and the doc comment of `ReorderPlanLadders` states the isolation. The alternative, `db.Begin(ctx, h.Database, db.ReadCommitted)` with no pin, would reach D2, the three handlers' count and the pinned-line break, for no gain before #221 moves the transaction. + +### D5. The documents + +`docs/go-conventions.md`, "Transactions and pool locks": the sentence "the paths that still call `BeginTx` directly are listed in #186" becomes "golangci-lint refuses `BeginTx` and `(*sql.DB).Begin` outside `internal/db`, tests and the test kit; three handlers #221 moves out of `internal/server` are its pinned exceptions." `docs/database-locks.md` gains a row lock (the ladder rows), a path (the reorder) and one sentence in the Registry section on `registryKind`. The comment on `entitlements.Materializing` drops its request that a `BeginTx` stay greppable, since the lint now refuses it. `docs/testing.md` names the rule among forbidigo's. + +## Failure table + +| # | Path or state | Today | After | Test | +|---|---|---|---|---| +| F1 | A registry allocation's begin or commit fails | `domains: begin tx: ` or `domains: commit: ` | `` as the runner returns it; nothing written | none: log text no caller compares; `TestInTx` (`internal/db/tx_test.go`) holds the runner's propagation | +| F2 | A registry allocation's lock fails | `domains: registry advisory lock: ` after the begin | the same text, from the kind's first statement | none: the text is unchanged | +| F3 | A registry composite returns an error or panics | rolled back; the error as is, or the panic | the same; the rollback precedes the panic's rise | the registry's DB tests, unchanged | +| F4 | A registry composite succeeds | committed | committed | the registry's and FedWiki's DB tests, unchanged | +| F5 | A Discourse sweep step's begin fails | `begin tx: `; Temporal retries the activity | ``; Temporal retries | none: log text | +| F6 | A catalog sync's enqueue | as today | as today | `TestSyncProductToStripeAnswers` goldens, unchanged | +| F7 | An invoice reconcile's converge | as today | as today | the invoice goldens under `internal/integrations/stripe/workflows/testdata`, unchanged | +| F8 | The System tenant's boot step fails to begin or commit | `system tenant: begin tx: `, `system tenant: commit (adopt): ` or `system tenant: commit: `, wrapped by the caller | `` wrapped by `cmd/bootstate.go` as `ensure system tenant: `, and unwrapped by `internal/integrations/fedwiki/reconcile_domains.go:64`, its second caller, which returns the error as it comes (accepted as log text); the boot fails as before | none: log text | +| F9 | The System tenant exists or is created | committed at one of two points | committed by the runner | `internal/systemtenant`'s tests and every `world.New`, unchanged | +| F10 | Two reorders change two ladders in opposite orders at once | one ends in a deadlock (40P01): "Failed to reorder ladders." | the second waits for the first; both answer "Ladder order updated." and the later one's order stands | `TestReordersTakeLaddersInAscendingIdOrder` | +| F11 | A reorder commits while another waits, or reads just before it | the later one skips ladders whose stale position matched and can leave two at one position | the later one reads the committed order under its lock and writes its whole submitted order, at the server's default isolation and at a REPEATABLE READ default | `TestAReorderAfterAnotherWritesItsWholeOrder` | +| F12 | A conferral, a tier insert or an org type's default write references a ladder a reorder holds | no wait (the reorder held only changed rows, `FOR NO KEY UPDATE`) | no wait: `FOR NO KEY UPDATE` on every row does not conflict with `FOR KEY SHARE` | `TestAReorderLeavesKeyShareFree` | +| F13 | The reorder's lock read fails | (the list read before the transaction fails) "Failed to reorder ladders." | the lock read inside the transaction fails: the same copy, the transaction rolled back | none: the same copy as the read it replaces | +| F14 | A new `BeginTx` outside the runner | passes the lint | fails `make lint` | breaks 4.6 (`BeginTx` in `catalogsync.enqueue`, `Begin` on a `*sql.DB` in `systemtenant.Ensure`) | +| F15 | A pinned handler's line changes | (no pin) | fails `make lint` | break 4.6 (the topology pin) | + +## Locks and transactions + +The only lock this change adds is the reorder's: every `core.plan_ladders` row, `FOR NO KEY UPDATE`, ascending id, first in its transaction, which takes no other lock. Nothing else takes two ladder rows, so it waits in no cycle. The registry's lock is unchanged in key, mode and place, still its transaction's first statement. Every other converted transaction takes the locks it took, in the order it took them, at the isolation it ran at. + +## Tests + +`TestReordersTakeLaddersInAscendingIdOrder`, `TestAReorderAfterAnotherWritesItsWholeOrder` and `TestAReorderLeavesKeyShareFree` are database tests in `internal/server` that commit their ladders through `world` and drive the handler; each holds a side transaction open and waits for the reorder's block with `dbtest.AwaitLockWait`, whose `Abort` reports a reorder that answered without waiting. `TestAReorderAfterAnotherWritesItsWholeOrder` runs once on the default handle and once on a handle whose default isolation is REPEATABLE READ; `TestAReorderLeavesKeyShareFree` ends with a `NOWAIT` probe that must find the ladder locked, so it cannot pass when the reorder locks nothing. The lint rule is proven by command breaks. The five conversions keep every existing test's result; their behaviour changes are log text (F1, F5, F8). + +## Integration contract + +The integration contract (#171 to #174) moves the Discourse integration out of the console (#174). Both touch `internal/integrations/discourse/workflows/activities.go`: this change rewrites `inTx` to three lines on the runner, which #174 deletes with the package or keeps as it is. The registry stays in core (#174, "the domains registry staying in core"); `internal/domains/registry.go` changes only inside `WithLock`. + +## Risks / Trade-offs + +- **Calls forbidigo cannot name.** A call through an unnamed interface or a type parameter yields no match text, so neither pattern sees it. Such a call would pass until review; none exists at HEAD. +- **Every ladder row locked.** A reorder now waits for an operator's ladder edit in flight (its `UPDATE` takes the same lock on one row) and the edit waits for the reorder; both are brief and human-paced. + +## Open questions + +None. The handlers' transactions belong to #221, which already names them. diff --git a/openspec/changes/transactions-open-through-the-runner/proposal.md b/openspec/changes/transactions-open-through-the-runner/proposal.md new file mode 100644 index 00000000..8d87a58e --- /dev/null +++ b/openspec/changes/transactions-open-through-the-runner/proposal.md @@ -0,0 +1,45 @@ +## Why + +`docs/go-conventions.md` says new code opens a transaction through `db.InTx` or `db.Begin` (`internal/db/tx.go`) with a kind, and that the paths still calling `BeginTx` directly are listed in #186. Eight remain outside `internal/db`, tests and the test kit: `domains.Registry.WithLock`, the Discourse activities' `inTx`, the Stripe catalog sync's `enqueue`, the invoice reconcile's `converge`, `systemtenant.Ensure`, and the handlers `MakeDefaultPrice`, `ReorderPlanLadders` and `CreateWorkspace`. Nothing refuses a ninth. The only guard is a comment on `entitlements.Materializing` asking that a `BeginTx` beside a materialization stay greppable; a transaction opened by hand skips its kind, so a path that materializes without the rendezvous is found only when `core.assert_rendezvous` refuses it at run time. + +`ReorderPlanLadders` (`internal/server/operator_topology.go`) writes each ladder's `sort_order` in the order the drag submitted. Two reorders at the same moment, one dragging A before B and the other B before A, each lock the row the other then waits for, and Postgres ends one with a deadlock (SQLSTATE 40P01), answered "Failed to reorder ladders.". The handler also reads the current order before its transaction opens and skips every ladder already at its position, so a reorder that commits right after another can skip a write the other's commit made necessary and leave two ladders at one position. + +## What Changes + +- **Five transactions open through the runner.** `Registry.WithLock` opens with a kind of its own whose first statement is the registry's advisory lock; the Discourse activities' `inTx` and `systemtenant.Ensure` open with `db.InTx` and `db.Plain`; `catalogsync.enqueue` and `invoiceReconcile.converge` open with `db.Begin` and `db.Plain`, keeping their own commit and its labels. Each one's behaviour change is named in design.md D1; none changes what is written. +- **The three handlers stay as they are, pinned.** #221 moves the transactions of `MakeDefaultPrice`, `ReorderPlanLadders` and `CreateWorkspace` out of `internal/server` and opens the moved ones through the runner; each keeps its `BeginTx` line as a pinned exception (the reorder's names READ COMMITTED, below) in `.golangci.yml` whose comment names #221. +- **The ladder reorder takes its rows in one order.** Inside its transaction, before it reads or writes, the reorder locks every ladder row `FOR NO KEY UPDATE` in ascending id order through a new query, `LockPlanLadderOrder`, and reads the current order from what it locked. Two reorders wait for each other instead of deadlocking, and the later one writes its whole submitted order. The reorder opens at READ COMMITTED whatever the server's default, because at REPEATABLE READ the lock read that waited fails with a serialization error. The lock is the one the reorder's `UPDATE` takes anyway; it does not conflict with the key-share lock a tier, a position or an org type's default takes through its foreign key, so no conferral waits on a reorder. +- **golangci-lint refuses `BeginTx` and `(*sql.DB).Begin`** outside `internal/db`, tests and the test kit, through a forbidigo rule that names `db.InTx` and `db.Begin`. +- **Docs:** `docs/go-conventions.md` ("Transactions and pool locks") names the rule in place of the list in #186; `docs/database-locks.md` gains the ladder row lock and says how the registry's transaction opens; `docs/testing.md` lists the rule among forbidigo's. +- **Ride-along, listed under `CONTRIBUTING.md`, "How changes are made", item 1:** the modified topology requirement names the table `core.plan_ladders` in place of the stale `billing.plan_ladders`. + +## Out of scope + +- **The advisory keys, the lock helpers and their lint.** The second change of #186 builds every advisory key from a family and an id, takes every advisory lock through one helper, refuses advisory locks and pool row locks outside the helpers, and closes #186. This change touches the registry's lock statement only to move it into the registry's kind. +- **Moving the three handlers' transactions out of `internal/server`** (#221). +- **Tests and the test kit**, which open 107 transactions with `BeginTx` across 53 files to hold a lock while the code under test waits or to roll back a fixture. The rule does not reach them. + +## Capabilities + +### New Capabilities + +None. + +### Modified Capabilities + +- `go-static-analysis`: a new requirement, "Only the transaction runner opens a transaction"; "golangci-lint checks tests and production code under one configuration" allows a forbid rule's exclusion for tests when that rule's own requirement states it, matching that rule's message alone. +- `operator-topology-overview`: "Overview is read-only, role-gated, and drills into existing CRUD" requires two reorders at once to complete, the later one's order standing whole, and no conferral to wait on a reorder. + +## Impact + +- **Code:** `internal/domains/registry.go`, `internal/integrations/discourse/workflows/activities.go`, `internal/integrations/stripe/catalogsync/catalogsync.go`, `internal/integrations/stripe/workflows/invoice_reconcile.go`, `internal/systemtenant/systemtenant.go`, `internal/server/operator_topology.go`, `internal/billing/queries/plan_ladders.sql` and its generated code, `internal/entitlements/tx.go` (a comment), `.golangci.yml`. +- **Schema:** none. +- **Behaviour a deployment sees:** two operators reordering ladders at once both succeed; an error from a failed begin or commit in the registry, the Discourse activities and the System tenant's boot step reads without the label that named the step. +- **Tests:** new tests in `internal/server`; no golden, route table or existing test result changes. +- **Docs:** `docs/go-conventions.md`, `docs/database-locks.md`, `docs/testing.md`. + +## Related issues + +- **#186** lists the transactions still opened by hand and the reorder's inverted order. This change settles both and adds the rule; the second change of #186 closes it. +- **#221** moves the three handlers' transactions out of `internal/server`; their pins name it and go with it. +- **#174** moves the Discourse integration out of the console; `internal/integrations/discourse/workflows/activities.go` is a file both touch. diff --git a/openspec/changes/transactions-open-through-the-runner/specs/go-static-analysis/spec.md b/openspec/changes/transactions-open-through-the-runner/specs/go-static-analysis/spec.md new file mode 100644 index 00000000..bb2a8cbd --- /dev/null +++ b/openspec/changes/transactions-open-through-the-runner/specs/go-static-analysis/spec.md @@ -0,0 +1,56 @@ +## MODIFIED Requirements + +### Requirement: golangci-lint checks tests and production code under one configuration + +`.golangci.yml` SHALL enable golangci-lint's standard linters (errcheck, govet, ineffassign, staticcheck, unused) and unparam, dupl, gocognit, forbidigo, rowserrcheck, sqlclosecheck and errorlint (comparisons and type assertions), with gocognit's limit at 30 and dupl's threshold at 150. Test files SHALL be checked by the same linters at the same limits, with no exclusion for `_test.go`, except a forbid rule whose own requirement states that tests are outside it and why ("Only the transaction runner opens a transaction"); such an exclusion SHALL match that rule's message alone. errcheck SHALL exclude exactly three calls, by name: `(*database/sql.Tx).Rollback`, `(*database/sql.Rows).Close` and `(io.ReadCloser).Close`; golangci-lint's `std-error-handling` preset SHALL NOT be used, so `(*sql.DB).Close`, `Close` and `Flush` on anything written to, and writes to an `http.ResponseWriter` stay checked. golangci-lint SHALL report every finding on a line (`issues.uniq-by-line: false`), so a finding on a line that an exception covers for another linter still fails. `docs/go-conventions.md` SHALL state the error rules this configuration enforces and name `.golangci.yml` as their enforcement. + +#### Scenario: A deferred rollback needs no marker + +- **WHEN** a function writes `defer tx.Rollback()` after beginning a transaction +- **THEN** errcheck reports nothing for that line + +#### Scenario: A database close stays checked + +- **WHEN** a command writes `defer database.Close()` on a `*sql.DB` +- **THEN** errcheck reports the unchecked error + +#### Scenario: A test over the limit fails like production code + +- **WHEN** a test function's cognitive complexity rises above 30 and no exception names it +- **THEN** gocognit reports it and `make lint` fails + +#### Scenario: A second finding on an excepted line is reported + +- **WHEN** a function over the complexity limit is held back by an exception and unparam reports one of its parameters on the same line +- **THEN** the unparam finding is reported and `make lint` fails + +#### Scenario: A test's exclusion hides only its rule's finding + +- **WHEN** a test file opens a transaction with `BeginTx` and also calls `r.FormValue` +- **THEN** golangci-lint reports the `FormValue` call and not the `BeginTx` call + +## ADDED Requirements + +### Requirement: Only the transaction runner opens a transaction + +Outside `internal/db`, tests and the test kit (`internal/testkit`), no Go code SHALL call a method named `BeginTx`, whatever its receiver (a `*sql.DB`, a `*sql.Conn`, `db.Beginner` or an interface that carries it), or `(*sql.DB).Begin`. A transaction SHALL open through `db.InTx` or `db.Begin` with a kind, so that how it opens, and whether it takes the materialization rendezvous, is stated where it opens. golangci-lint's forbidigo SHALL refuse any other call with a message that names `db.InTx` and `db.Begin`. Tests and the test kit SHALL be outside the rule, because a test opens a raw transaction to hold a lock while the code under test waits, or to roll back its fixture, and such a transaction never materializes. A handler that still opens its own transaction SHALL be a pinned exception covering its one line, under a comment naming the issue that moves the transaction. + +#### Scenario: A new BeginTx outside the runner fails the lint + +- **WHEN** a file outside `internal/db`, tests and the test kit calls `BeginTx` on a database, a connection or an interface that carries the method +- **THEN** `make lint` fails, and the finding names `db.InTx` and `db.Begin` + +#### Scenario: A one-word rewrite does not pass + +- **WHEN** a file outside `internal/db`, tests and the test kit calls `Begin` on a `*sql.DB` +- **THEN** `make lint` fails, and the finding names `db.InTx` and `db.Begin` + +#### Scenario: A test opens a raw transaction + +- **WHEN** a test opens a transaction with `BeginTx` to hold a pool row while the code under test waits for it +- **THEN** golangci-lint reports nothing for that call + +#### Scenario: A pinned handler's line changes + +- **WHEN** the source line of a pinned handler's `BeginTx` changes its text +- **THEN** the pin matches nothing, the finding is reported, and `make lint` fails until the pin is edited or the transaction moves diff --git a/openspec/changes/transactions-open-through-the-runner/specs/operator-topology-overview/spec.md b/openspec/changes/transactions-open-through-the-runner/specs/operator-topology-overview/spec.md new file mode 100644 index 00000000..4ec1913b --- /dev/null +++ b/openspec/changes/transactions-open-through-the-runner/specs/operator-topology-overview/spec.md @@ -0,0 +1,64 @@ +## MODIFIED Requirements + +### Requirement: Overview is read-only, role-gated, and drills into existing CRUD + +The topology overview SHALL be read-only with respect to catalog content: it SHALL NOT present create, update, or delete affordances for ladders, tiers, or products, and each ladder column heading and tier cell SHALL link into the existing ladder-tier management and product surfaces for editing. The route SHALL be gated to the operator role. + +The one mutation permitted on this surface is **ladder display reordering**. Because the overview is the surface where ladder order is actually visible (columns are laid out left-to-right by `core.plan_ladders.sort_order`), each ladder column header SHALL be drag-and-drop reorderable by an explicit drag handle, so operators set order where it is visible. The pinned first column (rank labels) SHALL NOT be draggable and SHALL remain leftmost. On drop, the system SHALL persist the new left-to-right order by assigning each ladder a contiguous `sort_order` by position (`0..N-1`) — so the outcome is deterministic even when multiple ladders share the default `sort_order = 0` — and SHALL re-render the grid so the new order is immediately visible. The reorder route SHALL be gated to the operator role. The reorder affordance SHALL NOT rely on inline scripts or inline styles (the drag library is self-hosted and drag feedback is class-based, per the deployment's strict CSP). + +The reorder's transaction SHALL lock every ladder's row in ascending ladder id order before it reads the current positions or writes any, and SHALL read the positions it compares against from the rows it locked. Two reorders at the same moment SHALL therefore both complete, whatever orders they submit, and the one that commits later SHALL leave every ladder it submitted at the position it submitted. The lock SHALL be the one the reorder's update of `sort_order` takes anyway (`FOR NO KEY UPDATE`), which does not conflict with the key-share lock a foreign key takes on a referenced ladder, so no conferral, tier write or org type write waits on a reorder. The reorder's transaction SHALL run at READ COMMITTED whatever the server's default isolation, because at REPEATABLE READ the read that waited for another reorder would fail with a serialization error instead of returning the order that reorder committed. + +#### Scenario: Overview offers no content-mutation affordances + +- **WHEN** an operator views the topology overview +- **THEN** no create, rank-edit, update, or destructive (delete) affordances for ladders, tiers, or products are available on the surface itself +- **AND** the only mutation control present is the per-column drag-to-reorder handle on the ladder headers + +#### Scenario: Operator drags a ladder to a new position + +- **WHEN** an operator drags a ladder column header by its handle and drops it at a new position among the other ladder columns +- **THEN** the system SHALL persist the new left-to-right column order +- **AND** the overview SHALL re-render with the columns in the dropped order + +#### Scenario: Reorder is deterministic when ladders share a sort_order + +- **WHEN** the catalog has multiple ladders all at the default `sort_order = 0` and an operator reorders one of them by drag-and-drop +- **THEN** the system SHALL assign each ladder a contiguous `sort_order` by its new position (`0..N-1`) +- **AND** the resulting order SHALL reflect the drop rather than collapsing back to a name-only tiebreak + +#### Scenario: The rank column stays pinned + +- **WHEN** an operator drags a ladder column +- **THEN** the leftmost rank-label column SHALL NOT be draggable and SHALL remain the leftmost column + +#### Scenario: Cells drill into existing management screens + +- **WHEN** an operator activates a ladder column heading link or a tier cell +- **THEN** the operator is navigated to the existing ladder-tier management or product edit screen for that entity + +#### Scenario: Overview requires the operator role + +- **WHEN** a non-operator attempts to load `/operator/plan-topology` or to invoke the ladder-reorder route +- **THEN** access is denied by the operator-role guard + +#### Scenario: Two opposite reorders at once both complete + +- **WHEN** two operators drop ladders A and B in opposite orders at the same moment +- **THEN** the second reorder waits for the first instead of ending in a deadlock +- **AND** both answer "Ladder order updated." and the ladders stand in the order of the reorder that committed later + +#### Scenario: A reorder after another writes its whole order + +- **WHEN** a reorder commits a new position for ladder A while a second reorder, which submitted A at the position A held before, waits for it +- **THEN** the second reorder reads A's committed position under its lock and writes A back to the position it submitted + +#### Scenario: A reorder after another at a REPEATABLE READ default + +- **GIVEN** a server whose default isolation is REPEATABLE READ +- **WHEN** a reorder waits for another reorder that then commits +- **THEN** the second reorder answers "Ladder order updated." and writes its whole submitted order + +#### Scenario: A conferral does not wait on a reorder + +- **WHEN** a reorder holds the ladders' rows and a conferral inserts a position on one of those ladders +- **THEN** the conferral's foreign-key check takes its key-share lock without waiting diff --git a/openspec/changes/transactions-open-through-the-runner/tasks.md b/openspec/changes/transactions-open-through-the-runner/tasks.md new file mode 100644 index 00000000..960e51da --- /dev/null +++ b/openspec/changes/transactions-open-through-the-runner/tasks.md @@ -0,0 +1,244 @@ +Tests follow `docs/testing.md`: database tests open the database through `dbtest`, build their rows through the test kit (`world`) or the package's fixtures, filter to the rows their own fixture made, and pass twice in a row without a reset. A test that holds one transaction open while a second blocks waits for the block with `dbtest.AwaitLockWait`, never a sleep, and releases its side transaction in cleanup. The reorder tests commit their ladders, because the handler opens its own transaction on the database; they compare only their own ladders' positions, since other tests leave ladders in the shared database. + +Sections 1 to 4 are the parts of one commit; sections 5 and 6 are not commits. The commit passes `make test` with the test stack up, which runs `make lint` first and then every package one at a time (`-p 1`). No golden, route table or existing test result changes; any other change is a stop. `make sqlc-generate` changes `internal/billing/plan_ladders.sql.go` and `internal/billing/querier.go` by the one query of item 2.1 and nothing else; any other change is a stop. A script's usage is learned from its header or source, never by running it. + +**Breaks.** The list of item 4.6 is written to a file outside the repository and run with `go run ./scripts/mutation-check -list -side new` on the commit, from the repository root, with `test/.env` loaded as `make test` loads it (without it the database tests skip and every record reports SKIPPED), after its green control run; the tests are new, so there is no old side. A break of Go code is a test record (`name`, `file`, `old`, `new`, `pkg`, `run_new`, `want_new`); a break the lint sees is a command record (`name`, `file`, `old`, `new`, `cmd`, `exit`, `want_new`), per the header of `scripts/mutation-check/main.go`. Every record's `want_new` is a text the failing test or command prints only when it fails, never a test or subtest name alone. Every record must be CAUGHT. An `old` text below is the code as the section's items write it, with `\n` and `\t` standing for the line break and the tab gofmt writes; when the committed code words it differently, the record is corrected to the committed text before the run, never dropped, and its `new` text keeps the package compiling. A break that no test of the commit sees is named with the reason, and the commit message says so. The commit message lists every break with its result. + +## 1. The runner + +- [x] 1.1 `internal/domains/registry.go` imports `git.coopcloud.tech/wiki-cafe/member-console/internal/db` and gains, after `registryAllocationLock` (design D1): + + ```go + // registryKind opens a registry allocation's transaction: the registry's + // advisory lock is its first statement, so nothing in the transaction runs + // before the lock (docs/database-locks.md, Registry). + var registryKind = db.FirstRuns(func(ctx context.Context, tx *sql.Tx) error { + if _, err := tx.ExecContext(ctx, `SELECT pg_advisory_xact_lock($1)`, registryAllocationLock); err != nil { + return fmt.Errorf("domains: registry advisory lock: %w", err) + } + return nil + }) + ``` + + `WithLock` (`:465-487`) reads: + + ```go + // WithLock runs fn inside a transaction holding the registry advisory lock + // (released on commit or rollback), opened through the runner with + // registryKind. Returning an error rolls the whole composite back; returning + // nil commits. fn's error, a failed begin and a failed commit come back as + // the runner returns them. See registryAllocationLock for the lock-ordering + // invariant against entitlement usage rows. + func (r *Registry) WithLock(ctx context.Context, fn func(context.Context, *Tx) error) error { + if r.db == nil { + return ErrNoDatabase + } + return db.InTx(ctx, r.db, registryKind, func(tx *sql.Tx) error { + return fn(ctx, &Tx{q: New(tx), policy: r.Policy(), executor: tx}) + }) + } + ``` + +- [x] 1.2 `internal/integrations/discourse/workflows/activities.go` imports `internal/db`; `inTx` (`:62-73`) reads: + + ```go + // inTx runs fn inside one transaction against the activity's DB, opened + // through the runner; fn's error, a failed begin and a failed commit come + // back as the runner returns them. The transaction spans whatever Discourse + // calls fn makes. + func (a *Activities) inTx(ctx context.Context, fn func(q *dcmod.Queries) error) error { + return db.InTx(ctx, a.cfg.Database, db.Plain, func(tx *sql.Tx) error { + return fn(dcmod.New(tx)) + }) + } + ``` + +- [x] 1.3 `internal/integrations/stripe/catalogsync/catalogsync.go`, `enqueue` (`:183`): `tx, err := d.BeginTx(ctx, nil)` reads `tx, err := db.Begin(ctx, d, db.Plain)`. Nothing else in the function changes; `DB` already embeds `db.Beginner`. + +- [x] 1.4 `internal/integrations/stripe/workflows/invoice_reconcile.go` imports `internal/db`; `converge` (`:411`): `tx, err := r.db.BeginTx(ctx, nil)` reads `tx, err := db.Begin(ctx, r.db, db.Plain)`. The commit and its `r.dbErr("commit", err)` are unchanged. + +- [x] 1.5 `internal/systemtenant/systemtenant.go` imports `internal/db`. `Ensure` keeps its doc comment, which gains "It opens its transaction through the runner; a failed begin or commit comes back as the runner returns it.", and reads: + + ```go + func Ensure(ctx context.Context, database *sql.DB) (string, error) { + var workspaceID string + err := db.InTx(ctx, database, db.Plain, func(tx *sql.Tx) error { + var err error + workspaceID, err = ensure(ctx, tx) + return err + }) + if err != nil { + return "", err + } + return workspaceID, nil + } + + // ensure is Ensure's steps on its transaction: it adopts the System + // organization's workspace when there is one, and otherwise creates the + // chain user, person, org type, organization, workspace and resolves the + // workspace it made. + func ensure(ctx context.Context, tx *sql.Tx) (string, error) { + idQ := identity.New(tx) + orgQ := organization.New(tx) + + // (the fast-path comment, unchanged) + if ws, gErr := orgQ.GetSystemOrgWorkspace(ctx); gErr == nil { + return ws.WorkspaceID, nil + } else if !errors.Is(gErr, sql.ErrNoRows) { + return "", fmt.Errorf("system tenant: resolve workspace: %w", gErr) + } + var err error + // the steps from "Create the chain" to "resolve workspace after create", + // each statement, comment and label as today + return ws.WorkspaceID, nil + } + ``` + + The two commits and their labels (`system tenant: commit (adopt)`, `system tenant: commit`) and the begin's label go; every other label stays. + +- [x] 1.6 `internal/entitlements/tx.go`, the comment on `Materializing`: the sentence "A BeginTx in a file that also materializes, confers or reapplies defaults is the defect this kind exists to make greppable." reads "golangci-lint refuses BeginTx outside internal/db, so a transaction that materializes opens through this kind, or through a kind without the rendezvous, which core.assert_rendezvous then refuses." + +## 2. The ladder reorder + +- [x] 2.1 `internal/billing/queries/plan_ladders.sql`, after `SetPlanLadderSortOrder` (design D4): + + ```sql + -- name: LockPlanLadderOrder :many + -- Every ladder's display order, each row locked in ascending id order: the + -- topology reorder takes them first in its transaction, so two reorders wait + -- for each other instead of deadlocking, and the later one compares against + -- the order the earlier committed. FOR NO KEY UPDATE is the lock the + -- reorder's update of sort_order takes anyway; it does not conflict with the + -- FOR KEY SHARE a tier, a position or an org type's default takes on the + -- ladder through its foreign key, so no conferral waits on a reorder. + SELECT plan_ladder_id, sort_order + FROM core.plan_ladders + ORDER BY plan_ladder_id + FOR NO KEY UPDATE; + ``` + + `make sqlc-generate` writes `LockPlanLadderOrder` and `LockPlanLadderOrderRow` into `internal/billing`. + +- [x] 2.2 `internal/server/operator_topology.go`, `ReorderPlanLadders`: the read of `existing` through `h.BillingQ.ListPlanLadders` and the map built from it (`:370-380`) go. After `qtx := billing.New(tx)` (`:389`): + + ```go + // Every ladder row, locked in ascending id order before anything is read + // or written, so two reorders wait for each other instead of deadlocking + // and the later one compares against the order the earlier committed + // (LockPlanLadderOrder). + locked, err := qtx.LockPlanLadderOrder(ctx) + if err != nil { + h.Logger.Error("failed to lock plan ladders for reorder", slog.Any("error", err)) + h.renderPlanTopologyBody(w, r, "", "Failed to reorder ladders.") + return + } + // Map each known ladder to its current sort_order so we can skip no-op writes. + current := make(map[string]int32, len(locked)) + for _, l := range locked { + current[l.PlanLadderID] = l.SortOrder + } + ``` + + `ctx := r.Context()` stays before the `BeginTx` line, which reads `tx, err := h.Database.BeginTx(ctx, &sql.TxOptions{Isolation: sql.LevelReadCommitted})` (`db.ReadCommitted`'s options; the file imports `database/sql`), pinned by item 3.2. The doc comment's last sentence reads "Submitted IDs that don't exist are ignored; every ladder row is locked in ascending id order before anything is read (LockPlanLadderOrder)." and is followed by "It runs at READ COMMITTED whatever the server's default: the lock read that waited returns the order the earlier reorder committed, where REPEATABLE READ would fail it with a serialization error." + +## 3. The lint rule + +- [x] 3.1 `.golangci.yml`, `forbidigo.forbid`, after the `FormValue` pattern (design D3): + + ```yaml + - pattern: '\.BeginTx$' + msg: open the transaction through db.InTx or db.Begin with a kind (docs/go-conventions.md) + - pattern: '^sql\.DB\.Begin$' + pkg: '^database/sql$' + msg: open the transaction through db.InTx or db.Begin with a kind (docs/go-conventions.md) + ``` + + `exclusions.rules`, after the Stripe key's rule: + + ```yaml + # The runner opens every transaction (docs/go-conventions.md); tests and + # the test kit open raw ones to hold a lock or to roll back a fixture. + - path: '^internal/db/' + linters: [forbidigo] + text: 'open the transaction through db\.InTx or db\.Begin' + - path: '(_test\.go$|^internal/testkit/)' + linters: [forbidigo] + text: 'open the transaction through db\.InTx or db\.Begin' + ``` + +- [x] 3.2 The temporary rules, after the `# #205` group: + + ```yaml + # #221: each handler opens its own transaction until its act moves to the package that owns its tables. + - path: '^internal/server/operator_billing\.go$' + linters: [forbidigo] + text: 'open the transaction through db\.InTx or db\.Begin' + source: 'tx, err := h\.Database\.BeginTx\(ctx, nil\)' # #221 + - path: '^internal/server/operator_topology\.go$' + linters: [forbidigo] + text: 'open the transaction through db\.InTx or db\.Begin' + source: 'tx, err := h\.Database\.BeginTx\(ctx, &sql\.TxOptions\{Isolation: sql\.LevelReadCommitted\}\)' # #221 + - path: '^internal/server/workspace_partials\.go$' + linters: [forbidigo] + text: 'open the transaction through db\.InTx or db\.Begin' + source: 'tx, err := h\.Database\.BeginTx\(r\.Context\(\), nil\)' # #221 + ``` + + `./scripts/golangci-lint.sh` reports nothing on the commit; a finding at any other `BeginTx` is a call item 1 missed, and a stop. + +## 4. The tests and their breaks + +- [x] 4.1 `internal/domains/registry_db_test.go` gains `TestRegistryLockIsTheFirstStatement`: `counted, statements := dbtest.Counted(t)`; `NewRegistry(counted).WithLock(ctx, fn)` with an `fn` that calls `tx.PlacementByResource(ctx, "test", uuid.NewString())` and returns nil. `statements.Names()` begins `begin`, then `registryLockStatement`, a constant of the test file reading `SELECT pg_advisory_xact_lock($1)`; failure text `a registry transaction ran %v, want begin and then the registry lock`. + +- [x] 4.2 A new `internal/server/operator_topology_reorder_db_test.go`, package `server`, holds items 4.3 to 4.5 and their helpers: + - `newReorderEnv(t)`: `w := world.New(t, dbtest.Open(t))`; two ladders `w.Ladder("Reorder A")` and `w.Ladder("Reorder B")`, their ids sorted into `low` and `high`; the handler built as `operator_plan_ladders_mapfirst_test.go:50` builds its own (`BillingQ`, `EntitlementsQ`, `OrgQ` on `w.DB`, `Database: w.DB`, a discarding logger) with `Templates: NewSafeTemplates(OperatorSet(t), slog.Default())`. + - the two ladders stay `world.Ladder` values in `low` and `high`, sorted by `ID`; a ladder's `sort_order` is set with `w.SortOrder(l, pos)`, and the file has no `place` of its own. + - `reorder(ids ...string) *httptest.ResponseRecorder` builds `httptest.NewRequest(http.MethodPost, "/partials/operator/plan-ladders/reorder", body)` whose body posts `ladder=` for each id, in order, as `application/x-www-form-urlencoded`, and calls `h.ReorderPlanLadders` (the file imports `net/http`, which the break of item 4.6 also uses); `reorderInBackground` runs it on a goroutine and returns a channel of the recorder. + - `positions(ids ...string) []int32` reads each id's `sort_order` from `core.plan_ladders`. + - `side(t)` opens a transaction on `w.DB` with `BeginTx` (tests are outside the rule), reads its `pg_backend_pid()`, and rolls it back in cleanup. + - `toast(rec)` is the recorder's `HX-Trigger` header. + +- [x] 4.3 `TestReordersTakeLaddersInAscendingIdOrder` (F10): `w.SortOrder(low, 0)`, `w.SortOrder(high, 1)`. The side transaction locks `low` (`SELECT 1 FROM core.plan_ladders WHERE plan_ladder_id = $1 FOR NO KEY UPDATE`). `reorderInBackground(high, low)`; `dbtest.AwaitLockWait(t, w.DB, dbtest.Backend{BlockedBy: sidePID, Abort: ...})`, whose `Abort` returns `the reorder answered without waiting for the other transaction` once the reorder's channel holds its answer (`len(done) > 0`; the buffer of one keeps the answer). The side transaction then runs `SELECT 1 FROM core.plan_ladders WHERE plan_ladder_id = $1 FOR NO KEY UPDATE NOWAIT` on `high`; an error with SQLSTATE `55P03` fails with `the reorder held ladder %s while it waited for ladder %s: it did not take the ladders in ascending id order` (`t.Errorf`, `high`, `low`), and any other error with `the probe of ladder %s failed: %v`. The side transaction rolls back; the reorder's toast contains `Ladder order updated.` (failure text `the reorder answered %q, want the toast "Ladder order updated."`); `positions(high, low)` is `[0 1]` (failure text `after the reorder, the ladders read %v, want %v`). + +- [x] 4.4 `TestAReorderAfterAnotherWritesItsWholeOrder` (F11): a third ladder `w.Ladder("Reorder C")`; the three ids sorted into `l1 < l2 < l3`, placed 0, 1, 2. The side transaction plays an earlier reorder: it updates `l1` to 2 and `l3` to 0 and stays open. `reorderInBackground(l1, l2, l3)`; `dbtest.AwaitLockWait(t, w.DB, dbtest.Backend{BlockedBy: sidePID, Abort: ...})` with the `Abort` of 4.3; the side transaction commits. The reorder's toast contains `Ladder order updated.`; `positions(l1, l2, l3)` is `[0 1 2]`. The body runs twice, once on the default handle (`where` is `at the server's default`) and once with `e.h.Database` opened with `default_transaction_isolation=repeatable read` (`where` is `at a repeatable-read default`; the file carries its own copy of `openRepeatableRead`, which lives in package `server_test`); each failure text starts with `where`: `%s, the reorder answered %q, want the toast "Ladder order updated."` and `%s, after waiting on another reorder, the ladders read %v, want the submitted order %v`. + +- [x] 4.5 `TestAReorderLeavesKeyShareFree` (F12): the side transaction runs `billing.New(side).LockPlanLadderOrder(ctx)` and stays open. A second transaction on `w.DB` runs `SELECT 1 FROM core.plan_ladders WHERE plan_ladder_id = $1 FOR KEY SHARE NOWAIT` on `low`; an error with SQLSTATE `55P03` fails with `a key-share lock on ladder %s waited behind the reorder's lock: %v`, and any other error with `the key-share probe of ladder %s failed: %v`. It rolls back; a third transaction then runs `FOR NO KEY UPDATE NOWAIT` on `low`, and unless the error has SQLSTATE `55P03` the test fails with `ladder %s was free of the reorder's lock, so the key-share probe proved nothing: %v`, so the test cannot pass when the reorder locks nothing. + +- [x] 4.6 Breaks. Package `./internal/server` unless named; a test record's `run_new` is `^$`. + - the ladders locked in descending order (`internal/billing/plan_ladders.sql.go`, `old` `ORDER BY plan_ladder_id\nFOR NO KEY UPDATE`, `new` `ORDER BY plan_ladder_id DESC\nFOR NO KEY UPDATE`): caught by `TestReordersTakeLaddersInAscendingIdOrder`, `want_new` `the reorder held ladder`; + - the ladders locked `FOR UPDATE` (the same `old`, `new` `ORDER BY plan_ladder_id\nFOR UPDATE`): caught by `TestAReorderLeavesKeyShareFree`, `want_new` `a key-share lock on ladder`; + - no lock (the same `old`, `new` `ORDER BY plan_ladder_id`): caught by `TestAReorderAfterAnotherWritesItsWholeOrder`, `want_new` `the reorder answered without waiting`, by `TestReordersTakeLaddersInAscendingIdOrder`, `want_new` `the reorder held ladder`, and by `TestAReorderLeavesKeyShareFree`, `want_new` `was free of the reorder's lock`; + - the positions read outside the lock (`internal/server/operator_topology.go`, `old` `locked, err := qtx.LockPlanLadderOrder(ctx)`, `new` `locked, err := h.BillingQ.ListPlanLadders(ctx)\n\tif err == nil {\n\t\t_, err = qtx.LockPlanLadderOrder(ctx)\n\t}`, which keeps the lock and reads the positions before it; `ListPlanLadders` rows carry the same two fields): caught by `TestAReorderAfterAnotherWritesItsWholeOrder`, `want_new` `after waiting on another reorder, the ladders read`; + - the reorder at the server's default isolation (`internal/server/operator_topology.go`, `old` `tx, err := h.Database.BeginTx(ctx, &sql.TxOptions{Isolation: sql.LevelReadCommitted})`, `new` `tx, err := h.Database.BeginTx(ctx, &sql.TxOptions{})`, which keeps `database/sql` used): caught by `TestAReorderAfterAnotherWritesItsWholeOrder`, `want_new` `at a repeatable-read default`; + - the registry's lock left out of its kind (`internal/domains/registry.go`, `pkg` `./internal/domains`, `old` `return db.InTx(ctx, r.db, registryKind, func(tx *sql.Tx) error {`, `new` `return db.InTx(ctx, r.db, db.Plain, func(tx *sql.Tx) error {`): caught by `TestRegistryLockIsTheFirstStatement`, `want_new` `want begin and then the registry lock`; + - a `BeginTx` outside the runner (command record; `internal/integrations/stripe/catalogsync/catalogsync.go`, `old` `tx, err := db.Begin(ctx, d, db.Plain)`, `new` `tx, err := d.BeginTx(ctx, nil)`, `cmd` `./scripts/golangci-lint.sh`, `exit` 1): `want_new` `open the transaction through db.InTx or db.Begin`; + - a `Begin` on a `*sql.DB` (command record; `internal/systemtenant/systemtenant.go`, `old` `func Ensure(ctx context.Context, database *sql.DB) (string, error) {`, `new` the same followed by `\n\tif tx, err := database.Begin(); err == nil {\n\t\ttx.Rollback()\n\t}`, `cmd` `./scripts/golangci-lint.sh`, `exit` 1): `want_new` `open the transaction through db.InTx or db.Begin`; + - a pinned line whose text changes (command record; `internal/server/operator_topology.go`, `old` `tx, err := h.Database.BeginTx(ctx, &sql.TxOptions{Isolation: sql.LevelReadCommitted})`, `new` `tx, err := h.Database.BeginTx(r.Context(), &sql.TxOptions{Isolation: sql.LevelReadCommitted})`, the same `cmd` and `exit`): `want_new` `open the transaction through db.InTx or db.Begin`; + - the tests' exclusion hiding another rule (command record; `internal/server/operator_topology_reorder_db_test.go`, `old` `func TestReordersTakeLaddersInAscendingIdOrder(t *testing.T) {`, `new` the same followed by `\n\t_ = (&http.Request{}).FormValue("x")`, the file importing `net/http` for `http.Request`; the same `cmd` and `exit`): `want_new` `read a posted field from r.PostForm`. + + Named without a break, with the reason: the error labels items 1.1, 1.2 and 1.5 drop, which are log text no test or caller compares; items 1.3 and 1.4, which change no behaviour; the comment of item 1.6. + +## 5. The documents + +- [x] 5.1 `docs/go-conventions.md`, "Transactions and pool locks": "the paths that still call `BeginTx` directly are listed in #186" reads "golangci-lint refuses `BeginTx` and `(*sql.DB).Begin` outside `internal/db`, tests and the test kit (`.golangci.yml`); three handlers #221 moves out of `internal/server` are its pinned exceptions". +- [x] 5.2 `docs/database-locks.md`: + - The row-lock table gains, after the org type row: `| Plan ladder rows | every `core.plan_ladders` row | `FOR NO KEY UPDATE`, ascending `plan_ladder_id` | the `LockPlanLadderOrder` query, run by the topology reorder (`ReorderPlanLadders`) first in its transaction | Two reorders, so the later one waits and writes against the order the earlier committed. It does not conflict with the `FOR KEY SHARE` a tier, a position or an org type's default takes through its foreign key. |` + - "Tier add, tier removal, default change and reorder" gains, after its list: "The ladder reorder on the topology page (`ReorderPlanLadders`) is not a plan act: it changes only the ladders' display order, which no conferral reads. A READ COMMITTED transaction takes every ladder row (`LockPlanLadderOrder`), then updates the `sort_order` of each ladder whose position changed. It opens at READ COMMITTED whatever the server's default, because the lock read that waited returns the order the earlier reorder committed, where REPEATABLE READ would fail it with a serialization error." + - Registry: "`domains.Registry.WithLock` opens a plain transaction and takes the registry lock before anything else." reads "`domains.Registry.WithLock` opens its transaction through the runner with `registryKind`, whose first statement is the registry lock." +- [x] 5.3 `docs/testing.md`, "Static checks", the golangci-lint item: "forbidden identifiers (forbidigo), among them the rod waits the browser tests refuse (type 7)" reads "forbidden identifiers (forbidigo), among them the rod waits the browser tests refuse (type 7) and `BeginTx` or `(*sql.DB).Begin` outside `internal/db`, tests and the test kit". +- [x] 5.4 No other document changes; `design/` is not edited. + +## 6. Landing + +- [x] 6.1 With the test stack up, `make test` passes (it runs `make lint` first), and `openspec validate transactions-open-through-the-runner --strict` passes. +- [x] 6.2 The break list of item 4.6 reads CAUGHT in the commit message, with the breaks named without a record and why. +- [x] 6.3 The deploy changes no data and needs no step of its own. +- [ ] 6.4 After the push, post on #186 under `CONTRIBUTING.md`'s rules for forge text (item 1: it reads for someone who has never opened the repository, explains the console's terms once, and names no decision number, change name, date as narrative, deployment or count), without closing it. Draft: + + > The transaction runner is `db.InTx` and `db.Begin` in `internal/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 refuses `BeginTx` and `(*sql.DB).Begin` anywhere outside `internal/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, through `db.InTx`; and the Stripe product sync's and the invoice reconcile's, through `db.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`, `ReorderPlanLadders` and `CreateWorkspace` stay as pinned exceptions that name #221, which moves their transactions out of the handlers. + > + > The ladder reorder on the topology page now 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. + > + > Commit: … + +- [ ] 6.5 The archive waits for the maintainer's go; the deltas carry the spec changes, and no Purpose needs an edit a delta cannot make.