Harden subscription renewal retries - #5899
Conversation
Return early from subscription renewal processing when the existing order payment lock is already held, before either the Payment Intents or legacy SEPA charge path can contact Stripe. Add PHPUnit coverage for the held-lock guard across both renewal API paths. Fixes: STRIPE-1187
Removing the redundant isset( $order_helper ) and adding the renewal-path type docblocks made 19 baseline entries obsolete. Re-derive the baseline from develop with exactly those 19 removed, dropping the unrelated reorder churn the earlier regeneration introduced. The set of ignored errors is unchanged from the previous commit.
…attempt warning Add a WHY comment explaining the SEPA response normalization, and extend the held-lock renewal test to assert the duplicate-attempt warning is emitted (cards + SEPA) so the production short-circuit signal is covered by a test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- Name the lock_order_payment() boolean and document its contract for clarity. - Normalize SEPA array responses recursively so nested members survive the object conversion that process_response() relies on. - Restore the previous logger after the held-lock test's spy instead of nulling the process-global, and mirror WC_Stripe_Logger::warning()'s optional $context argument in the spy callback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The multi-line docblocks only narrowed types for static analysis; reduce them to the idiomatic single-line @var form, dropping the descriptive lines per the repo comment conventions. The @var tags are unchanged, so PHPStan narrowing is preserved. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Acquire the order payment lock before the renewal charge and hold it until the Stripe response has been fully processed, releasing it before each retry re-entry, in the error path, and in a finally once processing completes. This mirrors the regular checkout handler and closes a window where a concurrent scheduled renewal could acquire the lock after the charge but before the order was updated, and create a second charge. The charge helper is now lock-free. Add a regression test asserting the lock is held while process_response() runs and released afterward. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Wrap the renewal attempt so a non-Stripe error (a fatal Error, a malformed response, or a DB failure while adding an order note) releases the order payment lock before the error propagates, instead of leaving it held until the 5-minute TTL expires and blocking legitimate retries. The error is re-thrown so existing failure handling is unchanged. Covered by a regression test that throws a non-Stripe error mid-charge and asserts the lock is released. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Acquire the order payment lock once in process_subscription_payment() and release it in a finally block, so in-process retries run under the same lock instead of releasing and re-acquiring it between attempts. Rename the single-attempt helper to attempt_subscription_renewal_payment(), guard wp_json_encode() false returns before json_decode(), and give the inline @var docblocks the PHPCS-required short descriptions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Merging develop brought in its Radar-guard finally block, which releases the payment lock via $order_locked / $order_helper. Those variables no longer exist here: the lock is acquired and released once by process_subscription_payment(), so the merged block referenced undefined variables and could never run. Develop's Radar guard and the wc_stripe_subscription_renewal_blocked_by_radar action are kept. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Log the skipped duplicate renewal attempt at error level so it is recorded even when debug logging is disabled, and add an order note so merchants can see why no charge was attempted. - Stop retrying once the 5-minute payment lock has expired: the retry chain can outlive the lock, and continuing past it risks charging alongside a concurrent renewal attempt. - Only release the payment lock when it is still the one this process wrote, so a newer lock acquired by a concurrent process after ours lapsed is not deleted. - Resolve a bare charge id returned by get_latest_charge_from_intent() into the full charge object before handing it to process_response(). - Fix a test-isolation leak in the renewal tests: delete the 401 rate-limiter cache key before the Stripe settings, while the mode-prefixed cache key still resolves to the one the tests wrote. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The expiry check ran only before sleep(), so a lock lapsing during the backoff could still let the next Stripe attempt start. Re-checking after the sleep bails to the normal failure path instead, so an attempt is never started on a lock that expired while this process slept. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…yment-lock-renewal-path # Conflicts: # readme.txt
Validate renewal lock metadata, centralize response normalization, and strengthen lock coverage for card retries and SEPA renewals.
…yment-lock-renewal-path # Conflicts: # changelog.txt # readme.txt
d35d5d3 to
3f1fa0f
Compare
Test the buildIf you are a logged-in Automattic staff member: 🚀 Launch a Jurassic Ninja site with this branch and sample products 🚀 Otherwise, use the following manual flow: 🐢 Navigate to jurassic.ninja 🐢
Build info:
Note: the build is updated when a new commit is pushed to this PR. |
There was a problem hiding this comment.
🟡 Changes recommended
The lock budget is checked before a Stripe-backed source lookup can consume part of the reserved time.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Hardens subscription renewal retries against lock expiry and idempotency-key leakage.
Changes:
- Adds lock-time budgeting and scoped retry filters.
- Improves Stripe error handling and timeout reuse.
- Adds regression tests and release notes.
File summaries
| File | Description |
|---|---|
includes/compat/trait-wc-stripe-subscriptions.php |
Hardens renewal retries and cleanup. |
includes/class-wc-stripe-api.php |
Centralizes API timeout. |
includes/abstracts/abstract-wc-stripe-payment-gateway.php |
Improves retry error and key handling. |
tests/phpunit/compat/class-wc-stripe-subscription-renewal-test.php |
Tests renewal retry behavior. |
tests/phpunit/class-wc-stripe-payment-gateway-test.php |
Tests error classification and keys. |
phpstan-baseline.neon |
Removes resolved suppressions. |
changelog.txt |
Adds the changelog entry. |
readme.txt |
Adds the release note. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // A retry near the lock's expiry could leave a charge unrecorded; report the previous error instead. | ||
| if ( is_object( $previous_error ) && ! $this->subscription_payment_lock_covers_request_chain( $lock_expiry ) ) { | ||
| WC_Stripe_Logger::error( "Stripe: not retrying subscription renewal for order {$order_id} because its payment lock cannot cover another Stripe request chain." ); | ||
| throw $this->get_subscription_renewal_payment_exception( $renewal_order, (object) [ 'error' => $previous_error ] ); | ||
| } | ||
|
|
||
| // Get source from order | ||
| $prepared_source = $this->prepare_order_source( $renewal_order ); | ||
| $source_object = $prepared_source->source_object; |
The tests read the clock before process_subscription_payment() and compared it against the lock the gateway stamps mid-call, so a second ticking over during the call pushed the lock one second past the ceiling and failed the assertion. Compare against the clock read after the call instead. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ed2rjmsSbYTpV9xRt3Y9FX
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved critical and moderate idempotency-key issues could cause duplicate charges or mismatched requests.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
| // Match on type; the message text can change. This also matches the "request in | ||
| // progress" case; the order payment lock keeps same-key requests from overlapping. | ||
| return $error && isset( $error->type ) && 'idempotency_error' === $error->type; |
| $idempotency_key_filter = function ( $idempotency_key, $request ) use ( &$last_idempotency_key, &$keyed_request_count, &$reuse_idempotency_key, $rotate_idempotency_key ) { | ||
| if ( empty( $idempotency_key ) ) { | ||
| return $idempotency_key; | ||
| } |
ed10c26 to
7a86ae0
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Idempotency-key handling can cause repeated failures or duplicate charges, and release notes need correction.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (4)
changelog.txt:49
- The release note says server and connection errors are retried, but the PR's own behavior description notes that 5xx outages and connection failures throw before reaching this retry path. Describe this as a retryable Stripe API error instead so the public changelog does not claim unsupported recovery.
* Fix - Reuse the idempotency key when a subscription renewal retries after a Stripe server or connection error, detach the retry key filter after each attempt, and match idempotency errors by type
includes/abstracts/abstract-wc-stripe-payment-gateway.php:212
- Treating every
idempotency_erroras a parameter-mismatch error also rotates the key for Stripe's “request in progress” case acknowledged here. That case means an operation under the old key may still complete, so changing the key bypasses Stripe's duplicate protection and can double-charge; a local order lock cannot cancel an already-running Stripe operation. Distinguish key-in-use errors and retry them safely with the same key, rotating only for an actual parameter mismatch.
// Match on type; the message text can change. This also matches the "request in
// progress" case; the order payment lock keeps same-key requests from overlapping.
return $error && isset( $error->type ) && 'idempotency_error' === $error->type;
includes/compat/trait-wc-stripe-subscriptions.php:605
- This reuses the key from the previous attempt's last keyed request on the next attempt's first request without comparing request bodies. With Level 3 fallback, the saved key can belong to the fallback request without
level3, while the next attempt starts withlevel3again; Stripe then rejects the reused key because its parameters changed. Track keys by a stable request fingerprint and reuse only the key associated with the same request body.
} elseif ( $reuse_idempotency_key && 1 === $keyed_request_count && '' !== $last_idempotency_key ) {
$idempotency_key = $last_idempotency_key;
readme.txt:204
- The release note says server and connection errors are retried, but the PR's own behavior description notes that 5xx outages and connection failures throw before reaching this retry path. Describe this as a retryable Stripe API error instead so the public changelog does not claim unsupported recovery.
* Fix - Reuse the idempotency key when a subscription renewal retries after a Stripe server or connection error, detach the retry key filter after each attempt, and match idempotency errors by type
- Files reviewed: 7/7 changed files
- Comments generated: 1
- Review effort level: Balanced
| if ( is_object( $source_object ) && empty( $source_object->error ) && $this->need_update_idempotency_key( $source_object, $previous_error ) ) { | ||
| add_filter( 'wc_stripe_idempotency_key', [ $this, 'change_idempotency_key' ], 10, 2 ); | ||
| } | ||
| $rotate_idempotency_key = is_object( $source_object ) && empty( $source_object->error ) && $this->need_update_idempotency_key( $source_object, $previous_error ); |
There was a problem hiding this comment.
🟡 Changes recommended
Retry-key handling remains unsafe for UPE renewals and Level 3 fallbacks, and the regression test models impossible replay behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
includes/compat/trait-wc-stripe-subscriptions.php:617
- Always retaining the first request's key is unsafe when
request_with_level3_data()falls back. If Stripe rejectslevel3, that helper recordswc_stripe_level3_not_allowedand sends a second, unadorned request; if that fallback returnsapi_error, the next attempt is also unadorned but this code reuses the key belonging to the original Level 3 payload. Stripe then returns an idempotency mismatch instead of reconciling the potentially completed fallback request, and rotating afterward can create another charge. Preserve the key associated with the request shape that the next attempt will send, ideally by tracking the keyed payload as well.
// Keep the first request's key: a Level 3 fallback request has different parameters.
if ( 1 === $keyed_request_count ) {
$last_idempotency_key = (string) $idempotency_key;
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
| $rotate_idempotency_key = is_object( $source_object ) | ||
| && empty( $source_object->error ) | ||
| && 'chargeable' === ( $source_object->status ?? '' ) | ||
| && is_object( $previous_error ) | ||
| && 'idempotency_error' === ( $previous_error->type ?? '' ) | ||
| && 1 < (int) ( $this->retry_interval ?? 1 ); // @phpstan-ignore-line (retry_interval is defined on the main gateway) |
| return [ | ||
| 'headers' => [], | ||
| 'body' => file_get_contents( __DIR__ . '/dummy-data/subscription_renewal_response_success.json' ), |
There was a problem hiding this comment.
🟡 Changes recommended
One critical and two moderate idempotency and filter-cleanup issues remain unresolved.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 3
- Review effort level: Balanced
| } elseif ( $reuse_idempotency_key && isset( $last_keyed_request['request'] ) && wp_json_encode( $request ) === $last_keyed_request['request'] ) { | ||
| $idempotency_key = $last_keyed_request['key']; |
| if ( $rotate_idempotency_key ) { | ||
| // UPE payment methods use this trait but the key builder lives on the gateway. | ||
| $gateway = $this instanceof WC_Stripe_Payment_Gateway ? $this : WC_Stripe::get_instance()->get_main_stripe_gateway(); | ||
| $idempotency_key = $gateway->change_idempotency_key( $idempotency_key, $request ); | ||
| } elseif ( $reuse_idempotency_key && isset( $last_keyed_request['request'] ) && wp_json_encode( $request ) === $last_keyed_request['request'] ) { | ||
| $idempotency_key = $last_keyed_request['key']; | ||
| } | ||
|
|
||
| $last_keyed_request = [ | ||
| 'key' => (string) $idempotency_key, | ||
| 'request' => wp_json_encode( $request ), | ||
| ]; |
|
|
||
| $payment_attempt = $this->attempt_subscription_renewal_payment( $amount, $renewal_order, $prepared_source ); | ||
| try { | ||
| $payment_attempt = $this->attempt_subscription_renewal_payment( $amount, $renewal_order, $prepared_source ); |
Drop the response converter, the charge-id lookup, and the invalid-lock branch. They belong to the charge handling follow-up or guard a state the lock write cannot produce. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ed2rjmsSbYTpV9xRt3Y9FX
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ed2rjmsSbYTpV9xRt3Y9FX
A retry after an api_error or api_connection_error sends the key of the earlier request with the same parameters, so Stripe returns the stored result instead of charging again. Keys are kept per request. The filter is attached only when an attempt can retry, reuse, or rotate a key, and removed when the attempt ends. Renewal idempotency errors are matched by type. UPE payment method instances build the rotated key with their own retry count. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ed2rjmsSbYTpV9xRt3Y9FX
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ed2rjmsSbYTpV9xRt3Y9FX
a1b6f5b to
1b541f6
Compare
The mandate options set start_date to the request time, so no retry sent the same request and no idempotency key was ever reused. All attempts of one renewal now use the start date of the first. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Ed2rjmsSbYTpV9xRt3Y9FX
Fixes STRIPE-1187
This PR builds on #5591. Merge order: #5591, #5897, #5901, then this PR; #5900 does not touch these files. Change the base to
developafter #5591 merges, and rebase after #5901 merges.Changes proposed in this Pull Request:
api_errororapi_connection_errorsends the same idempotency key as the request that got the error. Stripe then returns the stored result of that request and does not charge again. Two cases never reach the retry code: a connection failure that throws before Stripe answers, and a 5xx response, whichWC_Stripe_APItreats as an outage and throws. So this applies to anapi_errorthat arrives with a 4xx status.wc_stripe_request_bodyfilter. A filter that changes the body between attempts gets the reused key, Stripe rejects the mismatch with an idempotency error, and that attempt is lost.start_dateof the request time. Attempts of one renewal now share one start date, so a retry sends the same request. On a test site, without this, every retry got a new key because the start date moved by a second.type, not by its message text. Checkout keeps the gateway's message match, so its retry behaviour does not change. The filter returns the original key for requests that have no order metadata.is_retryable_error()accepts an error response that has notype.change_idempotency_key()to the main gateway through__call(). The main gateway's retry count is not the count of the instance that runs the renewal, so on those classes the filter builds the rotated key with its own count. Before, PHPStan hid the forwarded call with 26 baseline entries; this PR removes them.Testing instructions
Environment: Docker (
npm run up), PHP dependencies installed.Action: Run
npm run test:php -- --filter='WC_Stripe_Subscription_Renewal_Test|WC_Stripe_Payment_Gateway_Test',npm run phpstan, andnpm run lint:php.Validation: All tests pass. A retry after an
api_errorsends the same key, and a replayed error fails the renewal with one key. A retry after a Level 3 fallback reuses the fallback request's key, with and without the account transient. A retry after an idempotency error uses the key{order}-2-{source}. A retry with the default source uses a new key. No filter stays active after the attempt.Changelog entry
Changelog Entry Comment
Comment
Fix - Reuse the idempotency key when a subscription renewal retries after a retryable Stripe API error, detach the retry key filter after each attempt, and match renewal idempotency errors by type