Unify API errors processing

With this change, we define a custom error handler as express middleware
which renders 503 error with JSON response that includes status and message
fields.

The handler also logs the error, so the stack can be inspected in supervisor
logs. It's also a point where we can report the error to analytics services.

This removes a bunch of error handlers written in every request handler
function. Behaviour should remain unchanged except the fact that
/healthy endpoint now returns 503 in case of failure instead of 500.

Change-type: patch
Signed-off-by: Roman Mazur <roman@balena.io>

src/api-binder.ts

@@ -935,7 +935,7 @@ export class APIBinder {
 		router.use(bodyParser.urlencoded({ limit: '10mb', extended: true }));
 		router.use(bodyParser.json({ limit: '10mb' }));
 
-		router.post('/v1/update', (req, res) => {
+		router.post('/v1/update', (req, res, next) => {
 			apiBinder.eventTracker.track('Update notification');
 			if (apiBinder.readyForUpdates) {
 				this.config
@@ -953,15 +953,7 @@ export class APIBinder {
 							res.sendStatus(202);
 						}
 					})
-					.catch(err => {
+					.catch(next);
-						const msg =
-							err.message != null
-								? err.message
-								: err != null
-								? err
-								: 'Unknown error';
-						res.status(503).send(msg);
-					});
 			} else {
 				res.sendStatus(202);
 			}

src/device-api/v1.coffee

@@ -9,7 +9,7 @@ exports.createV1Api = (router, applications) ->
 
 	{ eventTracker } = applications
 
-	router.post '/v1/restart', (req, res) ->
+	router.post '/v1/restart', (req, res, next) ->
 		appId = checkInt(req.body.appId)
 		force = checkTruthy(req.body.force)
 		eventTracker.track('Restart container (v1)', { appId })
@@ -18,10 +18,9 @@ exports.createV1Api = (router, applications) ->
 		doRestart(applications, appId, force)
 		.then ->
 			res.status(200).send('OK')
-		.catch (err) ->
+		.catch(next)
-			res.status(503).send(err?.message or err or 'Unknown error')
 
-	v1StopOrStart = (req, res, action) ->
+	v1StopOrStart = (req, res, next, action) ->
 		appId = checkInt(req.params.appId)
 		force = checkTruthy(req.body.force)
 		if !appId?
@@ -47,16 +46,14 @@ exports.createV1Api = (router, applications) ->
 					return service
 			.then (service) ->
 				res.status(200).json({ containerId: service.containerId })
-		.catch (err) ->
+		.catch(next)
-			res.status(503).send(err?.message or err or 'Unknown error')
 
-	router.post '/v1/apps/:appId/stop', (req, res) ->
+	createV1StopOrStartHandler = (action) -> _.partial(v1StopOrStart, _, _, _, action)
-		v1StopOrStart(req, res, 'stop')
 
-	router.post '/v1/apps/:appId/start', (req, res) ->
+	router.post('/v1/apps/:appId/stop', createV1StopOrStartHandler('stop'))
-		v1StopOrStart(req, res, 'start')
+	router.post('/v1/apps/:appId/start', createV1StopOrStartHandler('start'))
 
-	router.get '/v1/apps/:appId', (req, res) ->
+	router.get '/v1/apps/:appId', (req, res, next) ->
 		appId = checkInt(req.params.appId)
 		eventTracker.track('GET app (v1)', { appId })
 		if !appId?
@@ -82,10 +79,9 @@ exports.createV1Api = (router, applications) ->
 					appToSend.commit = status.commit
 				res.json(appToSend)
 		)
-		.catch (err) ->
+		.catch(next)
-			res.status(503).send(err?.message or err or 'Unknown error')
 
-	router.post '/v1/purge', (req, res) ->
+	router.post '/v1/purge', (req, res, next) ->
 		appId = checkInt(req.body.appId)
 		force = checkTruthy(req.body.force)
 		if !appId?
@@ -94,6 +90,4 @@ exports.createV1Api = (router, applications) ->
 		doPurge(applications, appId, force)
 		.then ->
 			res.status(200).json(Data: 'OK', Error: '')
-		.catch (err) ->
+		.catch(next)
-			res.status(503).send(err?.message or err or 'Unknown error')
-

src/device-api/v2.ts

@@ -1,5 +1,5 @@
 import * as Bluebird from 'bluebird';
-import { Request, Response, Router } from 'express';
+import { NextFunction, Request, Response, Router } from 'express';
 import * as _ from 'lodash';
 import { fs } from 'mz';
 
@@ -22,21 +22,10 @@ import { checkInt, checkTruthy } from '../lib/validation';
 export function createV2Api(router: Router, applications: ApplicationManager) {
 	const { _lockingIfNecessary, deviceState } = applications;
 
-	const messageFromError = (err?: Error | string | null): string => {
-		let message = 'Unknown error';
-		if (err != null) {
-			if (_.isError(err) && err.message != null) {
-				message = err.message;
-			} else {
-				message = err as string;
-			}
-		}
-		return message;
-	};
-
 	const handleServiceAction = (
 		req: Request,
 		res: Response,
+		next: NextFunction,
 		action: any,
 	): Bluebird<void> => {
 		const { imageId, serviceName, force } = req.body;
@@ -85,15 +74,16 @@ export function createV2Api(router: Router, applications: ApplicationManager) {
 							res.status(200).send('OK');
 						});
 				})
-				.catch(err => {
+				.catch(next);
-					res.status(503).send(messageFromError(err));
-				});
 		});
 	};
 
