Merge pull request #1095 from balena-io/roman/error-handling

Unify API errors processing

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,192 +120,181 @@ 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(
-		// It's kinda hacky to access the services and db via the application manager
+		'/v2/applications/state',
-		// maybe refactor this code
+		async (_req: Request, res: Response, next: NextFunction) => {
-		Bluebird.join(
+			// It's kinda hacky to access the services and db via the application manager
-			applications.services.getStatus(),
+			// maybe refactor this code
-			applications.images.getStatus(),
+			Bluebird.join(
-			applications.db.models('app').select(['appId', 'commit', 'name']),
+				applications.services.getStatus(),
-			(
+				applications.images.getStatus(),
-				services,
+				applications.db.models('app').select(['appId', 'commit', 'name']),
-				images,
+				(
-				apps: Array<{ appId: string; commit: string; name: string }>,
+					services,
-			) => {
+					images,
-				// Create an object which is keyed my application name
+					apps: Array<{ appId: string; commit: string; name: string }>,
-				const response: {
+				) => {
-					[appName: string]: {
+					// Create an object which is keyed my application name
-						appId: number;
+					const response: {
-						commit: string;
+						[appName: string]: {
-						services: {
+							appId: number;
-							[serviceName: string]: {
+							commit: string;
-								status: string;
+							services: {
-								releaseId: number;
+								[serviceName: string]: {
-								downloadProgress: number | null;
+									status: string;
+									releaseId: number;
+									downloadProgress: number | null;
+								};
 							};
 						};
-					};
+					} = {};
-				} = {};
 
-				const appNameById: { [id: number]: string } = {};
+					const appNameById: { [id: number]: string } = {};
 
-				apps.forEach(app => {
+					apps.forEach(app => {
-					const appId = parseInt(app.appId, 10);
+						const appId = parseInt(app.appId, 10);
-					response[app.name] = {
+						response[app.name] = {
-						appId,
+							appId,
-						commit: app.commit,
+							commit: app.commit,
-						services: {},
+							services: {},
-					};
+						};
 
-					appNameById[appId] = app.name;
+						appNameById[appId] = app.name;
-				});
-
-				images.forEach(img => {
-					const appName = appNameById[img.appId];
-					if (appName == null) {
-						log.warn(
-							`Image found for unknown application!\nImage: ${JSON.stringify(
-								img,
-							)}`,
-						);
-						return;
-					}
-
-					const svc = _.find(services, (service: Service) => {
-						return service.imageId === img.imageId;
 					});
 
-					let status: string;
+					images.forEach(img => {
-					if (svc == null) {
+						const appName = appNameById[img.appId];
-						status = img.status;
+						if (appName == null) {
-					} else {
+							log.warn(
-						status = svc.status || img.status;
+								`Image found for unknown application!\nImage: ${JSON.stringify(
-					}
+									img,
-					response[appName].services[img.serviceName] = {
+								)}`,
-						status,
+							);
-						releaseId: img.releaseId,
+							return;
-						downloadProgress: img.downloadProgress || null,
+						}
-					};
-				});
 
-				res.status(200).json(response);
+						const svc = _.find(services, (service: Service) => {
-			},
+							return service.imageId === img.imageId;
-		);
+						});
-	});
+
+						let status: string;
+						if (svc == null) {
+							status = img.status;
+						} else {
+							status = svc.status || img.status;
+						}
+						response[appName].services[img.serviceName] = {
+							status,
+							releaseId: img.releaseId,
+							downloadProgress: img.downloadProgress || null,
+						};
+					});
+
+					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
-				res.status(200).json(apps);
+				.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');
-			const localMode = await deviceState.config.get('localMode');
+		if (!localMode) {
-			if (!localMode) {
+			return res.status(400).json({
-				return res.status(400).json({
-					status: 'failed',
-					message: 'Target state can only be retrieved when in local mode',
-				});
-			}
-
-			const targetState = await deviceState.getTarget();
-
-			// We avoid using cloneDeep here, as the class
-			// instances can cause a maximum call stack exceeded
-			// error
-
-			// TODO: This should really return the config as it
-			// is returned from the api, but currently that's not
-			// the easiest thing due to the way they are stored and
-			// retrieved from the db - when all of the application
-			// manager is strongly typed, revisit this. The best
-			// thing to do would be to represent the input with
-			// io-ts and make sure the below conforms to it
-
-			const target: any = {
-				local: {
-					config: {},
-				},
-				dependent: {
-					config: {},
-				},
-			};
-			if (targetState.local != null) {
-				target.local = {
-					name: targetState.local.name,
-					config: _.cloneDeep(targetState.local.config),
-					apps: _.mapValues(targetState.local.apps, app => ({
-						appId: app.appId,
-						name: app.name,
-						commit: app.commit,
-						releaseId: app.releaseId,
-						services: _.map(app.services, s => s.toComposeObject()),
-						volumes: _.mapValues(app.volumes, v => v.toComposeObject()),
-						networks: _.mapValues(app.networks, n => n.toComposeObject()),
-					})),
-				};
-			}
-			if (targetState.dependent != null) {
-				target.dependent = _.cloneDeep(target.dependent);
-			}
-
-			res.status(200).json({
-				status: 'success',
-				state: target,
-			});
-		} catch (err) {
-			res.status(503).send({
 				status: 'failed',
-				message: messageFromError(err),
+				message: 'Target state can only be retrieved when in local mode',
 			});
 		}
+
+		const targetState = await deviceState.getTarget();
+
+		// We avoid using cloneDeep here, as the class
+		// instances can cause a maximum call stack exceeded
+		// error
+
+		// TODO: This should really return the config as it
+		// is returned from the api, but currently that's not
+		// the easiest thing due to the way they are stored and
+		// retrieved from the db - when all of the application
+		// manager is strongly typed, revisit this. The best
+		// thing to do would be to represent the input with
+		// io-ts and make sure the below conforms to it
+
+		const target: any = {
+			local: {
+				config: {},
+			},
+			dependent: {
+				config: {},
+			},
+		};
+		if (targetState.local != null) {
+			target.local = {
+				name: targetState.local.name,
+				config: _.cloneDeep(targetState.local.config),
+				apps: _.mapValues(targetState.local.apps, app => ({
+					appId: app.appId,
+					name: app.name,
+					commit: app.commit,
+					releaseId: app.releaseId,
+					services: _.map(app.services, s => s.toComposeObject()),
+					volumes: _.mapValues(app.volumes, v => v.toComposeObject()),
+					networks: _.mapValues(app.networks, n => n.toComposeObject()),
+				})),
+			};
+		}
+		if (targetState.dependent != null) {
+			target.dependent = _.cloneDeep(target.dependent);
+		}
+
+		res.status(200).json({
+			status: 'success',
+			state: target,
+		});
 	});
 
 	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');
-			const localMode = await deviceState.config.get('localMode');
+		if (!localMode) {
-			if (!localMode) {
+			return res.status(400).json({
-				return res.status(400).json({
-					status: 'failed',
-					message: 'Target state can only set when device is in local mode',
-				});
-			}
-
-			// Now attempt to set the state
-			const force = req.body.force;
-			const targetState = req.body;
-			try {
-				await deviceState.setTarget(targetState, true);
-				await deviceState.triggerApplyTarget({ force });
-				res.status(200).json({
-					status: 'success',
-					message: 'OK',
-				});
-			} catch (e) {
-				res.status(400).json({
-					status: 'failed',
-					message: e.message,
-				});
-			}
-		} catch (e) {
-			const message = 'Could not apply target state: ';
-			res.status(503).json({
 				status: 'failed',
-				message: message + messageFromError(e),
+				message: 'Target state can only set when device is in local mode',
+			});
+		}
+
+		// Now attempt to set the state
+		const force = req.body.force;
+		const targetState = req.body;
+		try {
+			await deviceState.setTarget(targetState, true);
+			await deviceState.triggerApplyTarget({ force });
+			res.status(200).json({
+				status: 'success',
+				message: 'OK',
+			});
+		} catch (e) {
+			res.status(400).json({
+				status: 'failed',
+				message: e.message,
 			});
 		}
 	});
@@ -340,29 +302,21 @@ export function createV2Api(router: Router, applications: ApplicationManager) {
 	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
-			// 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
-			// start-resin-supervisor script, changed in meta-resin - but until then, hardcode it
+		const data = await fs.readFile(
-			const data = await fs.readFile(
+			'/mnt/root/resin-boot/device-type.json',
-				'/mnt/root/resin-boot/device-type.json',
+			'utf8',
-				'utf8',
+		);
-			);
+		const deviceInfo = JSON.parse(data);
-			const deviceInfo = JSON.parse(data);
 
-			return res.status(200).json({
+		return res.status(200).json({
-				status: 'success',
+			status: 'success',
-				info: {
+			info: {
-					arch: deviceInfo.arch,
+				arch: deviceInfo.arch,
-					deviceType: deviceInfo.slug,
+				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,39 +346,29 @@ export function createV2Api(router: Router, applications: ApplicationManager) {
 	});
 
 	router.get('/v2/containerId', async (req, res) => {
-		try {
+		const services = await applications.services.getAll();
-			const services = await applications.services.getAll();
 
-			if (req.query.serviceName != null || req.query.service != null) {
+		if (req.query.serviceName != null || req.query.service != null) {
-				const serviceName = req.query.serviceName || req.query.service;
+			const serviceName = req.query.serviceName || req.query.service;
-				const service = _.find(
+			const service = _.find(services, svc => svc.serviceName === serviceName);
-					services,
+			if (service != null) {
-					svc => svc.serviceName === serviceName,
-				);
-				if (service != null) {
-					res.status(200).json({
-						status: 'success',
-						containerId: service.containerId,
-					});
-				} else {
-					res.status(503).json({
-						status: 'failed',
-						message: 'Could not find service with that name',
-					});
-				}
-			} else {
 				res.status(200).json({
 					status: 'success',
-					services: _(services)
+					containerId: service.containerId,
-						.keyBy('serviceName')
+				});
-						.mapValues('containerId')
+			} else {
-						.value(),
+				res.status(503).json({
+					status: 'failed',
+					message: 'Could not find service with that name',
 				});
 			}
-		} catch (e) {
+		} else {
-			res.status(503).json({
+			res.status(200).json({
-				status: 'failed',
+				status: 'success',
-				message: messageFromError(e),
+				services: _(services)
+					.keyBy('serviceName')
+					.mapValues('containerId')
+					.value(),
 			});
 		}
 	});
@@ -481,85 +425,51 @@ export function createV2Api(router: Router, applications: ApplicationManager) {
 	});
 
 	router.get('/v2/device/name', async (_req, res) => {
-		try {
+		const deviceName = await applications.config.get('name');
-			const deviceName = await applications.config.get('name');
+		res.json({
-			res.json({
+			status: 'success',
-				status: 'success',
+			deviceName,
-				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();
-			const tags = await applications.apiBinder.fetchDeviceTags();
+		return res.json({
-			return res.json({
+			status: 'success',
-				status: 'success',
+			tags,
-				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 targetState = await applications.getTargetApps();
+		const referencedVolumes: string[] = [];
-			const referencedVolumes: string[] = [];
+		_.each(targetState, app => {
-			_.each(targetState, app => {
+			_.each(app.volumes, vol => {
-				_.each(app.volumes, vol => {
+				referencedVolumes.push(Volume.generateDockerName(vol.appId, vol.name));
-					referencedVolumes.push(
-						Volume.generateDockerName(vol.appId, vol.name),
-					);
-				});
 			});
-			await applications.volumes.removeOrphanedVolumes(referencedVolumes);
+		});
-			res.json({
+		await applications.volumes.removeOrphanedVolumes(referencedVolumes);
-				status: 'success',
+		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 all = checkTruthy(req.body.all) || false;
+		const follow = checkTruthy(req.body.follow) || false;
-			const follow = checkTruthy(req.body.follow) || false;
+		const count = checkInt(req.body.count, { positive: true }) || undefined;
-			const count = checkInt(req.body.count, { positive: true }) || undefined;
+		const unit = req.body.unit;
-			const unit = req.body.unit;
+		const format = req.body.format || 'short';
-			const format = req.body.format || 'short';
 
-			const journald = spawnJournalctl({ all, follow, count, unit, format });
+		const journald = spawnJournalctl({ all, follow, count, unit, format });
-			res.status(200);
+		res.status(200);
-			journald.stdout.pipe(res);
+		journald.stdout.pipe(res);
-			res.on('close', () => {
+		res.on('close', () => {
-				journald.kill('SIGKILL');
+			journald.kill('SIGKILL');
-			});
+		});
-			journald.on('exit', () => {
+		journald.on('exit', () => {
-				journald.stdout.unpipe();
+			journald.stdout.unpipe();
-				res.end();
+			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()));
-				const healths = await Promise.all(this.healthchecks.map(fn => fn()));
+			if (!_.every(healths)) {
-				if (!_.every(healths)) {
+				throw new Error('Unhealthy');
-					throw new Error('Unhealthy');
-				}
-				return res.sendStatus(200);
-			} catch (e) {
-				res.sendStatus(500);
 			}
+			return res.sendStatus(200);
 		});
 
 		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();
-				const secret = await this.config.newUniqueKey();
+			await this.config.set({ apiSecret: secret });
-				await this.config.set({ apiSecret: secret });
+			res.status(200).send(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(