fix: move startup lifecycle back to plugin service

Signed-off-by: Camila Belo <camilaibs@gmail.com>
2024-12-11 12:16:43 +01:00
parent 0330de4d18
commit 29180ec3d4
12 changed files with 77 additions and 101 deletions
@@ -0,0 +1,5 @@
+---
+'@backstage/backend-defaults': patch
+---
+
+Fix server response time by moving the lifecycle startup hooks back to the plugin lifecycle service.
@@ -0,0 +1,10 @@
+---
+'@backstage/backend-defaults': minor
+---
+
+**BREAKING CHANGE**: The `LifecycleMiddlewareOptions.startupRequestPauseTimeout` has been removed. Use the `backend.lifecycle.startupRequestPauseTimeout` setting in your `app-config.yaml` file to customize how the `createLifecycleMiddleware` function should behave. Also the root config service is required as an options when calling the `createLifecycleMiddleware` function:
+
+```diff
+- createLifecycleMiddleware({ lifecycle, startupRequestPauseTimeout })
+ createLifecycleMiddleware({ config,  lifecycle })
+```
@@ -3,4 +3,3 @@
 ---

 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`.
@@ -10,7 +10,6 @@ import express from 'express';
 import { HttpAuthService } from '@backstage/backend-plugin-api';
 import { HttpRouterService } from '@backstage/backend-plugin-api';
 import { HttpRouterServiceAuthPolicy } from '@backstage/backend-plugin-api';
-import { HumanDuration } from '@backstage/types';
 import { LifecycleService } from '@backstage/backend-plugin-api';
 import { RequestHandler } from 'express';
 import { RootConfigService } from '@backstage/backend-plugin-api';
@@ -51,9 +50,10 @@ export const httpRouterServiceFactory: ServiceFactory<

 // @public
 export interface LifecycleMiddlewareOptions {
+  // (undocumented)
+  config: RootConfigService;
  // (undocumented)
  lifecycle: LifecycleService;
-  startupRequestPauseTimeout?: HumanDuration;
 }

 // (No @packageDocumentation comment for this package)
@@ -134,8 +134,6 @@ export interface RootHttpRouterConfigureContext {
  // (undocumented)
  lifecycle: LifecycleService;
  // (undocumented)
-  lifecycleMiddleware: RequestHandler;
-  // (undocumented)
  logger: LoggerService;
  // (undocumented)
  middleware: MiddlewareFactory;
@@ -78,31 +78,4 @@ describe('createLifecycleMiddleware', () => {
      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 - time in human duration', async () => {
-    jest.useFakeTimers();
-    const configuredTimeout = 20000;
-    const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger());
-    createLifecycleMiddleware({
-      lifecycle,
-      serverShutdownDelay: { milliseconds: configuredTimeout },
-    });
-    const beforeShutdownPromise = lifecycle.beforeShutdown().then(() => {
-      jest.useRealTimers();
-    });
-    jest.advanceTimersByTime(configuredTimeout);
-    return expect(beforeShutdownPromise).resolves.toBeUndefined();
-  });
 });
@@ -34,12 +34,6 @@ export interface LifecycleMiddlewareOptions {
   * Defaults to 5 seconds.
   */
  startupRequestPauseTimeout?: HumanDuration;
-  /**
-   * The maximum time that the server will wait for stop accepting traffic, before returning an error.
-   *
-   * Defaults to 0 seconds.
-   */
-  serverShutdownDelay?: HumanDuration;
 }

 /**
@@ -59,8 +53,7 @@ export interface LifecycleMiddlewareOptions {
 export function createLifecycleMiddleware(
  options: LifecycleMiddlewareOptions,
 ): RequestHandler {
-  const { lifecycle, startupRequestPauseTimeout, serverShutdownDelay } =
-    options;
+  const { lifecycle, startupRequestPauseTimeout } = options;

  let state: 'init' | 'up' | 'down' = 'init';
  const waiting = new Set<{
@@ -79,15 +72,6 @@ export function createLifecycleMiddleware(
    }
  });

-  lifecycle.addBeforeShutdownHook(async () => {
-    const timeoutMs = durationToMilliseconds(
-      serverShutdownDelay ?? DEFAULT_SERVER_SHUTDOWN_TIMEOUT,
-    );
-    return await new Promise(resolve => {
-      setTimeout(resolve, timeoutMs);
-    });
-  });
-
  lifecycle.addShutdownHook(async () => {
    state = 'down';
    for (const item of waiting) {
@@ -20,10 +20,14 @@ import { mockServices } from '@backstage/backend-test-utils';
 import { ServiceUnavailableError } from '@backstage/errors';

 describe('createLifecycleMiddleware', () => {
+  const config = mockServices.rootConfig.mock();
  it('should pause requests when plugin is not ready', async () => {
    const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger());

-    const middleware = createLifecycleMiddleware({ lifecycle });
+    const middleware = createLifecycleMiddleware({
+      config,
+      lifecycle,
+    });

    const next = jest.fn();
    middleware({} as any, {} as any, next);
@@ -41,7 +45,7 @@ describe('createLifecycleMiddleware', () => {

  it('should throw ServiceUnavailableError after shutdown', async () => {
    const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger());
-    const middleware = createLifecycleMiddleware({ lifecycle });
+    const middleware = createLifecycleMiddleware({ config, lifecycle });

    const next = jest.fn();
    middleware({} as any, {} as any, next);
@@ -65,7 +69,15 @@ describe('createLifecycleMiddleware', () => {
    const lifecycle = new BackendLifecycleImpl(mockServices.rootLogger());
    const middleware = createLifecycleMiddleware({
      lifecycle,
-      startupRequestPauseTimeout: { milliseconds: 1 },
+      config: mockServices.rootConfig({
+        data: {
+          backend: {
+            lifecycle: {
+              startupRequestPauseTimeout: { milliseconds: 1 },
+            },
+          },
+        },
+      }),
    });

    const next = jest.fn();
@@ -14,7 +14,11 @@
 * limitations under the License.
 */

-import { LifecycleService } from '@backstage/backend-plugin-api';
+import {
+  RootConfigService,
+  LifecycleService,
+} from '@backstage/backend-plugin-api';
+import { readDurationFromConfig } from '@backstage/config';
 import { ServiceUnavailableError } from '@backstage/errors';
 import { HumanDuration, durationToMilliseconds } from '@backstage/types';
 import { RequestHandler } from 'express';
@@ -26,13 +30,8 @@ export const DEFAULT_TIMEOUT = { seconds: 5 };
 * @public
 */
 export interface LifecycleMiddlewareOptions {
+  config: RootConfigService;
  lifecycle: LifecycleService;
-  /**
-   * The maximum time that paused requests will wait for the service to start, before returning an error.
-   *
-   * Defaults to 5 seconds.
-   */
-  startupRequestPauseTimeout?: HumanDuration;
 }

 /**
@@ -52,7 +51,7 @@ export interface LifecycleMiddlewareOptions {
 export function createLifecycleMiddleware(
  options: LifecycleMiddlewareOptions,
 ): RequestHandler {
-  const { lifecycle, startupRequestPauseTimeout = DEFAULT_TIMEOUT } = options;
+  const { lifecycle } = options;

  let state: 'init' | 'up' | 'down' = 'init';
  const waiting = new Set<{
@@ -81,6 +80,17 @@ export function createLifecycleMiddleware(
    waiting.clear();
  });

+  let startupRequestPauseTimeout: HumanDuration = DEFAULT_TIMEOUT;
+
+  if (
+    'config' in options &&
+    options.config.has('backend.lifecycle.startupRequestPauseTimeout')
+  ) {
+    startupRequestPauseTimeout = readDurationFromConfig(options.config, {
+      key: 'backend.lifecycle.startupRequestPauseTimeout',
+    });
+  }
+
  const timeoutMs = durationToMilliseconds(startupRequestPauseTimeout);

  return (_req, _res, next) => {
@@ -22,6 +22,7 @@ import {
  HttpRouterServiceAuthPolicy,
 } from '@backstage/backend-plugin-api';
 import {
+  createLifecycleMiddleware,
  createCookieAuthRefreshMiddleware,
  createCredentialsBarrier,
  createAuthIntegrationRouter,
@@ -42,11 +43,12 @@ 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 }) {
+  async factory({ auth, httpAuth, config, plugin, rootHttpRouter, lifecycle }) {
    const router = PromiseRouter();

    rootHttpRouter.use(`/api/${plugin.getId()}`, router);
@@ -57,6 +59,7 @@ export const httpRouterServiceFactory = createServiceFactory({
    });

    router.use(createAuthIntegrationRouter({ auth }));
+    router.use(createLifecycleMiddleware({ config, lifecycle }));
    router.use(credentialsBarrier.middleware);
    router.use(createCookieAuthRefreshMiddleware({ auth, httpAuth }));

@@ -207,20 +207,21 @@ describe('rootHttpRouterServiceFactory', () => {
  it('should wait the server to shutdown', async () => {
    jest.useFakeTimers();

+    const serverStopMock = jest.fn();
+
    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.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;
+          options.server.addListener('close', serverStopMock);
        },
      }),
      {
@@ -306,18 +307,6 @@ describe('rootHttpRouterServiceFactory', () => {

    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' });
@@ -327,6 +316,11 @@ describe('rootHttpRouterServiceFactory', () => {
      status: 'error',
    });

-    return await expect(beforeShutdownPromise).resolves.toBeUndefined();
+    return expect(
+      beforeShutdownPromise.then(() => {
+        expect(serverStopMock).toHaveBeenCalled();
+        jest.useRealTimers();
+      }),
+    ).resolves.toBeUndefined();
  });
 });
@@ -30,9 +30,8 @@ import {
 } from './http';
 import { DefaultRootHttpRouter } from './DefaultRootHttpRouter';
 import { createHealthRouter } from './createHealthRouter';
-import { createLifecycleMiddleware } from './createLifecycleMiddleware';
+import { durationToMilliseconds } from '@backstage/types';
 import { readDurationFromConfig } from '@backstage/config';
-import { HumanDuration } from '@backstage/types';

 /**
 * @public
@@ -46,7 +45,6 @@ export interface RootHttpRouterConfigureContext {
  logger: LoggerService;
  lifecycle: LifecycleService;
  healthRouter: RequestHandler;
-  lifecycleMiddleware: RequestHandler;
  applyDefaults: () => void;
 }

@@ -95,26 +93,6 @@ const rootHttpRouterServiceFactoryWithOptions = (

      const healthRouter = createHealthRouter({ config, health });

-      let startupRequestPauseTimeout: HumanDuration | undefined;
-      if (config.has('backend.lifecycle.startupRequestPauseTimeout')) {
-        startupRequestPauseTimeout = readDurationFromConfig(config, {
-          key: 'backend.lifecycle.startupRequestPauseTimeout',
-        });
-      }
-
-      let serverShutdownDelay: HumanDuration | undefined;
-      if (config.has('backend.lifecycle.serverShutdownDelay')) {
-        serverShutdownDelay = readDurationFromConfig(config, {
-          key: 'backend.lifecycle.serverShutdownDelay',
-        });
-      }
-
-      const lifecycleMiddleware = createLifecycleMiddleware({
-        lifecycle,
-        startupRequestPauseTimeout,
-        serverShutdownDelay,
-      });
-
      const server = await createHttpServer(
        app,
        readHttpServerOptions(config.getOptionalConfig('backend')),
@@ -130,7 +108,6 @@ const rootHttpRouterServiceFactoryWithOptions = (
        logger,
        lifecycle,
        healthRouter,
-        lifecycleMiddleware,
        applyDefaults() {
          if (process.env.NODE_ENV === 'development') {
            app.set('json spaces', 2);
@@ -140,13 +117,24 @@ 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());
        },
      });

+      if (config.has('backend.lifecycle.serverShutdownDelay')) {
+        const serverShutdownDelay = readDurationFromConfig(config, {
+          key: 'backend.lifecycle.serverShutdownDelay',
+        });
+        lifecycle.addBeforeShutdownHook(async () => {
+          const timeoutMs = durationToMilliseconds(serverShutdownDelay);
+          return await new Promise(resolve => {
+            setTimeout(resolve, timeoutMs);
+          });
+        });
+      }
+
      lifecycle.addShutdownHook(() => server.stop());

      await server.start();