fix(agent-bff): flush spans from the shutdown path, not from a signal handler

Two problems with the flush being its own SIGTERM listener, both raised in review.

It armed at --require time, before the CLI arms its shutdown handler. A signal in
that window was consumed by a listener that terminates nothing, so the process ran
on until SIGKILL — the same PID 1 trap as before, one layer up.

And it was detached, so the exit could cut an export still in flight.

The preload now parks the SDK on a shared handle and arms nothing; armShutdown
flushes through it and waits, so the exit cannot race the export. The flush gets its
own 2s deadline rather than the 10s in-flight requests get: telemetry is not worth
holding a shutdown past an orchestrator's patience. Worst case a dead collector adds
~3s. Verified: exit 0 either way.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Author: nbouliol

packages/agent-bff/README.md

@@ -84,8 +84,10 @@ The Docker image ships with [OpenTelemetry](https://opentelemetry.io/) APM built
 any OTLP-compatible backend (Datadog, Grafana Tempo, Jaeger, Honeycomb, etc.). It is **off by
 default** and turns on as soon as you point it at an OTLP receiver — no code changes or extra
 installs required. Tracing is set up before the app starts (auto-instrumentation for HTTP and the
-outbound calls to the agent and the Forest SaaS), and buffered spans are flushed on `SIGTERM` /
-`SIGINT`, alongside the graceful shutdown described above.
+outbound calls to the agent and the Forest SaaS). The graceful shutdown described above waits for
+the buffered spans to be exported before it exits, but gives that its own 2 second deadline rather
+than the 10 seconds in-flight requests get: an unreachable collector costs you the last spans, never
+the ability to stop. Worst case it adds ~3 seconds to a shutdown.
 
 Configure it entirely through the standard OTel environment variables:
 

packages/agent-bff/src/cli.ts

@@ -4,13 +4,17 @@ import createConsoleLogger from './adapters/console-logger';
 import { reportFatalError } from './cli-core';
 import dispatchCli from './cli-dispatch';
 import armShutdown from './shutdown';
