Take the pool row in every branch of the subscription reconcile
ReconcileSubscription changed a provision's status and materialized the pool holding only the shared materialization lock and the subscription's own lock (#193). An operator act on the same pool, such as issuing or revoking a grant, could run at the same time; each transaction computed the pool's entitlements without the other's uncommitted change, and the last commit won, leaving entitlements that missed one of the two changes. The reconcile now takes the pool row through LockPools after the subscription lock and before any write, in all three branches: - active: before the resume's sync_source_status; - suspended: before the suspension's sync_source_status; - ended: before the floor restoration. When a provision ended, end_conferral and resumption already hold the row and the call re-takes it; on a replayed cancellation that ends nothing, it is the first take, so the vacancy read and the materialization run under it. The order matches docs/database-locks.md: no path takes a pool row and then a subscription lock, so no cycle appears. docs/database-locks.md: the pool row's "Taken by" cell, "Subscription reconcile" and rule 2 name the three calls; rule 2 loses its exception and "Where the paths differ" loses the suspended branch's entry. docs/models/resource-pools.md: invariant 11 names the reconcile's own call in all three branches. TestAStatusChangeAndAGrantActOnOnePoolKeepBothChanges runs a suspension and a recovery beside a grant act on one pool, waits on the other side's lock (AwaitLockWait, no sleep), and compares the stored limit with the sum of the active provisions. Thirty-three trace goldens gain one LockResourcePool line each; the eight the tasks list stay unchanged. Race run (2.2): `go test -race -count=1 -p 1 ./internal/fulfillment` after test/reset-test-db.sh: ok (41.3 s). Breaks (scripts/mutation-check, 5 of 5 CAUGHT): - reconcileSuspended without its LockPools - reconcileItems without its LockPools - reconcileItems LockPools after SyncSourceStatus - reconcileSuspended LockPools after SyncSourceStatus - reconcileEnded without its LockPools make lint passes; openspec validate --strict passes; the suite on reset databases passes, 4616 tests, 15 skipped.
This commit is contained in:
+31
-27
@@ -40,7 +40,7 @@ lists (see the pool row below).
|
||||
|
||||
| Lock | Row | Mode | Taken by | Serializes |
|
||||
|---|---|---|---|---|
|
||||
| Pool row | `core.resource_pools` row of one pool | `FOR UPDATE`; `FOR KEY SHARE` in a plan act's listing batch | the functions `core.confer`, `core.end_conferral`, `core.align_conferral_shape`, `core.update_conferral_bounds` and `core.settle_obligation`; `LockPools` (through the `LockResourcePool` query), called by `RevokeGrantTx`, `ManageGrantTx`, `ResumeReplacedGrantsTx`, `settlePool`, `runPlanAct` (`AddTier`, `RemoveTier` and `EnactObligation`, a default change's move), and `runPlanAct` for a reorder's move (`EnactObligation`); `LockPoolOfGrant` (through the `LockGrantPool` query), called by `ExpireGrantTx` and `ExtendWaitingGrantTx`; and `core.write_plan_act_obligations`, which takes the pool rows of one listing batch `FOR KEY SHARE` | Everything that changes a pool's positions or entitlements: conferral, endings, grant acts, tier removal, a default change's move, rule-change settlement and grant resumption. A listing batch's key-share lock conflicts with `FOR UPDATE`, so a batch and a move or a conferral on one of its pools wait for each other, and two batches of one act share the rows. |
|
||||
| Pool row | `core.resource_pools` row of one pool | `FOR UPDATE`; `FOR KEY SHARE` in a plan act's listing batch | the functions `core.confer`, `core.end_conferral`, `core.align_conferral_shape`, `core.update_conferral_bounds` and `core.settle_obligation`; `LockPools` (through the `LockResourcePool` query), called by `RevokeGrantTx`, `ManageGrantTx`, `ResumeReplacedGrantsTx`, `settlePool`, `reconcileItems`, `reconcileSuspended` and `reconcileEnded` (the subscription reconcile's active, suspended and ended branches), `runPlanAct` (`AddTier`, `RemoveTier` and `EnactObligation`, a default change's move), and `runPlanAct` for a reorder's move (`EnactObligation`); `LockPoolOfGrant` (through the `LockGrantPool` query), called by `ExpireGrantTx` and `ExtendWaitingGrantTx`; and `core.write_plan_act_obligations`, which takes the pool rows of one listing batch `FOR KEY SHARE` | Everything that changes a pool's positions or entitlements: conferral, endings, a subscription's suspension and resume, grant acts, tier removal, a default change's move, rule-change settlement and grant resumption. A listing batch's key-share lock conflicts with `FOR UPDATE`, so a batch and a move or a conferral on one of its pools wait for each other, and two batches of one act share the rows. |
|
||||
| Entitlement set row and rule row | `core.entitlement_sets` row and `core.entitlement_set_rules` row | `FOR UPDATE` | `core.commit_rule_change` (the set row also by the `LockEntitlementSet` query, once per batch) | A rule change against a writer that changes the set or the rule without holding the rendezvous. |
|
||||
| Change obligation row | `core.entitlement_set_change_obligations` row of one (change, pool) | `FOR UPDATE` | `core.settle_obligation` | Two passes settling one owed pool, so each effect is written once. |
|
||||
| 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. |
|
||||
@@ -141,25 +141,32 @@ handlers, `ExpireGrantActivity` and the demo seed.
|
||||
transaction. The transaction then takes the rendezvous, then the subscription
|
||||
advisory lock, then the rest by branch.
|
||||
|
||||
- **Active:** `reconcileItems` confers a missing product (`core.confer`),
|
||||
changes a quantity (`core.update_conferral_bounds`) and ends a surplus
|
||||
provision (`core.end_conferral`). Each of those functions takes the pool row
|
||||
of the provision it touches, so the pool row is first taken at the first of
|
||||
those calls. Materialization follows. `convergeScheduledChanges` then
|
||||
writes the scheduled-change ledger. A spent Stripe subscription schedule
|
||||
it finds is released after the commit (`releaseSpentSchedule`), with no
|
||||
lock held. When the release succeeds and the schedule held a downgrade
|
||||
row still scheduled, `supersedeReleasedDowngrades` opens a second
|
||||
transaction, takes the subscription advisory lock and supersedes those
|
||||
rows; a released schedule that held none opens no second transaction. It
|
||||
takes no rendezvous and no pool row: it writes only scheduled-change
|
||||
rows.
|
||||
- **Active:** `reconcileItems` takes the pool row (`LockPools`) after the
|
||||
subscription lock and before `core.sync_source_status('active')`. It then
|
||||
mirrors the items, reads the live provisions and confers a missing product
|
||||
(`core.confer`), changes a quantity (`core.update_conferral_bounds`) and
|
||||
ends a surplus provision (`core.end_conferral`); each of those functions
|
||||
takes the row the transaction already holds. Materialization follows.
|
||||
`convergeScheduledChanges` then writes the scheduled-change ledger. A spent
|
||||
Stripe subscription schedule it finds is released after the commit
|
||||
(`releaseSpentSchedule`), with no lock held. When the release succeeds and
|
||||
the schedule held a downgrade row still scheduled,
|
||||
`supersedeReleasedDowngrades` opens a second transaction, takes the
|
||||
subscription advisory lock and supersedes those rows; a released schedule
|
||||
that held none opens no second transaction. It takes no rendezvous and no
|
||||
pool row: it writes only scheduled-change rows.
|
||||
- **Ended:** `core.end_conferral` by subscription (the pool row of each
|
||||
provision it ends), then, only when something ended, resumption (the pool
|
||||
row), then the floor restoration (`ReapplyDefaultsIfVacant`, which confers
|
||||
through `core.confer`), then materialization.
|
||||
- **Suspended:** `core.sync_source_status`, which takes no pool lock, then
|
||||
materialization with no pool row lock held.
|
||||
row), then the pool row again (`LockPools`, which is the first take when
|
||||
nothing ended), then the floor restoration (`ReapplyDefaultsIfVacant`,
|
||||
which confers through `core.confer`), then materialization.
|
||||
- **Suspended:** `reconcileSuspended` takes the pool row (`LockPools`) before
|
||||
`core.sync_source_status('suspended')`, then materializes when it suspended
|
||||
a provision.
|
||||
|
||||
No path takes a pool row and then a subscription's advisory lock, so the
|
||||
order cannot wait in a cycle with a grant act, which takes the pool row and
|
||||
never a subscription's lock.
|
||||
|
||||
### Tier add, tier removal, default change and reorder
|
||||
|
||||
@@ -465,10 +472,9 @@ transaction that allocates or releases the name.
|
||||
`core.assert_rendezvous` raises
|
||||
`materialization_rendezvous_missing` when a function that materializes or
|
||||
settles runs without it.
|
||||
2. The pool row is locked before positions or entitlements change, except in
|
||||
the suspended branch of the subscription reconcile (see "Where the paths
|
||||
differ"). The five conferral and settlement functions lock the row
|
||||
themselves, so a caller that goes straight to one of them still holds it.
|
||||
2. The pool row is locked before positions or entitlements change. The five
|
||||
conferral and settlement functions lock the row themselves, so a caller
|
||||
that goes straight to one of them still holds it.
|
||||
3. A transaction that locks several pools takes them in ascending `pool_id`
|
||||
order, through `LockPools`. The rule change, the tier add and the tier
|
||||
removal do. A plan act's listing batch locks its pools for key-share in the
|
||||
@@ -500,13 +506,11 @@ transaction that allocates or releases the name.
|
||||
- **The plan-ending paths take the pool lock at different points** (#180).
|
||||
Revocation and expiry lock the pool before they write. The subscription
|
||||
reconcile's ended branch ends positions first, so the pool row is taken
|
||||
inside `core.end_conferral`, and takes it again for resumption. Each path
|
||||
then runs resumption, the floor restoration and materialization in the same
|
||||
order.
|
||||
inside `core.end_conferral`, and takes it again for resumption and before
|
||||
the floor restoration. Each path then runs resumption, the floor
|
||||
restoration and materialization in the same order.
|
||||
- **Advisory keys are built four ways** (#186). The keys follow the four
|
||||
conventions above.
|
||||
- **The suspended branch of the reconcile materializes without a pool row
|
||||
lock** (#193). It holds the shared rendezvous and the subscription lock only.
|
||||
- **Recording a failed drain locks the obligation row before the pool row**
|
||||
(#196), where the drain and the commit take the pool first. The plan acts'
|
||||
failure recorder takes the obligation row alone, in a plain transaction, and
|
||||
|
||||
@@ -49,7 +49,7 @@ An invariant is a rule the model guarantees everywhere; code that would break on
|
||||
8. **[app]** A metered key's usage row holds an absolute count, never a running total. The key's owner sets it, and nothing increments or decrements it. An owner that counts its own rows recounts them under the usage row's lock, in the transaction that changes them. The materializer creates the row at zero and does not write it again.
|
||||
9. **[db-code]** A usage row exists only under a materialized numeric entitlement (foreign keys and a per-entitlement uniqueness reject orphans), and only the materializer creates one, at zero; invariant 8 says who writes it after.
|
||||
10. **[app]** Pool status never leaves `active` as built: nothing writes `suspended` or `archived`. The delivery-side resolvers filter on pool status, so suspending a pool would sever conferral; the quota paths consult only assignment status, so consumption would keep working — a writer must not be introduced before that cascade is defined (tracked as [forge issue #92](https://git.coopcloud.tech/wiki-cafe/member-console/issues/92)).
|
||||
11. **[app]** The pool row is the per-organization serialization lock. Grant acts, tier removal, the org-type default change, rule-change settlement and grant resumption take it before they change positions or entitlements. The subscription reconcile serializes per subscription first, and takes the pool row inside the conferral functions it calls and when it resumes grants. [Database Locks](../database-locks.md) lists every lock and the order each path takes them in.
|
||||
11. **[app]** The pool row is the per-organization serialization lock. Grant acts, tier removal, the org-type default change, rule-change settlement and grant resumption take it before they change positions or entitlements. The subscription reconcile takes its subscription's lock first and then the pool row itself: in its active and suspended branches before it changes a provision's status, and in its ended branch before the floor restoration, after `core.end_conferral` and resumption have taken it for any provision that ended (#180). [Database Locks](../database-locks.md) lists every lock and the order each path takes them in.
|
||||
12. **[app]** Operator grant issuance refuses a multi-pool organization outright rather than guessing which pool to target — an explicit placeholder for the unbuilt multi-pool future.
|
||||
|
||||
## Dimensions
|
||||
|
||||
@@ -200,11 +200,12 @@ func resolveAccountPool(
|
||||
return custMapping.BillingAccountID, pool.PoolID, nil
|
||||
}
|
||||
|
||||
// reconcileEnded is the terminal branch (1.10), moved unchanged from
|
||||
// ReconcileSubscription: it ends every live provision for this subscription
|
||||
// via end_conferral (idempotent — nothing live is a quiet success), resumes
|
||||
// reconcileEnded is the terminal branch (1.10): it ends every live provision
|
||||
// for this subscription via end_conferral (idempotent — nothing live is a quiet success), resumes
|
||||
// the grants it replaced, restores the floor, materializes, closes the
|
||||
// scheduled changes, marks the mapping deleted and commits.
|
||||
// scheduled changes, marks the mapping deleted and commits. It takes the
|
||||
// pool row before the floor restoration, so the vacancy read and the
|
||||
// materialization run under it even when nothing ended.
|
||||
func reconcileEnded(
|
||||
ctx context.Context,
|
||||
tx *sql.Tx,
|
||||
@@ -233,6 +234,14 @@ func reconcileEnded(
|
||||
return fmt.Errorf("reconcile: resume the grants subscription %s held: %w", subscriptionID, err)
|
||||
}
|
||||
}
|
||||
// The pool row is taken here for the case where nothing ended: then
|
||||
// end_conferral and resumption took no row, and the vacancy read and the
|
||||
// materialization below would run without it. When something ended, the
|
||||
// transaction already holds the row and this takes it again, so #180's
|
||||
// order does not move (docs/database-locks.md, rule 2).
|
||||
if _, err := entQ.LockPools(ctx, poolID); err != nil {
|
||||
return fmt.Errorf("reconcile: lock pool %s before the floor: %w", poolID, err)
|
||||
}
|
||||
// Cancellation to the org default is two calls, not one: end_conferral
|
||||
// never re-mints the baseline. Restore it here, guarded on the pool
|
||||
// being left vacant so a position held by another source (e.g. an
|
||||
@@ -267,8 +276,13 @@ func reconcileEnded(
|
||||
// reconcileSuspended is the commercially degraded branch (1.10): it suspends
|
||||
// this subscription's live provisions (never a position change;
|
||||
// sync_source_status never writes 'ended') and materializes when anything was
|
||||
// suspended.
|
||||
// suspended. The pool row is taken after the subscription's lock and before
|
||||
// the status change, so the suspension and its materialization run under it
|
||||
// (docs/database-locks.md, rule 2).
|
||||
func reconcileSuspended(ctx context.Context, entQ *entitlements.Queries, subUUID uuid.UUID, poolID string) error {
|
||||
if _, err := entQ.LockPools(ctx, poolID); err != nil {
|
||||
return fmt.Errorf("reconcile: lock pool %s before the suspension: %w", poolID, err)
|
||||
}
|
||||
suspended, err := entQ.SyncSourceStatus(ctx, entitlements.SyncSourceStatusParams{
|
||||
Status: "suspended",
|
||||
SubscriptionID: uuid.NullUUID{UUID: subUUID, Valid: true},
|
||||
@@ -399,6 +413,11 @@ func upsertSubscription(
|
||||
// provisions. Missing product -> confer; surplus product -> end_conferral;
|
||||
// quantity-only delta -> update_conferral_bounds. A product tiering into any
|
||||
// number of ladders is a single confer call — no ladder-count branching.
|
||||
//
|
||||
// The pool row is taken after the subscription's lock and before the resume,
|
||||
// so the resume, the reads and every write run under it and a conferral that
|
||||
// committed while the reconcile waited is visible (docs/database-locks.md,
|
||||
// rule 2).
|
||||
func reconcileItems(
|
||||
ctx context.Context,
|
||||
logger *slog.Logger,
|
||||
@@ -415,6 +434,10 @@ func reconcileItems(
|
||||
return nil
|
||||
}
|
||||
|
||||
if _, err := entQ.LockPools(ctx, poolID); err != nil {
|
||||
return fmt.Errorf("reconcile: lock pool %s before the resume: %w", poolID, err)
|
||||
}
|
||||
|
||||
// active/trialing: resume any provisions suspended by a prior past_due.
|
||||
resumed, err := entQ.SyncSourceStatus(ctx, entitlements.SyncSourceStatusParams{
|
||||
Status: "active",
|
||||
|
||||
@@ -0,0 +1,113 @@
|
||||
// SPDX-License-Identifier: AGPL-3.0-only OR LicenseRef-Commercial
|
||||
// SPDX-FileCopyrightText: 2025-2026 Christian Galo
|
||||
|
||||
package fulfillment_test
|
||||
|
||||
import (
|
||||
"context"
|
||||
"database/sql"
|
||||
"testing"
|
||||
|
||||
"github.com/google/uuid"
|
||||
stripe "github.com/stripe/stripe-go/v81"
|
||||
|
||||
"git.coopcloud.tech/wiki-cafe/member-console/internal/entitlements"
|
||||
"git.coopcloud.tech/wiki-cafe/member-console/internal/stripetest"
|
||||
"git.coopcloud.tech/wiki-cafe/member-console/internal/testkit/dbtest"
|
||||
"git.coopcloud.tech/wiki-cafe/member-console/internal/testkit/world"
|
||||
)
|
||||
|
||||
// addSiteLimit adds an additive fedwiki_sites limit to the set of productID
|
||||
// through the rule-change path.
|
||||
func addSiteLimit(t *testing.T, database *sql.DB, productID string, limit int64) {
|
||||
t.Helper()
|
||||
setID := scalarStr(t, database, `SELECT entitlement_set_id FROM core.products WHERE product_id = $1`, productID)
|
||||
world.New(t, database).Rule(world.Product{ID: productID, SetID: setID, Name: productID}, entitlements.RuleFields{
|
||||
RuleType: "limit",
|
||||
ResourceKey: sql.NullString{String: "fedwiki_sites", Valid: true},
|
||||
ResourceValue: sql.NullInt64{Int64: limit, Valid: true},
|
||||
StackingPolicy: sql.NullString{String: "additive", Valid: true},
|
||||
})
|
||||
}
|
||||
|
||||
// A status change that starts while a grant act on the same pool is
|
||||
// uncommitted waits for it: after both commit, the pool's limit is its active
|
||||
// provisions' sum. The trace goldens pin that the wait comes before the status
|
||||
// change.
|
||||
func TestAStatusChangeAndAGrantActOnOnePoolKeepBothChanges(t *testing.T) {
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
start stripe.SubscriptionStatus
|
||||
target stripe.SubscriptionStatus
|
||||
afterGrant int
|
||||
}{
|
||||
{"suspension", stripe.SubscriptionStatusActive, stripe.SubscriptionStatusPastDue, 2},
|
||||
{"recovery", stripe.SubscriptionStatusPastDue, stripe.SubscriptionStatusActive, 3},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
checkStatusChangeBesideGrant(t, tc.name, tc.start, tc.target, tc.afterGrant)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// checkStatusChangeBesideGrant reconciles a subscription from start to target
|
||||
// while a side transaction holds an uncommitted grant of an add-on on the same
|
||||
// pool, and compares the pool's stored limit with the sum its active
|
||||
// provisions give.
|
||||
func checkStatusChangeBesideGrant(t *testing.T, name string, start, target stripe.SubscriptionStatus, afterGrant int) {
|
||||
t.Helper()
|
||||
database := dbtest.Open(t)
|
||||
fx := seedFixture(t, database)
|
||||
addSiteLimit(t, database, fx.productID, 1)
|
||||
addOn, _ := addPriced(t, database, fx, "pool")
|
||||
addSiteLimit(t, database, addOn, 2)
|
||||
|
||||
mock := &stripetest.MockBackend{Sub: fx.activeSub(fx.planItem())}
|
||||
t.Cleanup(stripetest.Install(mock))
|
||||
reconcile(t, database, fx.stripeSubID, "test:created")
|
||||
if start != stripe.SubscriptionStatusActive {
|
||||
mock.Sub = withStatus(mock.Sub, start)
|
||||
reconcile(t, database, fx.stripeSubID, "test:"+string(start))
|
||||
}
|
||||
|
||||
ctx := context.Background()
|
||||
person := fixturePersonID(t, database, fx.orgID)
|
||||
side, sidePID := beginMaterializingSide(t, database)
|
||||
if _, err := entitlements.ConferGrantTx(ctx, side, entitlements.ConferGrantInput{
|
||||
ProductID: addOn,
|
||||
OrgID: fx.orgID,
|
||||
GrantedByPersonID: person,
|
||||
GrantReason: "complimentary",
|
||||
Quantity: 1,
|
||||
ActorType: "operator",
|
||||
ActorID: uuid.NullUUID{UUID: uuid.MustParse(person), Valid: true},
|
||||
TransitionReason: "test: issue grant",
|
||||
}); err != nil {
|
||||
t.Fatalf("grant act: %v", err)
|
||||
}
|
||||
|
||||
mock.Sub = withStatus(mock.Sub, target)
|
||||
r := startReconcile(ctx, database, fx)
|
||||
r.awaitBlockedBy(t, database, sidePID)
|
||||
if err := side.Commit(); err != nil {
|
||||
t.Fatalf("commit the grant act: %v", err)
|
||||
}
|
||||
if err := r.wait(); err != nil {
|
||||
t.Fatalf("reconcile: %v", err)
|
||||
}
|
||||
|
||||
limit := scalarInt(t, database,
|
||||
`SELECT resource_limit FROM core.numeric_entitlements WHERE pool_id = $1 AND resource_key = 'fedwiki_sites'`, fx.poolID)
|
||||
sum := scalarInt(t, database, `
|
||||
SELECT COALESCE(SUM(r.resource_value), 0)::int
|
||||
FROM core.pool_provisions pp
|
||||
JOIN core.entitlement_set_rules r ON r.set_id = pp.entitlement_set_id
|
||||
WHERE pp.pool_id = $1 AND pp.status = 'active' AND r.is_active
|
||||
AND r.rule_type = 'limit' AND r.resource_key = 'fedwiki_sites'`, fx.poolID)
|
||||
if sum != afterGrant {
|
||||
t.Fatalf("the active provisions' fedwiki_sites limits after the %s and the grant sum to %d, want %d", name, sum, afterGrant)
|
||||
}
|
||||
if limit != sum {
|
||||
t.Errorf("the pool's limit after the %s and the grant = %d, want %d from the active provisions", name, limit, sum)
|
||||
}
|
||||
}
|
||||
@@ -19,6 +19,7 @@ import (
|
||||
|
||||
stripe "github.com/stripe/stripe-go/v81"
|
||||
|
||||
"git.coopcloud.tech/wiki-cafe/member-console/internal/entitlements"
|
||||
"git.coopcloud.tech/wiki-cafe/member-console/internal/fulfillment"
|
||||
"git.coopcloud.tech/wiki-cafe/member-console/internal/stripetest"
|
||||
"git.coopcloud.tech/wiki-cafe/member-console/internal/testkit/dbtest"
|
||||
@@ -216,6 +217,24 @@ func beginSide(t *testing.T, database *sql.DB) (*sql.Tx, int) {
|
||||
if err != nil {
|
||||
t.Fatalf("begin the side transaction: %v", err)
|
||||
}
|
||||
return trackSide(t, side)
|
||||
}
|
||||
|
||||
// beginMaterializingSide is beginSide for a transaction that holds the shared
|
||||
// rendezvous, as an operator's grant act does.
|
||||
func beginMaterializingSide(t *testing.T, database *sql.DB) (*sql.Tx, int) {
|
||||
t.Helper()
|
||||
side, err := entitlements.BeginMaterializing(context.Background(), database)
|
||||
if err != nil {
|
||||
t.Fatalf("begin the materializing side transaction: %v", err)
|
||||
}
|
||||
return trackSide(t, side)
|
||||
}
|
||||
|
||||
// trackSide rolls side back when the test ends and returns it with its
|
||||
// backend's pid.
|
||||
func trackSide(t *testing.T, side *sql.Tx) (*sql.Tx, int) {
|
||||
t.Helper()
|
||||
t.Cleanup(func() { _ = side.Rollback() })
|
||||
var pid int
|
||||
if err := side.QueryRow(`SELECT pg_backend_pid()`).Scan(&pid); err != nil {
|
||||
@@ -261,7 +280,7 @@ func (r *backgroundReconcile) awaitBlockedBy(t *testing.T, database *sql.DB, sid
|
||||
dbtest.AwaitLockWait(t, database, dbtest.Backend{BlockedBy: sidePID, Abort: func() error {
|
||||
select {
|
||||
case <-r.finished:
|
||||
return fmt.Errorf("the reconcile returned (%v) without waiting for the subscription lock", r.err)
|
||||
return fmt.Errorf("the reconcile returned (%v) without waiting for the other transaction", r.err)
|
||||
default:
|
||||
return nil
|
||||
}
|
||||
|
||||
@@ -14,6 +14,7 @@ GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
CreateSubscriptionChange
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -13,6 +13,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -13,6 +13,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -14,6 +14,7 @@ GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
SELECT core.end_conferral($1, $2, $3, $4, $5, $6, $7, $8)
|
||||
LockResourcePool
|
||||
SELECT EXISTS (
|
||||
<materialize>
|
||||
ApplyScheduledChangesForSubscription
|
||||
|
||||
@@ -20,6 +20,7 @@ ListResumableGrantsHeldBy
|
||||
SAVEPOINT grant_resumption
|
||||
SELECT provision_id, outcome FROM core.confer($1, $2, $3, $4, $5, $6, $7, $8, $9, $10, $11)
|
||||
RELEASE SAVEPOINT grant_resumption
|
||||
LockResourcePool
|
||||
SELECT EXISTS (
|
||||
<materialize>
|
||||
ApplyScheduledChangesForSubscription
|
||||
|
||||
@@ -17,6 +17,7 @@ CreateSubscriptionChange
|
||||
SELECT core.end_conferral($1, $2, $3, $4, $5, $6, $7, $8)
|
||||
LockResourcePool
|
||||
ListResumableGrantsHeldBy
|
||||
LockResourcePool
|
||||
SELECT EXISTS (
|
||||
GetResourcePoolByID
|
||||
SELECT t.default_plan_ladder_id::text
|
||||
|
||||
@@ -14,6 +14,7 @@ GetSubscriptionMappingByStripeID
|
||||
CreateSubscription
|
||||
UpsertSubscriptionMapping
|
||||
CreateSubscriptionChange
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -13,6 +13,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -13,6 +13,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -14,6 +14,7 @@ GetSubscriptionMappingByStripeID
|
||||
CreateSubscription
|
||||
UpsertSubscriptionMapping
|
||||
CreateSubscriptionChange
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -14,6 +14,7 @@ GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
CreateSubscriptionChange
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
<materialize>
|
||||
commit
|
||||
|
||||
@@ -13,6 +13,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -14,6 +14,7 @@ GetSubscriptionMappingByStripeID
|
||||
CreateSubscription
|
||||
UpsertSubscriptionMapping
|
||||
CreateSubscriptionChange
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -13,6 +13,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -14,6 +14,7 @@ GetSubscriptionMappingByStripeID
|
||||
CreateSubscription
|
||||
UpsertSubscriptionMapping
|
||||
CreateSubscriptionChange
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -13,6 +13,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -14,6 +14,7 @@ GetSubscriptionMappingByStripeID
|
||||
CreateSubscription
|
||||
UpsertSubscriptionMapping
|
||||
CreateSubscriptionChange
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
+1
@@ -13,6 +13,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
+1
@@ -13,6 +13,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -13,6 +13,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -13,6 +13,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -13,6 +13,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -13,6 +13,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
+1
@@ -13,6 +13,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
+1
@@ -13,6 +13,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -20,6 +20,7 @@ UpdateSubscriptionStatus
|
||||
SELECT core.end_conferral($1, $2, $3, $4, $5, $6, $7, $8)
|
||||
LockResourcePool
|
||||
ListResumableGrantsHeldBy
|
||||
LockResourcePool
|
||||
SELECT EXISTS (
|
||||
GetResourcePoolByID
|
||||
SELECT t.default_plan_ladder_id::text
|
||||
|
||||
@@ -21,6 +21,7 @@ CreateSubscriptionChange
|
||||
SELECT core.end_conferral($1, $2, $3, $4, $5, $6, $7, $8)
|
||||
LockResourcePool
|
||||
ListResumableGrantsHeldBy
|
||||
LockResourcePool
|
||||
SELECT EXISTS (
|
||||
GetResourcePoolByID
|
||||
SELECT t.default_plan_ladder_id::text
|
||||
|
||||
@@ -18,6 +18,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -22,6 +22,7 @@ CreateSubscriptionChange
|
||||
SELECT core.end_conferral($1, $2, $3, $4, $5, $6, $7, $8)
|
||||
LockResourcePool
|
||||
ListResumableGrantsHeldBy
|
||||
LockResourcePool
|
||||
SELECT EXISTS (
|
||||
GetResourcePoolByID
|
||||
SELECT t.default_plan_ladder_id::text
|
||||
|
||||
@@ -22,6 +22,7 @@ CreateSubscriptionChange
|
||||
SELECT core.end_conferral($1, $2, $3, $4, $5, $6, $7, $8)
|
||||
LockResourcePool
|
||||
ListResumableGrantsHeldBy
|
||||
LockResourcePool
|
||||
SELECT EXISTS (
|
||||
GetResourcePoolByID
|
||||
SELECT t.default_plan_ladder_id::text
|
||||
|
||||
@@ -18,6 +18,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -18,6 +18,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -23,6 +23,7 @@ GetDefaultPoolByOrgID
|
||||
GetSubscriptionMappingByStripeID
|
||||
GetSubscriptionByID
|
||||
UpdateSubscriptionStatus
|
||||
LockResourcePool
|
||||
SELECT core.sync_source_status($1, $2, $3, $4, $5)
|
||||
GetPriceMappingByStripeID
|
||||
GetPrice
|
||||
|
||||
@@ -0,0 +1,2 @@
|
||||
schema: spec-driven
|
||||
created: 2026-10-24
|
||||
@@ -0,0 +1,75 @@
|
||||
## Context
|
||||
|
||||
`ReconcileSubscription` (`internal/fulfillment/reconcile.go:47-137`) reads the subscription from Stripe before it opens a transaction, opens a `Materializing` transaction whose first statement takes the shared rendezvous (`internal/entitlements/tx.go:44-71`), takes the subscription's advisory lock (`reconcile.go:78-81`), resolves the billing account and the organization's default pool, upserts the core subscription row, and branches on `classifyStatus` (`:777-788`): `active` and `trialing` run `reconcileItems`; `past_due`, `unpaid` and `paused` run `reconcileSuspended`; `canceled` and `incomplete_expired` run `reconcileEnded`; `incomplete` records only.
|
||||
|
||||
`reconcileItems` (`:402-453`) resumes the subscription's provisions through `core.sync_source_status('active')` (`:419-425`), then mirrors the items, reads the live provisions and confers, updates or ends. No pool row lock is taken before `core.sync_source_status`; the first lock on the pool row is the one `core.confer`, `core.update_conferral_bounds` or `core.end_conferral` takes inside, at the first call that reaches it. `reconcileSuspended` (`:267-285`) calls `core.sync_source_status('suspended')` and materializes with the rendezvous and the subscription's lock held and no pool row lock at all (`docs/database-locks.md`, "Subscription reconcile" and "Where the paths differ"). `reconcileEnded` takes the row only through `core.end_conferral` (for each provision it ends) and resumption (only when something ended); when nothing ended, as on a replayed cancellation, `ReapplyDefaultsIfVacant` returns on an occupied pool without calling `core.confer`, and the materialization runs with no pool row. The committed trace goldens show it: `GetLivePoolProvisionsBySubscriptionID` and then `core.confer` with no lock statement between, and `core.sync_source_status` with none before it.
|
||||
|
||||
Every other path that changes a pool's positions or entitlements locks the pool row first (`docs/database-locks.md`, rule 2): the grant acts take it through `LockPools` or `LockPoolOfGrant`, and the conferral functions other than `core.sync_source_status`, and `core.settle_obligation`, take it themselves. A transaction that holds the row computes the pool's entitlements from the provisions it can see, and the materialization rewrites the pool's entitlement rows. Two transactions on one pool that do not serialize on the row each compute without the other's uncommitted change, and the later commit overwrites the earlier result.
|
||||
|
||||
## Goals / Non-Goals
|
||||
|
||||
**Goals:**
|
||||
- The reconcile changes a provision's status, and materializes, only under the pool row, in the active, suspended and ended branches.
|
||||
- A test pins the stored entitlements against the provisions when a status change and a grant act run on one pool at the same time.
|
||||
- Rule 2 of `docs/database-locks.md` has no exception left for the reconcile.
|
||||
|
||||
**Non-Goals:**
|
||||
- The ended branch's lock order (#180).
|
||||
- Any change to what the reconcile confers or ends (`displaced-subscription-waits`).
|
||||
|
||||
## Decisions
|
||||
|
||||
### D1. Where the reconcile takes the pool row
|
||||
|
||||
Pick: `entQ.LockPools(ctx, poolID)` once in each branch: in `reconcileItems` after its `sub.Items == nil` return and before `core.sync_source_status('active')`, in `reconcileSuspended` before `core.sync_source_status('suspended')`, and in `reconcileEnded` after the resumption block and before `ReapplyDefaultsIfVacant`. So the resume, the item mirror, the live read and every conferral of the active branch, the suspension and its materialization, and the floor's vacancy read and the ended branch's materialization, run under the pool row, taken after the subscription's advisory lock. The ended branch's order does not move (#180): it still takes the pool row inside `core.end_conferral` and again in resumption, and its new call comes after both. When something ended, the transaction already holds the row at that point and the call takes it again at no cost; when nothing ended, it is the first take, and it covers the vacancy read and the materialization.
|
||||
|
||||
Taking the lock once before the status switch, for all three branches, was rejected: it would move the ended branch's order, which is #180's. The ended branch's call sits after the resumption block for that reason.
|
||||
|
||||
`LockPools` (`internal/entitlements/tx.go:114-135`) runs the `LockResourcePool` query, `SELECT ... FOR UPDATE` on the pool row. The row is taken once; `core.confer` and the other functions then take a row the transaction already holds, which costs nothing.
|
||||
|
||||
### D2. The order is safe
|
||||
|
||||
The subscription lock is taken before the pool row today already, implicitly inside `core.confer` in the active branch and through resumption in the ended branch. No path takes a pool row and then a subscription advisory lock: only `reconcile.go:79` and `:976` take that lock, and the grant acts, the plan acts, settlement and resumption take the rendezvous and the pool row only. Two reconciles of two subscriptions of one organization take different advisory keys and then the one pool row, so they form no cycle. Grant issuance, revocation, expiry and Manage take the rendezvous and the pool row and never a subscription's lock, so no cycle forms with them either.
|
||||
|
||||
`core.sync_source_status` takes no pool row (`00005:401-437`); the row is the reconcile's own `LockPools` call.
|
||||
|
||||
The 5 s `lock_timeout` the rendezvous sets bounds the new wait: a wait past it fails the reconcile with SQLSTATE 55P03, as a wait inside `core.confer` does today.
|
||||
|
||||
### D3. The documents
|
||||
|
||||
`docs/database-locks.md` changes with the code: the pool row's "Taken by" cell gains the reconcile's three branches, "Subscription reconcile" states the order above, rule 2 loses its exception, and "Where the paths differ" loses the suspended branch's entry (#193). `docs/models/resource-pools.md`, invariant 11, says that the reconcile takes the pool row itself in all three branches (tasks 3.4).
|
||||
|
||||
## Failure table
|
||||
|
||||
| # | State when the reconcile runs | Today | After |
|
||||
|---|---|---|---|
|
||||
| F1 | A subscription turns `past_due` while an operator's grant act on the pool is uncommitted | No pool lock: the suspension materializes without the grant; the last commit writes entitlements that miss one change | The reconcile waits for the pool row; the entitlements reflect both |
|
||||
| F2 | A subscription returns to `active` in the same situation | The resume runs before any lock | Same as F1 |
|
||||
| F3 | A status change with no concurrent act | Unchanged result | Unchanged result, plus one lock statement |
|
||||
| F4 | A reconcile of a subscription that is `canceled` or `incomplete_expired` and ends a provision | Pool row taken inside `core.end_conferral` and in resumption | Unchanged order (#180); one more take of the held row before the floor |
|
||||
| F5 | The same reconcile when nothing ends, as on a replayed cancellation, while a grant act on the pool is uncommitted | No pool row: the floor's vacancy read and the materialization run without the grant | The reconcile waits for the pool row before the floor; the entitlements reflect both |
|
||||
|
||||
## Locks and transactions
|
||||
|
||||
Active branch, in order: the shared rendezvous (first statement), the subscription advisory lock, the `core.subscriptions` row, **new:** the pool row (`LockPools`), `core.sync_source_status('active')`, the item mirror, the live read, the conferrals, materialization, the scheduled-change rows, commit; the Stripe schedule release after the commit.
|
||||
|
||||
Suspended branch: the rendezvous, the subscription lock, the `core.subscriptions` row, **new:** the pool row, `core.sync_source_status('suspended')`, materialization when anything was suspended.
|
||||
|
||||
Ended branch, in order: the rendezvous, the subscription lock, `core.end_conferral` (the pool row of each provision it ends), resumption when something ended, **new:** the pool row (`LockPools`; the first take when nothing ended), the floor restoration, materialization, the scheduled-change rows, the mapping row, commit.
|
||||
|
||||
Against `docs/database-locks.md`: the rendezvous comes first, the subscription advisory lock second, the pool row after both, the order of every path that takes them. Rule 2 ("the pool row is locked before positions or entitlements change", `docs/database-locks.md`) holds for all three branches, and its exception goes. Invariant 11 of `docs/models/resource-pools.md` names the reconcile's own call in all three branches (task 3.4).
|
||||
|
||||
## Tests
|
||||
|
||||
Each test claim is pinned by a break (`docs/testing.md`, "Prove a test with deliberate breaks"); the tasks list them with the texts each failing test prints. The goldens' change is characterized by the unchanged-list in the tasks: a golden changes only by the added `LockResourcePool` line.
|
||||
|
||||
## What is unverified
|
||||
|
||||
- **Where a broken reconcile waits.** The breaks of `TestAStatusChangeAndAGrantActOnOnePoolKeepBothChanges` (the pool lock removed) are expected to block on the pool's entitlement rows that the grant act's materialization holds, then overwrite them with a value computed before the grant committed. That is read from the code, not run; the section's mutation run confirms it, and a different failure text is a finding, not a record to rewrite.
|
||||
|
||||
## Risks / Trade-offs
|
||||
|
||||
- **Every active reconcile now waits for the pool row, a no-op renewal included.** Before, a reconcile that wrote nothing took the row only if a conferral call reached it. A hold on the row past the 5 s `lock_timeout` now fails a reconcile that would have written nothing. The webhook workflow retries a failed reconcile, so the failure delays the reconcile and loses nothing.
|
||||
- **The suspended branch waits too**, where it did not, for the length of an operator's grant act on the same pool.
|
||||
- **Every trace golden that reaches `core.sync_source_status` changes by one line**, and so do the seven ended-branch goldens (`canceled_replay`, `canceled_resumes_grant`, `canceled_to_default`, `already_canceled`, `already_ended`, `due_cancellation`, `lost_claim`); the tasks name them, and no other golden may change.
|
||||
- **Every ended reconcile now waits for the pool row**, a replay that ends nothing included, where it did not.
|
||||
@@ -0,0 +1,44 @@
|
||||
## Why
|
||||
|
||||
When a Stripe subscription becomes past due, unpaid or paused, `ReconcileSubscription` (`internal/fulfillment/reconcile.go`) marks the subscription's provisions suspended through `core.sync_source_status` and materializes the pool without taking the pool row lock. When the subscription is active again, the same function resumes those provisions through `core.sync_source_status` before any call that takes the lock. Every other path that changes a pool's positions or entitlements locks the pool row first, and the conferral functions other than `core.sync_source_status`, and `core.settle_obligation`, take it themselves (`docs/database-locks.md`).
|
||||
|
||||
The reconcile holds only the shared materialization lock and the subscription's own lock. An operator act on the same pool, such as issuing or revoking a grant, can therefore run at the same time. Each transaction computes the pool's entitlements without the other's uncommitted change, and whichever commits last writes its result. The pool keeps entitlements that miss one of the two changes until something materializes it again.
|
||||
|
||||
#193 names two branches: the suspended one takes no pool row, and the active one resumes before any lock and, when it confers nothing, materializes without one. The ended branch has the same defect when it ends nothing, as on a replayed `customer.subscription.deleted`: `core.end_conferral` takes the row only for a provision it ends, resumption is skipped, and `ReapplyDefaultsIfVacant` returns on an occupied pool without calling `core.confer`, so the floor's vacancy read and the materialization run without the row. The displacement rule of `displaced-subscription-waits` (#212) reads the pool's positions in that branch, so it needs the row first.
|
||||
|
||||
## What Changes
|
||||
|
||||
- **The reconcile takes the pool row before it changes a provision's status or its pool's positions, in all three branches.** `reconcileItems` takes it after its `sub.Items == nil` return and before `core.sync_source_status('active')`; `reconcileSuspended` takes it before `core.sync_source_status('suspended')`; `reconcileEnded` takes it after the resumption block and before `ReapplyDefaultsIfVacant`, with the error `reconcile: lock pool %s before the floor: %w`. All three calls come after the subscription's advisory lock, the order the ended branch already uses through `core.end_conferral` and resumption, so no lock order changes. The ended branch's call is a fix that rides along: when something ended, the transaction already holds the row and the call takes it again; when nothing ended, it is the first take.
|
||||
- **A test runs a status change and a grant act on one pool at the same time** and compares the pool's stored entitlements with its active provisions.
|
||||
- **`docs/database-locks.md` loses the suspended branch's exception**: the pool row's "Taken by" cell and "Subscription reconcile" name the new calls (the ended branch's included), rule 2 loses its exception, and "Where the paths differ" loses the suspended branch's entry; invariant 11 of `docs/models/resource-pools.md` names the reconcile's own call.
|
||||
- **The pool-lock rule is stated once**, in `plan-transitions`, "IssueGrant serializes concurrent operators on the same pool", which this change modifies (and paragraph 1's claim that every function takes the lock, corrected) to name every path that takes the row first and the reconcile's three branches. `displaced-subscription-waits` points at that requirement by name and restates nothing.
|
||||
|
||||
## Out of scope
|
||||
|
||||
- **The ended branch's order** (#180): it takes the pool row inside `core.end_conferral` and again in resumption, and that stays. Taking the row once before the status switch, for all three branches, was rejected because it would move that order; the ended branch's new call comes after resumption, where the row is already held whenever something ended.
|
||||
- **The displacement rule**, which `displaced-subscription-waits` adds on top of this lock.
|
||||
- **`world.Suspend`** (`internal/testkit/world/plans.go`) suspends without the pool row. It builds fixtures one at a time, so no row or result depends on the lock; giving it the reconcile's `LockPools` call is left to the next change that touches the builder.
|
||||
|
||||
## Capabilities
|
||||
|
||||
### New Capabilities
|
||||
|
||||
None.
|
||||
|
||||
### Modified Capabilities
|
||||
|
||||
- `plan-transitions`: modified "IssueGrant serializes concurrent operators on the same pool" (which paths take the pool row before they read or change a provision, the subscription reconcile's three branches among them).
|
||||
|
||||
## Impact
|
||||
|
||||
- **Code:** `internal/fulfillment/reconcile.go` (three calls and three doc comments); the test helpers in `internal/fulfillment` (`beginMaterializingSide`, a `Materializing` variant of the side-transaction helper, one abort message).
|
||||
- **Schema, Stripe calls, routes:** none.
|
||||
- **Behaviour a deployment sees:** a status change and an operator's act on one pool run one after the other; a reconcile waits for the pool row where it did not.
|
||||
- **Tests:** one lock test with two rows, and every trace golden that reaches `core.sync_source_status` gains the lock statement, and seven ended-branch goldens gain one for the ended branch (`canceled_replay`, `canceled_resumes_grant`, `canceled_to_default`, and the sweep's `already_canceled`, `already_ended`, `due_cancellation`, `lost_claim`).
|
||||
- **Docs:** `docs/database-locks.md`, `docs/models/resource-pools.md`.
|
||||
|
||||
## Related issues
|
||||
|
||||
- **#193** is this change; it closes with it.
|
||||
- **#212** (`displaced-subscription-waits`) depends on it and lands after it.
|
||||
- **#180** (the plan-ending paths' lock order) is unchanged.
|
||||
@@ -0,0 +1,27 @@
|
||||
## MODIFIED Requirements
|
||||
|
||||
### Requirement: IssueGrant serializes concurrent operators on the same pool
|
||||
|
||||
Serialization of concurrent operators targeting the same pool SHALL be provided by `entitlements.confer`'s own pool-row lock (`SELECT ... FOR UPDATE` on `entitlements.resource_pools`, acquired inside the function before resolving any ladder state — owned by the `entitlement-conferral` capability's contract). The operator grant-issuance handler at `POST /partials/operator/organizations/{orgID}/grant/create` (per the `plan-enrollment-administration` capability) SHALL NOT itself acquire a row lock on the pool before calling `confer`; it SHALL rely entirely on the lock `confer` takes internally. A handler that only calls `confer` or `end_conferral` needs no lock of its own, because those functions take the pool row as their first act; `core.sync_source_status` takes none, and a path that decides from what it reads takes the row itself, as the next paragraph says.
|
||||
|
||||
A path that reads the pool's positions and then decides what to confer or end SHALL take the same pool row first, through `LockPools` or `LockPoolOfGrant`, so that the reads it decides on are made under the row and a conferral that committed while it waited is visible to them; the conferral functions then take the row the transaction already holds. The grant acts (revocation, expiry, extension of a waiting grant, Manage), grant resumption, rule-change settlement, the plan acts and the subscription reconcile's active branch are such paths. The reconcile's suspended branch SHALL take the row too, before it changes a provision's status, so that its suspension and the materialization that follows run under the row, and its ended branch SHALL take it before the floor restoration, so that the vacancy read and the materialization run under the row when nothing ended. The subscription reconcile SHALL take the row after the subscription's own lock, and no path SHALL take a pool row and then a subscription's lock.
|
||||
|
||||
#### Scenario: Concurrent trial-grant issuance on the same pool serializes inside confer
|
||||
|
||||
- **WHEN** two operators concurrently POST `/partials/operator/organizations/{orgID}/grant/create` for the same org's pool with different target products
|
||||
- **THEN** the two transactions SHALL serialize on the pool-row lock `entitlements.confer` acquires internally
|
||||
- **AND** the first transaction SHALL commit with a valid conferral recorded
|
||||
- **AND** the second SHALL observe the first's committed state (a `noop` if the same product/source, or a correct supersession otherwise)
|
||||
- **AND** no `pool_provision_ladders` row SHALL exist in a state that violates the GiST exclusion constraint
|
||||
|
||||
#### Scenario: The handler acquires no lock of its own
|
||||
|
||||
- **WHEN** the operator grant-issuance handler processes a request
|
||||
- **THEN** it SHALL NOT execute a `SELECT ... FOR UPDATE` against `resource_pools` itself
|
||||
- **AND** it SHALL rely on the lock `entitlements.confer` acquires as part of its own contract
|
||||
|
||||
#### Scenario: A status change and a grant act on one pool keep both changes
|
||||
|
||||
- **WHEN** a subscription turns `past_due`, or returns to `active`, while an operator's grant act on the same pool holds the pool row, uncommitted
|
||||
- **THEN** the reconcile SHALL wait for the pool row before it changes the provision's status
|
||||
- **AND** after both commit, the pool's materialized entitlements SHALL be those of its active provisions, the grant's included
|
||||
@@ -0,0 +1,46 @@
|
||||
Tests follow `docs/testing.md`: database tests open the database through `dbtest`, build their rows through the test kit or the package's fixtures, filter to the rows their own fixture made, and pass twice in a row without a reset. A Stripe test drives `stripetest.MockBackend` with the API state Stripe would return, never a hand-built event payload, and runs without `t.Parallel()`. A test of a lock holds one transaction open and waits for the block with `dbtest.AwaitLockWait`, never a sleep.
|
||||
|
||||
Sections 1 to 4 are the parts of one commit; section 5 is not a commit. 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 test result changes except where a section's items name it; any other change is a stop. A golden is rewritten with `-update` only where an item says so, and its diff is read before the commit. The lock test runs a reconcile on a goroutine, so it runs with `go test -race -p 1 -count=1` after `test/reset-test-db.sh`. A script's usage is learned from its header or source, never by running it.
|
||||
|
||||
**Breaks.** The list of section 2 is written to a file outside the repository and run with `go run ./scripts/mutation-check -list <file> -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 test is new, so there is no old side. A break of Go code is a test record (`file`, `old`, `new`, `pkg`, `run_new`, `want_new`). Every record's `want_new` is a text the failing test prints only when it fails, never a test or subtest name; for a golden it is `<path> differs from what the code produced`, which `internal/testkit/golden/golden.go:42` prints only on a mismatch, with the path as the test passes it. Every record must be CAUGHT. An `old` text below is the code as the section's items write it; when the committed code words it differently, the record is corrected to the committed text before the run, never dropped. 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 reconcile takes the pool row before it changes a provision's status
|
||||
|
||||
- [x] 1.1 `internal/fulfillment/reconcile.go` (design D1): `reconcileItems`, after its `sub.Items == nil` return and before `SyncSourceStatus`, takes the pool row: `if _, err := entQ.LockPools(ctx, poolID); err != nil { return fmt.Errorf("reconcile: lock pool %s before the resume: %w", poolID, err) }`. Its doc comment says that the pool row is taken after the subscription's lock and before the resume, so the resume, the reads and every write run under it and a conferral that committed while the reconcile waited is visible (`docs/database-locks.md`, rule 2).
|
||||
- [x] 1.2 `reconcileSuspended` takes the pool row the same way before its `SyncSourceStatus`, with the error `reconcile: lock pool %s before the suspension: %w`, and its doc comment says the suspension and its materialization run under the pool row.
|
||||
- [x] 1.3 `reconcileEnded` takes the pool row after the resumption block and before `ReapplyDefaultsIfVacant`: `if _, err := entQ.LockPools(ctx, poolID); err != nil { return fmt.Errorf("reconcile: lock pool %s before the floor: %w", poolID, err) }`. Its doc comment says the row is taken before the floor restoration, so the vacancy read and the materialization run under it even when nothing ended. When something ended the transaction already holds the row, so #180's order does not move.
|
||||
|
||||
## 2. The test and its breaks
|
||||
|
||||
- [x] 2.1 `TestAStatusChangeAndAGrantActOnOnePoolKeepBothChanges` (a new `internal/fulfillment/reconcile_pool_lock_test.go`, package `fulfillment_test`), rows `suspension` and `recovery` through a named function (`checkStatusChangeBesideGrant`). The fixture is `seedFixture`'s, its product's set given an additive limit of 1 on `fedwiki_sites`, and an add-on product (`addPriced`) whose set gives an additive limit of 2 on the same key; the subscription is reconciled `active`, and for `recovery` then `past_due`. A side transaction from `entitlements.BeginMaterializing` issues an operator's grant of the add-on through `entitlements.ConferGrantTx` and stays open, holding the pool row through `core.confer`. The reconcile of the row's target status (`past_due` for `suspension`, `active` for `recovery`) starts in the background (`startReconcile`), waits on the side (`awaitBlockedBy`), the side commits, the reconcile ends without error, and the pool's numeric limit for the key is compared with the sum its active provisions give, read from `core.pool_provisions` and the sets' active limit rules; that sum is itself checked against 2 for `suspension` and 3 for `recovery`. A failure of the comparison reads `the pool's limit after the %s and the grant = %d, want %d from the active provisions`. The helper that opens the side gains a `Materializing` variant, `beginMaterializingSide`, beside `beginSide`, and `awaitBlockedBy`'s abort message becomes `the reconcile returned (%v) without waiting for the other transaction`, since it now serves a second lock; `TestReconcileWaitsForTheSubscriptionLock` keeps its result.
|
||||
- [x] 2.2 `go test -race -p 1 -count=1 -run '^TestAStatusChangeAndAGrantActOnOnePoolKeepBothChanges$' ./internal/fulfillment` passes after `test/reset-test-db.sh`; the commit message records it.
|
||||
- [x] 2.3 Breaks:
|
||||
- `reconcileSuspended` without its `LockPools` call (the record's `old` is that call with its `before the suspension` error): caught by `TestAStatusChangeAndAGrantActOnOnePoolKeepBothChanges/suspension`, `want_new` `want 2 from the active provisions`;
|
||||
- `reconcileItems` without its `LockPools` call (the record's `old` is that call with its `before the resume` error): caught by `TestAStatusChangeAndAGrantActOnOnePoolKeepBothChanges/recovery`, `want_new` `want 3 from the active provisions`;
|
||||
- `reconcileSuspended`'s `LockPools` moved after `SyncSourceStatus`: not caught by the lock test, because the suspension writes a row the grant act does not touch and the materialization still runs under the row; caught by `TestReconcileTrace/past_due`, `want_new` `testdata/reconcile_trace/past_due.golden differs from what the code produced`;
|
||||
- `reconcileEnded` without its `LockPools` call (the record's `old` is that call with its `before the floor` error): caught by `TestReconcileTrace/canceled_replay`, `want_new` `testdata/reconcile_trace/canceled_replay.golden differs from what the code produced`;
|
||||
- `reconcileItems`' `LockPools` moved after `SyncSourceStatus`: not caught by the lock test, because the resume writes a row the grant act does not touch and the materialization still runs under the row; caught by `TestReconcileTrace/active_again`, `want_new` `testdata/reconcile_trace/active_again.golden differs from what the code produced`.
|
||||
|
||||
## 3. The documents
|
||||
|
||||
- [x] 3.1 The pool row's "Taken by" cell names `reconcileItems`, `reconcileSuspended` and `reconcileEnded` among the callers of `LockPools`; "Serializes" names a subscription's suspension and resume.
|
||||
- [x] 3.2 "Subscription reconcile" says that the active branch takes the pool row after the subscription lock and before `core.sync_source_status('active')`, then mirrors the items, reads the live provisions and confers, updates or ends, each function taking the row the transaction holds, that the suspended branch takes the pool row before `core.sync_source_status`, then materializes when it suspended a provision, and that the ended branch takes the pool row again before the floor restoration, the first take when nothing ended. It adds that no path takes a pool row and then a subscription's lock, so the order cannot wait in a cycle with a grant act, which takes the pool row and never a subscription's lock.
|
||||
- [x] 3.3 Rule 2 loses ", except in the suspended branch of the subscription reconcile (see "Where the paths differ")". "Where the paths differ" loses the suspended branch's entry (#193).
|
||||
- [x] 3.4 `docs/models/resource-pools.md`, invariant 11 (the sentence that begins "The subscription reconcile serializes per subscription first"): the sentence is replaced by "The subscription reconcile takes its subscription's lock first and then the pool row itself: in its active and suspended branches before it changes a provision's status, and in its ended branch before the floor restoration, after `core.end_conferral` and resumption have taken it for any provision that ended (#180)." The edit is made when this change is built.
|
||||
|
||||
## 4. Goldens
|
||||
|
||||
- [x] 4.1 Every golden under `internal/fulfillment/testdata/reconcile_trace`, `scheduled_change_trace` and `sweep_trace` whose trace holds `SELECT core.sync_source_status($1, $2, $3, $4, $5)` gains `LockResourcePool` on the line before each such line, and no other line, written with `-update` and read in the diff. The seven ended-branch goldens `reconcile_trace/canceled_replay`, `canceled_resumes_grant` and `canceled_to_default`, and `sweep_trace/already_canceled`, `already_ended`, `due_cancellation` and `lost_claim` (the sweep reconciles a canceled subscription), each gain one `LockResourcePool` line, before the floor restoration's statements.
|
||||
- [x] 4.2 The goldens without that statement do not change. In `reconcile_trace`: `fetch_refused`, `incomplete`, `no_customer` and `unreachable`. In `sweep_trace`: `quantity_change`, `term_boundary_switch_refused`, `unreachable_item` and `unreachable_target_price`. `scheduled_change_trace` has none without it.
|
||||
|
||||
## 5. Landing
|
||||
|
||||
- [x] 5.1 With the test stack up, `make test` passes (it runs `make lint` first), and `openspec validate reconcile-takes-pool-lock --strict` passes.
|
||||
- [x] 5.2 The race run of item 2.2 is in the commit message, and the break list of item 2.3 reads CAUGHT or names its unseen breaks with the reason.
|
||||
- [ ] 5.3 After the push, post on #193 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), and close it. Draft:
|
||||
|
||||
> The subscription reconcile now takes the pool row lock before it changes a provision's status or its pool's positions, in all three branches: after the subscription's own lock and before the suspension, the resume or the floor restoration, so an operator act on the same pool and a status change run one after the other. A test runs a suspension, and a recovery, while a grant on the same pool is uncommitted, and checks the pool's stored entitlements against its active provisions. The suspended branch's entry under "Where the paths differ" in `docs/database-locks.md` is gone.
|
||||
>
|
||||
> Commit: …
|
||||
|
||||
- [ ] 5.4 The archive waits for the maintainer's go; the delta carries the spec change, and no Purpose needs an edit a delta cannot make. `displaced-subscription-waits` archives after this change.
|
||||
Reference in New Issue
Block a user