Retry based on Stripe-Should-Retry and Retry-After headers (stripe#692)

* Allow a header to dictate whether the client should retry

* Allow a header to recommend a minimum time after which to retry

* Rename the header, remove 429 for now, clean up

* Fix tests

* Cleaner handling of inputs

* Add comments and a constant

Author: rattrayalex-stripe

lib/StripeResource.js

@@ -18,6 +18,7 @@ StripeResource.method = require('./StripeMethod');
 StripeResource.BASIC_METHODS = require('./StripeMethod.basic');
 
 StripeResource.MAX_BUFFERED_REQUEST_METRICS = 100;
+const MAX_RETRY_AFTER_WAIT = 60;
 
 /**
  * Encapsulates request logic for a Stripe Resource
@@ -217,6 +218,7 @@ StripeResource.prototype = {
     };
   },
 
+  // For more on when and how to retry API requests, see https://stripe.com/docs/error-handling#safely-retrying-requests-with-idempotency
   _shouldRetry(res, numRetries) {
     // Do not retry if we are out of retries.
     if (numRetries >= this._stripe.getMaxNetworkRetries()) {
@@ -228,21 +230,33 @@ StripeResource.prototype = {
       return true;
     }
 
-    // Retry on conflict and availability errors.
-    if (res.statusCode === 409 || res.statusCode === 503) {
+    // The API may ask us not to retry (eg; if doing so would be a no-op)
+    // or advise us to retry (eg; in cases of lock timeouts); we defer to that.
+    if (res.headers && res.headers['stripe-should-retry'] === 'false') {
+      return false;
+    }
+    if (res.headers && res.headers['stripe-should-retry'] === 'true') {
+      return true;
+    }
+
+    // Retry on conflict errors.
+    if (res.statusCode === 409) {
       return true;
     }
 
-    // Retry on 5xx's, except POST's, which our idempotency framework
-    // would just replay as 500's again anyway.
-    if (res.statusCode >= 500 && res.req._requestEvent.method !== 'POST') {
+    // Retry on 500, 503, and other internal errors.
+    //
+    // Note that we expect the stripe-should-retry header to be false
+    // in most cases when a 500 is returned, since our idempotency framework
+    // would typically replay it anyway.
+    if (res.statusCode >= 500) {
       return true;
     }
 
     return false;
   },
 
-  _getSleepTimeInMS(numRetries) {
+  _getSleepTimeInMS(numRetries, retryAfter = null) {
     const initialNetworkRetryDelay = this._stripe.getInitialNetworkRetryDelay();
     const maxNetworkRetryDelay = this._stripe.getMaxNetworkRetryDelay();
 
@@ -261,6 +275,11 @@ StripeResource.prototype = {
     // But never sleep less than the base sleep seconds.
     sleepSeconds = Math.max(initialNetworkRetryDelay, sleepSeconds);
 
+    // And never sleep less than the time the API asks us to wait, assuming it's a reasonable ask.
+    if (Number.isInteger(retryAfter) && retryAfter <= MAX_RETRY_AFTER_WAIT) {
+      sleepSeconds = Math.max(sleepSeconds, retryAfter);
+    }
+
     return sleepSeconds * 1000;
   },
 
@@ -342,10 +361,16 @@ StripeResource.prototype = {
   _request(method, host, path, data, auth, options, callback) {
     let requestData;
 
-    const retryRequest = (requestFn, apiVersion, headers, requestRetries) => {
+    const retryRequest = (
+      requestFn,
+      apiVersion,
+      headers,
+      requestRetries,
+      retryAfter
+    ) => {
       return setTimeout(
         requestFn,
-        this._getSleepTimeInMS(requestRetries),
+        this._getSleepTimeInMS(requestRetries, retryAfter),
         apiVersion,
         headers,
         requestRetries + 1
@@ -394,15 +419,27 @@ StripeResource.prototype = {
 
       req.once('response', (res) => {
         if (this._shouldRetry(res, requestRetries)) {
-          return retryRequest(makeRequest, apiVersion, headers, requestRetries);
+          return retryRequest(
+            makeRequest,
+            apiVersion,
+            headers,
+            requestRetries,
+            ((res || {}).headers || {})['retry-after']
+          );
         } else {
           return this._responseHandler(req, callback)(res);
         }
       });
 
       req.on('error', (error) => {
         if (this._shouldRetry(null, requestRetries)) {
-          return retryRequest(makeRequest, apiVersion, headers, requestRetries);
+          return retryRequest(
+            makeRequest,
+            apiVersion,
+            headers,
+            requestRetries,
+            null
+          );
         } else {
           return this._errorHandler(req, requestRetries, callback)(error);
         }

test/StripeResource.spec.js

@@ -240,14 +240,18 @@ describe('StripeResource', () => {
         });
       });
 
-      it('should not retry on a 500 error when the method is POST', (done) => {
+      it('should not retry when a header says not to', (done) => {
         nock(`https://${options.host}`)
           .post(options.path, options.params)
-          .reply(500, {
-            error: {
-              type: 'api_error',
+          .reply(
+            500,
+            {
+              error: {
+                type: 'api_error',
+              },
             },
-          });
+            {'stripe-should-retry': 'false'}
+          );
 
         realStripe.setMaxNetworkRetries(1);
 
@@ -257,6 +261,29 @@ describe('StripeResource', () => {
         });
       });
 
+      it('should retry when a header says it should, even on status codes we ordinarily wouldnt', (done) => {
+        nock(`https://${options.host}`)
+          .post(options.path, options.params)
+          .reply(
+            400,
+            {error: {type: 'your_fault'}},
+            {'stripe-should-retry': 'true'}
+          )
+          .post(options.path, options.params)
+          .reply(200, {
+            id: 'ch_123',
+            object: 'charge',
+            amount: 1000,
+          });
+
+        realStripe.setMaxNetworkRetries(1);
+
+        realStripe.charges.create(options.data, (err, charge) => {
+          expect(charge.id).to.equal('ch_123');
+          done(err);
+        });
+      });
+
       it('should handle OAuth errors gracefully', (done) => {
         nock('https://connect.stripe.com')
           .post('/oauth/token')
@@ -471,17 +498,29 @@ describe('StripeResource', () => {
 
     describe('_getSleepTimeInMS', () => {
       it('should not exceed the maximum or minimum values', () => {
-        let sleepSeconds;
         const max = stripe.getMaxNetworkRetryDelay();
         const min = stripe.getInitialNetworkRetryDelay();
 
         for (let i = 0; i < 10; i++) {
-          sleepSeconds = stripe.invoices._getSleepTimeInMS(i) / 1000;
+          const sleepSeconds = stripe.invoices._getSleepTimeInMS(i) / 1000;
 
           expect(sleepSeconds).to.be.at.most(max);
           expect(sleepSeconds).to.be.at.least(min);
         }
       });
+
+      it('should allow a maximum override', () => {
+        const maxSec = stripe.getMaxNetworkRetryDelay();
+        const minMS = stripe.getInitialNetworkRetryDelay() * 1000;
+
+        expect(stripe.invoices._getSleepTimeInMS(3, 0)).to.be.gt(minMS);
+        expect(stripe.invoices._getSleepTimeInMS(2, 3)).to.equal(3000);
+        expect(stripe.invoices._getSleepTimeInMS(0, 3)).to.equal(3000);
+        expect(stripe.invoices._getSleepTimeInMS(0, 0)).to.equal(minMS);
+        expect(stripe.invoices._getSleepTimeInMS(0, maxSec * 2)).to.equal(
+          maxSec * 2 * 1000
+        );
+      });
     });
   });
 });