Some HTTP handlers still open their own transaction or run hand-written SQL #221

New Issue

2026-10-16T01:51:32Z

cgalo5758 commented

2026-10-16 01:51:32 +00:00

What a contributor runs into

A handler in this console parses the request, calls one function, maps that function's errors to a status and a sentence, and renders. It opens no transaction, takes no lock and runs no SQL. The function it calls lives in the package that owns the tables, opens its transaction through db.InTx or db.Begin (internal/db/tx.go), and reads and writes through that package's sqlc queries. IssueGrant (internal/server/operator_enrollment.go:609) calling entitlements.ConferGrant, and the tier handlers calling the acts in internal/entitlements/ladder_acts.go, have that shape.

Six handlers in internal/server still open their transaction themselves, and six functions there send SQL written as a string in the handler file. The Stripe webhook receiver does the same. A contributor who copies the nearest handler can copy either pattern, and nothing refuses it.

Why it costs

These handlers render their failures inside the transaction, before the deferred rollback. ManageGrant and RevokeGrantAndTransition open theirs with entitlements.BeginMaterializing, which takes the shared materialization lock first. Each refusal renders the whole organization page while that lock is held, so a rule change that starts meanwhile waits for the page, up to its five-second lock timeout.
sqlc checks a query against the schema when it generates code. A string in a handler file gets no such check: a migration that renames a column it reads breaks the page at run time. A contributor changing a table looks for its queries in the owning package and misses these. core.webhook_events has no sqlc query in any package, so every statement against it is hand-written, including the five listed here.
The comments that justify the hand-written reads go stale unnoticed. ownedResourceKeys (internal/server/operator_pages.go:337) says the entitlements ResourceKey model has no provider column; the model carries Provider (internal/entitlements/models.go:193).

Where

Transactions opened in a handler:

internal/server/operator_enrollment.go:838, ManageGrant: entitlements.BeginMaterializing, then entitlements.ManageGrantTx, commit at :928.
internal/server/operator_enrollment.go:1043, RevokeGrantAndTransition: the same shape around entitlements.RevokeGrantTx, commit at :1075.
internal/server/operator_billing.go:1287, MakeDefaultPrice: BeginTx, then ClearDefaultPrice and MarkDefaultPrice.
internal/server/operator_topology.go:382, ReorderPlanLadders: BeginTx, then SetPlanLadderSortOrder for each moved ladder.
internal/server/workspace_partials.go:121, CreateWorkspace: BeginTx, then provisioning.CreateWorkspaceWithPrimaryAssignment.
internal/server/operator_products.go:304, createProductWithSet under CreateProduct: db.InTx around CreateEntitlementSet and CreateProduct.

SQL written in the handler file:

internal/server/operator_billing.go:259, latestProcessedWebhookEventAt: core.webhook_events.
internal/server/operator_integrations_stripe.go:156, loadInboundEvents, and :207, loadInboundDeadLetterEntries: core.webhook_events.
internal/server/operator_integrations_stripe.go:318, loadDeadLetterEntries: core.outbox, whose queries are in internal/integration/queries/outbox.sql.
internal/server/operator_integrations_stripe.go:458, countMappedStripeIDs: stripe.product_mappings and stripe.price_mappings, whose queries are in internal/integrations/stripe/store/queries/.
internal/server/operator_pages.go:343, ownedResourceKeys: core.resource_keys, whose queries are in internal/entitlements/queries/resource_keys.sql.
internal/integrations/stripe/web/webhook.go:219 and :251, in the Stripe receiver's ServeHTTP: the insert and the duplicate read on core.webhook_events.

Done when

Each of the six acts is a function in the package that owns its tables. It opens its transaction through db.InTx or db.Begin with the kind it needs and returns typed errors, and its handler renders after it returns. ManageGrantTx, RevokeGrantTx and CreateWorkspaceWithPrimaryAssignment keep their transaction-taking form for the callers that already hold one.
Each read and write above is a sqlc query in the package that owns its table. core.webhook_events gets one owning package, and the Stripe receiver uses its queries too.
No non-test file in internal/server calls BeginTx, db.InTx, db.Begin, entitlements.BeginMaterializing, QueryContext, QueryRowContext or ExecContext, and a forbidigo rule in .golangci.yml keeps it that way.

Related: #186 lists the BeginTx calls in MakeDefaultPrice, ReorderPlanLadders and CreateWorkspace among the transactions opened by hand; opening the moved transactions through db.InTx settles those three entries there. #174 removes the FedWiki and Discourse handlers that do the same (internal/integrations/fedwiki/web/site_actions.go:735 and :794, internal/integrations/fedwiki/web/partials.go:411, internal/integrations/discourse/web/webhook.go:125 and :151). #207 covers what these reads show when they fail. #121 covers which file a handler belongs in.

