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(