Fix/join crm durability#117
Merged
Merged
Conversation
Investigating a member who paid on 2026-06-19 but has no demographic data in Action Network. Their AN record was created 11 days later by the newsletter form, carrying only a newsletter-signup tag; the join flow never wrote to AN at all. They were also charged twice, 91 seconds apart. ActionNetworkService::signup() is the only path that writes given_name, family_name, postal_addresses, phone and dob. Everything else that touches AN (updateCustomFields, addTag) writes only email, tags and custom fields by design, so once the signup is missed the demographics never arrive. Amount verification cancelPreviousAndGetCurrentSubscription() reported the current subscription's amount by scanning the customer's subscription list and initialising $amount = 0. A missing subscription ID, a wrong-customer subscription, or a Stripe error inside the swallowing catch all left that 0 in place and returned it as though it were real. The caller read 0.0 as a mismatch and threw before ever reaching the CRM write. Split into getSubscriptionAmount(), which retrieves by ID and returns null for unknown while letting API errors propagate, and cancelPreviousSubscriptions(). The caller now distinguishes "cannot verify" (code 9, retryable) from a real mismatch (code 8), compares with a tolerance, and verifies BEFORE cancelling — previously a failed check left the member's old subscription cancelled with nothing to show for it. CRM failure no longer fails the member's request The payment has already succeeded by the time AN is called, so a rethrow showed a paying member an error they could not act on and invited the resubmit that creates a duplicate subscription. Failures now write a JOIN_FORM_PENDING_CRM_* record instead. ensureStripeSubscriptionsCreated() likewise no longer discards join data for a cancelled-but-paid subscription. That record is the only copy of what the member submitted. Recovery snapshot freshness join.php saves the snapshot at create-subscription time, before the form is finished. handleJoin() now refreshes it with the payload it was actually handed, preserving the original createdAt so the expiry check still fires. Duplicate subscriptions createSubscription() now takes a Stripe idempotency key derived from the form session and the member's choices, so resubmitting the same request returns the existing subscription. The payment page skips confirmation when the returned intent has already succeeded. Also fixes a precedence slip where `(float) $x ?? 0` parsed as `((float) $x) ?? 0`, making the fallback dead code. Note: JOIN_FORM_PENDING_CRM_* records have no consumer yet — the retry worker is the next commit. Until then they accumulate, which is still strictly better than the data being destroyed. The Stripe idempotency change alters live payment behaviour and wants test-mode validation before deploy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Adds the consumer for the JOIN_FORM_PENDING_CRM_* records written by the
previous commit, so a member whose payment succeeded but whose CRM write
failed is finished automatically instead of silently missing.
ensureCrmPushesCompleted(), hourly:
- reads Stripe only to confirm the payment still stands
- takes the same per-email lock as /join and the webhooks
- retries just the services that failed, with 1h/2h/4h... backoff
capped at 24h
- after 12 attempts parks the record for a human. It is never deleted:
it holds the only copy of what the member submitted.
The "don't touch working payments" property is structural, not a
convention. The CRM writes are extracted out of tryHandleJoin() into
pushToCrms(), which contains no payment code at all, so the retry path
cannot reach subscription cancellation even if edited carelessly.
Mailchimp and Zetkin failures now queue too, rather than being logged
and forgotten.
Recovery replays no longer cancel subscriptions
ensureStripeSubscriptionsCreated() and the GoCardless cron replay
handleJoin() from data saved hours or days earlier. That ran
cancelPreviousSubscriptions() against a stale view of which
subscription is current — so if the member had joined again since, the
replay would cancel their live subscription. Replays now set
isRecoveryReplay and only read.
To make that possible, cancelPreviousSubscriptions() is split into a
read (getSubscriptionDates) and a write (the cancellation), with
customer resolution pulled out into resolveCustomerId().
Reconciler no longer accepts mere existence as proof of a join
reconcileRecentMemberships() treated "an Action Network person exists"
as evidence the join completed. A newsletter signup creates a person
from an email alone, so it masked exactly the case being investigated
and no alert fired. It now also checks for a name, which the join flow
always collects and a newsletter signup never sets.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves “join” durability across Stripe and downstream CRMs by preventing duplicate subscriptions/charges on resubmits, making CRM writes retryable when payments already succeeded, and adding safeguards for webhook races and out-of-order delivery.
Changes:
- Add a stable Stripe subscription idempotency key and client-side handling for idempotent resubmits where the PaymentIntent already succeeded.
- Introduce durable CRM retry records + an hourly worker, plus daily reconciliation/heartbeat cron jobs.
- Harden Stripe webhook handling around first-invoice events, subscription cancellation lapsing, and late-settlement scenarios; add focused unit tests for the new behaviors.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/join-flow/src/pages/payment-details.page.tsx | Avoid re-confirming already-succeeded PaymentIntents on idempotent resubmits. |
| packages/join-block/tests/SubscriptionIdempotencyKeyTest.php | Tests for stable/distinct Stripe subscription idempotency keys. |
| packages/join-block/tests/JoinServiceMailchimpTest.php | Ensures Mailchimp failures queue a CRM retry record instead of being silently swallowed. |
| packages/join-block/tests/FirstInvoiceClassifierTest.php | Tests for classifying first-invoice webhook handling (recover/defer/settle). |
| packages/join-block/tests/CrmRetryBackoffTest.php | Tests for CRM retry backoff schedule and due-ness logic. |
| packages/join-block/src/Services/StripeService.php | Idempotent subscription creation, webhook race handling, reconciliation helpers, safer subscription amount retrieval. |
| packages/join-block/src/Services/JoinService.php | CRM retry queue/worker, safer amount verification, and recovery snapshot refresh. |
| packages/join-block/src/Services/ActionNetworkService.php | Add read-only person snapshot for reconciliation logic. |
| packages/join-block/join.php | Wire idempotency key into create-subscription route; add CRM retry/reconciliation/heartbeat cron hooks. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+44
to
+52
| private static function crmRetryKey($data) | ||
| { | ||
| $subscriptionId = $data['stripeSubscriptionId'] ?? ''; | ||
| if ($subscriptionId) { | ||
| return self::CRM_RETRY_OPTION_PREFIX . $subscriptionId; | ||
| } | ||
| $email = strtolower(trim((string) ($data['email'] ?? ''))); | ||
| return self::CRM_RETRY_OPTION_PREFIX . 'email_' . sha1($email); | ||
| } |
Comment on lines
+745
to
+752
| $record = json_decode($result->option_value, true); | ||
| $data = $record['data'] ?? null; | ||
| $email = $data['email'] ?? ''; | ||
|
|
||
| if (!$data || !$email) { | ||
| $joinBlockLog->error("ensureCrmPushesCompleted: $name holds no usable join data, skipping"); | ||
| continue; | ||
| } |
Three defects in the retry queue, all found in review.
crmRetryKey() could collide
With no stripeSubscriptionId it hashed the email — but handleJoin()
can be called without one (it falls back to sessionToken for its
lock), so sha1('') gave every such record the same option name and
each queued member silently overwrote the last. Since the record is
the only copy of what the member submitted, that destroyed data.
Now falls through subscription -> email -> sessionToken -> payload
hash, treating blank as absent at each step rather than hashing it.
Records were autoloaded
Each carries the full join payload, and new options default to
autoload=yes, so a backlog would be read into memory on every page
request. Now written with autoload disabled, matching the existing
CK_JOIN_FLOW_COMPLETED_JOINS convention. Applied to the recovery
snapshots too, including the pre-existing Stripe, GoCardless and
Chargebee unprocessed-request options, which have the same problem.
Unusable records were re-logged forever
A record with no decodable JSON or no email was logged and skipped,
leaving it in place to be re-scanned and re-logged on every run. Such
a record can never succeed — without an email there is no CRM to push
to and no lock to take — so it is now parked. A decode failure keeps
the raw value, which remains the only copy of whatever was written.
Parking is consolidated into parkCrmRetry(), which records the reason
and timestamp and never deletes. Every path through the worker now
either progresses, parks, or skips for a bounded reason.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.