Merge pull request #8796 from nightscout/bf/activity-read-error

A failed storage read on GET /api/v1/activity or /api/v1/profile/current is answered with a 500 instead of ending the server (BF-155)

Author: bewest

lib/api/activity/index.js

@@ -28,10 +28,14 @@ function configure(app, wares, ctx) {
     api.get('/activity', function(req, res) {
         var ifModifiedSince = req.get('If-Modified-Since');
         ctx.activity.list(req.query, function(err, results) {
-            // This callback never checked err at all: on a failed read `results`
-            // is null and the forEach below throws, so a refused filter surfaced
-            // as an unhandled 500 from the express error handler.
-            if (sendQueryValidationError(res, err)) return;
+            // On a failed read `results` is null. This callback runs inside a
+            // promise handler, so reading it would throw an unhandled rejection
+            // and end the process: answer every error here instead.
+            if (err) {
+                if (sendQueryValidationError(res, err)) return;
+                console.error('Error reading activity', err);
+                return res.sendJSONStatus(res, consts.HTTP_INTERNAL_ERROR, 'Internal Server Error');
+            }
 
             var d1 = null;
 

lib/api/profile/index.js

@@ -65,7 +65,14 @@ function configure (app, wares, ctx) {
     // List current active record (in current state LAST record is current active)
     api.get('/profile/current', function(req, res) {
         ctx.profile.last( function(err, records) {
-            return res.json(records.length > 0 ? records[0] : null);
+            // Runs inside a promise handler: reading a null `records` here would
+            // end the process, so a failed read answers an error instead.
+            if (err) {
+                if (sendQueryValidationError(res, err)) return;
+                console.error('Error reading the current profile', err);
+                return res.sendJSONStatus(res, consts.HTTP_INTERNAL_ERROR, 'Internal Server Error');
+            }
+            return res.json(records && records.length > 0 ? records[0] : null);
         });
     });
 

tests/api.read-storage-failure.test.js

@@ -0,0 +1,77 @@
+'use strict';
+
+// A failed storage read reaches the API v1 read callbacks from a promise
+// handler (lib/storage/run-with-callback.js). A callback that reads the null
+// result there throws an unhandled rejection, which ends the process. These
+// tests fail the read the same way and check the request is answered and the
+// server keeps serving.
+
+var request = require('supertest');
+require('should');
+var language = require('../lib/language')();
+var runWithCallback = require('../lib/storage/run-with-callback');
+
+describe('API v1 reads when storage fails', function () {
+  this.timeout(15000);
+  var self = this;
+  var known = 'b723e97aa97846eb92d5264f084b2823f57c4aa1';
+
+  before(function (done) {
+    process.env.API_SECRET = 'this is my long pass phrase';
+    self.env = require('../lib/server/env')();
+    self.env.settings.authDefaultRoles = 'readable';
+    self.env.settings.enable = ['careportal', 'api'];
+    self.app = require('express')();
+    self.app.enable('api');
+    require('../lib/server/bootevent')(self.env, language).boot(function booted (ctx) {
+      self.ctx = ctx;
+      self.ctx.ddata = require('../lib/data/ddata')();
+      self.app.use('/api', require('../lib/api/')(self.env, ctx));
+      done();
+    });
+  });
+
+  // Stands in for a storage call whose promise rejects, delivered the way the
+  // storage modules deliver it.
+  function failingRead () {
+    var fn = Array.prototype.slice.call(arguments).find(function (arg) { return typeof arg === 'function'; });
+    return runWithCallback(function () { throw new Error('storage unavailable at db.internal:27017'); }, fn);
+  }
+
+  function expectAnsweredAndAlive (path, done) {
+    request(self.app)
+      .get(path)
+      .set('api-secret', known)
+      .timeout(5000)
+      .expect(500)
+      .end(function (err, res) {
+        if (err) return done(err);
+        JSON.stringify(res.body).should.not.match(/db\.internal|27017|storage unavailable/);
+        request(self.app)
+          .get('/api/status.json')
+          .timeout(5000)
+          .expect(200)
+          .end(done);
+      });
+  }
+
+  describe('GET /api/v1/activity', function () {
+    var original;
+    before(function () { original = self.ctx.activity.list; self.ctx.activity.list = failingRead; });
+    after(function () { self.ctx.activity.list = original; });
+
+    it('answers 500 without the storage error text, and the server keeps serving', function (done) {
+      expectAnsweredAndAlive('/api/activity/', done);
+    });
+  });
+
+  describe('GET /api/v1/profile/current', function () {
+    var original;
+    before(function () { original = self.ctx.profile.last; self.ctx.profile.last = failingRead; });
+    after(function () { self.ctx.profile.last = original; });
+
+    it('answers 500 without the storage error text, and the server keeps serving', function (done) {
+      expectAnsweredAndAlive('/api/profile/current', done);
+    });
+  });
+});