Skip to content

Commit 614249a

Browse files
authored
🐛 Fixed orphaned Stripe prices when importing members by tier (#28955)
fixes #22115 When you import a member with an `import_tier` and a Stripe customer, `forceStripeSubscriptionToProduct()` creates a new price on the tier's product, then updates the customer's subscription to use it. That new price is only recorded for archival (it gets cleaned up after the import) once the update succeeds. So if the update throws, for example because Stripe rejects it or the subscription is schedule-managed, the price is never archived and stays orphaned on the product, building up on every import. The subscription update is now wrapped so that on failure the just-created price is archived before the error propagates. The original error always surfaces, even if archiving itself fails (that's logged as a warning). This is what @9larsons suggested on the issue: "archive the just-created price if the update throws."
1 parent c5a500b commit 614249a

2 files changed

Lines changed: 126 additions & 6 deletions

File tree

ghost/core/core/server/services/members/importer/members-csv-importer-stripe-utils.js

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
const {DataImportError} = require('@tryghost/errors');
2+
const logging = require('@tryghost/logging');
23
const tpl = require('@tryghost/tpl');
34

45
const messages = {
@@ -150,12 +151,25 @@ module.exports = class MembersCSVImporterStripeUtils {
150151
interval: stripeSubscriptionItemPriceInterval
151152
});
152153

153-
await this._stripeAPIService.updateSubscriptionItemPrice(
154-
stripeSubscription.id,
155-
stripeSubscriptionItem.id,
156-
newStripePrice.id,
157-
{prorationBehavior: 'none'}
158-
);
154+
try {
155+
await this._stripeAPIService.updateSubscriptionItemPrice(
156+
stripeSubscription.id,
157+
stripeSubscriptionItem.id,
158+
newStripePrice.id,
159+
{prorationBehavior: 'none'}
160+
);
161+
} catch (err) {
162+
// The subscription update failed after we created a new price, which would
163+
// otherwise leave the price orphaned on the Stripe product (these accumulate
164+
// across imports). Archive it, then surface the original error regardless of
165+
// whether archiving succeeds. Ref: https://github.com/TryGhost/Ghost/issues/22115
166+
try {
167+
await this.archivePrice(newStripePrice.id);
168+
} catch (archiveErr) {
169+
logging.warn(`Failed to archive orphaned Stripe price ${newStripePrice.id} after a failed subscription update: ${archiveErr.message}`);
170+
}
171+
throw err;
172+
}
159173

160174
stripePriceId = newStripePrice.id;
161175
isNewStripePrice = true;

ghost/core/test/unit/server/services/members/importer/members-csv-importer-stripe-utils.test.js

Lines changed: 106 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -361,6 +361,112 @@ describe('MembersCSVImporterStripeUtils', function () {
361361
NEW_STRIPE_PRICE_ID,
362362
{prorationBehavior: 'none'}
363363
), true);
364+
365+
// Assert the new price was not archived — that cleanup only runs when the update fails (#22115)
366+
sinon.assert.notCalled(stripeAPIServiceStub.updatePrice);
367+
});
368+
369+
it('archives the newly-created price if updating the subscription fails (#22115)', async function () {
370+
const stripeAPIServiceStub = getStripeApiServiceStub();
371+
const NEW_STRIPE_PRICE_ID = 'new_stripe_price_id';
372+
const updateError = new Error('Stripe rejected the subscription update');
373+
374+
stripeAPIServiceStub.createPrice.resolves({id: NEW_STRIPE_PRICE_ID});
375+
stripeAPIServiceStub.updateSubscriptionItemPrice.rejects(updateError);
376+
377+
const productRepositoryStub = getProductRepositoryStub({resolveGhostProductPrice: false});
378+
const membersCSVImporterStripeUtils = new MembersCSVImporterStripeUtils({
379+
stripeAPIService: stripeAPIServiceStub,
380+
productRepository: productRepositoryStub
381+
});
382+
383+
// The original error surfaces...
384+
await assert.rejects(
385+
membersCSVImporterStripeUtils.forceStripeSubscriptionToProduct({
386+
customer_id: CUSTOMER_ID,
387+
product_id: PRODUCT_ID
388+
}, OPTIONS),
389+
err => err === updateError
390+
);
391+
392+
// ...and the price we just created is archived so it can't be orphaned/accumulate
393+
sinon.assert.calledOnce(stripeAPIServiceStub.updatePrice);
394+
assert.equal(stripeAPIServiceStub.updatePrice.calledWithExactly(NEW_STRIPE_PRICE_ID, {active: false}), true);
395+
});
396+
397+
it('surfaces the original update error even if archiving the orphaned price also fails (#22115)', async function () {
398+
const stripeAPIServiceStub = getStripeApiServiceStub();
399+
const NEW_STRIPE_PRICE_ID = 'new_stripe_price_id';
400+
const updateError = new Error('Stripe rejected the subscription update');
401+
402+
stripeAPIServiceStub.createPrice.resolves({id: NEW_STRIPE_PRICE_ID});
403+
stripeAPIServiceStub.updateSubscriptionItemPrice.rejects(updateError);
404+
stripeAPIServiceStub.updatePrice.rejects(new Error('archiving failed too'));
405+
406+
const productRepositoryStub = getProductRepositoryStub({resolveGhostProductPrice: false});
407+
const membersCSVImporterStripeUtils = new MembersCSVImporterStripeUtils({
408+
stripeAPIService: stripeAPIServiceStub,
409+
productRepository: productRepositoryStub
410+
});
411+
412+
// The original update error must surface, not the archive failure
413+
await assert.rejects(
414+
membersCSVImporterStripeUtils.forceStripeSubscriptionToProduct({
415+
customer_id: CUSTOMER_ID,
416+
product_id: PRODUCT_ID
417+
}, OPTIONS),
418+
err => err === updateError
419+
);
420+
});
421+
422+
it('does not archive anything when updating to an existing matching price fails — nothing was created (#22115)', async function () {
423+
const stripeAPIServiceStub = getStripeApiServiceStub();
424+
const updateError = new Error('Stripe rejected the subscription update');
425+
stripeAPIServiceStub.updateSubscriptionItemPrice.rejects(updateError);
426+
427+
// A matching Ghost price already exists, but with a different Stripe price id, so the
428+
// subscription genuinely needs updating (and can therefore fail) — yet no new price is
429+
// created on this path, so there must be nothing to archive.
430+
const productRepositoryStub = getProductRepositoryStub({
431+
resolveGhostProductPrice: true,
432+
ghostProductStripePriceId: 'existing_stripe_price_id'
433+
});
434+
const membersCSVImporterStripeUtils = new MembersCSVImporterStripeUtils({
435+
stripeAPIService: stripeAPIServiceStub,
436+
productRepository: productRepositoryStub
437+
});
438+
439+
await assert.rejects(
440+
membersCSVImporterStripeUtils.forceStripeSubscriptionToProduct({
441+
customer_id: CUSTOMER_ID,
442+
product_id: PRODUCT_ID
443+
}, OPTIONS),
444+
err => err === updateError
445+
);
446+
sinon.assert.notCalled(stripeAPIServiceStub.createPrice);
447+
sinon.assert.notCalled(stripeAPIServiceStub.updatePrice);
448+
});
449+
450+
it('does not update the subscription or archive when creating the new price itself fails (#22115)', async function () {
451+
const stripeAPIServiceStub = getStripeApiServiceStub();
452+
const createError = new Error('Stripe rejected createPrice');
453+
stripeAPIServiceStub.createPrice.rejects(createError);
454+
455+
const productRepositoryStub = getProductRepositoryStub({resolveGhostProductPrice: false});
456+
const membersCSVImporterStripeUtils = new MembersCSVImporterStripeUtils({
457+
stripeAPIService: stripeAPIServiceStub,
458+
productRepository: productRepositoryStub
459+
});
460+
461+
await assert.rejects(
462+
membersCSVImporterStripeUtils.forceStripeSubscriptionToProduct({
463+
customer_id: CUSTOMER_ID,
464+
product_id: PRODUCT_ID
465+
}, OPTIONS),
466+
err => err === createError
467+
);
468+
sinon.assert.notCalled(stripeAPIServiceStub.updateSubscriptionItemPrice);
469+
sinon.assert.notCalled(stripeAPIServiceStub.updatePrice);
364470
});
365471

366472
it('creates a new product in Stripe if one does not already existing for the Ghost product', async function () {

0 commit comments

Comments
 (0)