Subscription status regress to `Open` with concurrent status update

[rvdsteege]

We came across an issue in https://freescout.pronamic.nl/conversation/11328?folder_id=55 where a subscription was updated from Open > Active > Open, while payment status had been updated correctly to Success.

The issue appears to be caused by concurrent processing of webhook and user return requests. Upon further investigation I suspect that the issue is a stale in-memory subscription object being saved after the webhook already persisted a newer status.

The gateway status update (in this case from Mollie) can use a payment and subscription while they are still pending. The first request (I suspect the webhook) updates the payment status to Success, which subsequently triggers the subscription status to be updated to Active through the pronamic_payment_status_update action. Shortly after that, the second request (the user return?) continues with its older in-memory version of the subscription (still Open) and saves it (through https://github.com/pronamic/wp-pronamic-pay-mollie/blob/939429d447b07591800b38d994316843d77ce9eb/src/Gateway.php#L1049 or https://github.com/pronamic/wp-pronamic-pay-mollie/blob/939429d447b07591800b38d994316843d77ce9eb/src/Gateway.php#L1284), unintentionally overwriting Active back to Open.

The payment status is protected more reliably, because previous payment status is read from post meta before firing status transition hooks:

wp-pay-core/src/Payments/PaymentsDataStoreCPT.php

Lines 531 to 535 in 2d14a84

	$previous_status = $this->get_meta( $id, 'status' );
	$this->update_meta( $id, 'status', $payment->status );
	if ( $previous_status !== $payment->status ) {

The same check for subscriptions can still result in a status update, when meta status is Active and in-memory subscription is Open:

wp-pay-core/src/Subscriptions/SubscriptionsDataStoreCPT.php

Lines 536 to 540 in 2d14a84

	$previous_status = $this->get_meta( $id, 'status' );
	$this->update_meta( $id, 'status', $subscription->status );
	if ( $previous_status !== $subscription->status ) {

I was unable to find any paths that should update a subscription status back to Open. If that is never expected, the easiest way to resolve the issue is by updating a Open status in the in-memory subscription to the actual status from post meta during a subscription update:

wp-pay-core/src/Subscriptions/SubscriptionsDataStoreCPT.php

Lines 242 to 257 in 2d14a84

	public function update( $subscription ) {
	$id = $subscription->get_id();
	if ( empty( $id ) ) {
	return false;
	}
	$result = \wp_update_post(
	$this->get_post_data(
	$subscription,
	[
	'ID' => $id,
	]
	),
	true
	);

An alternative solution could be to use locking in status updates to prevent concurrent processing, but I'm afraid that will be more prone to issues.

[remcotolsma]

I asked AI for an explanation and possible solutions (in Dutch):

Race Condition & Data Inconsistency

Dit is inderdaad een klassiek race condition probleem, specifiek een "lost update" scenario. Laten we het stap voor stap doorlopen:

Het Probleem

Tijd  Proces 1                    Database          Proces 2
────────────────────────────────────────────────────────────
t1    payment laden (status=open) [open]
t2    status → 'completed'        [open]
t3                                              payment laden (status=open)
t4                                              status → 'processing'
t5    opslaan → get_meta()        [processing] ← Proces 2 was sneller!
t6    get_meta() retourneert 'processing' (verwacht 'open')

Dit heet een "check-then-act" race condition" of "read-modify-write" conflict.

Naamgeving

• Race condition: Twee processen racen naar dezelfde resource
• Lost update: Proces 1's wijziging wordt overschreven
• Dirty read: We lezen data die inmiddels verouderd is

Oplossingen (in volgorde van geschiktheid)

1. Optimistic Locking ✅ (Aanbevolen)

Bij het laden van de payment sla je een versie-nummer op. Bij het opslaan controleer je of dit nog geldig is:

// Bij laden
$payment->version = $this->get_meta( $id, '_version' );
$payment->status = $this->get_meta( $id, 'status' );

// Bij opslaan
$current_version = $this->get_meta( $id, '_version' );

if ( $current_version !== $payment->version ) {
    // Iemand anders heeft het bijgewerkt - reject of merge
    throw new \Exception( 'Payment was modified by another process' );
}

// Update zowel status als versie
$this->update_meta( $id, 'status', $new_status );
$this->update_meta( $id, '_version', $payment->version + 1 );

2. Pessimistic Locking (Heavier-weight)

Lock de payment terwijl je ermee werkt:

// Bij laden
$this->update_meta( $id, '_locked', true );

// ... werk ermee ...

// Bij opslaan
$this->update_meta( $id, '_locked', false );

3. Change Tracking (Dirty Checking)

Track welke velden daadwerkelijk zijn gewijzigd:

class Payment {
    private $original_status;
    private $current_status;
    private $dirty_fields = [];
    
    public function set_status( $status ) {
        if ( $this->current_status !== $status ) {
            $this->dirty_fields['status'] = true;
            $this->current_status = $status;
        }
    }
    
    public function get_dirty_fields() {
        return array_keys( $this->dirty_fields );
    }
}

Dit helpt, maar lost de race condition niet volledig op.

4. Database-level Locking (Meest robuust)

Gebruik transacties en SELECT FOR UPDATE:

// Pseudo-code (WordPress ondersteunt dit niet direct)
$wpdb->query( "SELECT * FROM posts WHERE ID = %d FOR UPDATE", $id );
// ... wijzig data ...
$wpdb->query( "COMMIT" );

Aanbeveling voor jouw codebase

Gezien dit een WordPress Pay Core project is, raad ik Optimistic Locking (#1) aan omdat:

• ✅ Geen locking/blocking van resources
• ✅ Schaalt beter met concurrent processes
• ✅ Eenvoudig met post meta implementeerbaar
• ✅ WordPress-friendly (geen low-level DB-transacties nodig)

Het voorkómt dat je stale data gebruikt bij het opslaan. In plaats van get_meta() opnieuw op te vragen, check je tegen de versie die je bij het laden hebt opgeslagen.

For Mollie we have a small improvement suggestion:

• Defer webhook processing with Action Scheduler wp-pronamic-pay-mollie#105

It doesn't solve the core of the problem, but asynchronous processing of the webhook might be better anyway?