From f56ebd0828833fff938605eee7f9ad3660da3100 Mon Sep 17 00:00:00 2001 From: Jakub Zych Date: Fri, 16 Oct 2026 14:06:44 +0200 Subject: [PATCH] docs(18.1): add code review report --- .../18.1-REVIEW-DISPOSITION.md | 18 + .../18.1-REVIEW.md | 537 +++++++----------- 2 files changed, 223 insertions(+), 332 deletions(-) create mode 100644 .planning/phases/18.1-payment-gateway-subscriptions/18.1-REVIEW-DISPOSITION.md diff --git a/.planning/phases/18.1-payment-gateway-subscriptions/18.1-REVIEW-DISPOSITION.md b/.planning/phases/18.1-payment-gateway-subscriptions/18.1-REVIEW-DISPOSITION.md new file mode 100644 index 0000000..d73095c --- /dev/null +++ b/.planning/phases/18.1-payment-gateway-subscriptions/18.1-REVIEW-DISPOSITION.md @@ -0,0 +1,18 @@ +# Phase 18.1: Code Review Disposition + +| Finding | Severity | Disposition | Source | +|---------|----------|-------------|--------| +| WR-01 | warning | open | - | +| WR-02 | warning | open | - | +| WR-03 | warning | open | - | +| WR-04 | warning | open | - | +| WR-05 | warning | open | - | +| WR-06 | warning | open | - | +| WR-07 | warning | open | - | +| IN-01 | info | open | - | +| IN-02 | info | open | - | +| IN-03 | info | open | - | +| IN-04 | info | open | - | +| IN-05 | info | open | - | + +_Recorded 2026-10-16T12:06:44Z after the gap-closure re-review (18.1-07..09)._ diff --git a/.planning/phases/18.1-payment-gateway-subscriptions/18.1-REVIEW.md b/.planning/phases/18.1-payment-gateway-subscriptions/18.1-REVIEW.md index d39f053..5f335e9 100644 --- a/.planning/phases/18.1-payment-gateway-subscriptions/18.1-REVIEW.md +++ b/.planning/phases/18.1-payment-gateway-subscriptions/18.1-REVIEW.md @@ -1,434 +1,307 @@ --- phase: 18.1-payment-gateway-subscriptions -reviewed: 2026-10-14T19:22:09Z +reviewed: 2026-10-16T12:04:37Z depth: standard -files_reviewed: 96 +review_kind: re-review after gap closure (plans 18.1-07, 18.1-08, 18.1-09) +files_reviewed: 44 files_reviewed_list: - - summercms.go/admin/src/components/form/PreviewField.vue - - summercms.go/admin/src/components/form/fields/TaglistField.vue - - summercms.go/admin/src/components/form/registry.ts - - summercms.go/admin/src/components/reorder/ReorderBranch.vue - - summercms.go/admin/src/components/reorder/tree.ts - - summercms.go/admin/src/views/ReorderView.vue - - summercms.go/modules/cabana/admin_openapi.go + - summercms.go/modules/cabana/bulk_detached_test.go - summercms.go/modules/cabana/crud.go - - summercms.go/modules/cabana/field_file.go - - summercms.go/modules/cabana/field_taglist.go - - summercms.go/modules/cabana/form_schema.go - - summercms.go/modules/cabana/list_schema.go - - summercms.go/modules/cabana/registry.go - - summercms.go/modules/cabana/relation_child.go - - summercms.go/modules/cabana/relation_form.go - - summercms.go/modules/cabana/reorder.go - - summercms.go/modules/cabana/schema_types.go - - summercms.go/modules/cabana/settings.go + - summercms.go/modules/pact/capabilities.go + - summercms.go/scripts/check-phase10.sh - summercms.go/scripts/check-phase18.1.sh - - sm-paymentgatewaysubscriptions-plugin/.gitignore - - sm-paymentgatewaysubscriptions-plugin/admin.go - - sm-paymentgatewaysubscriptions-plugin/admin_navigation.go - - sm-paymentgatewaysubscriptions-plugin/admin_permissions.go - - sm-paymentgatewaysubscriptions-plugin/api/errors.go + - sm-paymentgatewaysubscriptions-plugin/admin_sync_detached_test.go - sm-paymentgatewaysubscriptions-plugin/api/json.go - - sm-paymentgatewaysubscriptions-plugin/api/plans.go - sm-paymentgatewaysubscriptions-plugin/api/subscription.go - - sm-paymentgatewaysubscriptions-plugin/api/urls.go - - sm-paymentgatewaysubscriptions-plugin/assets/js/fields.js - - sm-paymentgatewaysubscriptions-plugin/boot.go + - sm-paymentgatewaysubscriptions-plugin/apply_events_test.go + - sm-paymentgatewaysubscriptions-plugin/checkout_guard_test.go - sm-paymentgatewaysubscriptions-plugin/classes/apply.go + - sm-paymentgatewaysubscriptions-plugin/classes/lock.go - sm-paymentgatewaysubscriptions-plugin/classes/service.go - sm-paymentgatewaysubscriptions-plugin/classes/user_subscribable.go - - sm-paymentgatewaysubscriptions-plugin/config/config.yaml - sm-paymentgatewaysubscriptions-plugin/contracts/gateway.go - - sm-paymentgatewaysubscriptions-plugin/contracts/registry.go - sm-paymentgatewaysubscriptions-plugin/contracts/subscriber.go - - sm-paymentgatewaysubscriptions-plugin/controllers/admin_registry.go - - sm-paymentgatewaysubscriptions-plugin/controllers/base.go - sm-paymentgatewaysubscriptions-plugin/controllers/subscriptionplans.go - - sm-paymentgatewaysubscriptions-plugin/controllers/subscriptionplans/config_form.yaml - - sm-paymentgatewaysubscriptions-plugin/controllers/subscriptionplans/config_list.yaml - - sm-paymentgatewaysubscriptions-plugin/controllers/subscriptionplans/config_reorder.yaml - - sm-paymentgatewaysubscriptions-plugin/controllers/subscriptions.go - - sm-paymentgatewaysubscriptions-plugin/controllers/subscriptions/_subscription_plan.htm - - sm-paymentgatewaysubscriptions-plugin/controllers/subscriptions/_subscription_subscriber.htm - - sm-paymentgatewaysubscriptions-plugin/controllers/subscriptions/config_filter.yaml - - sm-paymentgatewaysubscriptions-plugin/controllers/subscriptions/config_form.yaml - - sm-paymentgatewaysubscriptions-plugin/controllers/subscriptions/config_list.yaml - - sm-paymentgatewaysubscriptions-plugin/events/events.go - - sm-paymentgatewaysubscriptions-plugin/field_types.go - - sm-paymentgatewaysubscriptions-plugin/go.mod - - sm-paymentgatewaysubscriptions-plugin/go.work - - sm-paymentgatewaysubscriptions-plugin/models/feature_list.go - - sm-paymentgatewaysubscriptions-plugin/models/filters.go - - sm-paymentgatewaysubscriptions-plugin/models/processed_webhook_event.go - - sm-paymentgatewaysubscriptions-plugin/models/registry.go + - sm-paymentgatewaysubscriptions-plugin/core_smoke_test.go + - sm-paymentgatewaysubscriptions-plugin/customer_api_authz_test.go + - sm-paymentgatewaysubscriptions-plugin/customer_api_smoke_test.go + - sm-paymentgatewaysubscriptions-plugin/fixture_plugin_test.go + - sm-paymentgatewaysubscriptions-plugin/models/checkout_price_test.go - sm-paymentgatewaysubscriptions-plugin/models/subscription.go - - sm-paymentgatewaysubscriptions-plugin/models/subscription/columns.yaml - - sm-paymentgatewaysubscriptions-plugin/models/subscription/fields.yaml - sm-paymentgatewaysubscriptions-plugin/models/subscription_plan.go - - sm-paymentgatewaysubscriptions-plugin/models/subscriptionplan/columns.yaml - - sm-paymentgatewaysubscriptions-plugin/models/subscriptionplan/fields.yaml - - sm-paymentgatewaysubscriptions-plugin/plugin.go - - sm-paymentgatewaysubscriptions-plugin/routes.go - - sm-paymentgatewaysubscriptions-plugin/updates/202610150001_create_golem15_pgsub_baseline.go - - sm-paymentgatewaysubscriptions-plugin/updates/202610150002_add_stripe_customer_id_to_users.go - - sm-paymentgatewaysubscriptions-plugin/updates/registry.go - - sm-pgstripe-plugin/client.go - - sm-pgstripe-plugin/config/config.yaml - - sm-pgstripe-plugin/console/import_winter.go - - sm-pgstripe-plugin/go.mod - - sm-pgstripe-plugin/go.work - - sm-pgstripe-plugin/models/settings.go - - sm-pgstripe-plugin/models/settings/fields.yaml - - sm-pgstripe-plugin/subscriptions/config/config.yaml + - sm-paymentgatewaysubscriptions-plugin/plan_resolution_test.go + - sm-paymentgatewaysubscriptions-plugin/service_api_test.go + - sm-paymentgatewaysubscriptions-plugin/sync_subscription_test.go - sm-pgstripe-plugin/subscriptions/decode.go + - sm-pgstripe-plugin/subscriptions/decode_test.go + - sm-pgstripe-plugin/subscriptions/fresh_state_smoke_test.go - sm-pgstripe-plugin/subscriptions/gateway.go + - sm-pgstripe-plugin/subscriptions/gateway_hardening_test.go + - sm-pgstripe-plugin/subscriptions/gateway_test.go + - sm-pgstripe-plugin/subscriptions/host_test.go - sm-pgstripe-plugin/subscriptions/job.go - - sm-pgstripe-plugin/subscriptions/plugin.go - - sm-pgstripe-plugin/subscriptions/sync_plan.go - - sm-pgstripe-plugin/subscriptions/webhook.go - - sm-pgstripe-plugin/updates/202610150001_add_subscription_webhook_secrets.go - - vue-paymentgateway-layer/.playground/app/pages/subscriptions/index.vue - - vue-paymentgateway-layer/app/assets/css/pg-base.css + - sm-pgstripe-plugin/subscriptions/job_test.go + - sm-pgstripe-plugin/subscriptions/ordering_test.go + - sm-pgstripe-plugin/subscriptions/stripefake_test.go - vue-paymentgateway-layer/app/components/Pg/ManageSubscriptionButton.vue - vue-paymentgateway-layer/app/components/Pg/MySubscription.vue - - vue-paymentgateway-layer/app/components/Pg/Plans.vue - vue-paymentgateway-layer/app/components/Pg/SubscribeButton.vue - - vue-paymentgateway-layer/app/composables/usePlans.ts - - vue-paymentgateway-layer/app/composables/useSubscription.ts - - vue-paymentgateway-layer/app/composables/useSubscriptionsApi.ts - vue-paymentgateway-layer/app/utils/subscribe.ts - - vue-paymentgateway-layer/app/utils/subscriptionsClient.ts - - vue-paymentgateway-layer/nuxt.config.ts - - vue-paymentgateway-layer/package.json + - vue-paymentgateway-layer/tests/subscribe.test.ts + - vue-paymentgateway-layer/tests/subscriptionComponents.test.ts findings: - critical: 2 + critical: 0 warning: 7 - info: 8 - total: 17 + info: 5 + total: 12 status: issues_found --- -# Phase 18.1: Code Review Report +# Phase 18.1: Code Review Report (re-review after gap closure) -**Reviewed:** 2026-10-14T19:22:09Z +**Reviewed:** 2026-10-16T12:04:37Z **Depth:** standard -**Files Reviewed:** 96 (four repositories; every path below carries its repository prefix) +**Files Reviewed:** 44 (four repositories; every path carries its repository prefix) **Status:** issues_found ## Summary -The review covered the cabana taglist field and flat reorder (framework and admin SPA), the pgsub core, its admin screens and customer API, the Stripe subscription gateway with its webhook, River job and plan sync, and the Nuxt layer pieces. +This is the re-review after the gap closure. Plans 18.1-07, 18.1-08 and 18.1-09 targeted CR-01, CR-02, WR-01, WR-02, WR-03, WR-04, WR-06 and WR-07 of the first review (2026-10-14). Scope: +- summercms.go: commits after `929cfd8`; +- pgsub: commits after `472b75b`; +- pgstripe: commits after `baf3c55`; +- the layer: commits after `30046f5`. -The framework half is in good shape. The taglist save path normalises and checks values at every save site before any transaction. The flat reorder writes only the sort column, and a submitted parent is refused. The webhook verifies the signature on the raw body before decoding it, uses a separate secret per endpoint, and claims the processed-event marker inside the transaction that applies the event. The customer API never takes a subscriber id, a subscription id or a redirect URL from the request. +Called code outside the list was read where a finding depends on it: `lagoon.Transaction`, cabana's toolbar action path, `sync_plan.go`, conga's default attempts, and the pre-fix `subscriptionplans.go` and `SyncPlan`. -Two money-relevant defects block shipping: +Both blockers are closed. The webhook job now retrieves the subscription's state from Stripe outside any transaction. It does this under a per-subscription session advisory lock, and the claim-first marker transaction is unchanged. Checkout no longer falls back to another currency's price. -1. **CR-01: stale webhook data can overwrite newer data.** The webhook job applies the subscription snapshot carried in each Stripe event, with nothing to tell an old event from a new one. Stripe delivers events out of order, and River runs and retries jobs concurrently. A paid subscription can drop back to `incomplete` and lose access. A deleted subscription can come back as active. -2. **CR-02: checkout can charge a different currency or amount than the user was shown.** The customer API checks the currency against the plan's local prices. The gateway then silently falls back to `stripe_price_id`, which plan sync sets to the USD price. +The lock implementation holds up under tracing: +- **Pinned connection.** GORM's `Connection` keeps the `*sql.Conn`. The `NewDB` session keeps the `ConnPool`, and `lagoon.Transaction` begins its transaction on that same connection. +- **Unlock and discard.** The unlock runs with `context.WithoutCancel`. A failed lock or unlock marks the connection bad through `Conn.Raw` returning `driver.ErrBadConn`, so database/sql closes it and the server releases the lock. +- **Framework change.** The cabana `Detached` branch keeps the list scope, the empty and partial-selection rules and the permission check. The new field is `json:"-"` and opt-in, so existing consumers (the `users` bulk actions in the apps) compile and behave as before. +- **Layer.** The bfcache restore works in all three components, and the listener is removed on unmount. -The warnings cover: -- Stripe calls made while holding a database transaction and row locks. -- Duplicate Stripe customers and duplicate subscriptions under concurrent or repeated checkout. -- Subscriptions that are dropped after a plan is repriced. -- A grace period inherited from PHP that grants about a full unpaid period. -- A lost-update risk in SyncSubscription. -- Subscribe and manage buttons in the layer that stay disabled after the browser Back button. +No new blocker was found. The seven warnings are: +- **Residual gaps in fixed findings (WR-01, WR-04, WR-06).** `Service.Cancel` still writes a gateway-owned column without the subscription lock, so the spurious `reactivated` of CR-01 sequence 3 is still reachable through a race. The open-subscription guard cannot see unpaid Checkout Sessions. Checkout bills the last-synced amount, not the amount shown. +- **New behaviour introduced by the fixes (WR-02, WR-03, WR-05, WR-07):** + - fresh-state retrieval can fire `SubscriptionCanceled` twice; + - the detached plan sync dropped the row lock that serialised concurrent syncs of one plan; + - the strict price map makes plans in gbp, cad, aud or chf impossible to buy once they are synced; + - treating `incomplete` as open locks a user out of checkout for up to about 23 hours. -Byte compatibility: the webhook's five plain-text answers, their order, the event names, the table, column and constraint names and the permission codes match what the SUMMARYs claim. The documented exception is `text/plain` instead of Laravel's `text/html`. CR-01, CR-02 (the usd/eur/pln loop and its fallback) and WR-05 are defects inherited from the frozen PHP source. They are reported anyway, because the Go port adds concurrent workers and a new customer API that make them easier to reach. +## Resolution of the first review's findings + +| Finding | Status | Evidence | +|---|---|---| +| CR-01 stale webhook snapshots | **Resolved** (one residual race, WR-01 below) | `sm-pgstripe-plugin/subscriptions/job.go:206-244` retrieves fresh state under `classes.WithSessionLock`. `apply.go:191-195` retries an unknown deletion. Sequences 1 to 3 are covered by `TestFreshStateSmoke` and `TestEventOrdering`. `Service.Cancel` does not take the lock (WR-01). | +| CR-02 cross-currency checkout | **Resolved** for currency (amount residual, WR-06; side effect, WR-05) | `models/subscription_plan.go:129-144` `CheckoutPriceID` has no fallback. The API answers 422 at `api/subscription.go:128-131`, and the gateway refuses with `ErrNoPriceForCurrency` at `gateway.go:199-202`. The yen is dropped from `supportedCurrencies` (`gateway.go:44`). | +| WR-01 Stripe calls inside the bulk transaction | **Resolved** (regression, WR-03) | `controllers/subscriptionplans.go:55` sets `Detached: true`. The branch is at `crud.go:370-373, 412-435`. `SyncPlan(ctx, nil, plan)` saves with `context.WithoutCancel` (`service.go:597-605`), and a failed save becomes a failure line. | +| WR-02 duplicate Stripe customers | **Resolved** | `user_subscribable.go` writes the first value only and re-reads the stored one. `gateway.go:163-164` sets an idempotency key on the customer create. `ensureCustomer` stores through `WithoutCancel` (`service.go:631`). | +| WR-03 already-subscribed guard | **Partially resolved** (WR-04 and WR-07 below) | `ScopeOpen` (`models/subscription.go:114-121`), `CheckoutGuarded` under `SubscriberLockKey` (`service.go:319-344`), and a checkout idempotency key (`gateway.go:245-246`). Unpaid open sessions are still not seen. | +| WR-04 subscriptions on archived prices | **Resolved** | `apply.go:327-339` resolves the plan by price, then product, then plan code. `decode.go` carries `product_id`/`plan_code`. | +| WR-05 grace period from the advanced period end | **Open (deferred)** | `models/subscription.go:149`, unchanged and kept PHP-identical by user decision (`deferred-items.md`). Not counted again below. | +| WR-06 SyncSubscription lost update | **Resolved** | `service.go:418-488`: under the lock, `FOR UPDATE` re-read, `Select` of the gateway-owned columns, plan refreshed on the retriever path. | +| WR-07 buttons stuck after Back | **Resolved** | `subscribe.ts:57-64, 66-71, 138, 195`. All three components wire `watchPageRestore` in `onMounted` and remove it in `onBeforeUnmount`. `MySubscription` also refreshes its data. | +| IN-01 to IN-08 | **Open (deferred)** | `deferred-items.md`. Not counted again below. | ## Narrative Findings (AI reviewer) -## Critical Issues - -### CR-01: Webhook job applies stale event snapshots with no ordering guard, so a newer subscription state can be overwritten by an older event - -**Files:** -- `sm-pgstripe-plugin/subscriptions/job.go:119-147, 155-170, 187-205` -- `sm-paymentgatewaysubscriptions-plugin/classes/apply.go:28-126` (lookup and lock at 52, unconditional status write at 96, reactivated diff at 138), `apply.go:170-205` (unknown row at 183) -- `sm-paymentgatewaysubscriptions-plugin/classes/service.go:308-330` - -**Issue:** `work` maps `data.object` from the Stripe event into `contracts.Update`, and `Apply` writes that status, period and cancel flag onto the row as they are. Neither the event's `created` timestamp nor any version is decoded (the `event` struct in `decode.go:30-36` has only id, type and data), so nothing stops an older event from winning. Stripe does not guarantee delivery order, and River runs several workers and retries failed jobs with backoff. Several realistic sequences break: - -1. **Checkout race on a new subscription.** Stripe Checkout emits `customer.subscription.created` (status `incomplete`) and `customer.subscription.updated` (status `active`) within milliseconds. - - Both jobs run `SELECT ... FOR UPDATE`, which finds no row and so locks nothing, and both INSERT. - - If the `updated` job wins the insert, it stores `active` and fires `activated`. The `created` job fails on the unique index on `stripe_subscription_id`, rolls back its marker and is retried. - - On retry it finds the row and writes `incomplete` over `active`. No event fires. - - The customer has paid but `HasAccess` is false until Stripe's next update, typically the next renewal a month later. -2. **Deleted before created.** If the `created` job is retrying (for example "no plan holds price id" until an admin syncs the plan) and `customer.subscription.deleted` arrives: - - `ApplyDeleted` finds no row, logs a warning and returns `nil, nil`, so the deletion marker is committed. - - The `created` retry then inserts the row as `active`. - - The canceled subscription grants access with no end, and `activated` fires. -3. **Spurious `reactivated`.** `Service.Cancel` writes `cancel_at_period_end = true` locally (`service.go:324-325`). An older queued `customer.subscription.updated` carrying `cancel_at_period_end: false` then meets `previousCancel && !sub.CancelAtPeriodEnd && active` (`apply.go:138`) and fires `SubscriptionReactivated` for a cancellation the user never withdrew. - -PHP's job had the same snapshot semantics. The Go port adds concurrent workers and retry-with-backoff, which makes the race in sequence 1 likely on every Checkout rather than rare. - -**Fix:** Apply authoritative state rather than the snapshot, as Stripe recommends. In the job, outside the transaction, retrieve the subscription from Stripe (the gateway already has the code in `SyncSubscription`) and feed that into `Apply`, keeping the claim-first transaction. Alternatively, decode `event.created` and refuse any update older than the last one applied. That needs a stored per-subscription marker, such as a column or a side table keyed by `stripe_subscription_id`. Also treat an unknown row in `ApplyDeleted` as retryable instead of committing the marker: - -```go -// decode.go -type event struct { - ID string `json:"id"` - Type string `json:"type"` - Created int64 `json:"created"` - Data struct{ Object json.RawMessage `json:"object"` } `json:"data"` -} - -// job.go: route() for created/updated -fresh, err := p.gateway().Retrieve(ctx, s.ID) // outside tx, before lagoon.Transaction -... -_, err = svc.Apply(ctx, tx, fresh.update()) - -// apply.go: ApplyDeleted -if len(rows) == 0 { - return nil, fmt.Errorf("golem15.paymentgatewaysubscriptions: apply deleted %s: subscription not stored yet", subscriptionID) // retried -} -``` - -### CR-02: Checkout can charge a Stripe price in a different currency or amount than the customer API shows and validated - -**Files:** -- `sm-paymentgatewaysubscriptions-plugin/api/subscription.go:112-119, 140-149` -- `sm-pgstripe-plugin/subscriptions/gateway.go:128` -- `sm-paymentgatewaysubscriptions-plugin/models/subscription_plan.go:109-119` -- `sm-pgstripe-plugin/subscriptions/sync_plan.go:15, 55-69` -- `sm-paymentgatewaysubscriptions-plugin/api/json.go:62-75` - -**Issue:** `Checkout` accepts a currency when `priced(plan, currency)` and `SupportsCurrency(currency)` are both true. `priced` only looks at the local `currency_prices` and `price_cents`/`currency`. The gateway then picks the Stripe price with `StripePriceIDFor(currency)`, which silently falls back to `stripe_price_id` when `stripe_prices` has no entry for that currency. - -`SyncPlanToGateway` creates prices only for `usd`, `eur` and `pln` and sets `stripe_price_id` to the USD price. A plan whose `currency` is any other supported code (`gbp`, `cad`, `aud`, `chf`, `jpy`; the admin form accepts any 3-letter text) therefore goes like this: -- `/plans` lists it as, for example, `29.99 GBP`. -- Checkout passes the 422 gate. -- Stripe Checkout runs on the **USD** price created from the same `price_cents`: `$29.99` instead of `£29.99`. - -The same happens when `currency_prices` names a currency the plan was never synced in, or when an admin typed a `stripe_price_id` for one currency and the customer asks for another. - -JPY is wrong a second way. `money.FromMinor` divides by 100 for every currency, so `/plans` shows `¥5` for `price_cents: 500`, while a JPY price would bill `¥500`. - -**Fix:** Before calling the gateway, require a gateway price that belongs to the requested currency, and answer 422 otherwise. Never fall back across currencies: - -```go -// api/subscription.go, after the priced/SupportsCurrency check -if !gatewayPriced(plan, currency) { - writeValidation(w, map[string][]string{"currency": {msgCurrencyInvalid}}) - return -} - -func gatewayPriced(plan *models.SubscriptionPlan, currency string) bool { - if plan.StripePrices.Valid { - if id, ok := plan.StripePrices.Data[strings.ToLower(currency)]; ok && id != "" { - return true - } - return false // synced plan without this currency: never fall back to stripe_price_id - } - // unsynced plan with a hand-entered price id: only its own currency - return plan.StripePriceID != nil && *plan.StripePriceID != "" && strings.EqualFold(plan.Currency, currency) -} -``` - -Also make `CreateCheckoutSession` refuse the fallback in the same way (`ErrNoPriceForCurrency`), because other callers of `Service.Checkout` hit the same path. Either drop `jpy` from `supportedCurrencies` or handle zero-decimal currencies in `planPrices`. - ## Warnings -### WR-01: Plan sync calls Stripe while holding the bulk-action transaction and row locks; a rollback loses the created Stripe ids +### WR-01: `Service.Cancel` writes `cancel_at_period_end` without the subscription lock, so a racing webhook job still fires a spurious `SubscriptionReactivated` (CR-01 sequence 3 residual) **Files:** -- `sm-paymentgatewaysubscriptions-plugin/controllers/subscriptionplans.go:130-153, 155-178` -- `sm-paymentgatewaysubscriptions-plugin/classes/service.go:362-388` +- `sm-paymentgatewaysubscriptions-plugin/classes/service.go:373-395` +- `sm-pgstripe-plugin/subscriptions/job.go:219-243` +- `sm-paymentgatewaysubscriptions-plugin/classes/apply.go:143` +- `sm-paymentgatewaysubscriptions-plugin/classes/lock.go:25-28` -**Issue:** cabana's `BulkAction` opens a transaction, locks the selected plans with `FOR UPDATE` and runs `runSyncSelected` inside it (`summercms.go/modules/cabana/crud.go:369-388`). `syncOne` then makes one product call and up to three price list, create and archive calls per plan, all while the transaction and the row locks are held. This contradicts the `contracts.Gateway` doc ("no transaction is held across a provider call"). +**Issue:** The lock's documented rule is "every writer of the gateway state of that subscription takes it", and plan 07 made that the established pattern. `Service.Cancel` (used by `POST /subscription/cancel`) calls Stripe and then runs `UPDATE ... SET cancel_at_period_end = true` without `SubscriptionLockKey`. One interleaving reproduces the original sequence 3: +1. A webhook job for an unrelated `customer.subscription.updated` takes the lock and retrieves the subscription from Stripe (`cancel_at_period_end: false`). +2. The user cancels. Stripe sets the flag, and the local UPDATE commits `true`. +3. The job opens its transaction. `Apply` reads the row `FOR UPDATE` with `previousCancel = true`, writes `false` and fires `SubscriptionReactivated` (`apply.go:143`). -Stripe can create a product and its prices and then the transaction can still roll back: -- the admin's request is cancelled or times out (the context is shared); -- a lock wait or statement times out; -- the commit fails. - -In each case the saved `stripe_product_id`/`stripe_prices` are lost, and the next sync creates a duplicate product. `runSyncAll` does the same per plan inside `lagoon.Transaction`, discards the transaction error (`_ = lagoon.Transaction(...)`, line 173), and has already counted the plan as `synced`. - -**Fix:** -1. Make the bulk action collect only the plan ids. -2. Run each plan's gateway call outside any transaction, then save the ids in a short transaction of its own, the way `ensureCustomer` already does. -3. In `runSyncAll`, check the returned error and count a failed commit as a failure: +Stripe's own update event later restores `true`, but the listeners have already received a reactivation the user never asked for. Until then the row shows no scheduled cancellation. The window is the time between the job's Stripe response and its `SELECT ... FOR UPDATE`. It is short, but nothing closes it. +**Fix:** Take the subscription lock around the gateway call and the local write: ```go -if err := lagoon.Transaction(ctx, db, func(ctx context.Context, tx *gorm.DB) error { ... }); err != nil { - tally.failures = append(tally.failures, plan.Code+": save failed") +func (s *Service) Cancel(ctx context.Context, sub *models.Subscription, atPeriodEnd bool) error { + ... + if sub.StripeSubscriptionID == nil || *sub.StripeSubscriptionID == "" { /* existing gateway refusal */ } + return WithSessionLock(ctx, s.db, SubscriptionLockKey(*sub.StripeSubscriptionID), func(conn *gorm.DB) error { + if err := gw.CancelSubscription(ctx, sub, atPeriodEnd); err != nil { ... } + if !atPeriodEnd { return nil } + return conn.WithContext(ctx).Model(&models.Subscription{}).Where("id = ?", sub.ID). + Update("cancel_at_period_end", true).Error + }) } ``` +Add a `TestEventOrdering` case that blocks the fake's retrieve, runs `Cancel` and then releases the retrieve. -### WR-02: Concurrent or retried checkouts create duplicate Stripe customers, and the last write wins +### WR-02: With fresh-state retrieval, `SubscriptionCanceled` fires twice when a created/updated job runs after Stripe canceled the subscription but before its deletion job **Files:** -- `sm-paymentgatewaysubscriptions-plugin/classes/service.go:392-411` -- `sm-paymentgatewaysubscriptions-plugin/classes/user_subscribable.go:89` -- `sm-pgstripe-plugin/subscriptions/gateway.go:76-108` +- `sm-pgstripe-plugin/subscriptions/job.go:221-241` +- `sm-paymentgatewaysubscriptions-plugin/classes/apply.go:161-165, 208-211` +- `sm-pgstripe-plugin/subscriptions/ordering_test.go:115-152`, `fresh_state_smoke_test.go:130-170` -**Issue:** `ensureCustomer` checks `sub.CustomerID()` on a subscriber loaded before the call, creates a Stripe customer without an idempotency key, and then runs `UPDATE users SET stripe_customer_id = ? WHERE id = ?` unconditionally. Two parallel checkout requests (a double click in two tabs, or a client retry after a timeout) each create a customer, and the second UPDATE overwrites the first. If the user completes the session tied to the overwritten customer: -- the subscription lives under a customer the app no longer knows; -- `POST /subscription/portal` opens the other customer's empty portal; -- the user cannot manage or cancel that subscription from the site. +**Issue:** A created or updated job now applies Stripe's current status. If Stripe already canceled the subscription, that status is `canceled`. `fireStateChangeEvents` sees a change to canceled and fires `SubscriptionCanceled`. The deletion job then runs `ApplyDeleted`, which fires `SubscriptionCanceled` unconditionally (`apply.go:208-211`, the PHP rule). Two sequences lead there: +- **CR-01 sequence 2, the case plan 07 targeted:** the deletion is retried with `ErrSubscriptionNotStored`, the late created event stores `canceled` and fires once, and the retried deletion fires again. +- **An immediate cancel while an update job is queued, retrying, or simply wins the lock first.** -**Fix:** Write the id only when none is stored, and re-read on conflict. Also pass an idempotency key derived from alias and id to `V1Customers.Create`: +Under PHP's snapshot semantics a created or updated snapshot practically never carried `canceled`, so the unconditional fire was harmless. The fresh path changes that. Both ordering tests assert `activated == 0` but never count `canceled`, so the double fire passes the gate. Listeners such as a "your subscription ended" mail or an access revocation that writes an audit row run twice. -```go -res := tx.Exec(`UPDATE users SET stripe_customer_id = ? WHERE id = ? AND (stripe_customer_id IS NULL OR stripe_customer_id = '')`, id, u.id) -if res.RowsAffected == 0 { /* re-read the stored id and use it; optionally delete the orphan customer */ } -``` +**Fix:** In `ApplyDeleted`, fire only on a transition (`previous := sub.StripeStatus` before the write; fire when `previous != models.StatusCanceled`). Record it as a deviation next to the D-21 amendment. Then add `rec.Count(events.SubscriptionCanceledEventName) == 1` to both `deleted_before_created` subtests. -```go -params.SetIdempotencyKey("pgsub-customer-" + s.MorphAlias() + "-" + strconv.FormatUint(uint64(s.Key()), 10)) -``` - -### WR-03: Checkout's "Already subscribed" guard misses non-active live subscriptions and is racy, so users can be billed twice +### WR-03: The detached plan sync lost the row lock that serialised concurrent syncs of one plan; parallel syncs create duplicate Stripe products and prices **Files:** -- `sm-paymentgatewaysubscriptions-plugin/api/subscription.go:87-93, 198-205` +- `sm-paymentgatewaysubscriptions-plugin/controllers/subscriptionplans.go:48-57, 86-101, 162-176` +- `sm-paymentgatewaysubscriptions-plugin/classes/service.go:584-611` +- `summercms.go/modules/cabana/crud.go:412-435` +- `sm-pgstripe-plugin/subscriptions/sync_plan.go:92-117` (called) -**Issue:** Checkout refuses with 409 only when `Active()` (active or trialing) finds a row. A user whose subscription is `past_due` (still in grace and still billed by Stripe's retries), `unpaid`, `incomplete` or `paused` can start a second Checkout. A completed second Checkout gives two live Stripe subscriptions on one customer, both charging. +**Issue:** Before plan 08, `sync_to_stripe` ran inside cabana's transaction with the selected plans locked `FOR UPDATE`, so two admins, or two tabs, syncing the same plan were serialised. The detached branch loads the plans without locks (`loadScoped(..., false)`), and `SyncPlan` takes no lock of its own. Two concurrent syncs of a plan with no `stripe_product_id` each call `V1Products.Create`, which carries no idempotency key. Each then lists the prices by lookup key, finds none, and creates its own. The last `Updates` wins, and the other product and its prices stay active and orphaned in Stripe. This is the duplicate-product outcome WR-01 set out to prevent. -The check is also not atomic. Two parallel requests both pass it and both get a session URL; the layer's busy guard protects only one tab. `POST /subscription/cancel` then cancels only the newest active row, leaving the other one billing. - -**Fix:** -- Refuse checkout while any subscription of the subscriber is in a non-terminal status (anything except `canceled` and `incomplete_expired`). Send `past_due`/`unpaid` users to the portal instead. -- Serialize per subscriber, either with `SELECT ... FOR UPDATE` on the users row or `pg_advisory_xact_lock(hashtext('pgsub:user:'||id))` around the check, or with a Stripe idempotency key per user and plan for a short window. - -### WR-04: A subscription whose price was archived by a later sync (or whose plan code changed) is never stored, although the customer is billed - -**Files:** -- `sm-paymentgatewaysubscriptions-plugin/classes/apply.go:72-85, 300-323` -- `sm-pgstripe-plugin/subscriptions/sync_plan.go:141-151` - -**Issue:** `planForPrice` resolves a plan only from the current `stripe_price_id` or the current `stripe_prices` values. Re-syncing a repriced plan archives the old price and replaces the ids in `stripe_prices`. A Checkout session created before the resync (valid for 24 h) and completed after it produces a subscription on the old price. Its `customer.subscription.created`/`updated` events then fail with "no plan holds price id", are retried for conga's default attempts and are discarded. The webhook already answered 200, so Stripe never redelivers. The customer is charged and the app has no subscription row. Changing a plan's `code` (editable in the form) has the same effect for new checkouts on the old lookup key. - -**Fix:** Fall back to the `plan_code` metadata the gateway writes on every Checkout subscription (`gateway.go:153`). Carry it in `SubscriptionArgs`, add `PlanCode` to `contracts.Update`, and in `Apply` use `ActivePlanByCode` (or any plan by code) when `planForPrice` returns nil: +The toolbar `sync_all_to_stripe` never locked. It has no confirm dialog (`subscriptionplans.go:59-63`), so a double click starts two full syncs. +**Fix:** Serialise per plan with the existing helper, and sync from a fresh copy of the row: ```go -if plan == nil && u.PlanCode != "" { - plan, err = planByCode(db, u.PlanCode) +func (s *Service) SyncPlan(ctx context.Context, tx *gorm.DB, plan *models.SubscriptionPlan) (map[string]string, error) { + ... + if tx == nil { + var out map[string]string + err := WithSessionLock(ctx, s.db, lockKeyPrefix+"plan:"+strconv.FormatUint(uint64(plan.ID), 10), func(conn *gorm.DB) error { + if err := conn.WithContext(ctx).First(plan, plan.ID).Error; err != nil { return err } // fresh stripe ids + var err error + out, err = s.syncPlanLocked(ctx, conn, plan) + return err + }) + return out, err + } + ... } ``` +Also give `V1Products.Create` an idempotency key over the plan id, code and name, in the same window as the customer key. -### WR-05: The grace period is computed from the already advanced period end, so a past_due subscription keeps access for about a full unpaid period plus grace_period_days +### WR-04: The open-subscription guard cannot see unpaid Checkout Sessions, so a subscriber can still complete two Checkouts and be billed twice (WR-03 partially resolved) **Files:** -- `sm-paymentgatewaysubscriptions-plugin/models/subscription.go:137-142` +- `sm-paymentgatewaysubscriptions-plugin/classes/service.go:319-344` +- `sm-pgstripe-plugin/subscriptions/gateway.go:245-246` +- `sm-paymentgatewaysubscriptions-plugin/checkout_guard_test.go` ("two checkouts of one subscriber never overlap and share one customer") -**Issue:** `InGracePeriod` grants access while `now < current_period_end + graceDays`. When Stripe renews, it moves `current_period_start`/`current_period_end` to the new period before the renewal invoice is paid. A `past_due` subscription therefore already carries the end of the period that was not paid for. With monthly billing and the default 7 days, `has_access` stays true for about 37 days after the failed payment, not 7. PHP had the same formula (inherited), but `has_access` is now exposed through the new customer API and `Service.HasAccess`. +**Issue:** `CheckoutGuarded` refuses only when a stored row is open. A Checkout Session creates no row until the customer pays and the webhook is applied. The subscriber lock serialises the two requests but does not make the second one fail: the test named above asserts that both checkouts succeed. The Stripe idempotency key returns the same session only when every parameter matches within the same 10-minute window. So these still produce two open sessions for one customer: +- two tabs with different plans or currencies; +- the same plan more than 10 minutes apart (sessions stay valid for 24 h); +- a second click in the gap between payment and webhook processing. -**Fix:** Base grace on the start of the unpaid period (or on the timestamp of the move to `past_due`): +Completing both produces two live Stripe subscriptions, both billing. That is the outcome WR-03 described. +**Fix:** Before creating a new session for a customer, expire that customer's open subscription-mode sessions inside the subscriber lock (Stripe: list Checkout Sessions with `customer` and `status=open`, then `Expire`). Optionally, when `Apply` stores a new subscription for a subscriber who already has another open row, log at Error or fire a dedicated event so the duplicate is caught. + +### WR-05: Once synced, plans priced in gbp, cad, aud or chf can no longer be bought, although `/plans` lists them and `SupportsCurrency` accepts them + +**Files:** +- `sm-paymentgatewaysubscriptions-plugin/models/subscription_plan.go:129-144` +- `sm-pgstripe-plugin/subscriptions/sync_plan.go:15, 55-69` (called) +- `sm-pgstripe-plugin/subscriptions/gateway.go:44` +- `sm-paymentgatewaysubscriptions-plugin/api/subscription.go:121-131` + +**Issue:** `CheckoutPriceID` lets a non-empty `stripe_prices` map decide alone. Plan sync fills that map only for usd, eur and pln, and it overwrites `stripe_price_id` with the USD price. A plan with `currency: GBP` therefore: +1. sells correctly while it carries a hand-entered GBP `stripe_price_id`; +2. after an admin presses "Sync to Stripe" (reported as successful), loses its GBP price id and gets a map without `gbp`; +3. answers 422 "unavailable" to every checkout from then on, while `/plans` still shows `29.99 GBP` with a Subscribe button. + +The same happens to any `currency_prices` key outside the three sync currencies. This is no longer a wrong charge, but the strict map combined with the unchanged fixed sync list makes a supported currency unsellable without any visible error. + +**Fix:** Sync every currency the plan prices that the gateway supports, instead of the fixed list: ```go -return now.Before(s.CurrentPeriodStart.AddDate(0, 0, graceDays)) +currencies := []string{strings.ToLower(plan.Currency)} +for code := range plan.CurrencyPrices.Data { currencies = append(currencies, strings.ToLower(code)) } +// dedupe, keep only g.SupportsCurrency(code), then upsertPrice for each ``` +At least report the currencies the plan lists but has no price for as a sync failure line. Alternatively, have `/plans` mark a price unbuyable when `CheckoutPriceID` is empty. -If PHP parity must be kept, document the behaviour in the README and the API contract. - -### WR-06: SyncSubscription saves a stale, unlocked row in full and never refreshes the plan +### WR-06: Checkout bills the amount of the last sync, not the price the API shows; an edit of `price_cents` or `currency_prices` without a re-sync charges the old amount (CR-02 amount residual) **Files:** -- `sm-paymentgatewaysubscriptions-plugin/classes/service.go:335-352` -- `sm-pgstripe-plugin/subscriptions/gateway.go:263-306` +- `sm-paymentgatewaysubscriptions-plugin/models/subscription_plan.go:129-144` +- `sm-paymentgatewaysubscriptions-plugin/api/json.go:113-126` +- `sm-paymentgatewaysubscriptions-plugin/api/subscription.go:121-131` -**Issue:** The caller hands in a row loaded earlier without a lock. The gateway changes some fields in memory, and the service then runs `Save(sub)`, which writes every column. Changes that `Apply` made between the load and the save are overwritten with the stale values: `subscription_plan_id` after a plan switch, `subscriber_*`, `stripe_customer_id` and `canceled_at`. The gateway also ignores the retrieved price, so a plan change made in the portal is never reconciled by a sync. +**Issue:** `/plans` renders `currency_prices`/`price_cents` from the row (`planPrices`). The checkout gate (`priced`) checks the same local amounts. The Stripe price comes from the `stripe_prices` id, whose `unit_amount` was fixed at the last sync. Nothing ties the two together: there is no save hook on the plan form, and `CheckoutPriceID` never compares amounts. An admin who raises a plan from 29.99 to 39.99 and forgets to press Sync leaves customers who are shown 39.99 being billed 29.99, and the reverse after a price cut. CR-02 was "charge a different currency or amount than the user was shown". The currency half is fixed; the amount half is not. -**Fix:** -1. Re-load the row `FOR UPDATE` inside the short transaction. -2. Copy only the gateway-owned fields onto it. -3. Map the retrieved price to a plan with `planForPrice`. +**Fix:** One of: +1. Store the synced amount per currency next to the id (for example `stripe_prices` values as `{id, amount}`, or a parallel `stripe_price_amounts` map). Have `CheckoutPriceID` return "" when `PriceCentsFor(currency)` differs, which gives a 422 and an admin log line naming the plan. +2. In the gateway, `V1Prices.Retrieve(priceID)` before the session and refuse with `ErrNoPriceForCurrency` when `UnitAmount != plan.PriceCentsFor(currency)`, `Currency` differs or `Active` is false. +3. Clear `stripe_prices`/`stripe_price_id` in a `FormBeforeUpdate` when a price field changes. -Or `Select(...)` only the columns the gateway sets. - -### WR-07: After a redirect the subscribe and manage buttons stay disabled for good, including after the browser Back button +### WR-07: Treating `incomplete` as open locks a user out of checkout for up to about 23 hours after an abandoned or failed first payment **Files:** -- `vue-paymentgateway-layer/app/utils/subscribe.ts:74, 85, 115, 125, 134` +- `sm-paymentgatewaysubscriptions-plugin/models/subscription.go:114-121` +- `sm-paymentgatewaysubscriptions-plugin/api/subscription.go:92-98` +- `sm-paymentgatewaysubscriptions-plugin/classes/service.go:325-332` -**Issue:** `state.redirecting` is set to true before `location.assign` and never reset. When the user goes to Stripe Checkout or the portal and presses Back, browsers with a back/forward cache restore the page with its JavaScript state. The button then shows "Subscribing..." and stays disabled, and `cancel()` returns null without a request. The only way out is a full reload. Leaving Checkout with Back is a common path. +**Issue:** `ScopeOpen` excludes only `canceled` and `incomplete_expired`. A subscription whose first invoice failed, or which needs 3-D Secure the customer abandoned, is stored as `incomplete` from the `customer.subscription.created` event. Stripe expires it after about 23 h. Until then every checkout answers 409, and the layer offers only the billing portal (`SubscribeButton.vue:56`). The portal is not a reliable way to finish the first payment of an incomplete subscription, so a customer with a declined card cannot retry with another card the same day. The other statuses that block (`past_due`, `unpaid`, `paused`) have an invoice the portal can settle; `incomplete` is the exception. -**Fix:** Reset the state when the page is shown again: - -```ts -if (typeof window !== 'undefined') { - window.addEventListener('pageshow', (e) => { if ((e as PageTransitionEvent).persisted) { state.redirecting = false; state.busy = false } }) -} -``` - -Register the listener in the components' `onMounted` and remove it in `onBeforeUnmount`. +**Fix:** Leave `incomplete` out of the checkout guard. Stripe expires it, and an incomplete subscription never charges again on its own. If double billing through a later-completed incomplete subscription is a concern, have the guarded checkout cancel the subscriber's `incomplete` subscriptions at Stripe, inside the lock, before it creates the new session. ## Info -### IN-01: The per-user throttle key of the customer API never applies - -**File:** `sm-paymentgatewaysubscriptions-plugin/routes.go:54-60, 77-82` - -**Issue:** `throttle:paymentgatewaysubscriptions-api` is group middleware, and it runs before `pgapi.RequireJWT`. Nothing sets the bouncer principal until the jwt guard runs (`bouncer/jwt.go:63`), so `bouncer.User` is always empty when the bucket key is computed. Every request is keyed by client IP, and the documented "120 per minute per frontend user" is not what happens. The order API has the same pattern. - -**Fix:** Run the guard before the throttle on the four Bearer routes. Alternatively, drop the per-user branch and correct the comment and README. - -### IN-02: The `metadata_app` filter lets untagged events from other applications through - -**File:** `sm-pgstripe-plugin/subscriptions/job.go:136` - -**Issue:** `want != "" && got != "" && got != want` skips only events tagged with a different app. On a shared Stripe account, an application that writes no tag (including the PHP apps before cutover) gets every subscription event applied. Each fails on "no plan holds price id" and is retried until it is discarded, which floods the error logs. - -**Fix:** When `metadata_app` is set, skip (and mark) untagged events too, or make that behaviour a config option. - -### IN-03: The admin sync result shows the raw Stripe error text - -**File:** `sm-paymentgatewaysubscriptions-plugin/controllers/subscriptionplans.go:86-92` - -**Issue:** `cause.Error()` (the Stripe message) is put into the toast. Elsewhere the gateway deliberately logs only Stripe's type, code and request id, because "the message may echo request data". - -**Fix:** Map `*stripe.Error` to a localized line naming the code, or reuse the `failed()` attributes. - -### IN-04: The plans table decides "free" differently from the server - -**File:** `vue-paymentgateway-layer/app/components/Pg/Plans.vue:35-38, 78-81` - -**Issue:** The server treats a plan as free when `price_cents == 0` (checkout answers 422, `api/subscription.go:105-108`). The layer treats it as free when every listed price is zero. A plan with `price_cents: 0` and non-zero `currency_prices` shows a Subscribe button that always fails with "unavailable". A plan with mixed zero and non-zero prices shows `0.00` with a Subscribe button. - -**Fix:** Expose a `free` boolean in `PlanJSON`, or apply the same rule on both sides. - -### IN-05: TaglistField leaves a typed separator in the input when the draft is empty - -**File:** `summercms.go/admin/src/components/form/fields/TaglistField.vue:86-99` - -**Issue:** Typing `,` (or a space in space mode) into an empty input leaves `draft` at `''`, which is unchanged, so Vue does not patch the input. The separator stays visible until the next keystroke or blur. - -**Fix:** Write the remaining text back to the element (`(event.target as HTMLInputElement).value = rest`) after splitting. - -### IN-06: Taglist required message is not localized; `customTags: false` without options is accepted at boot - -**File:** `summercms.go/modules/cabana/field_taglist.go:300, 314, 252-263` - -**Issue:** The required error is a hard-coded English string, while the other taglist errors use phrase keys. A field with `customTags: false` and no `options` compiles and then refuses every tag at save time. - -**Fix:** Use the localized required message. Fail boot when `customTags: false` has no options. - -### IN-07: Test-mode and live-mode Stripe customer ids share `users.stripe_customer_id` +### IN-01: The `ErrSubscriptionNotStored` retry lasts only conga's 3 attempts; a deletion of a subscription that is never stored ends as a discarded job with error logs **Files:** -- `sm-paymentgatewaysubscriptions-plugin/updates/202610150002_add_stripe_customer_id_to_users.go:18-21` -- `sm-pgstripe-plugin/subscriptions/gateway.go:80-82` +- `sm-paymentgatewaysubscriptions-plugin/classes/apply.go:191-195` +- `summercms.go/modules/conga/client.go:18-19` (called) -**Issue:** A customer created while `test_mode` is on is stored in the same column and reused after switching to live. Live checkout and portal calls then fail with "No such customer" and the user gets a permanent 502. Nothing recovers from it. +**Issue:** River retries the deletion 3 times with backoff, roughly 20 s in total. CR-01 sequence 2 is actually closed by the fresh retrieval of the late created event, not by this retry. A subscription that never gets a row (pre-cutover PHP subscriptions, or the untagged events of another application, IN-02 of the first review) now produces three Error lines and a discarded River job per deletion, where it used to leave a marker. -**Fix:** When Stripe answers `resource_missing` for the stored customer, clear it and create a new one. Alternatively, store the customer per mode. +**Fix:** On the last attempt (`river.JobFromContext(ctx).Attempt >= MaxAttempts`), claim the marker and log at Warn instead of failing. Alternatively, document that the discarded jobs are expected. -### IN-08: Repository hygiene of the new plugin module +### IN-02: The session lock holds a pooled connection while it waits, and its holder needs a second one; with a capped pool or a re-entrant listener this stalls until the job timeout **Files:** -- `sm-paymentgatewaysubscriptions-plugin/go.work:9` -- `sm-paymentgatewaysubscriptions-plugin/go.mod:5-7, 132-134` +- `sm-paymentgatewaysubscriptions-plugin/classes/lock.go:63-88` +- `sm-pgstripe-plugin/subscriptions/gateway.go:114-128` +- `sm-paymentgatewaysubscriptions-plugin/classes/service.go:325-339` -**Issue:** -- The committed `go.work` replaces `sm-user-plugin` with `../sm-summercmsio-app/...`, which names a consuming application from a shared core plugin repository. -- `go.mod` requires `sm-paymentgateway-plugin v0.0.0`, which resolves only through the module's own `replace` directives. Consumers ignore those, so every consumer must add its own replace. -- The tagged `summercms v0.1.26` requirement is shadowed by `replace ../summercms.go` in every local build. +**Issue:** Each waiter keeps a pinned connection blocked in `pg_advisory_lock`. The holder needs another connection: `LoadSettings` in `client()`, and in `CheckoutGuarded` the `Open`, `Subscriber` and `ensureCustomer` calls on `s.db`. +- **Capped pool.** With `MaxOpenConns` at or below the number of concurrent jobs for one subscription, the holder starves until River's job timeout. Each timeout uses one of only 3 attempts, so an event can be discarded. +- **Re-entrant listener.** An event listener that calls `Service.SyncSubscription` for the same subscription waits on a lock its own job holds. That also lasts until the context deadline. -**Fix:** Point the workspace at a neutral path or leave `go.work` untracked. Require tagged versions of the sibling plugins once they exist. +lagoon sets no cap today, so the default deployment is unaffected (plan 07 noted this). + +**Fix:** Document the pool requirement (at least 2 x workers). Consider `SET lock_timeout` on the pinned connection before the lock call, or `pg_try_advisory_lock` with a River snooze. Note in the events doc that listeners must not call `SyncSubscription` or `Cancel` for the subscription they are handling. + +### IN-03: A plan changed between the handler's price check and the gateway call answers 502 instead of 422 + +**File:** `sm-paymentgatewaysubscriptions-plugin/api/subscription.go:128-131, 141-148` + +**Issue:** `ErrNoPriceForCurrency` from the gateway (for example, the plan was re-synced without that currency in between) goes through `writeGatewayError` and becomes 502 "provider". It is logged without the error. + +**Fix:** Map `errors.Is(err, ErrNoPriceForCurrency)` to the same 422 `currency` validation answer. That needs a gateway-neutral sentinel in `contracts`, because pgsub must not import pgstripe. + +### IN-04: The component wiring spec asserts source text, not behaviour + +**File:** `vue-paymentgateway-layer/tests/subscriptionComponents.test.ts:423-440` + +**Issue:** The test "the three components wire watchPageRestore on mount and remove it on unmount" matches regexes against the `.vue` sources. A refactor that keeps the strings but breaks the wiring, for example by registering on a different target, still passes. A correct rewrite, such as a composable `usePageRestore(flow)`, fails. + +**Fix:** Mount each component with `@vue/test-utils` (or the Nuxt test utils) and dispatch a `pageshow` with `persisted: true` on `window`. Assert that the button is enabled again and that `removeEventListener` runs on unmount. + +### IN-05: `/plans` still shows yen amounts divided by 100 + +**File:** `sm-paymentgatewaysubscriptions-plugin/api/json.go:122, 126` + +**Issue:** Dropping `jpy` from `supportedCurrencies` stops yen checkouts, but a plan whose `currency` is JPY, or whose `currency_prices` include `jpy`, is still listed through `money.FromMinor` as one hundredth of its price (¥5 for 500). This was the second half of CR-02's yen note. + +**Fix:** Leave out of `planPrices` any currency the stripe gateway does not support, or refuse `jpy` in the plan form's validation. --- -_Reviewed: 2026-10-14T19:22:09Z_ +_Reviewed: 2026-10-16T12:04:37Z_ _Reviewer: Claude (msd-code-reviewer)_ _Depth: standard_