## What a contributor runs into A handler in this console parses the request, calls one function, maps that function's errors to a status and a sentence, and renders. It opens no transaction, takes no lock and runs no SQL. The function it calls lives in the package that owns the tables, opens its transaction through `db.InTx` or `db.Begin` (`internal/db/tx.go`), and reads and writes through that package's sqlc queries. `IssueGrant` (`internal/server/operator_enrollment.go:609`) calling `entitlements.ConferGrant`, and the tier handlers calling the acts in `internal/entitlements/ladder_acts.go`, have that shape. Six handlers in `internal/server` still open their transaction themselves, and six functions there send SQL written as a string in the handler file. The Stripe webhook receiver does the same. A contributor who copies the nearest handler can copy either pattern, and nothing refuses it. ## Why it costs - These handlers render their failures inside the transaction, before the deferred rollback. `ManageGrant` and `RevokeGrantAndTransition` open theirs with `entitlements.BeginMaterializing`, which takes the shared materialization lock first. Each refusal renders the whole organization page while that lock is held, so a rule change that starts meanwhile waits for the page, up to its five-second lock timeout. - sqlc checks a query against the schema when it generates code. A string in a handler file gets no such check: a migration that renames a column it reads breaks the page at run time. A contributor changing a table looks for its queries in the owning package and misses these. `core.webhook_events` has no sqlc query in any package, so every statement against it is hand-written, including the five listed here. - The comments that justify the hand-written reads go stale unnoticed. `ownedResourceKeys` (`internal/server/operator_pages.go:337`) says the entitlements `ResourceKey` model has no provider column; the model carries `Provider` (`internal/entitlements/models.go:193`). ## Where Transactions opened in a handler: - `internal/server/operator_enrollment.go:838`, `ManageGrant`: `entitlements.BeginMaterializing`, then `entitlements.ManageGrantTx`, commit at `:928`. - `internal/server/operator_enrollment.go:1043`, `RevokeGrantAndTransition`: the same shape around `entitlements.RevokeGrantTx`, commit at `:1075`. - `internal/server/operator_billing.go:1287`, `MakeDefaultPrice`: `BeginTx`, then `ClearDefaultPrice` and `MarkDefaultPrice`. - `internal/server/operator_topology.go:382`, `ReorderPlanLadders`: `BeginTx`, then `SetPlanLadderSortOrder` for each moved ladder. - `internal/server/workspace_partials.go:121`, `CreateWorkspace`: `BeginTx`, then `provisioning.CreateWorkspaceWithPrimaryAssignment`. - `internal/server/operator_products.go:304`, `createProductWithSet` under `CreateProduct`: `db.InTx` around `CreateEntitlementSet` and `CreateProduct`. SQL written in the handler file: - `internal/server/operator_billing.go:259`, `latestProcessedWebhookEventAt`: `core.webhook_events`. - `internal/server/operator_integrations_stripe.go:156`, `loadInboundEvents`, and `:207`, `loadInboundDeadLetterEntries`: `core.webhook_events`. - `internal/server/operator_integrations_stripe.go:318`, `loadDeadLetterEntries`: `core.outbox`, whose queries are in `internal/integration/queries/outbox.sql`. - `internal/server/operator_integrations_stripe.go:458`, `countMappedStripeIDs`: `stripe.product_mappings` and `stripe.price_mappings`, whose queries are in `internal/integrations/stripe/store/queries/`. - `internal/server/operator_pages.go:343`, `ownedResourceKeys`: `core.resource_keys`, whose queries are in `internal/entitlements/queries/resource_keys.sql`. - `internal/integrations/stripe/web/webhook.go:219` and `:251`, in the Stripe receiver's `ServeHTTP`: the insert and the duplicate read on `core.webhook_events`. ## Done when - Each of the six acts is a function in the package that owns its tables. It opens its transaction through `db.InTx` or `db.Begin` with the kind it needs and returns typed errors, and its handler renders after it returns. `ManageGrantTx`, `RevokeGrantTx` and `CreateWorkspaceWithPrimaryAssignment` keep their transaction-taking form for the callers that already hold one. - Each read and write above is a sqlc query in the package that owns its table. `core.webhook_events` gets one owning package, and the Stripe receiver uses its queries too. - No non-test file in `internal/server` calls `BeginTx`, `db.InTx`, `db.Begin`, `entitlements.BeginMaterializing`, `QueryContext`, `QueryRowContext` or `ExecContext`, and a forbidigo rule in `.golangci.yml` keeps it that way. Related: #186 lists the `BeginTx` calls in `MakeDefaultPrice`, `ReorderPlanLadders` and `CreateWorkspace` among the transactions opened by hand; opening the moved transactions through `db.InTx` settles those three entries there. #174 removes the FedWiki and Discourse handlers that do the same (`internal/integrations/fedwiki/web/site_actions.go:735` and `:794`, `internal/integrations/fedwiki/web/partials.go:411`, `internal/integrations/discourse/web/webhook.go:125` and `:151`). #207 covers what these reads show when they fail. #121 covers which file a handler belongs in.

cgalo5758 added the

kind

debt

area/entitlements area/billing area/integrations area/operator-ui

priority

low

labels 2026-10-16 01:51:33 +00:00

cgalo5758 referenced this issue

2026-10-16 01:51:38 +00:00

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

cgalo5758 referenced this issue from a commit

2026-10-16 05:01:40 +00:00