mirror of
https://github.com/Unleash/unleash.git
synced 2025-11-24 20:06:55 +01:00
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
This commit is contained in:
Gastón Fournier
GitHub
parent
712cecf38d
commit
f6cc18a6e6
@ -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<void>[],
|
||||
): Promise<boolean> {
|
||||
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<void, void, ClientMetricsSchema>,
|
||||
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<void>[] = [];
|
||||
|
||||
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();
|
||||
}
|
||||
}
|
||||
|
||||
@ -227,8 +227,20 @@ export async function createApp(
|
||||
createMetricsMonitor,
|
||||
},
|
||||
): Promise<IUnleash> {
|
||||
// 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);
|
||||
|
||||
211
src/test/e2e/api/client/bulk-metrics-rejections.e2e.test.ts
Normal file
211
src/test/e2e/api/client/bulk-metrics-rejections.e2e.test.ts
Normal file
@ -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<string, any>,
|
||||
status: number,
|
||||
): Promise<void> => {
|
||||
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);
|
||||
}
|
||||
});
|
||||
Loading…
Reference in New Issue
Block a user