+	const createServiceActionHandler = (action: string) =>
+		_.partial(handleServiceAction, _, _, _, action);
+
 	router.post(
 		'/v2/applications/:appId/purge',
-		(req: Request, res: Response) => {
+		(req: Request, res: Response, next: NextFunction) => {
 			const { force } = req.body;
 			const { appId } = req.params;
 
@@ -101,45 +91,28 @@ export function createV2Api(router: Router, applications: ApplicationManager) {
 				.then(() => {
 					res.status(200).send('OK');
 				})
-				.catch(err => {
+				.catch(next);
-					let message;
-					if (err != null) {
-						message = err.message;
-						if (message == null) {
-							message = err;
-						}
-					} else {
-						message = 'Unknown error';
-					}
-					res.status(503).send(message);
-				});
 		},
 	);
 
 	router.post(
 		'/v2/applications/:appId/restart-service',
-		(req: Request, res: Response) => {
+		createServiceActionHandler('restart'),
-			return handleServiceAction(req, res, 'restart');
-		},
 	);
 
 	router.post(
 		'/v2/applications/:appId/stop-service',
-		(req: Request, res: Response) => {
+		createServiceActionHandler('stop'),
-			return handleServiceAction(req, res, 'stop');
-		},
 	);
 
 	router.post(
 		'/v2/applications/:appId/start-service',
-		(req: Request, res: Response) => {
+		createServiceActionHandler('start'),
-			return handleServiceAction(req, res, 'start');
-		},
 	);
 
 	router.post(
 		'/v2/applications/:appId/restart',
-		(req: Request, res: Response) => {
+		(req: Request, res: Response, next: NextFunction) => {
 			const { force } = req.body;
 			const { appId } = req.params;
 
@@ -147,14 +120,14 @@ export function createV2Api(router: Router, applications: ApplicationManager) {
 				.then(() => {
 					res.status(200).send('OK');
 				})
-				.catch(err => {
+				.catch(next);
-					res.status(503).send(messageFromError(err));
-				});
 		},
 	);
 
 	// TODO: Support dependent applications when this feature is complete
-	router.get('/v2/applications/state', async (_req: Request, res: Response) => {
+	router.get(
+		'/v2/applications/state',
+		async (_req: Request, res: Response, next: NextFunction) => {
 			// It's kinda hacky to access the services and db via the application manager
 			// maybe refactor this code
 			Bluebird.join(
@@ -224,21 +197,24 @@ export function createV2Api(router: Router, applications: ApplicationManager) {
 
 					res.status(200).json(response);
 				},
+			).catch(next);
+		},
 	);
-	});
 
 	router.get(
 		'/v2/applications/:appId/state',
-		(_req: Request, res: Response) => {
+		(_req: Request, res: Response, next: NextFunction) => {
 			// Get all services and their statuses, and return it
-			applications.getStatus().then(apps => {
+			applications
+				.getStatus()
+				.then(apps => {
 					res.status(200).json(apps);
-			});
+				})
+				.catch(next);
 		},
 	);
 
 	router.get('/v2/local/target-state', async (_req, res) => {
-		try {
 		const localMode = await deviceState.config.get('localMode');
 		if (!localMode) {
 			return res.status(400).json({
@@ -292,18 +268,11 @@ export function createV2Api(router: Router, applications: ApplicationManager) {
 			status: 'success',
 			state: target,
 		});
-		} catch (err) {
-			res.status(503).send({
-				status: 'failed',
-				message: messageFromError(err),
-			});
-		}
 	});
 
 	router.post('/v2/local/target-state', async (req, res) => {
 		// let's first ensure that we're in local mode, otherwise
 		// this function should not do anything
-		try {
 		const localMode = await deviceState.config.get('localMode');
 		if (!localMode) {
 			return res.status(400).json({
@@ -328,19 +297,11 @@ export function createV2Api(router: Router, applications: ApplicationManager) {
 				message: e.message,
 			});
 		}
-		} catch (e) {
-			const message = 'Could not apply target state: ';
-			res.status(503).json({
-				status: 'failed',
-				message: message + messageFromError(e),
-			});
-		}
 	});
 
 	router.get('/v2/local/device-info', async (_req, res) => {
 		// Return the device type and slug so that local mode builds can use this to
 		// resolve builds
-		try {
 		// FIXME: We should be mounting the following file into the supervisor from the
 		// start-resin-supervisor script, changed in meta-resin - but until then, hardcode it
 		const data = await fs.readFile(
@@ -356,13 +317,6 @@ export function createV2Api(router: Router, applications: ApplicationManager) {
 				deviceType: deviceInfo.slug,
 			},
 		});
-		} catch (e) {
-			const message = 'Could not fetch device information: ';
-			res.status(503).json({
-				status: 'failed',
-				message: message + messageFromError(e),
-			});
-		}
 	});
 
 	router.get('/v2/local/logs', async (_req, res) => {
@@ -392,15 +346,11 @@ export function createV2Api(router: Router, applications: ApplicationManager) {
 	});
 
 	router.get('/v2/containerId', async (req, res) => {
-		try {
 		const services = await applications.services.getAll();
 
 		if (req.query.serviceName != null || req.query.service != null) {
 			const serviceName = req.query.serviceName || req.query.service;
-				const service = _.find(
+			const service = _.find(services, svc => svc.serviceName === serviceName);
-					services,
-					svc => svc.serviceName === serviceName,
-				);
 			if (service != null) {
 				res.status(200).json({
 					status: 'success',
@@ -421,12 +371,6 @@ export function createV2Api(router: Router, applications: ApplicationManager) {
 					.value(),
 			});
 		}
-		} catch (e) {
-			res.status(503).json({
-				status: 'failed',
-				message: messageFromError(e),
-			});
-		}
 	});
 
 	router.get('/v2/state/status', async (_req, res) => {
@@ -481,60 +425,36 @@ export function createV2Api(router: Router, applications: ApplicationManager) {
 	});
 
 	router.get('/v2/device/name', async (_req, res) => {
-		try {
 		const deviceName = await applications.config.get('name');
 		res.json({
 			status: 'success',
 			deviceName,
 		});
-		} catch (e) {
-			res.status(503).json({
-				status: 'failed',
-				message: messageFromError(e),
-			});
-		}
 	});
 
 	router.get('/v2/device/tags', async (_req, res) => {
-		try {
 		const tags = await applications.apiBinder.fetchDeviceTags();
 		return res.json({
 			status: 'success',
 			tags,
 		});
-		} catch (e) {
-			res.status(503).json({
-				status: 'failed',
-				message: messageFromError(e),
-			});
-		}
 	});
 
 	router.get('/v2/cleanup-volumes', async (_req, res) => {
-		try {
 		const targetState = await applications.getTargetApps();
 		const referencedVolumes: string[] = [];
 		_.each(targetState, app => {
 			_.each(app.volumes, vol => {
-					referencedVolumes.push(
+				referencedVolumes.push(Volume.generateDockerName(vol.appId, vol.name));
-						Volume.generateDockerName(vol.appId, vol.name),
-					);
 			});
 		});
 		await applications.volumes.removeOrphanedVolumes(referencedVolumes);
 		res.json({
 			status: 'success',
 		});
-		} catch (e) {
-			res.status(503).json({
-				status: 'failed',
-				message: messageFromError(e),
-			});
-		}
 	});
 
 	router.post('/v2/journal-logs', (req, res) => {
-		try {
 		const all = checkTruthy(req.body.all) || false;
 		const follow = checkTruthy(req.body.follow) || false;
 		const count = checkInt(req.body.count, { positive: true }) || undefined;
@@ -551,15 +471,5 @@ export function createV2Api(router: Router, applications: ApplicationManager) {
 			journald.stdout.unpipe();
 			res.end();
 		});
-		} catch (e) {
-			log.error('There was an error reading the journalctl process', e);
-			if (res.headersSent) {
-				return;
-			}
-			res.json({
-				status: 'failed',
-				message: messageFromError(e),
-			});
-		}
 	});
 }

src/supervisor-api.ts

@@ -1,3 +1,4 @@
+import { NextFunction, Request, Response } from 'express';
 import * as express from 'express';
 import { Server } from 'http';
 import * as _ from 'lodash';
@@ -102,15 +103,11 @@ export class SupervisorAPI {
 		this.api.use(expressLogger);
 
 		this.api.get('/v1/healthy', async (_req, res) => {
-			try {
 			const healths = await Promise.all(this.healthchecks.map(fn => fn()));
 			if (!_.every(healths)) {
 				throw new Error('Unhealthy');
 			}
 			return res.sendStatus(200);
-			} catch (e) {
-				res.sendStatus(500);
-			}
 		});
 
 		this.api.get('/ping', (_req, res) => res.send('OK'));
@@ -127,19 +124,44 @@ export class SupervisorAPI {
 		// Expires the supervisor's API key and generates a new one.
 		// It also communicates the new key to the balena API.
 		this.api.post('/v1/regenerate-api-key', async (_req, res) => {
-			try {
 			const secret = await this.config.newUniqueKey();
 			await this.config.set({ apiSecret: secret });
 			res.status(200).send(secret);
-			} catch (e) {
-				res.status(503).send(e != null ? e.message : e || 'Unknown error');
-			}
 		});
 
 		// And assign all external routers
 		for (const router of this.routers) {
 			this.api.use(router);
 		}
+
+		// Error handling.
+
+		const messageFromError = (err?: Error | string | null): string => {
+			let message = 'Unknown error';
+			if (err != null) {
+				if (_.isError(err) && err.message != null) {
+					message = err.message;
+				} else {
+					message = err as string;
+				}
+			}
+			return message;
+		};
+
+		this.api.use(
+			(err: Error, req: Request, res: Response, next: NextFunction) => {
+				if (res.headersSent) {
+					// Error happens while we are writing the response - default handler closes the connection.
+					next(err);
+					return;
+				}
+				log.error(`Error on ${req.method} ${req.path}: `, err);
+				res.status(503).send({
+					status: 'failed',
+					message: messageFromError(err),
+				});
+			},
+		);
 	}
 
 	public async listen(