Fix: Return 444 status code for closed and aborted requests (#1579)

lib/plugins/metrics.js

@@ -47,12 +47,8 @@ function createMetrics(opts, callback) {
             // connection state can currently only have the following values:
             // 'close' | 'aborted' | undefined.
             //
-            // it is possible to get a 200 statusCode with a connectionState
-            // value of 'close' or 'aborted'. i.e., the client timed out,
-            // but restify thinks it "sent" a response. connectionState should
-            // always be the primary source of truth here, and check it first
-            // before consuming statusCode. otherwise, it may result in skewed
-            // metrics.
+            // if the connection state is 'close' or 'aborted'
+            // the status code will be set to 444
             connectionState: req.connectionState && req.connectionState(),
             unfinishedRequests:
                 opts.server.inflightRequests && opts.server.inflightRequests(),

lib/request.js

@@ -826,7 +826,7 @@ function patch(Request) {
      * @memberof Request
      * @instance
      * @function connectionState
-     * @returns {String} connection state (`"closed"`, `"aborted"`)
+     * @returns {String} connection state (`"close"`, `"aborted"`)
      */
     Request.prototype.connectionState = function connectionState() {
         var self = this;

lib/response.js

@@ -380,6 +380,14 @@ function patch(Response) {
         // Now lets try to derive values for optional arguments that we were not
         // provided, otherwise we choose sane defaults.
 
+        // Request was aborted or closed. Override the status code
+        if (
+            self.req.connectionState() === 'close' ||
+            self.req.connectionState() === 'aborted'
+        ) {
+            code = 444;
+        }
+
         // If the body is an error object and we were not given a status code,
         // try to derive it from the error object, otherwise default to 500
         if (!code && body instanceof Error) {

test/plugins/audit.test.js

@@ -605,4 +605,74 @@ describe('audit logger', function() {
             assert.ifError(err);
         });
     });
+
+    it('should log 444 status code for aborted request', function(done) {
+        SERVER.once(
+            'after',
+            restify.plugins.auditLogger({
+                log: bunyan.createLogger({
+                    name: 'audit',
+                    streams: [
+                        {
+                            level: 'info',
+                            stream: process.stdout
+                        }
+                    ]
+                }),
+                server: SERVER,
+                event: 'after'
+            })
+        );
+
+        SERVER.once('audit', function(data) {
+            assert.ok(data);
+            assert.ok(data.req_id);
+            assert.isNumber(data.latency);
+            assert.equal(444, data.res.statusCode);
+            done();
+        });
+
+        SERVER.get('/audit', function(req, res, next) {
+            req.emit('aborted');
+            res.send();
+            next();
+        });
+
+        CLIENT.get('/audit', function(err, req, res) {});
+    });
+
+    it('should log 444 for closed request', function(done) {
+        SERVER.once(
+            'after',
+            restify.plugins.auditLogger({
+                log: bunyan.createLogger({
+                    name: 'audit',
+                    streams: [
+                        {
+                            level: 'info',
+                            stream: process.stdout
+                        }
+                    ]
+                }),
+                server: SERVER,
+                event: 'after'
+            })
+        );
+
+        SERVER.once('audit', function(data) {
+            assert.ok(data);
+            assert.ok(data.req_id);
+            assert.isNumber(data.latency);
+            assert.equal(444, data.res.statusCode);
+            done();
+        });
+
+        SERVER.get('/audit', function(req, res, next) {
+            req.emit('close');
+            res.send();
+            next();
+        });
+
+        CLIENT.get('/audit', function(err, req, res) {});
+    });
 });

test/plugins/metrics.test.js

@@ -96,7 +96,7 @@ describe('request metrics plugin', function() {
                     assert.equal(err.name, 'RequestCloseError');
 
                     assert.isObject(metrics, 'metrics');
-                    assert.equal(metrics.statusCode, 202);
+                    assert.equal(metrics.statusCode, 444);
                     assert.isAtLeast(metrics.latency, 200);
                     assert.equal(metrics.path, '/foo');
                     assert.equal(metrics.method, 'GET');
@@ -137,7 +137,7 @@ describe('request metrics plugin', function() {
                     assert.equal(err.name, 'RequestAbortedError');
 
                     assert.isObject(metrics, 'metrics');
-                    assert.equal(metrics.statusCode, 202);
+                    assert.equal(metrics.statusCode, 444);
                     assert.isAtLeast(metrics.latency, 200);
                     assert.equal(metrics.path, '/foo');
                     assert.equal(metrics.method, 'GET');
@@ -155,7 +155,6 @@ describe('request metrics plugin', function() {
         });
 
         CLIENT.get('/foo?a=1', function(err, _, res) {
-            assert.ifError(err);
             return done();
         });
     });