+import { flushTracing } from './tracing-handle';
 
 if (require.main === module) {
   dispatchCli(process.argv.slice(2), process.env)
     .then(({ exitCode, server }) => {
       // Only the server command has anything to shut down; `openapi` and the flags
       // have already finished by the time they return.
-      if (server) armShutdown({ server, logger: createConsoleLogger() });
+      // flushTracing is a no-op unless the image's tracing preload armed an SDK. Wiring it here,
+      // rather than letting the preload arm its own signal handler, is what makes the exit wait
+      // for the export instead of racing it.
+      if (server) armShutdown({ server, logger: createConsoleLogger(), flush: flushTracing });
       if (exitCode !== 0) process.exitCode = exitCode;
     })
     .catch(reportFatalError);

packages/agent-bff/src/shutdown.ts

@@ -10,6 +10,19 @@ export interface ShutdownOptions {
   logger?: Logger;
   /** How long in-flight requests get before their sockets are cut. */
   graceMs?: number;
+  /**
+   * Anything that must reach its destination before the process ends — today the buffered
+   * OpenTelemetry spans. It runs alongside `server.stop()` and is awaited with it, so the exit
+   * cannot cut an export that is still in flight.
+   */
+  flush?: () => Promise<void>;
+  /**
+   * The flush's own, much shorter deadline. It deliberately does not share `graceMs`: finishing a
+   * request someone is waiting on is worth ten seconds, telemetry is not, and an unreachable
+   * collector that held the process for the full grace period would be SIGKILLed by an orchestrator
+   * whose own patience starts at the same ten seconds — losing the shutdown, not just the spans.
+   */
+  flushMs?: number;
   /** Signal registration, as a seam: a test must not arm a handler on the real process. */
   onSignal?: (signal: NodeJS.Signals, handler: () => void) => void;
   exit?: (code: number) => void;
@@ -18,6 +31,7 @@ export interface ShutdownOptions {
 export const SIGNALS: NodeJS.Signals[] = ['SIGTERM', 'SIGINT'];
 export const DEFAULT_GRACE_MS = 10_000;
 export const FORCE_EXIT_MS = 1_000;
+export const DEFAULT_FLUSH_MS = 2_000;
 
 /**
  * Ends the process without `process.exit`, which would discard whatever is still buffered on stdout
@@ -34,6 +48,24 @@ function defaultExit(code: number): void {
   setTimeout(() => process.exit(code), FORCE_EXIT_MS).unref();
 }
 
+/**
+ * Gives a promise a deadline, resolving either way — a slow collector should cost the shutdown its
+ * last spans, not its ability to end. The timer is cleared on settle so it cannot hold the loop open
+ * once the work is done.
+ */
+function bounded(work: Promise<void>, ms: number): Promise<void> {
+  return new Promise(resolve => {
+    const timer = setTimeout(resolve, ms);
+
+    void work
+      .catch(() => undefined)
+      .then(() => {
+        clearTimeout(timer);
+        resolve();
+      });
+  });
+}
+
 /**
  * Stops the server on a termination signal, then exits.
  *
@@ -51,6 +83,8 @@ export default function armShutdown(options: ShutdownOptions): void {
     server,
     logger,
     graceMs = DEFAULT_GRACE_MS,
+    flush,
+    flushMs = DEFAULT_FLUSH_MS,
     onSignal = (signal, handler) => {
       process.on(signal, handler);
     },
@@ -75,8 +109,7 @@ export default function armShutdown(options: ShutdownOptions): void {
     stopping = true;
     logger?.('Info', 'Shutting down', { signal, graceMs });
 
-    server
-      .stop(graceMs)
+    Promise.all([server.stop(graceMs), flush ? bounded(flush(), flushMs) : undefined])
       .then(() => {
         if (interrupted) return;
 

packages/agent-bff/src/tracing-handle.ts

@@ -0,0 +1,38 @@
+/**
+ * The one piece of state the preload and the CLI have to share.
+ *
+ * `--require` runs the preload in its own module, before `cli.js` is even loaded, so the SDK it
+ * builds has nowhere to go but a module both sides resolve to the same instance. Keeping it here —
+ * rather than having the preload arm a signal handler of its own — is what lets the shutdown path
+ * WAIT for the flush instead of racing it, and keeps the preload from consuming a signal the CLI
+ * has not armed a handler for yet.
+ *
+ * Nothing OpenTelemetry-specific is imported here, so the npm bin pays only for an empty variable.
+ */
+
+export interface TracingHandle {
+  shutdown(): Promise<void>;
+}
+
+let handle: TracingHandle | undefined;
+
+export function setTracingHandle(sdk: TracingHandle | undefined): void {
+  handle = sdk;
+}
+
+export function getTracingHandle(): TracingHandle | undefined {
+  return handle;
+}
+
+/**
+ * Flushes buffered spans, or resolves immediately when tracing was never armed — the shutdown path
+ * should not have to know which. A failing export is swallowed: a dead collector must not turn a
+ * clean shutdown into a failed one.
+ */
+export async function flushTracing(): Promise<void> {
+  try {
+    await handle?.shutdown();
+  } catch {
+    /* istanbul ignore next — nothing to do about it, and it must not change the exit code. */
+  }
+}

packages/agent-bff/src/tracing-preload.ts

@@ -1,5 +1,10 @@
 // The `--require` entry point of the Docker image (see the Dockerfile's ENTRYPOINT). Separate from
 // `tracing.ts` on purpose: importing the setup must never arm an SDK, only calling it should.
+//
+// The SDK is handed to `tracing-handle` rather than wired to a signal here. The CLI arms the only
+// termination handler, and it flushes through that handle — so a signal arriving before the CLI is
+// up cannot be swallowed by a listener that does not terminate anything.
 import initTracing from './tracing';
+import { setTracingHandle } from './tracing-handle';
 
-initTracing();
+setTracingHandle(initTracing());

packages/agent-bff/src/tracing.ts

@@ -8,7 +8,10 @@ import createConsoleLogger from './adapters/console-logger';
  * apart is what lets this module be imported by a test without arming an SDK.
  *
  * The SDK is initialised only when `OTEL_EXPORTER_OTLP_ENDPOINT` is set, so an install that never
- * opted into APM pays nothing. Ending the process is not this module's business — see `armFlush`.
+ * opted into APM pays nothing. Neither ending the process nor flushing on the way out is this
+ * module's business: it hands the SDK back, the preload parks it in `tracing-handle`, and the
+ * shutdown path flushes through that. Arming a signal handler here would consume a signal the CLI
+ * has not armed its own handler for yet, and nothing would then terminate the process.
  * All configuration goes through the standard OTel environment variables:
  *
  *   OTEL_EXPORTER_OTLP_ENDPOINT   OTLP receiver (e.g. http://localhost:4318)
@@ -23,7 +26,7 @@ import createConsoleLogger from './adapters/console-logger';
 
 export const DEFAULT_SERVICE_NAME = 'forestadmin-agent-bff';
 
-interface OtelSdk {
+export interface OtelSdk {
   start(): void;
   shutdown(): Promise<void>;
 }
@@ -39,8 +42,6 @@ export interface TracingOptions {
   logger?: Logger;
   /** The dynamic require, as a seam: the packages exist only in the Docker image. */
   load?: () => OtelModules | undefined;
-  /** Signal registration, as a seam: a test must not arm a handler on the real process. */
-  onSignal?: (signal: NodeJS.Signals, handler: () => void) => void;
 }
 
 /** The packages this image installs for APM, and the export it takes from each. */
@@ -71,27 +72,6 @@ export function loadOtelModules(load: ModuleLoader = require): OtelModules | und
   }
 }
 
-/**
- * Flushes buffered spans on the way out, alongside the shutdown `armShutdown` runs — this handler
- * does not end the process and must not try to. Ending it is `src/shutdown.ts`'s job: it closes the
- * server, bounds the wait for in-flight requests and exits explicitly, which is the only thing that
- * works when node is PID 1 (the kernel gives PID 1 no default disposition, so "let the default
- * action terminate" ends in a SIGKILL rather than a clean exit).
- *
- * So this only flushes. Once both the flush and that shutdown settle the loop empties and the
- * process exits on its own. A rejected flush is swallowed for the same reason: it must not become
- * an unhandled rejection that outlives the shutdown it is running beside.
- */
-function armFlush(sdk: OtelSdk, onSignal: NonNullable<TracingOptions['onSignal']>): void {
-  const signals: NodeJS.Signals[] = ['SIGTERM', 'SIGINT'];
-
-  for (const signal of signals) {
-    onSignal(signal, () => {
-      void sdk.shutdown().catch(() => undefined);
-    });
-  }
-}
-
 /**
  * The OTel specification defines its boolean environment variables as case-insensitive, and this
  * one is the kill switch: reading `TRUE` as "not disabled" would leave tracing running for someone
@@ -102,14 +82,7 @@ function isDisabled(raw: string | undefined): boolean {
 }
 
 export default function initTracing(options: TracingOptions = {}): OtelSdk | undefined {
-  const {
-    env = process.env,
-    logger = createConsoleLogger(),
-    load = loadOtelModules,
-    onSignal = (signal, handler) => {
-      process.once(signal, handler);
-    },
-  } = options;
+  const { env = process.env, logger = createConsoleLogger(), load = loadOtelModules } = options;
 
   const endpoint = env.OTEL_EXPORTER_OTLP_ENDPOINT?.trim();
 
@@ -135,7 +108,6 @@ export default function initTracing(options: TracingOptions = {}): OtelSdk | und
   });
 
   sdk.start();
-  armFlush(sdk, onSignal);
 
   logger('Info', 'OpenTelemetry tracing enabled', { serviceName, endpoint });
 

packages/agent-bff/test/shutdown.test.ts

@@ -2,7 +2,7 @@ import type { Logger } from '../src/ports/logger-port';
 
 import armShutdown, { DEFAULT_GRACE_MS, FORCE_EXIT_MS } from '../src/shutdown';
 
-const flush = () =>
+const settle = () =>
   new Promise(resolve => {
     setImmediate(resolve);
   });
@@ -38,7 +38,7 @@ describe('armShutdown', () => {
       arm(500);
 
       handlers.SIGTERM();
-      await flush();
+      await settle();
 
       expect(stop).toHaveBeenCalledWith(500);
       expect(exit).toHaveBeenCalledWith(0);
@@ -48,7 +48,7 @@ describe('armShutdown', () => {
       arm();
 
       handlers.SIGINT();
-      await flush();
+      await settle();
 
       expect(stop).toHaveBeenCalledWith(DEFAULT_GRACE_MS);
     });
@@ -60,7 +60,7 @@ describe('armShutdown', () => {
       arm();
 
       handlers.SIGTERM();
-      await flush();
+      await settle();
 
       expect(exit).toHaveBeenCalledWith(1);
       expect(logger).toHaveBeenCalledWith('Error', 'Shutdown failed, exiting anyway', {
@@ -69,6 +69,61 @@ describe('armShutdown', () => {
     });
   });
 
+  describe('with a flush to run', () => {
+    it('should run it alongside the stop and exit only once both settle', async () => {
+      let releaseFlush: () => void = () => undefined;
+      const flush = jest.fn(
+        () =>
+          new Promise<void>(resolve => {
+            releaseFlush = resolve;
+          }),
+      );
+      armShutdown({ server: { stop }, logger, onSignal, exit, flush, flushMs: 500 });
+
+      handlers.SIGTERM();
+      await settle();
+
+      expect(flush).toHaveBeenCalledTimes(1);
+      expect(exit).not.toHaveBeenCalled();
+
+      releaseFlush();
+      await settle();
+
+      expect(exit).toHaveBeenCalledWith(0);
+    });
+
+    // A collector that never answers must cost the shutdown its last spans, not its ability to end.
+    // The deadline is the flush's own: telemetry does not get the grace period in-flight requests do.
+    it('should give up on the flush once its own deadline elapses', async () => {
+      jest.useFakeTimers();
+      const flush = jest.fn(
+        () =>
+          new Promise<void>(() => {
+            /* never settles */
+          }),
+      );
+      armShutdown({ server: { stop }, logger, onSignal, exit, flush, flushMs: 500 });
+
+      handlers.SIGTERM();
+      await Promise.resolve();
+      jest.advanceTimersByTime(500);
+      jest.useRealTimers();
+      await settle();
+
+      expect(exit).toHaveBeenCalledWith(0);
+    });
+
+    it('should not let a failing flush change the exit code', async () => {
+      const flush = jest.fn().mockRejectedValue(new Error('collector down'));
+      armShutdown({ server: { stop }, logger, onSignal, exit, flush });
+
+      handlers.SIGTERM();
+      await settle();
+
+      expect(exit).toHaveBeenCalledWith(0);
+    });
+  });
+
   describe('on a second signal', () => {
     it('should exit 1 instead of stopping the server again', async () => {
       let release: () => void = () => undefined;
@@ -86,7 +141,7 @@ describe('armShutdown', () => {
       expect(exit).toHaveBeenCalledWith(1);
 
       release();
-      await flush();
+      await settle();
     });
 
     it('should not let the interrupted stop report success once it finishes', async () => {
@@ -101,7 +156,7 @@ describe('armShutdown', () => {
       handlers.SIGTERM();
       handlers.SIGINT();
       release();
-      await flush();
+      await settle();
 
       expect(exit).toHaveBeenCalledTimes(1);
       expect(exit).toHaveBeenCalledWith(1);
@@ -120,7 +175,7 @@ describe('armShutdown', () => {
       handlers.SIGTERM();
       handlers.SIGINT();
       fail(new Error('close failed'));
-      await flush();
+      await settle();
 
       expect(exit).toHaveBeenCalledTimes(1);
       expect(logger).not.toHaveBeenCalledWith(