From 7d0d43c51a40143c1482b87b2fafbfbfd700f9ae Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Mon, 2 Dec 2024 17:07:51 +0100 Subject: [PATCH 01/10] fix: drain request after shutdown Signed-off-by: Camila Belo --- .../src/wiring/BackendInitializer.ts | 33 ++++++++++++++----- .../rootHealthServiceFactory.test.ts | 18 ++++++++++ .../rootHealth/rootHealthServiceFactory.ts | 3 +- 3 files changed, 44 insertions(+), 10 deletions(-) diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index 3e7b46fc9e..101c9abeb7 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -65,9 +65,18 @@ const instanceRegistry = new (class InstanceRegistry { if (!this.#registered) { this.#registered = true; - process.addListener('SIGTERM', this.#exitHandler); - process.addListener('SIGINT', this.#exitHandler); - process.addListener('beforeExit', this.#exitHandler); + process.addListener('SIGTERM', () => { + this.#exitHandler(); + }); + process.addListener('SIGINT', () => { + this.#exitHandler(); + }); + process.addListener('SIGQUIT', () => { + process.exit(0); + }); + process.addListener('beforeExit', () => { + this.#exitHandler(); + }); } this.#instances.add(instance); @@ -79,12 +88,18 @@ const instanceRegistry = new (class InstanceRegistry { #exitHandler = async () => { try { - const results = await Promise.allSettled( - Array.from(this.#instances).map(b => b.stop()), - ); - const errors = results.flatMap(r => - r.status === 'rejected' ? [r.reason] : [], - ); + // This signals to the healthcheck service that the process is shutting down + process.exitCode = 0; + + const results = await Promise.race([ + // Give the backend 30 seconds to shut down, then force exit + new Promise(resolve => setTimeout(resolve, 30000)), + Promise.allSettled(Array.from(this.#instances).map(b => b.stop())), + ]); + + const errors = Array.isArray(results) + ? results.flatMap(r => (r.status === 'rejected' ? [r.reason] : [])) + : []; if (errors.length > 0) { for (const error of errors) { diff --git a/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.test.ts b/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.test.ts index 0eee5c18c6..c84c225fac 100644 --- a/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.test.ts +++ b/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.test.ts @@ -51,6 +51,24 @@ describe('DefaultRootHealthService', () => { }); }); + it(`should return a 503 response if the server is stopping`, async () => { + const service = new DefaultRootHealthService({ + lifecycle: mockServices.rootLifecycle.mock(), + }); + + process.exitCode = 1; + + await expect(service.getReadiness()).resolves.toEqual({ + status: 503, + payload: { + message: 'Backend has not started yet', + status: 'error', + }, + }); + + process.exitCode = undefined; // Reset exitCode for other tests + }); + it(`should return a 500 response if the server has stopped`, async () => { let mockServerStartedFn = () => {}; let mockServerStoppedFn = () => {}; diff --git a/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.ts b/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.ts index c8b4ad5394..b37a6e589f 100644 --- a/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.ts @@ -39,7 +39,8 @@ export class DefaultRootHealthService implements RootHealthService { } async getReadiness(): Promise<{ status: number; payload?: any }> { - if (!this.#isRunning) { + // If the process has been told to exit, or if the backend has not started yet + if (process.exitCode !== undefined || !this.#isRunning) { return { status: 503, payload: { message: 'Backend has not started yet', status: 'error' }, From 5171d5c742d140b0965ad452a34b64f3d51e6be6 Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Tue, 3 Dec 2024 13:36:18 +0100 Subject: [PATCH 02/10] refactor: create pre shutdown lifecycle Signed-off-by: Camila Belo --- .../src/wiring/BackendInitializer.ts | 42 +++++++------------ .../wiring/createSpecializedBackend.test.ts | 2 + .../src/CreateBackend.test.ts | 2 + .../rootHealthServiceFactory.test.ts | 24 ++--------- .../rootHealth/rootHealthServiceFactory.ts | 5 +-- .../rootHttpRouterServiceFactory.ts | 2 +- .../rootLifecycleServiceFactory.ts | 31 ++++++++++++++ .../lib/PluginTaskSchedulerImpl.test.ts | 6 ++- .../definitions/RootLifecycleService.ts | 4 +- .../src/next/services/mockServices.ts | 1 + 10 files changed, 66 insertions(+), 53 deletions(-) diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index 101c9abeb7..86a9aa6405 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -65,18 +65,9 @@ const instanceRegistry = new (class InstanceRegistry { if (!this.#registered) { this.#registered = true; - process.addListener('SIGTERM', () => { - this.#exitHandler(); - }); - process.addListener('SIGINT', () => { - this.#exitHandler(); - }); - process.addListener('SIGQUIT', () => { - process.exit(0); - }); - process.addListener('beforeExit', () => { - this.#exitHandler(); - }); + process.addListener('SIGTERM', this.#exitHandler); + process.addListener('SIGINT', this.#exitHandler); + process.addListener('beforeExit', this.#exitHandler); } this.#instances.add(instance); @@ -88,18 +79,12 @@ const instanceRegistry = new (class InstanceRegistry { #exitHandler = async () => { try { - // This signals to the healthcheck service that the process is shutting down - process.exitCode = 0; - - const results = await Promise.race([ - // Give the backend 30 seconds to shut down, then force exit - new Promise(resolve => setTimeout(resolve, 30000)), - Promise.allSettled(Array.from(this.#instances).map(b => b.stop())), - ]); - - const errors = Array.isArray(results) - ? results.flatMap(r => (r.status === 'rejected' ? [r.reason] : [])) - : []; + const results = await Promise.allSettled( + Array.from(this.#instances).map(b => b.stop()), + ); + const errors = results.flatMap(r => + r.status === 'rejected' ? [r.reason] : [], + ); if (errors.length > 0) { for (const error of errors) { @@ -464,6 +449,11 @@ export class BackendInitializer { // The startup failed, but we may still want to do cleanup so we continue silently } + const rootLifecycleService = await this.#getRootLifecycleImpl(); + + // Root services like the health one need to immediatelly be notified of the shutdown + await rootLifecycleService.preShutdown(); + // Get all plugins. const allPlugins = new Set(); for (const feature of this.#registrations) { @@ -483,14 +473,14 @@ export class BackendInitializer { ); // Once all plugin shutdown hooks are done, run root shutdown hooks. - const lifecycleService = await this.#getRootLifecycleImpl(); - await lifecycleService.shutdown(); + await rootLifecycleService.shutdown(); } // Bit of a hacky way to grab the lifecycle services, potentially find a nicer way to do this async #getRootLifecycleImpl(): Promise< RootLifecycleService & { startup(): Promise; + preShutdown(): Promise; shutdown(): Promise; } > { diff --git a/packages/backend-app-api/src/wiring/createSpecializedBackend.test.ts b/packages/backend-app-api/src/wiring/createSpecializedBackend.test.ts index 1fd7fcbbec..32c1fbcd0e 100644 --- a/packages/backend-app-api/src/wiring/createSpecializedBackend.test.ts +++ b/packages/backend-app-api/src/wiring/createSpecializedBackend.test.ts @@ -36,6 +36,7 @@ describe('createSpecializedBackend', () => { deps: {}, factory: async () => ({ addStartupHook: () => {}, + addBeforeShutdownHook: () => {}, addShutdownHook: () => {}, }), }), @@ -44,6 +45,7 @@ describe('createSpecializedBackend', () => { deps: {}, factory: async () => ({ addStartupHook: () => {}, + addBeforeShutdownHook: () => {}, addShutdownHook: () => {}, }), }), diff --git a/packages/backend-defaults/src/CreateBackend.test.ts b/packages/backend-defaults/src/CreateBackend.test.ts index 55fdbe5a6a..57977e9647 100644 --- a/packages/backend-defaults/src/CreateBackend.test.ts +++ b/packages/backend-defaults/src/CreateBackend.test.ts @@ -47,6 +47,7 @@ describe('createBackend', () => { deps: {}, factory: async () => ({ addStartupHook: () => {}, + addBeforeShutdownHook: () => {}, addShutdownHook: () => {}, }), }), @@ -57,6 +58,7 @@ describe('createBackend', () => { deps: {}, factory: async () => ({ addStartupHook: () => {}, + addBeforeShutdownHook: () => {}, addShutdownHook: () => {}, }), }), diff --git a/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.test.ts b/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.test.ts index c84c225fac..32fc10a77f 100644 --- a/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.test.ts +++ b/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.test.ts @@ -51,31 +51,13 @@ describe('DefaultRootHealthService', () => { }); }); - it(`should return a 503 response if the server is stopping`, async () => { - const service = new DefaultRootHealthService({ - lifecycle: mockServices.rootLifecycle.mock(), - }); - - process.exitCode = 1; - - await expect(service.getReadiness()).resolves.toEqual({ - status: 503, - payload: { - message: 'Backend has not started yet', - status: 'error', - }, - }); - - process.exitCode = undefined; // Reset exitCode for other tests - }); - it(`should return a 500 response if the server has stopped`, async () => { let mockServerStartedFn = () => {}; - let mockServerStoppedFn = () => {}; + let mockServerBeforeStoppedFn = () => {}; const lifecycle = mockServices.rootLifecycle.mock({ addStartupHook: jest.fn(fn => (mockServerStartedFn = fn)), - addShutdownHook: jest.fn(fn => (mockServerStoppedFn = fn)), + addBeforeShutdownHook: jest.fn(fn => (mockServerBeforeStoppedFn = fn)), }); const service = new DefaultRootHealthService({ @@ -83,7 +65,7 @@ describe('DefaultRootHealthService', () => { }); mockServerStartedFn(); - mockServerStoppedFn(); + mockServerBeforeStoppedFn(); await expect(service.getReadiness()).resolves.toEqual({ status: 503, payload: { diff --git a/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.ts b/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.ts index b37a6e589f..83efc5a055 100644 --- a/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.ts @@ -29,7 +29,7 @@ export class DefaultRootHealthService implements RootHealthService { options.lifecycle.addStartupHook(() => { this.#isRunning = true; }); - options.lifecycle.addShutdownHook(() => { + options.lifecycle.addPreShutdownHook(() => { this.#isRunning = false; }); } @@ -39,8 +39,7 @@ export class DefaultRootHealthService implements RootHealthService { } async getReadiness(): Promise<{ status: number; payload?: any }> { - // If the process has been told to exit, or if the backend has not started yet - if (process.exitCode !== undefined || !this.#isRunning) { + if (!this.#isRunning) { return { status: 503, payload: { message: 'Backend has not started yet', status: 'error' }, diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts index 4e3483d67e..3a5747057f 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts @@ -120,7 +120,7 @@ const rootHttpRouterServiceFactoryWithOptions = ( }, }); - lifecycle.addShutdownHook(() => server.stop()); + lifecycle.addPreShutdownHook(() => server.stop()); await server.start(); diff --git a/packages/backend-defaults/src/entrypoints/rootLifecycle/rootLifecycleServiceFactory.ts b/packages/backend-defaults/src/entrypoints/rootLifecycle/rootLifecycleServiceFactory.ts index e724c2f5cb..9711bd36fd 100644 --- a/packages/backend-defaults/src/entrypoints/rootLifecycle/rootLifecycleServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/rootLifecycle/rootLifecycleServiceFactory.ts @@ -65,6 +65,37 @@ export class BackendLifecycleImpl implements RootLifecycleService { ); } + #hasPreShutdown = false; + #preShutdownTasks: Array<{ hook: () => void }> = []; + + addPreShutdownHook(hook: () => void): void { + if (this.#hasPreShutdown) { + throw new Error('Attempted to add pre shutdown hook after pre shutdown'); + } + this.#preShutdownTasks.push({ hook }); + } + + async preShutdown(): Promise { + if (this.#hasPreShutdown) { + return; + } + this.#hasPreShutdown = true; + + this.logger.debug( + `Running ${this.#preShutdownTasks.length} pre shutdown tasks...`, + ); + await Promise.all( + this.#preShutdownTasks.map(async ({ hook }) => { + try { + await hook(); + this.logger.debug(`Pre shutdown hook succeeded`); + } catch (error) { + this.logger.error(`Pre shutdown hook failed, ${error}`); + } + }), + ); + } + #hasShutdown = false; #shutdownTasks: Array<{ hook: LifecycleServiceShutdownHook; diff --git a/packages/backend-defaults/src/entrypoints/scheduler/lib/PluginTaskSchedulerImpl.test.ts b/packages/backend-defaults/src/entrypoints/scheduler/lib/PluginTaskSchedulerImpl.test.ts index 3c641feadf..1ce1401829 100644 --- a/packages/backend-defaults/src/entrypoints/scheduler/lib/PluginTaskSchedulerImpl.test.ts +++ b/packages/backend-defaults/src/entrypoints/scheduler/lib/PluginTaskSchedulerImpl.test.ts @@ -55,7 +55,11 @@ describe('PluginTaskManagerImpl', () => { const manager = new PluginTaskSchedulerImpl( async () => knex, mockServices.logger.mock(), - { addShutdownHook, addStartupHook: jest.fn() }, + { + addShutdownHook, + addBeforeShutdownHook: jest.fn(), + addStartupHook: jest.fn(), + }, ); return { knex, manager }; } diff --git a/packages/backend-plugin-api/src/services/definitions/RootLifecycleService.ts b/packages/backend-plugin-api/src/services/definitions/RootLifecycleService.ts index b962fa68ba..15b744e35b 100644 --- a/packages/backend-plugin-api/src/services/definitions/RootLifecycleService.ts +++ b/packages/backend-plugin-api/src/services/definitions/RootLifecycleService.ts @@ -23,4 +23,6 @@ import { LifecycleService } from './LifecycleService'; * * @public */ -export interface RootLifecycleService extends LifecycleService {} +export interface RootLifecycleService extends LifecycleService { + addPreShutdownHook(hook: () => void): void; +} diff --git a/packages/backend-test-utils/src/next/services/mockServices.ts b/packages/backend-test-utils/src/next/services/mockServices.ts index a2c3b34ead..b9bca62767 100644 --- a/packages/backend-test-utils/src/next/services/mockServices.ts +++ b/packages/backend-test-utils/src/next/services/mockServices.ts @@ -472,6 +472,7 @@ export namespace mockServices { export const factory = () => rootLifecycleServiceFactory; export const mock = simpleMock(coreServices.rootLifecycle, () => ({ addShutdownHook: jest.fn(), + addBeforeShutdownHook: jest.fn(), addStartupHook: jest.fn(), })); } From 255e829be2c006fe0651324ea91ed7d6815a932c Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Tue, 3 Dec 2024 14:44:40 +0100 Subject: [PATCH 03/10] refactor: use before shutdown hook name Signed-off-by: Camila Belo --- .../src/wiring/BackendInitializer.ts | 4 +-- .../rootHealth/rootHealthServiceFactory.ts | 2 +- .../rootHttpRouterServiceFactory.ts | 2 +- .../rootLifecycleServiceFactory.ts | 28 ++++++++++--------- .../definitions/RootLifecycleService.ts | 2 +- 5 files changed, 20 insertions(+), 18 deletions(-) diff --git a/packages/backend-app-api/src/wiring/BackendInitializer.ts b/packages/backend-app-api/src/wiring/BackendInitializer.ts index 86a9aa6405..53c87c06c2 100644 --- a/packages/backend-app-api/src/wiring/BackendInitializer.ts +++ b/packages/backend-app-api/src/wiring/BackendInitializer.ts @@ -452,7 +452,7 @@ export class BackendInitializer { const rootLifecycleService = await this.#getRootLifecycleImpl(); // Root services like the health one need to immediatelly be notified of the shutdown - await rootLifecycleService.preShutdown(); + await rootLifecycleService.beforeShutdown(); // Get all plugins. const allPlugins = new Set(); @@ -480,7 +480,7 @@ export class BackendInitializer { async #getRootLifecycleImpl(): Promise< RootLifecycleService & { startup(): Promise; - preShutdown(): Promise; + beforeShutdown(): Promise; shutdown(): Promise; } > { diff --git a/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.ts b/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.ts index 83efc5a055..ab9fc86a41 100644 --- a/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.ts @@ -29,7 +29,7 @@ export class DefaultRootHealthService implements RootHealthService { options.lifecycle.addStartupHook(() => { this.#isRunning = true; }); - options.lifecycle.addPreShutdownHook(() => { + options.lifecycle.addBeforeShutdownHook(() => { this.#isRunning = false; }); } diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts index 3a5747057f..755535dc70 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts @@ -120,7 +120,7 @@ const rootHttpRouterServiceFactoryWithOptions = ( }, }); - lifecycle.addPreShutdownHook(() => server.stop()); + lifecycle.addBeforeShutdownHook(() => server.stop()); await server.start(); diff --git a/packages/backend-defaults/src/entrypoints/rootLifecycle/rootLifecycleServiceFactory.ts b/packages/backend-defaults/src/entrypoints/rootLifecycle/rootLifecycleServiceFactory.ts index 9711bd36fd..9a8f64edb5 100644 --- a/packages/backend-defaults/src/entrypoints/rootLifecycle/rootLifecycleServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/rootLifecycle/rootLifecycleServiceFactory.ts @@ -65,32 +65,34 @@ export class BackendLifecycleImpl implements RootLifecycleService { ); } - #hasPreShutdown = false; - #preShutdownTasks: Array<{ hook: () => void }> = []; + #hasBeforeShutdown = false; + #beforeShutdownTasks: Array<{ hook: () => void }> = []; - addPreShutdownHook(hook: () => void): void { - if (this.#hasPreShutdown) { - throw new Error('Attempted to add pre shutdown hook after pre shutdown'); + addBeforeShutdownHook(hook: () => void): void { + if (this.#hasBeforeShutdown) { + throw new Error( + 'Attempt to add before shutdown hook after shutdown has started', + ); } - this.#preShutdownTasks.push({ hook }); + this.#beforeShutdownTasks.push({ hook }); } - async preShutdown(): Promise { - if (this.#hasPreShutdown) { + async beforeShutdown(): Promise { + if (this.#hasBeforeShutdown) { return; } - this.#hasPreShutdown = true; + this.#hasBeforeShutdown = true; this.logger.debug( - `Running ${this.#preShutdownTasks.length} pre shutdown tasks...`, + `Running ${this.#beforeShutdownTasks.length} before shutdown tasks...`, ); await Promise.all( - this.#preShutdownTasks.map(async ({ hook }) => { + this.#beforeShutdownTasks.map(async ({ hook }) => { try { await hook(); - this.logger.debug(`Pre shutdown hook succeeded`); + this.logger.debug(`Before shutdown hook succeeded`); } catch (error) { - this.logger.error(`Pre shutdown hook failed, ${error}`); + this.logger.error(`Before shutdown hook failed, ${error}`); } }), ); diff --git a/packages/backend-plugin-api/src/services/definitions/RootLifecycleService.ts b/packages/backend-plugin-api/src/services/definitions/RootLifecycleService.ts index 15b744e35b..085c21e9e2 100644 --- a/packages/backend-plugin-api/src/services/definitions/RootLifecycleService.ts +++ b/packages/backend-plugin-api/src/services/definitions/RootLifecycleService.ts @@ -24,5 +24,5 @@ import { LifecycleService } from './LifecycleService'; * @public */ export interface RootLifecycleService extends LifecycleService { - addPreShutdownHook(hook: () => void): void; + addBeforeShutdownHook(hook: () => void): void; } From d18e5b3650ce43845813a24efde5a2c695cab047 Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Tue, 3 Dec 2024 15:21:14 +0100 Subject: [PATCH 04/10] docs: update api report Signed-off-by: Camila Belo --- packages/backend-plugin-api/report.api.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/backend-plugin-api/report.api.md b/packages/backend-plugin-api/report.api.md index a886ba4271..5db4a9a180 100644 --- a/packages/backend-plugin-api/report.api.md +++ b/packages/backend-plugin-api/report.api.md @@ -507,7 +507,10 @@ export interface RootHttpRouterService { } // @public -export interface RootLifecycleService extends LifecycleService {} +export interface RootLifecycleService extends LifecycleService { + // (undocumented) + addBeforeShutdownHook(hook: () => void): void; +} // @public export interface RootLoggerService extends LoggerService {} From 0e9c9fafd1c460998a795e6fd199680de150f0b1 Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Tue, 3 Dec 2024 15:42:14 +0100 Subject: [PATCH 05/10] docs: add changeset files Signed-off-by: Camila Belo --- .changeset/green-carpets-smell.md | 5 +++++ .changeset/large-birds-watch.md | 5 +++++ .changeset/pretty-kiwis-travel.md | 5 +++++ .changeset/young-cobras-add.md | 5 +++++ 4 files changed, 20 insertions(+) create mode 100644 .changeset/green-carpets-smell.md create mode 100644 .changeset/large-birds-watch.md create mode 100644 .changeset/pretty-kiwis-travel.md create mode 100644 .changeset/young-cobras-add.md diff --git a/.changeset/green-carpets-smell.md b/.changeset/green-carpets-smell.md new file mode 100644 index 0000000000..b9d547e707 --- /dev/null +++ b/.changeset/green-carpets-smell.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-plugin-api': patch +--- + +The `RootLifecycleService` now has a new `addBeforeShutdownHook` method, and hooks added through this method will run immediately when a termination event is received. diff --git a/.changeset/large-birds-watch.md b/.changeset/large-birds-watch.md new file mode 100644 index 0000000000..24d63492bc --- /dev/null +++ b/.changeset/large-birds-watch.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-test-utils': patch +--- + +Mock the new `RootLifecycleService.addBeforeShutdownHook` method. diff --git a/.changeset/pretty-kiwis-travel.md b/.changeset/pretty-kiwis-travel.md new file mode 100644 index 0000000000..f3d5cbeb27 --- /dev/null +++ b/.changeset/pretty-kiwis-travel.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-defaults': patch +--- + +Implements the `DefaultRootLifecycleService.addBeforeShutdownHook` method, and updates `DefaultRootHttpRouterService` and `DefaultRootHealthService` to listen to that event to stop accepting traffic and close service connections. diff --git a/.changeset/young-cobras-add.md b/.changeset/young-cobras-add.md new file mode 100644 index 0000000000..f9172d3800 --- /dev/null +++ b/.changeset/young-cobras-add.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-app-api': patch +--- + +As soon as a backend termination signal is received, call before shutting down root lifecycle hooks. From 7ad9d1a0ee45812f7b963dc5841fe1d6a5456787 Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Wed, 4 Dec 2024 11:37:34 +0100 Subject: [PATCH 06/10] docs: update backend-plugin-api changeset Co-authored-by: Patrik Oldsberg Signed-off-by: Camila Belo --- .changeset/green-carpets-smell.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/.changeset/green-carpets-smell.md b/.changeset/green-carpets-smell.md index b9d547e707..0cda68de22 100644 --- a/.changeset/green-carpets-smell.md +++ b/.changeset/green-carpets-smell.md @@ -3,3 +3,5 @@ --- The `RootLifecycleService` now has a new `addBeforeShutdownHook` method, and hooks added through this method will run immediately when a termination event is received. + +The backend will not proceed with the shutdown and run the `Shutdown` hooks until all `BeforeShutdown` hooks have completed. From ad39962dc9a3d1088812be3ac00ec538b3ffc24a Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Wed, 4 Dec 2024 17:57:45 +0100 Subject: [PATCH 07/10] refator: move lifecycle middleware to root http Signed-off-by: Camila Belo --- .../core-services/root-http-router.md | 15 +++ packages/backend-defaults/config.d.ts | 13 ++ packages/backend-defaults/package.json | 1 - .../report-rootHttpRouter.api.md | 2 + .../httpRouter/httpRouterServiceFactory.ts | 5 +- .../createLifecycleMiddleware.test.ts | 108 +++++++++++++++ .../createLifecycleMiddleware.ts | 124 ++++++++++++++++++ .../rootHttpRouter/http/createHttpServer.ts | 10 +- .../rootHttpRouterServiceFactory.test.ts | 74 ++++++++++- .../rootHttpRouterServiceFactory.ts | 29 +++- yarn.lock | 1 - 11 files changed, 365 insertions(+), 17 deletions(-) create mode 100644 packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts create mode 100644 packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts diff --git a/docs/backend-system/core-services/root-http-router.md b/docs/backend-system/core-services/root-http-router.md index a2316b957b..530ea09544 100644 --- a/docs/backend-system/core-services/root-http-router.md +++ b/docs/backend-system/core-services/root-http-router.md @@ -42,6 +42,21 @@ createBackendPlugin({ ## Configuring the service +### Via `app-config.yaml` + +The `app-config.yaml` file provides configurable options that can be adjusted to meet your `RootHttpRouterService` specific requirements: + +```yaml +backend: + lifecycle: + # (Optional) The maximum time that paused requests will wait for the service to start, before returning an error (defaults to 5 seconds). + startupRequestPauseTimeout: { seconds: 10 } + # (Optional) The maximum time that the server will wait for stop accepting traffic, before returning an error (defaults to 30 seconds). + shutdownRequestPauseTimeout: { seconds: 20 } +``` + +### Via Code + There's additional options that you can pass to configure the root HTTP Router service. These options are passed when you call `createBackend`. - `indexPath` - optional path to forward all unmatched requests to. Defaults to `/api/app` which is the `app-backend` plugin responsible for serving the frontend application through the backend. diff --git a/packages/backend-defaults/config.d.ts b/packages/backend-defaults/config.d.ts index 0f7f9906f8..82fb19c14f 100644 --- a/packages/backend-defaults/config.d.ts +++ b/packages/backend-defaults/config.d.ts @@ -28,6 +28,19 @@ export interface Config { */ baseUrl: string; + lifecycle?: { + /** + * The maximum time that paused requests will wait for the service to start, before returning an error. + * Defaults to 5 seconds. + */ + startupRequestPauseTimeout?: HumanDuration; + /** + * The maximum time that the server will wait for stop accepting traffic, before returning an error. + * Defaults to 30 seconds. + */ + shutdownRequestPauseTimeout?: HumanDuration; + }; + /** Address that the backend should listen to. */ listen?: | string diff --git a/packages/backend-defaults/package.json b/packages/backend-defaults/package.json index d8240c429c..88d3ca79b1 100644 --- a/packages/backend-defaults/package.json +++ b/packages/backend-defaults/package.json @@ -175,7 +175,6 @@ "pg-format": "^1.0.4", "raw-body": "^2.4.1", "selfsigned": "^2.0.0", - "stoppable": "^1.1.0", "tar": "^6.1.12", "triple-beam": "^1.4.1", "uuid": "^11.0.0", diff --git a/packages/backend-defaults/report-rootHttpRouter.api.md b/packages/backend-defaults/report-rootHttpRouter.api.md index 41830ebf17..4489fed107 100644 --- a/packages/backend-defaults/report-rootHttpRouter.api.md +++ b/packages/backend-defaults/report-rootHttpRouter.api.md @@ -134,6 +134,8 @@ export interface RootHttpRouterConfigureContext { // (undocumented) lifecycle: LifecycleService; // (undocumented) + lifecycleMiddleware: RequestHandler; + // (undocumented) logger: LoggerService; // (undocumented) middleware: MiddlewareFactory; diff --git a/packages/backend-defaults/src/entrypoints/httpRouter/httpRouterServiceFactory.ts b/packages/backend-defaults/src/entrypoints/httpRouter/httpRouterServiceFactory.ts index 0557cb636d..a448827e16 100644 --- a/packages/backend-defaults/src/entrypoints/httpRouter/httpRouterServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/httpRouter/httpRouterServiceFactory.ts @@ -22,7 +22,6 @@ import { HttpRouterServiceAuthPolicy, } from '@backstage/backend-plugin-api'; import { - createLifecycleMiddleware, createCookieAuthRefreshMiddleware, createCredentialsBarrier, createAuthIntegrationRouter, @@ -43,12 +42,11 @@ export const httpRouterServiceFactory = createServiceFactory({ deps: { plugin: coreServices.pluginMetadata, config: coreServices.rootConfig, - lifecycle: coreServices.lifecycle, rootHttpRouter: coreServices.rootHttpRouter, auth: coreServices.auth, httpAuth: coreServices.httpAuth, }, - async factory({ auth, httpAuth, config, plugin, rootHttpRouter, lifecycle }) { + async factory({ auth, httpAuth, config, plugin, rootHttpRouter }) { const router = PromiseRouter(); rootHttpRouter.use(`/api/${plugin.getId()}`, router); @@ -59,7 +57,6 @@ export const httpRouterServiceFactory = createServiceFactory({ }); router.use(createAuthIntegrationRouter({ auth })); - router.use(createLifecycleMiddleware({ lifecycle })); router.use(credentialsBarrier.middleware); router.use(createCookieAuthRefreshMiddleware({ auth, httpAuth })); diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts new file mode 100644 index 0000000000..0ac8f704bf --- /dev/null +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts @@ -0,0 +1,108 @@ +/* + * Copyright 2022 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { createLifecycleMiddleware } from './createLifecycleMiddleware'; +import { BackendLifecycleImpl } from '../rootLifecycle/rootLifecycleServiceFactory'; +import { mockServices } from '@backstage/backend-test-utils'; +import { ServiceUnavailableError } from '@backstage/errors'; + +describe('createLifecycleMiddleware', () => { + it('should pause requests when plugin is not ready', async () => { + const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); + + const middleware = createLifecycleMiddleware({ lifecycle }); + + const next = jest.fn(); + middleware({} as any, {} as any, next); + expect(next).not.toHaveBeenCalled(); + await lifecycle.startup(); + + // pending call + expect(next).toHaveBeenCalledWith(); + + // new call + const next2 = jest.fn(); + middleware({} as any, {} as any, next2); + expect(next2).toHaveBeenCalledWith(); + }); + + it('should throw ServiceUnavailableError after shutdown', async () => { + const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); + const middleware = createLifecycleMiddleware({ lifecycle }); + + const next = jest.fn(); + middleware({} as any, {} as any, next); + expect(next).not.toHaveBeenCalled(); + await lifecycle.shutdown(); + + // pending call + expect(next).toHaveBeenCalledWith( + new ServiceUnavailableError('Service is shutting down'), + ); + + // new call + const next2 = jest.fn(); + middleware({} as any, {} as any, next2); + expect(next2).toHaveBeenCalledWith( + new ServiceUnavailableError('Service is shutting down'), + ); + }); + + it('should throw ServiceUnavailableError after timeout', async () => { + const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); + const middleware = createLifecycleMiddleware({ + lifecycle, + startupRequestPauseTimeout: { milliseconds: 1 }, + }); + + const next = jest.fn(); + middleware({} as any, {} as any, next); + expect(next).not.toHaveBeenCalled(); + + await new Promise(r => setTimeout(r, 2)); + + expect(next).toHaveBeenCalledWith( + new ServiceUnavailableError('Service has not started up yet'), + ); + }); + + it('should delay service shutdown for the default timeout duration', async () => { + jest.useFakeTimers(); + const defaultTimeout = 30000; + const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); + createLifecycleMiddleware({ lifecycle }); + const beforeShutdownPromise = lifecycle.beforeShutdown().then(() => { + jest.useRealTimers(); + }); + jest.advanceTimersByTime(defaultTimeout); + return expect(beforeShutdownPromise).resolves.toBeUndefined(); + }); + + it('should delay service shutdown for the configured timeout duration', async () => { + jest.useFakeTimers(); + const configuredTimeout = 20000; + const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); + createLifecycleMiddleware({ + lifecycle, + shutdownRequestPauseTimeout: { milliseconds: configuredTimeout }, + }); + const beforeShutdownPromise = lifecycle.beforeShutdown().then(() => { + jest.useRealTimers(); + }); + jest.advanceTimersByTime(configuredTimeout); + return expect(beforeShutdownPromise).resolves.toBeUndefined(); + }); +}); diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts new file mode 100644 index 0000000000..615da13d83 --- /dev/null +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts @@ -0,0 +1,124 @@ +/* + * Copyright 2022 The Backstage Authors + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +import { RootLifecycleService } from '@backstage/backend-plugin-api'; +import { ServiceUnavailableError } from '@backstage/errors'; +import { HumanDuration, durationToMilliseconds } from '@backstage/types'; +import { RequestHandler } from 'express'; + +export const DEFAULT_STARTUP_REQUEST_PAUSE_TIMEOUT = { seconds: 5 }; +export const DEFAULT_SHUTDOWN_REQUEST_PAUSE_TIMEOUT = { seconds: 30 }; + +/** + * Options for {@link createLifecycleMiddleware}. + * @public + */ +export interface LifecycleMiddlewareOptions { + lifecycle: RootLifecycleService; + /** + * The maximum time that paused requests will wait for the service to start, before returning an error. + * + * Defaults to 5 seconds. + */ + startupRequestPauseTimeout?: HumanDuration; + /** + * The maximum time that the server will wait for stop accepting traffic, before returning an error. + * + * Defaults to 30 seconds. + */ + shutdownRequestPauseTimeout?: HumanDuration; +} + +/** + * Creates a middleware that pauses requests until the service has started. + * + * @remarks + * + * Requests that arrive before the service has started will be paused until startup is complete. + * If the service does not start within the provided timeout, the request will be rejected with a + * {@link @backstage/errors#ServiceUnavailableError}. + * + * If the service is shutting down, all requests will be rejected with a + * {@link @backstage/errors#ServiceUnavailableError}. + * + * @public + */ +export function createLifecycleMiddleware( + options: LifecycleMiddlewareOptions, +): RequestHandler { + const { lifecycle, startupRequestPauseTimeout, shutdownRequestPauseTimeout } = + options; + + let state: 'init' | 'up' | 'down' = 'init'; + const waiting = new Set<{ + next: (err?: Error) => void; + timeout: NodeJS.Timeout; + }>(); + + lifecycle.addStartupHook(async () => { + if (state === 'init') { + state = 'up'; + for (const item of waiting) { + clearTimeout(item.timeout); + item.next(); + } + waiting.clear(); + } + }); + + lifecycle.addBeforeShutdownHook(async () => { + const timeoutMs = durationToMilliseconds( + shutdownRequestPauseTimeout ?? DEFAULT_SHUTDOWN_REQUEST_PAUSE_TIMEOUT, + ); + return await new Promise(resolve => { + setTimeout(resolve, timeoutMs); + }); + }); + + lifecycle.addShutdownHook(async () => { + state = 'down'; + for (const item of waiting) { + clearTimeout(item.timeout); + item.next(new ServiceUnavailableError('Service is shutting down')); + } + waiting.clear(); + }); + + return (_req, _res, next) => { + if (state === 'up') { + next(); + return; + } else if (state === 'down') { + next(new ServiceUnavailableError('Service is shutting down')); + return; + } + + const timeoutMs = durationToMilliseconds( + startupRequestPauseTimeout ?? DEFAULT_STARTUP_REQUEST_PAUSE_TIMEOUT, + ); + + const item = { + next, + timeout: setTimeout(() => { + if (waiting.delete(item)) { + next(new ServiceUnavailableError('Service has not started up yet')); + } + }, timeoutMs), + }; + + waiting.add(item); + }; +} diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/http/createHttpServer.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/http/createHttpServer.ts index 5c526ba1e2..9449291628 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/http/createHttpServer.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/http/createHttpServer.ts @@ -16,7 +16,6 @@ import * as http from 'http'; import * as https from 'https'; -import stoppableServer from 'stoppable'; import { RequestListener } from 'http'; import { LoggerService } from '@backstage/backend-plugin-api'; import { HttpServerOptions, ExtendedHttpServer } from './types'; @@ -33,13 +32,6 @@ export async function createHttpServer( deps: { logger: LoggerService }, ): Promise { const server = await createServer(listener, options, deps); - - const stopper = stoppableServer(server, 0); - // The stopper here is actually the server itself, so if we try - // to call stopper.stop() down in the stop implementation, we'll - // be calling ourselves. - const stopServer = stopper.stop.bind(stopper); - return Object.assign(server, { start() { return new Promise((resolve, reject) => { @@ -61,7 +53,7 @@ export async function createHttpServer( stop() { return new Promise((resolve, reject) => { - stopServer((error?: Error) => { + server.close(error => { if (error) { reject(error); } else { diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts index f058c87900..f04df36a9b 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts @@ -21,7 +21,12 @@ import { import { Express } from 'express'; import request from 'supertest'; import { rootHttpRouterServiceFactory } from './rootHttpRouterServiceFactory'; -import { ServiceFactory, coreServices } from '@backstage/backend-plugin-api'; +import { + ServiceFactory, + coreServices, + createServiceFactory, +} from '@backstage/backend-plugin-api'; +import { BackendLifecycleImpl } from '../rootLifecycle/rootLifecycleServiceFactory'; async function createExpressApp(...dependencies: ServiceFactory[]) { let app: Express | undefined = undefined; @@ -198,4 +203,71 @@ describe('rootHttpRouterServiceFactory', () => { 'Invalid header value in at backend.health.headers, must be a non-empty string', ); }); + + it('should wait the server shutdown', async () => { + jest.useFakeTimers(); + + let app: Express | undefined = undefined; + const lifecycleMock = new BackendLifecycleImpl(mockServices.rootLogger()); + + const tester = ServiceFactoryTester.from( + rootHttpRouterServiceFactory({ + configure(options) { + console.log('configure'); + options.app.use(options.lifecycleMiddleware); + options.app.get('/test', (_req, res) => { + res.status(200).send({ status: 'ok' }).end(); + }); + app = options.app; + }, + }), + { + dependencies: [ + mockServices.rootConfig.factory({ + data: { + app: { baseUrl: 'http://localhost' }, + backend: { + baseUrl: 'http://localhost', + listen: { host: '', port: 0 }, + }, + }, + }), + createServiceFactory({ + service: coreServices.rootLifecycle, + deps: {}, + factory() { + return lifecycleMock; + }, + }), + ], + }, + ); + + await tester.getSubject(); + + // Trigger creation of the http service, accessing the app instance through the configure callback + const lifecycle = await tester.getService(coreServices.rootLifecycle); + + await (lifecycle as any).startup(); // Trigger startup by calling the private startup method + + await request(app!).get('/test').expect(200, { + status: 'ok', + }); + + const beforeShutdownPromise = (lifecycle as any) + .beforeShutdown() + .then(() => { + return (lifecycle as any).shutdown(); + }); + + await request(app!).get('/test').expect(200, { + status: 'ok', + }); + + jest.advanceTimersByTime(30000); + + await request(app!).get('/test').expect(500, {}); + + return await expect(beforeShutdownPromise).resolves.toBeUndefined(); + }); }); diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts index 755535dc70..fc2d5bb3a3 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts @@ -21,6 +21,8 @@ import { LifecycleService, LoggerService, } from '@backstage/backend-plugin-api'; +import { HumanDuration } from '@backstage/types'; +import { readDurationFromConfig } from '@backstage/config'; import express, { RequestHandler, Express } from 'express'; import type { Server } from 'node:http'; import { @@ -30,6 +32,7 @@ import { } from './http'; import { DefaultRootHttpRouter } from './DefaultRootHttpRouter'; import { createHealthRouter } from './createHealthRouter'; +import { createLifecycleMiddleware } from './createLifecycleMiddleware'; /** * @public @@ -43,6 +46,7 @@ export interface RootHttpRouterConfigureContext { logger: LoggerService; lifecycle: LifecycleService; healthRouter: RequestHandler; + lifecycleMiddleware: RequestHandler; applyDefaults: () => void; } @@ -90,6 +94,27 @@ const rootHttpRouterServiceFactoryWithOptions = ( const routes = router.handler(); const healthRouter = createHealthRouter({ config, health }); + + let startupRequestPauseTimeout: HumanDuration | undefined; + if (config.has('backend.lifecycle.startupRequestPauseTimeout')) { + startupRequestPauseTimeout = readDurationFromConfig(config, { + key: 'backend.lifecycle.startupRequestPauseTimeout', + }); + } + + let shutdownRequestPauseTimeout: HumanDuration | undefined; + if (config.has('backend.lifecycle.shutdownRequestPauseTimeout')) { + shutdownRequestPauseTimeout = readDurationFromConfig(config, { + key: 'backend.lifecycle.shutdownRequestPauseTimeout', + }); + } + + const lifecycleMiddleware = createLifecycleMiddleware({ + lifecycle, + startupRequestPauseTimeout, + shutdownRequestPauseTimeout, + }); + const server = await createHttpServer( app, readHttpServerOptions(config.getOptionalConfig('backend')), @@ -105,6 +130,7 @@ const rootHttpRouterServiceFactoryWithOptions = ( logger, lifecycle, healthRouter, + lifecycleMiddleware, applyDefaults() { if (process.env.NODE_ENV === 'development') { app.set('json spaces', 2); @@ -114,13 +140,14 @@ const rootHttpRouterServiceFactoryWithOptions = ( app.use(middleware.compression()); app.use(middleware.logging()); app.use(healthRouter); + app.use(lifecycleMiddleware); app.use(routes); app.use(middleware.notFound()); app.use(middleware.error()); }, }); - lifecycle.addBeforeShutdownHook(() => server.stop()); + lifecycle.addShutdownHook(() => server.stop()); await server.start(); diff --git a/yarn.lock b/yarn.lock index 463f04a644..353786ab44 100644 --- a/yarn.lock +++ b/yarn.lock @@ -3651,7 +3651,6 @@ __metadata: pg-format: ^1.0.4 raw-body: ^2.4.1 selfsigned: ^2.0.0 - stoppable: ^1.1.0 supertest: ^7.0.0 tar: ^6.1.12 triple-beam: ^1.4.1 From d0cbd829659101ee28df4c471e592b698748adad Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Mon, 9 Dec 2024 18:43:38 +0100 Subject: [PATCH 08/10] refactor: address latest review comments Signed-off-by: Camila Belo --- .changeset/rude-pears-tie.md | 5 + packages/backend-defaults/config.d.ts | 6 +- .../createLifecycleMiddleware.test.ts | 2 +- .../createLifecycleMiddleware.ts | 8 +- .../rootHttpRouterServiceFactory.test.ts | 106 +++++++++++++++++- .../rootHttpRouterServiceFactory.ts | 41 ++++--- 6 files changed, 144 insertions(+), 24 deletions(-) create mode 100644 .changeset/rude-pears-tie.md diff --git a/.changeset/rude-pears-tie.md b/.changeset/rude-pears-tie.md new file mode 100644 index 0000000000..953b7da2b9 --- /dev/null +++ b/.changeset/rude-pears-tie.md @@ -0,0 +1,5 @@ +--- +'@backstage/backend-defaults': patch +--- + +Remove use of the `stoppable` library as Node's native http server [close](https://nodejs.org/api/http.html#serverclosecallback) method already drains requests. diff --git a/packages/backend-defaults/config.d.ts b/packages/backend-defaults/config.d.ts index 82fb19c14f..20c1c2cf74 100644 --- a/packages/backend-defaults/config.d.ts +++ b/packages/backend-defaults/config.d.ts @@ -31,14 +31,16 @@ export interface Config { lifecycle?: { /** * The maximum time that paused requests will wait for the service to start, before returning an error. + * If you pass a number or string, it will be interpreted as milliseconds. * Defaults to 5 seconds. */ - startupRequestPauseTimeout?: HumanDuration; + startupRequestPauseTimeout?: number | string | HumanDuration; /** * The maximum time that the server will wait for stop accepting traffic, before returning an error. + * If you pass a number or string, it will be interpreted as milliseconds. * Defaults to 30 seconds. */ - shutdownRequestPauseTimeout?: HumanDuration; + shutdownRequestDelayTimeout?: number | string | HumanDuration; }; /** Address that the backend should listen to. */ diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts index 0ac8f704bf..eae5f57dcf 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts @@ -97,7 +97,7 @@ describe('createLifecycleMiddleware', () => { const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); createLifecycleMiddleware({ lifecycle, - shutdownRequestPauseTimeout: { milliseconds: configuredTimeout }, + shutdownRequestDelayTimeout: { milliseconds: configuredTimeout }, }); const beforeShutdownPromise = lifecycle.beforeShutdown().then(() => { jest.useRealTimers(); diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts index 615da13d83..c86467edc2 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts @@ -20,7 +20,7 @@ import { HumanDuration, durationToMilliseconds } from '@backstage/types'; import { RequestHandler } from 'express'; export const DEFAULT_STARTUP_REQUEST_PAUSE_TIMEOUT = { seconds: 5 }; -export const DEFAULT_SHUTDOWN_REQUEST_PAUSE_TIMEOUT = { seconds: 30 }; +export const DEFAULT_SHUTDOWN_REQUEST_DELAY_TIMEOUT = { seconds: 30 }; /** * Options for {@link createLifecycleMiddleware}. @@ -39,7 +39,7 @@ export interface LifecycleMiddlewareOptions { * * Defaults to 30 seconds. */ - shutdownRequestPauseTimeout?: HumanDuration; + shutdownRequestDelayTimeout?: HumanDuration; } /** @@ -59,7 +59,7 @@ export interface LifecycleMiddlewareOptions { export function createLifecycleMiddleware( options: LifecycleMiddlewareOptions, ): RequestHandler { - const { lifecycle, startupRequestPauseTimeout, shutdownRequestPauseTimeout } = + const { lifecycle, startupRequestPauseTimeout, shutdownRequestDelayTimeout } = options; let state: 'init' | 'up' | 'down' = 'init'; @@ -81,7 +81,7 @@ export function createLifecycleMiddleware( lifecycle.addBeforeShutdownHook(async () => { const timeoutMs = durationToMilliseconds( - shutdownRequestPauseTimeout ?? DEFAULT_SHUTDOWN_REQUEST_PAUSE_TIMEOUT, + shutdownRequestDelayTimeout ?? DEFAULT_SHUTDOWN_REQUEST_DELAY_TIMEOUT, ); return await new Promise(resolve => { setTimeout(resolve, timeoutMs); diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts index f04df36a9b..6c9d124f3a 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts @@ -20,13 +20,55 @@ import { } from '@backstage/backend-test-utils'; import { Express } from 'express'; import request from 'supertest'; -import { rootHttpRouterServiceFactory } from './rootHttpRouterServiceFactory'; +import { + getConfigInHumanDuration, + rootHttpRouterServiceFactory, +} from './rootHttpRouterServiceFactory'; import { ServiceFactory, coreServices, createServiceFactory, } from '@backstage/backend-plugin-api'; import { BackendLifecycleImpl } from '../rootLifecycle/rootLifecycleServiceFactory'; +import { ConfigReader } from '@backstage/config'; + +describe('getConfigInHumanDuration', () => { + const values = ['20000', 20000, { milliseconds: 20000 }]; + + it.each(values)( + 'should parse the lifecycle startup request timeout from a %s config value', + async value => { + const key = 'backend.lifecycle.startupRequestPauseTimeout'; + const config = new ConfigReader({ + backend: { + lifecycle: { + startupRequestPauseTimeout: value, + }, + }, + }); + expect(getConfigInHumanDuration(config, key)).toMatchObject({ + milliseconds: 20000, + }); + }, + ); + + it.each(values)( + 'should parse the lifecycle shutdown delay timeout from a %s config value', + async value => { + const key = 'backend.lifecycle.shutdownRequestDelayTimeout'; + const config = new ConfigReader({ + backend: { + lifecycle: { + shutdownRequestDelayTimeout: value, + }, + }, + }); + expect(getConfigInHumanDuration(config, key)).toMatchObject({ + milliseconds: 20000, + }); + }, + ); +}); async function createExpressApp(...dependencies: ServiceFactory[]) { let app: Express | undefined = undefined; @@ -204,7 +246,7 @@ describe('rootHttpRouterServiceFactory', () => { ); }); - it('should wait the server shutdown', async () => { + it('should wait the server to shutdown', async () => { jest.useFakeTimers(); let app: Express | undefined = undefined; @@ -214,10 +256,12 @@ describe('rootHttpRouterServiceFactory', () => { rootHttpRouterServiceFactory({ configure(options) { console.log('configure'); + options.app.use(options.healthRouter); options.app.use(options.lifecycleMiddleware); options.app.get('/test', (_req, res) => { res.status(200).send({ status: 'ok' }).end(); }); + options.app.use(options.middleware.error()); app = options.app; }, }), @@ -254,19 +298,73 @@ describe('rootHttpRouterServiceFactory', () => { status: 'ok', }); + await request(app) + .get('/.backstage/health/v1/liveness') + .expect(200, { status: 'ok' }); + + await request(app).get('/.backstage/health/v1/readiness').expect(200, { + status: 'ok', + }); + const beforeShutdownPromise = (lifecycle as any) .beforeShutdown() .then(() => { return (lifecycle as any).shutdown(); }); + // Continue accepting requests await request(app!).get('/test').expect(200, { status: 'ok', }); - jest.advanceTimersByTime(30000); + await request(app) + .get('/.backstage/health/v1/liveness') + .expect(200, { status: 'ok' }); - await request(app!).get('/test').expect(500, {}); + // Immediately start failing the readiness health check + await request(app).get('/.backstage/health/v1/readiness').expect(503, { + message: 'Backend has not started yet', + status: 'error', + }); + + jest.advanceTimersByTime(29999); + + // Still accepting requests 1 ms before shutdown + await request(app!).get('/test').expect(200, { + status: 'ok', + }); + + await request(app) + .get('/.backstage/health/v1/liveness') + .expect(200, { status: 'ok' }); + + await request(app).get('/.backstage/health/v1/readiness').expect(503, { + message: 'Backend has not started yet', + status: 'error', + }); + + jest.advanceTimersByTime(1); + + // No longer accepting requests after shutdown + await request(app!) + .get('/test') + .expect(503, { + error: { + name: 'ServiceUnavailableError', + message: 'Service is shutting down', + }, + request: { method: 'GET', url: '/test' }, + response: { statusCode: 503 }, + }); + + await request(app) + .get('/.backstage/health/v1/liveness') + .expect(200, { status: 'ok' }); + + await request(app).get('/.backstage/health/v1/readiness').expect(503, { + message: 'Backend has not started yet', + status: 'error', + }); return await expect(beforeShutdownPromise).resolves.toBeUndefined(); }); diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts index fc2d5bb3a3..33cc627df5 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts @@ -34,6 +34,25 @@ import { DefaultRootHttpRouter } from './DefaultRootHttpRouter'; import { createHealthRouter } from './createHealthRouter'; import { createLifecycleMiddleware } from './createLifecycleMiddleware'; +export function getConfigInHumanDuration( + config: RootConfigService, + key: string, +): HumanDuration | undefined { + const value = config.getOptional(key); + if (typeof value === 'undefined') { + return undefined; + } + if (typeof value === 'number') { + return { milliseconds: value }; + } + if (typeof value === 'string') { + return { + milliseconds: parseInt(value, 10), + }; + } + return readDurationFromConfig(config, { key }); +} + /** * @public */ @@ -95,24 +114,20 @@ const rootHttpRouterServiceFactoryWithOptions = ( const healthRouter = createHealthRouter({ config, health }); - let startupRequestPauseTimeout: HumanDuration | undefined; - if (config.has('backend.lifecycle.startupRequestPauseTimeout')) { - startupRequestPauseTimeout = readDurationFromConfig(config, { - key: 'backend.lifecycle.startupRequestPauseTimeout', - }); - } + const startupRequestPauseTimeout = getConfigInHumanDuration( + config, + 'backend.lifecycle.startupRequestPauseTimeout', + ); - let shutdownRequestPauseTimeout: HumanDuration | undefined; - if (config.has('backend.lifecycle.shutdownRequestPauseTimeout')) { - shutdownRequestPauseTimeout = readDurationFromConfig(config, { - key: 'backend.lifecycle.shutdownRequestPauseTimeout', - }); - } + const shutdownRequestDelayTimeout = getConfigInHumanDuration( + config, + 'backend.lifecycle.shutdownRequestDelayTimeout', + ); const lifecycleMiddleware = createLifecycleMiddleware({ lifecycle, startupRequestPauseTimeout, - shutdownRequestPauseTimeout, + shutdownRequestDelayTimeout, }); const server = await createHttpServer( From 8397edc1bfaddaf833651f9bf75f0d8c6c230ab3 Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Tue, 10 Dec 2024 09:30:27 +0100 Subject: [PATCH 09/10] fix: accept only config as string or human duration Signed-off-by: Camila Belo --- .../core-services/root-http-router.md | 8 ++++ packages/backend-defaults/config.d.ts | 18 +++++--- .../createLifecycleMiddleware.test.ts | 4 +- .../createLifecycleMiddleware.ts | 8 ++-- .../rootHttpRouterServiceFactory.test.ts | 44 +----------------- .../rootHttpRouterServiceFactory.ts | 45 +++++++------------ 6 files changed, 43 insertions(+), 84 deletions(-) diff --git a/docs/backend-system/core-services/root-http-router.md b/docs/backend-system/core-services/root-http-router.md index 530ea09544..ebd9258310 100644 --- a/docs/backend-system/core-services/root-http-router.md +++ b/docs/backend-system/core-services/root-http-router.md @@ -50,8 +50,16 @@ The `app-config.yaml` file provides configurable options that can be adjusted to backend: lifecycle: # (Optional) The maximum time that paused requests will wait for the service to start, before returning an error (defaults to 5 seconds). + # Supported formats: + # - A string in the format of '1d', '2 seconds' etc. as supported by the `ms` library. + # - A standard ISO formatted duration string, e.g. 'P2DT6H' or 'PT1M'. + # - An object with individual units (in plural) as keys, e.g. `{ days: 2, hours: 6 }`. startupRequestPauseTimeout: { seconds: 10 } # (Optional) The maximum time that the server will wait for stop accepting traffic, before returning an error (defaults to 30 seconds). + # Supported formats: + # - A string in the format of '1d', '2 seconds' etc. as supported by the `ms` library. + # - A standard ISO formatted duration string, e.g. 'P2DT6H' or 'PT1M'. + # - An object with individual units (in plural) as keys, e.g. `{ days: 2, hours: 6 }`. shutdownRequestPauseTimeout: { seconds: 20 } ``` diff --git a/packages/backend-defaults/config.d.ts b/packages/backend-defaults/config.d.ts index 20c1c2cf74..e523f2da36 100644 --- a/packages/backend-defaults/config.d.ts +++ b/packages/backend-defaults/config.d.ts @@ -31,16 +31,24 @@ export interface Config { lifecycle?: { /** * The maximum time that paused requests will wait for the service to start, before returning an error. - * If you pass a number or string, it will be interpreted as milliseconds. - * Defaults to 5 seconds. + * Defaults to 5 seconds + * Supported formats: + * - A string in the format of '1d', '2 seconds' etc. as supported by the `ms` + * library. + * - A standard ISO formatted duration string, e.g. 'P2DT6H' or 'PT1M'. + * - An object with individual units (in plural) as keys, e.g. `{ days: 2, hours: 6 }`. */ - startupRequestPauseTimeout?: number | string | HumanDuration; + startupRequestPauseTimeout?: string | HumanDuration; /** * The maximum time that the server will wait for stop accepting traffic, before returning an error. - * If you pass a number or string, it will be interpreted as milliseconds. * Defaults to 30 seconds. + * Supported formats: + * - A string in the format of '1d', '2 seconds' etc. as supported by the `ms` + * library. + * - A standard ISO formatted duration string, e.g. 'P2DT6H' or 'PT1M'. + * - An object with individual units (in plural) as keys, e.g. `{ days: 2, hours: 6 }`. */ - shutdownRequestDelayTimeout?: number | string | HumanDuration; + serverShutdownDelay?: string | HumanDuration; }; /** Address that the backend should listen to. */ diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts index eae5f57dcf..76af28cc6e 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.test.ts @@ -91,13 +91,13 @@ describe('createLifecycleMiddleware', () => { return expect(beforeShutdownPromise).resolves.toBeUndefined(); }); - it('should delay service shutdown for the configured timeout duration', async () => { + it('should delay service shutdown for the configured timeout duration - time in human duration', async () => { jest.useFakeTimers(); const configuredTimeout = 20000; const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger()); createLifecycleMiddleware({ lifecycle, - shutdownRequestDelayTimeout: { milliseconds: configuredTimeout }, + serverShutdownDelay: { milliseconds: configuredTimeout }, }); const beforeShutdownPromise = lifecycle.beforeShutdown().then(() => { jest.useRealTimers(); diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts index c86467edc2..3a7b5b10eb 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts @@ -20,7 +20,7 @@ import { HumanDuration, durationToMilliseconds } from '@backstage/types'; import { RequestHandler } from 'express'; export const DEFAULT_STARTUP_REQUEST_PAUSE_TIMEOUT = { seconds: 5 }; -export const DEFAULT_SHUTDOWN_REQUEST_DELAY_TIMEOUT = { seconds: 30 }; +export const DEFAULT_SERVER_SHUTDOWN_TIMEOUT = { seconds: 30 }; /** * Options for {@link createLifecycleMiddleware}. @@ -39,7 +39,7 @@ export interface LifecycleMiddlewareOptions { * * Defaults to 30 seconds. */ - shutdownRequestDelayTimeout?: HumanDuration; + serverShutdownDelay?: HumanDuration; } /** @@ -59,7 +59,7 @@ export interface LifecycleMiddlewareOptions { export function createLifecycleMiddleware( options: LifecycleMiddlewareOptions, ): RequestHandler { - const { lifecycle, startupRequestPauseTimeout, shutdownRequestDelayTimeout } = + const { lifecycle, startupRequestPauseTimeout, serverShutdownDelay } = options; let state: 'init' | 'up' | 'down' = 'init'; @@ -81,7 +81,7 @@ export function createLifecycleMiddleware( lifecycle.addBeforeShutdownHook(async () => { const timeoutMs = durationToMilliseconds( - shutdownRequestDelayTimeout ?? DEFAULT_SHUTDOWN_REQUEST_DELAY_TIMEOUT, + serverShutdownDelay ?? DEFAULT_SERVER_SHUTDOWN_TIMEOUT, ); return await new Promise(resolve => { setTimeout(resolve, timeoutMs); diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts index 6c9d124f3a..c0983706c3 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts @@ -20,55 +20,13 @@ import { } from '@backstage/backend-test-utils'; import { Express } from 'express'; import request from 'supertest'; -import { - getConfigInHumanDuration, - rootHttpRouterServiceFactory, -} from './rootHttpRouterServiceFactory'; +import { rootHttpRouterServiceFactory } from './rootHttpRouterServiceFactory'; import { ServiceFactory, coreServices, createServiceFactory, } from '@backstage/backend-plugin-api'; import { BackendLifecycleImpl } from '../rootLifecycle/rootLifecycleServiceFactory'; -import { ConfigReader } from '@backstage/config'; - -describe('getConfigInHumanDuration', () => { - const values = ['20000', 20000, { milliseconds: 20000 }]; - - it.each(values)( - 'should parse the lifecycle startup request timeout from a %s config value', - async value => { - const key = 'backend.lifecycle.startupRequestPauseTimeout'; - const config = new ConfigReader({ - backend: { - lifecycle: { - startupRequestPauseTimeout: value, - }, - }, - }); - expect(getConfigInHumanDuration(config, key)).toMatchObject({ - milliseconds: 20000, - }); - }, - ); - - it.each(values)( - 'should parse the lifecycle shutdown delay timeout from a %s config value', - async value => { - const key = 'backend.lifecycle.shutdownRequestDelayTimeout'; - const config = new ConfigReader({ - backend: { - lifecycle: { - shutdownRequestDelayTimeout: value, - }, - }, - }); - expect(getConfigInHumanDuration(config, key)).toMatchObject({ - milliseconds: 20000, - }); - }, - ); -}); async function createExpressApp(...dependencies: ServiceFactory[]) { let app: Express | undefined = undefined; diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts index 33cc627df5..4afdfb1ec2 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.ts @@ -21,8 +21,6 @@ import { LifecycleService, LoggerService, } from '@backstage/backend-plugin-api'; -import { HumanDuration } from '@backstage/types'; -import { readDurationFromConfig } from '@backstage/config'; import express, { RequestHandler, Express } from 'express'; import type { Server } from 'node:http'; import { @@ -33,25 +31,8 @@ import { import { DefaultRootHttpRouter } from './DefaultRootHttpRouter'; import { createHealthRouter } from './createHealthRouter'; import { createLifecycleMiddleware } from './createLifecycleMiddleware'; - -export function getConfigInHumanDuration( - config: RootConfigService, - key: string, -): HumanDuration | undefined { - const value = config.getOptional(key); - if (typeof value === 'undefined') { - return undefined; - } - if (typeof value === 'number') { - return { milliseconds: value }; - } - if (typeof value === 'string') { - return { - milliseconds: parseInt(value, 10), - }; - } - return readDurationFromConfig(config, { key }); -} +import { readDurationFromConfig } from '@backstage/config'; +import { HumanDuration } from '@backstage/types'; /** * @public @@ -114,20 +95,24 @@ const rootHttpRouterServiceFactoryWithOptions = ( const healthRouter = createHealthRouter({ config, health }); - const startupRequestPauseTimeout = getConfigInHumanDuration( - config, - 'backend.lifecycle.startupRequestPauseTimeout', - ); + let startupRequestPauseTimeout: HumanDuration | undefined; + if (config.has('backend.lifecycle.startupRequestPauseTimeout')) { + startupRequestPauseTimeout = readDurationFromConfig(config, { + key: 'backend.lifecycle.startupRequestPauseTimeout', + }); + } - const shutdownRequestDelayTimeout = getConfigInHumanDuration( - config, - 'backend.lifecycle.shutdownRequestDelayTimeout', - ); + let serverShutdownDelay: HumanDuration | undefined; + if (config.has('backend.lifecycle.serverShutdownDelay')) { + serverShutdownDelay = readDurationFromConfig(config, { + key: 'backend.lifecycle.serverShutdownDelay', + }); + } const lifecycleMiddleware = createLifecycleMiddleware({ lifecycle, startupRequestPauseTimeout, - shutdownRequestDelayTimeout, + serverShutdownDelay, }); const server = await createHttpServer( From ba082fd50740cf4e7cc63f2aae321f8b558f42ec Mon Sep 17 00:00:00 2001 From: Camila Belo Date: Tue, 10 Dec 2024 11:03:07 +0100 Subject: [PATCH 10/10] refactor: apply more review suggestions MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Fredrik Adelöw Co-authored-by: Patrik Oldsberg Signed-off-by: Camila Belo --- .changeset/rude-pears-tie.md | 3 ++- .../core-services/root-http-router.md | 4 ++-- packages/backend-defaults/config.d.ts | 4 ++-- .../rootHealth/rootHealthServiceFactory.test.ts | 2 +- .../rootHealth/rootHealthServiceFactory.ts | 14 ++++++++++---- .../rootHttpRouter/createLifecycleMiddleware.ts | 4 ++-- .../rootHttpRouterServiceFactory.test.ts | 9 ++++++--- .../rootLifecycle/rootLifecycleServiceFactory.ts | 2 +- packages/backend-plugin-api/report.api.md | 2 +- .../services/definitions/RootLifecycleService.ts | 2 +- 10 files changed, 28 insertions(+), 18 deletions(-) diff --git a/.changeset/rude-pears-tie.md b/.changeset/rude-pears-tie.md index 953b7da2b9..c0cb970727 100644 --- a/.changeset/rude-pears-tie.md +++ b/.changeset/rude-pears-tie.md @@ -2,4 +2,5 @@ '@backstage/backend-defaults': patch --- -Remove use of the `stoppable` library as Node's native http server [close](https://nodejs.org/api/http.html#serverclosecallback) method already drains requests. +Remove use of the `stoppable` library on the `DefaultRootHttpRouterService` as Node's native http server [close](https://nodejs.org/api/http.html#serverclosecallback) method already drains requests. +Also, we pass a new `lifecycleMiddleware` to the `rootHttpRouterServiceFactory` configure function that must be called manually if you don't call `applyDefaults`. diff --git a/docs/backend-system/core-services/root-http-router.md b/docs/backend-system/core-services/root-http-router.md index ebd9258310..9ee8599eb5 100644 --- a/docs/backend-system/core-services/root-http-router.md +++ b/docs/backend-system/core-services/root-http-router.md @@ -55,12 +55,12 @@ backend: # - A standard ISO formatted duration string, e.g. 'P2DT6H' or 'PT1M'. # - An object with individual units (in plural) as keys, e.g. `{ days: 2, hours: 6 }`. startupRequestPauseTimeout: { seconds: 10 } - # (Optional) The maximum time that the server will wait for stop accepting traffic, before returning an error (defaults to 30 seconds). + # (Optional) The minimum time that the HTTP server will delay the shutdown of the backend. During this delay health checks will be set to failing, allowing traffic to drain (defaults to 0 seconds). # Supported formats: # - A string in the format of '1d', '2 seconds' etc. as supported by the `ms` library. # - A standard ISO formatted duration string, e.g. 'P2DT6H' or 'PT1M'. # - An object with individual units (in plural) as keys, e.g. `{ days: 2, hours: 6 }`. - shutdownRequestPauseTimeout: { seconds: 20 } + serverShutdownTimeout: { seconds: 20 } ``` ### Via Code diff --git a/packages/backend-defaults/config.d.ts b/packages/backend-defaults/config.d.ts index e523f2da36..e2a748f85d 100644 --- a/packages/backend-defaults/config.d.ts +++ b/packages/backend-defaults/config.d.ts @@ -40,8 +40,8 @@ export interface Config { */ startupRequestPauseTimeout?: string | HumanDuration; /** - * The maximum time that the server will wait for stop accepting traffic, before returning an error. - * Defaults to 30 seconds. + * The minimum time that the HTTP server will delay the shutdown of the backend. During this delay health checks will be set to failing, allowing traffic to drain. + * Defaults to 0 seconds. * Supported formats: * - A string in the format of '1d', '2 seconds' etc. as supported by the `ms` * library. diff --git a/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.test.ts b/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.test.ts index 32fc10a77f..ca9085273c 100644 --- a/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.test.ts +++ b/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.test.ts @@ -69,7 +69,7 @@ describe('DefaultRootHealthService', () => { await expect(service.getReadiness()).resolves.toEqual({ status: 503, payload: { - message: 'Backend has not started yet', + message: 'Backend is shuttting down', status: 'error', }, }); diff --git a/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.ts b/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.ts index ab9fc86a41..b201079b4f 100644 --- a/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/rootHealth/rootHealthServiceFactory.ts @@ -23,14 +23,14 @@ import { /** @internal */ export class DefaultRootHealthService implements RootHealthService { - #isRunning = false; + #state: 'init' | 'up' | 'down' = 'init'; constructor(readonly options: { lifecycle: RootLifecycleService }) { options.lifecycle.addStartupHook(() => { - this.#isRunning = true; + this.#state = 'up'; }); options.lifecycle.addBeforeShutdownHook(() => { - this.#isRunning = false; + this.#state = 'down'; }); } @@ -39,12 +39,18 @@ export class DefaultRootHealthService implements RootHealthService { } async getReadiness(): Promise<{ status: number; payload?: any }> { - if (!this.#isRunning) { + if (this.#state === 'init') { return { status: 503, payload: { message: 'Backend has not started yet', status: 'error' }, }; } + if (this.#state === 'down') { + return { + status: 503, + payload: { message: 'Backend is shuttting down', status: 'error' }, + }; + } return { status: 200, payload: { status: 'ok' } }; } diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts index 3a7b5b10eb..3745f7f48d 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/createLifecycleMiddleware.ts @@ -20,7 +20,7 @@ import { HumanDuration, durationToMilliseconds } from '@backstage/types'; import { RequestHandler } from 'express'; export const DEFAULT_STARTUP_REQUEST_PAUSE_TIMEOUT = { seconds: 5 }; -export const DEFAULT_SERVER_SHUTDOWN_TIMEOUT = { seconds: 30 }; +export const DEFAULT_SERVER_SHUTDOWN_TIMEOUT = { seconds: 0 }; /** * Options for {@link createLifecycleMiddleware}. @@ -37,7 +37,7 @@ export interface LifecycleMiddlewareOptions { /** * The maximum time that the server will wait for stop accepting traffic, before returning an error. * - * Defaults to 30 seconds. + * Defaults to 0 seconds. */ serverShutdownDelay?: HumanDuration; } diff --git a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts index c0983706c3..fb0d6cdad0 100644 --- a/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts +++ b/packages/backend-defaults/src/entrypoints/rootHttpRouter/rootHttpRouterServiceFactory.test.ts @@ -231,6 +231,9 @@ describe('rootHttpRouterServiceFactory', () => { backend: { baseUrl: 'http://localhost', listen: { host: '', port: 0 }, + lifecycle: { + serverShutdownDelay: '30s', + }, }, }, }), @@ -281,7 +284,7 @@ describe('rootHttpRouterServiceFactory', () => { // Immediately start failing the readiness health check await request(app).get('/.backstage/health/v1/readiness').expect(503, { - message: 'Backend has not started yet', + message: 'Backend is shuttting down', status: 'error', }); @@ -297,7 +300,7 @@ describe('rootHttpRouterServiceFactory', () => { .expect(200, { status: 'ok' }); await request(app).get('/.backstage/health/v1/readiness').expect(503, { - message: 'Backend has not started yet', + message: 'Backend is shuttting down', status: 'error', }); @@ -320,7 +323,7 @@ describe('rootHttpRouterServiceFactory', () => { .expect(200, { status: 'ok' }); await request(app).get('/.backstage/health/v1/readiness').expect(503, { - message: 'Backend has not started yet', + message: 'Backend is shuttting down', status: 'error', }); diff --git a/packages/backend-defaults/src/entrypoints/rootLifecycle/rootLifecycleServiceFactory.ts b/packages/backend-defaults/src/entrypoints/rootLifecycle/rootLifecycleServiceFactory.ts index 9a8f64edb5..abba4beece 100644 --- a/packages/backend-defaults/src/entrypoints/rootLifecycle/rootLifecycleServiceFactory.ts +++ b/packages/backend-defaults/src/entrypoints/rootLifecycle/rootLifecycleServiceFactory.ts @@ -66,7 +66,7 @@ export class BackendLifecycleImpl implements RootLifecycleService { } #hasBeforeShutdown = false; - #beforeShutdownTasks: Array<{ hook: () => void }> = []; + #beforeShutdownTasks: Array<{ hook: () => void | Promise }> = []; addBeforeShutdownHook(hook: () => void): void { if (this.#hasBeforeShutdown) { diff --git a/packages/backend-plugin-api/report.api.md b/packages/backend-plugin-api/report.api.md index 5db4a9a180..8416c6c200 100644 --- a/packages/backend-plugin-api/report.api.md +++ b/packages/backend-plugin-api/report.api.md @@ -509,7 +509,7 @@ export interface RootHttpRouterService { // @public export interface RootLifecycleService extends LifecycleService { // (undocumented) - addBeforeShutdownHook(hook: () => void): void; + addBeforeShutdownHook(hook: () => void | Promise): void; } // @public diff --git a/packages/backend-plugin-api/src/services/definitions/RootLifecycleService.ts b/packages/backend-plugin-api/src/services/definitions/RootLifecycleService.ts index 085c21e9e2..2fc9556938 100644 --- a/packages/backend-plugin-api/src/services/definitions/RootLifecycleService.ts +++ b/packages/backend-plugin-api/src/services/definitions/RootLifecycleService.ts @@ -24,5 +24,5 @@ import { LifecycleService } from './LifecycleService'; * @public */ export interface RootLifecycleService extends LifecycleService { - addBeforeShutdownHook(hook: () => void): void; + addBeforeShutdownHook(hook: () => void | Promise): void; }