From f6cc18a6e61e5b833f3b40da12900dbd6610c66a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Gast=C3=B3n=20Fournier?= Date: Mon, 24 Nov 2025 17:56:04 +0100 Subject: [PATCH] chore: handle unawaited promises and tests (#11014) ## About the changes This adds handling of unhandled rejected promises at the process level, so nodejs doesn't panic. Also, adds tests around processing metrics and a refactor from previous pr: #11010 --- src/lib/features/metrics/instance/metrics.ts | 42 ++-- src/lib/server-impl.ts | 14 +- .../bulk-metrics-rejections.e2e.test.ts | 211 ++++++++++++++++++ 3 files changed, 245 insertions(+), 22 deletions(-) create mode 100644 src/test/e2e/api/client/bulk-metrics-rejections.e2e.test.ts diff --git a/src/lib/features/metrics/instance/metrics.ts b/src/lib/features/metrics/instance/metrics.ts index d51a440228..b898802eed 100644 --- a/src/lib/features/metrics/instance/metrics.ts +++ b/src/lib/features/metrics/instance/metrics.ts @@ -150,6 +150,23 @@ export default class ClientMetricsController extends Controller { // Note: Custom metrics GET endpoints are now handled by the admin API } + private async processPromiseResults( + promises: Promise[], + ): Promise { + const results = await Promise.allSettled(promises); + const rejected = results.filter( + (result): result is PromiseRejectedResult => + result.status === 'rejected', + ); + if (rejected.length) { + this.logger.warn( + 'Some promise tasks failed', + rejected.map((r) => r.reason?.message || r.reason), + ); + } + return rejected.length === 0; + } + async registerMetrics( req: IAuthRequest, res: Response, @@ -240,6 +257,7 @@ export default class ClientMetricsController extends Controller { const { body, ip: clientIp } = req; const { metrics, applications, impactMetrics } = body; const promises: Promise[] = []; + try { for (const app of applications) { if ( @@ -287,32 +305,14 @@ export default class ClientMetricsController extends Controller { ); } - const results = await Promise.allSettled(promises); - const rejected = results.filter( - (result): result is PromiseRejectedResult => - result.status === 'rejected', - ); - if (rejected.length) { - this.logger.warn( - 'Some bulkMetrics tasks failed', - rejected.map((r) => r.reason?.message || r.reason), - ); + const ok = await this.processPromiseResults(promises); + if (!ok) { res.status(400).end(); } else { res.status(202).end(); } } catch (e) { - const results = await Promise.allSettled(promises); - const rejected = results.filter( - (result): result is PromiseRejectedResult => - result.status === 'rejected', - ); - if (rejected.length) { - this.logger.warn( - 'Some bulkMetrics tasks failed', - rejected.map((r) => r.reason?.message || r.reason), - ); - } + await this.processPromiseResults(promises); res.status(400).end(); } } diff --git a/src/lib/server-impl.ts b/src/lib/server-impl.ts index 9f302a4a24..2a04c67580 100644 --- a/src/lib/server-impl.ts +++ b/src/lib/server-impl.ts @@ -227,8 +227,20 @@ export async function createApp( createMetricsMonitor, }, ): Promise { - // Database dependencies (stateful) const logger = config.getLogger('server-impl.js'); + + // Surface unhandled promise rejections to logs so they don't crash the process + process.on('unhandledRejection', (reason: unknown) => { + if (reason instanceof Error) { + logger.error('Unhandled promise rejection detected', reason); + } else { + logger.error( + `Unhandled promise rejection detected: ${String(reason)}`, + ); + } + }); + + // Database dependencies (stateful) const serverVersion = config.enterpriseVersion ?? version; const db = fm.createDb(config); const stores = fm.createStores(config, db); diff --git a/src/test/e2e/api/client/bulk-metrics-rejections.e2e.test.ts b/src/test/e2e/api/client/bulk-metrics-rejections.e2e.test.ts new file mode 100644 index 0000000000..d848a72939 --- /dev/null +++ b/src/test/e2e/api/client/bulk-metrics-rejections.e2e.test.ts @@ -0,0 +1,211 @@ +import dbInit, { type ITestDb } from '../../helpers/database-init.js'; +import { + type IUnleashTest, + setupAppWithCustomConfig, +} from '../../helpers/test-helper.js'; +import getLogger from '../../../fixtures/no-logger.js'; +import { minutesToMilliseconds } from 'date-fns'; + +let app: IUnleashTest; +let db: ITestDb; + +const postAndFlushBulkMetrics = async ( + body: Record, + status: number, +): Promise => { + await app.request + .post('/api/client/metrics/bulk') + .send(body) + .expect(status); + // flush metrics to the database + await app.services.clientMetricsServiceV2.bulkAdd(); + await app.services.clientInstanceService.bulkAdd(); +}; + +beforeAll(async () => { + db = await dbInit('bulk_metrics_rejections', getLogger); + app = await setupAppWithCustomConfig( + db.stores, + { + experimental: { + flags: { + strictSchemaValidation: true, + }, + }, + metricsRateLimiting: { + clientMetricsMaxPerMinute: 1000, + frontendMetricsMaxPerMinute: 1000, + frontendRegisterMaxPerMinute: 1000, + source: 'predefined', + enable: false, + enabledWarning: false, + }, + }, + db.rawDatabase, + ); +}); + +afterAll(async () => { + await app?.destroy(); + await db?.destroy(); +}); + +afterEach(async () => { + await db.stores.clientApplicationsStore.deleteAll?.(); + await db.stores.clientInstanceStore.deleteAll?.(); +}); + +test('returns 400 and stores nothing when applications contain invalid registrations and metrics also fail validation', async () => { + const started = new Date().toISOString(); + await postAndFlushBulkMetrics( + { + applications: [ + { + appName: null, + instanceId: 'instance-1', + environment: 'development', + interval: minutesToMilliseconds(1), + started, + strategies: ['default'], + }, + ], + metrics: [ + { + featureName: 'demo-toggle', + appName: null, + environment: 'development', + timestamp: new Date().toISOString(), + yes: 1, + no: 0, + variants: { variantA: 1 }, + }, + ], + }, + 400, + ); + + const apps = await db.stores.clientApplicationsStore.getAll(); + const instances = await db.stores.clientInstanceStore.getAll(); + + expect(apps).toHaveLength(0); + expect(instances).toHaveLength(0); +}); + +test('returns 400 and stores nothing when application registration rejects but metrics are otherwise valid', async () => { + const started = new Date().toISOString(); + await postAndFlushBulkMetrics( + { + applications: [ + { + appName: null, + instanceId: 'instance-1', + environment: 'development', + interval: minutesToMilliseconds(1), + started, + strategies: ['default'], + }, + ], + metrics: [ + { + featureName: 'demo-toggle', + appName: 'edge-client', + environment: 'development', + timestamp: new Date().toISOString(), + yes: 1, + no: 0, + variants: { variantA: 1 }, + }, + ], + }, + 400, + ); + + const apps = await db.stores.clientApplicationsStore.getAll(); + const instances = await db.stores.clientInstanceStore.getAll(); + + expect(apps).toHaveLength(0); + expect(instances).toHaveLength(0); +}); + +test('accepts valid bulk payload and stores registrations and instances', async () => { + const started = new Date().toISOString(); + await postAndFlushBulkMetrics( + { + applications: [ + { + appName: 'valid-app', + instanceId: 'instance-1', + environment: 'development', + interval: minutesToMilliseconds(1), + started, + strategies: ['default'], + }, + ], + metrics: [ + { + featureName: 'demo-toggle', + appName: 'valid-app', + environment: 'development', + timestamp: new Date().toISOString(), + yes: 1, + no: 0, + variants: { variantA: 1 }, + }, + ], + }, + 202, + ); + + const apps = await db.stores.clientApplicationsStore.getAll(); + const instances = await db.stores.clientInstanceStore.getAll(); + + expect(apps.length).toBe(1); + expect(apps[0].appName).toBe('valid-app'); + expect(instances.length).toBe(1); + expect(instances[0].appName).toBe('valid-app'); + expect(instances[0].instanceId).toBe('instance-1'); +}); + +test('does not emit unhandled rejections when application registration rejects and metrics validation fails', async () => { + const unhandled: unknown[] = []; + const handler = (reason: unknown) => { + unhandled.push(reason); + }; + process.on('unhandledRejection', handler); + + try { + await postAndFlushBulkMetrics( + { + applications: [ + { + appName: null, + instanceId: 'instance-1', + environment: 'development', + interval: minutesToMilliseconds(1), + started: new Date().toISOString(), + strategies: ['default'], + }, + ], + metrics: [ + { + featureName: 'demo-toggle', + appName: null, + environment: 'development', + timestamp: new Date().toISOString(), + yes: 1, + no: 0, + variants: { variantA: 1 }, + }, + ], + }, + 400, + ); + + // Let any pending unhandled rejections surface + await new Promise((resolve) => setTimeout(resolve, 1000)); + + expect(unhandled).toHaveLength(0); + } finally { + process.off('unhandledRejection', handler); + } +});