Client instance service (#5126)

src/lib/features/client-feature-toggles/tests/client-feature-toggle.e2e.test.ts

@@ -24,9 +24,6 @@ async function getSetup() {
         base,
         clientFeatureToggleStore: stores.clientFeatureToggleStore,
         request: supertest(app),
-        destroy: () => {
-            services.clientInstanceService.destroy();
-        },
     };
 }
 
@@ -43,7 +40,6 @@ const callGetAll = async (controller: FeatureController) => {
 
 let base;
 let request;
-let destroy;
 
 let flagResolver;
 
@@ -51,7 +47,6 @@ beforeEach(async () => {
     const setup = await getSetup();
     base = setup.base;
     request = setup.request;
-    destroy = setup.destroy;
     flagResolver = {
         isEnabled: () => {
             return false;
@@ -59,10 +54,6 @@ beforeEach(async () => {
     };
 });
 
-afterEach(() => {
-    destroy();
-});
-
 test('should get empty getFeatures via client', () => {
     expect.assertions(1);
     return request

src/lib/routes/admin-api/config.test.ts

@@ -28,25 +28,16 @@ async function getSetup() {
     return {
         base,
         request: supertest(app),
-        destroy: () => {
-            services.clientInstanceService.destroy();
-        },
     };
 }
 
 let request;
 let base;
-let destroy;
 
 beforeEach(async () => {
     const setup = await getSetup();
     request = setup.request;
     base = setup.base;
-    destroy = setup.destroy;
-});
-
-afterEach(() => {
-    destroy();
 });
 
 test('should get ui config', async () => {

src/lib/routes/admin-api/context.test.ts

@@ -20,25 +20,16 @@ async function getSetup() {
     return {
         base,
         request: supertest(app),
-        destroy: () => {
-            services.clientInstanceService.destroy();
-        },
     };
 }
 
 let base;
 let request;
-let destroy;
 
 beforeEach(async () => {
     const setup = await getSetup();
     base = setup.base;
     request = setup.request;
-    destroy = setup.destroy;
-});
-
-afterEach(async () => {
-    await destroy();
 });
 
 test('should get all context definitions', () => {

src/lib/routes/admin-api/metrics.test.ts

@@ -19,25 +19,16 @@ async function getSetup() {
         stores,
         perms,
         config,
-        destroy: () => {
-            services.clientInstanceService.destroy();
-        },
     };
 }
 
 let stores;
 let request;
-let destroy;
 
 beforeEach(async () => {
     const setup = await getSetup();
     stores = setup.stores;
     request = setup.request;
-    destroy = setup.destroy;
-});
-
-afterEach(() => {
-    destroy();
 });
 
 test('/api/admin/metrics/seen-toggles is deprecated', () => {

src/lib/routes/admin-api/public-signup.test.ts

@@ -33,16 +33,11 @@ describe('Public Signup API', () => {
             request: supertest(app),
             stores,
             perms,
-            destroy: () => {
-                services.clientInstanceService.destroy();
-                services.publicSignupTokenService.destroy();
-            },
         };
     }
 
     let stores;
     let request;
-    let destroy;
 
     const user = {
         username: 'some-username',
@@ -55,12 +50,8 @@ describe('Public Signup API', () => {
         const setup = await getSetup();
         stores = setup.stores;
         request = setup.request;
-        destroy = setup.destroy;
     });
 
-    afterEach(() => {
-        destroy();
-    });
     const expireAt = (addDays: number = 7): Date => {
         const now = new Date();
         now.setDate(now.getDate() + addDays);

src/lib/routes/admin-api/strategy.test.ts

@@ -5,8 +5,6 @@ import permissions from '../../../test/fixtures/permissions';
 import getApp from '../../app';
 import { createServices } from '../../services';
 
-let destroy;
-
 async function getSetup() {
     const randomBase = `/random${Math.round(Math.random() * 1000)}`;
     const perms = permissions();
@@ -18,10 +16,6 @@ async function getSetup() {
     const services = createServices(stores, config);
     const app = await getApp(config, stores, services);
 
-    destroy = () => {
-        services.clientInstanceService.destroy();
-    };
-
     return {
         base: randomBase,
         strategyStore: stores.strategyStore,
@@ -30,10 +24,6 @@ async function getSetup() {
     };
 }
 
-afterEach(() => {
-    destroy();
-});
-
 test('add version numbers for /strategies', async () => {
     const { request, base } = await getSetup();
     return request

src/lib/routes/admin-api/tag.test.ts

@@ -21,26 +21,18 @@ async function getSetup() {
         perms,
         tagStore: stores.tagStore,
         request: supertest(app),
-        destroy: () => {
-            services.clientInstanceService.destroy();
-        },
     };
 }
 
 let base;
 let tagStore;
 let request;
-let destroy;
 
 beforeEach(async () => {
     const setup = await getSetup();
     base = setup.base;
     tagStore = setup.tagStore;
     request = setup.request;
-    destroy = setup.destroy;
-});
-afterEach(() => {
-    destroy();
 });
 
 test('should get empty getTags via admin', () => {

src/lib/routes/backstage.test.ts

@@ -19,5 +19,4 @@ test('should enable prometheus', async () => {
         .get('/internal-backstage/prometheus')
         .expect('Content-Type', /text/)
         .expect(200);
-    services.clientInstanceService.destroy();
 });

src/lib/routes/client-api/metrics.test.ts

@@ -19,10 +19,7 @@ async function getSetup(opts?: IUnleashOptions) {
         request: supertest(app),
         stores: db.stores,
         services,
-        destroy: async () => {
+        destroy: db.destroy,
-            services.clientInstanceService.destroy();
-            await db.destroy();
-        },
     };
 }
 
@@ -31,7 +28,7 @@ let stores: IUnleashStores;
 let services: IUnleashServices;
 let destroy;
 
-beforeEach(async () => {
+beforeAll(async () => {
     const setup = await getSetup();
     request = setup.request;
     stores = setup.stores;
@@ -39,10 +36,14 @@ beforeEach(async () => {
     services = setup.services;
 });
 
-afterEach(() => {
+afterAll(() => {
     destroy();
 });
 
+afterEach(async () => {
+    await stores.featureToggleStore.deleteAll();
+});
+
 test('should validate client metrics', () => {
     return request
         .post('/api/client/metrics')

src/lib/routes/client-api/register.test.ts

@@ -14,20 +14,14 @@ async function getSetup() {
     return {
         request: supertest(app),
         stores,
-        destroy: () => {
-            services.clientInstanceService.destroy();
-        },
     };
 }
 let request;
-let destroy;
 beforeEach(async () => {
     const setup = await getSetup();
     request = setup.request;
-    destroy = setup.destroy;
 });
 afterEach(() => {
-    destroy();
     getLogger.setMuteError(false);
 });
 

src/lib/routes/health-check.test.ts

@@ -15,21 +15,15 @@ async function getSetup() {
     return {
         request: supertest(app),
         stores,
-        destroy: () => {
-            services.clientInstanceService.destroy();
-        },
     };
 }
 let request;
-let destroy;
 beforeEach(async () => {
     const setup = await getSetup();
     request = setup.request;
-    destroy = setup.destroy;
 });
 
 afterEach(() => {
-    destroy();
     getLogger.setMuteError(false);
 });
 

src/lib/routes/public-invite.test.ts

@@ -39,16 +39,11 @@ describe('Public Signup API', () => {
             request: supertest(app),
             stores,
             perms,
-            destroy: () => {
-                services.clientInstanceService.destroy();
-                services.publicSignupTokenService.destroy();
-            },
         };
     }
 
     let stores;
     let request;
-    let destroy;
 
     const user = {
         username: 'some-username',
@@ -61,12 +56,8 @@ describe('Public Signup API', () => {
         const setup = await getSetup();
         stores = setup.stores;
         request = setup.request;
-        destroy = setup.destroy;
     });
 
-    afterEach(() => {
-        destroy();
-    });
     const expireAt = (addDays: number = 7): Date => {
         const now = new Date();
         now.setDate(now.getDate() + addDays);

src/lib/services/client-metrics/instance-service.test.ts

@@ -5,45 +5,11 @@ import FakeEventStore from '../../../test/fixtures/fake-event-store';
 import { createTestConfig } from '../../../test/config/test-config';
 import { FakePrivateProjectChecker } from '../../features/private-project/fakePrivateProjectChecker';
 
-/**
- * A utility to wait for any pending promises in the test subject code.
- * For instance, if the test needs to wait for a timeout/interval handler,
- * and that handler does something async, advancing the timers is not enough:
- * We have to explicitly wait for the second promise.
- * For more info, see https://stackoverflow.com/a/51045733/2868829
- *
- * Usage in test code after advancing timers, but before making assertions:
- *
- * test('hello', async () => {
- *    jest.useFakeTimers('modern');
- *
- *    // Schedule a timeout with a callback that does something async
- *    // before calling our spy
- *    const spy = jest.fn();
- *    setTimeout(async () => {
- *        await Promise.resolve();
- *        spy();
- *    }, 1000);
- *
- *    expect(spy).not.toHaveBeenCalled();
- *
- *    jest.advanceTimersByTime(1500);
- *    await flushPromises(); // this is required to make it work!
- *
- *    expect(spy).toHaveBeenCalledTimes(1);
- *
- *    jest.useRealTimers();
- * });
- */
-function flushPromises() {
-    return Promise.resolve(setImmediate);
-}
 let config;
 beforeAll(() => {
     config = createTestConfig({});
 });
 test('Multiple registrations of same appname and instanceid within same time period should only cause one registration', async () => {
-    jest.useFakeTimers();
     const appStoreSpy = jest.fn();
     const bulkSpy = jest.fn();
     const clientApplicationsStore: any = {
@@ -75,8 +41,8 @@ test('Multiple registrations of same appname and instanceid within same time per
     await clientMetrics.registerClient(client1, '127.0.0.1');
     await clientMetrics.registerClient(client1, '127.0.0.1');
     await clientMetrics.registerClient(client1, '127.0.0.1');
-    jest.advanceTimersByTime(7000);
+
-    await flushPromises();
+    await clientMetrics.bulkAdd(); // in prod called by a SchedulerService
 
     expect(appStoreSpy).toHaveBeenCalledTimes(1);
     expect(bulkSpy).toHaveBeenCalledTimes(1);
@@ -93,7 +59,6 @@ test('Multiple registrations of same appname and instanceid within same time per
 });
 
 test('Multiple unique clients causes multiple registrations', async () => {
-    jest.useFakeTimers();
     const appStoreSpy = jest.fn();
     const bulkSpy = jest.fn();
     const clientApplicationsStore: any = {
@@ -136,16 +101,14 @@ test('Multiple unique clients causes multiple registrations', async () => {
     await clientMetrics.registerClient(client2, '127.0.0.1');
     await clientMetrics.registerClient(client2, '127.0.0.1');
 
-    jest.advanceTimersByTime(7000);
+    await clientMetrics.bulkAdd(); // in prod called by a SchedulerService
-    await flushPromises();
 
     const registrations = appStoreSpy.mock.calls[0][0];
 
     expect(registrations.length).toBe(2);
-    jest.useRealTimers();
 });
+
 test('Same client registered outside of dedup interval will be registered twice', async () => {
-    jest.useFakeTimers();
     const appStoreSpy = jest.fn();
     const bulkSpy = jest.fn();
     const clientApplicationsStore: any = {
@@ -155,8 +118,6 @@ test('Same client registered outside of dedup interval will be registered twice'
         bulkUpsert: bulkSpy,
     };
 
-    const bulkInterval = secondsToMilliseconds(2);
-
     const clientMetrics = new ClientInstanceService(
         {
             clientMetricsStoreV2: null,
@@ -168,7 +129,6 @@ test('Same client registered outside of dedup interval will be registered twice'
         },
         config,
         new FakePrivateProjectChecker(),
-        bulkInterval,
     );
     const client1 = {
         appName: 'test_app',
@@ -181,14 +141,13 @@ test('Same client registered outside of dedup interval will be registered twice'
     await clientMetrics.registerClient(client1, '127.0.0.1');
     await clientMetrics.registerClient(client1, '127.0.0.1');
 
-    jest.advanceTimersByTime(3000);
+    await clientMetrics.bulkAdd(); // in prod called by a SchedulerService
 
     await clientMetrics.registerClient(client1, '127.0.0.1');
     await clientMetrics.registerClient(client1, '127.0.0.1');
     await clientMetrics.registerClient(client1, '127.0.0.1');
 
-    jest.advanceTimersByTime(3000);
+    await clientMetrics.bulkAdd(); // in prod called by a SchedulerService
-    await flushPromises();
 
     expect(appStoreSpy).toHaveBeenCalledTimes(2);
     expect(bulkSpy).toHaveBeenCalledTimes(2);
@@ -198,11 +157,9 @@ test('Same client registered outside of dedup interval will be registered twice'
 
     expect(firstRegistrations.appName).toBe(secondRegistrations.appName);
     expect(firstRegistrations.instanceId).toBe(secondRegistrations.instanceId);
-    jest.useRealTimers();
 });
 
 test('No registrations during a time period will not call stores', async () => {
-    jest.useFakeTimers();
     const appStoreSpy = jest.fn();
     const bulkSpy = jest.fn();
     const clientApplicationsStore: any = {
@@ -211,7 +168,7 @@ test('No registrations during a time period will not call stores', async () => {
     const clientInstanceStore: any = {
         bulkUpsert: bulkSpy,
     };
-    new ClientInstanceService(
+    const clientMetrics = new ClientInstanceService(
         {
             clientMetricsStoreV2: null,
             strategyStore: null,
@@ -223,8 +180,9 @@ test('No registrations during a time period will not call stores', async () => {
         config,
         new FakePrivateProjectChecker(),
     );
-    jest.advanceTimersByTime(6000);
+
+    await clientMetrics.bulkAdd(); // in prod called by a SchedulerService
+
     expect(appStoreSpy).toHaveBeenCalledTimes(0);
     expect(bulkSpy).toHaveBeenCalledTimes(0);
-    jest.useRealTimers();
 });

src/lib/services/client-metrics/instance-service.ts

@@ -29,8 +29,6 @@ export default class ClientInstanceService {
 
     seenClients: Record<string, IClientApp> = {};
 
-    private timers: NodeJS.Timeout[] = [];
-
     private clientMetricsStoreV2: IClientMetricsStoreV2;
 
     private strategyStore: IStrategyStore;
@@ -47,10 +45,6 @@ export default class ClientInstanceService {
 
     private flagResolver: IFlagResolver;
 
-    private bulkInterval: number;
-
-    private announcementInterval: number;
-
     constructor(
         {
             clientMetricsStoreV2,
@@ -73,8 +67,6 @@ export default class ClientInstanceService {
             flagResolver,
         }: Pick<IUnleashConfig, 'getLogger' | 'flagResolver'>,
         privateProjectChecker: IPrivateProjectChecker,
-        bulkInterval = secondsToMilliseconds(5),
-        announcementInterval = minutesToMilliseconds(5),
     ) {
         this.clientMetricsStoreV2 = clientMetricsStoreV2;
         this.strategyStore = strategyStore;
@@ -87,18 +79,6 @@ export default class ClientInstanceService {
         this.logger = getLogger(
             '/services/client-metrics/client-instance-service.ts',
         );
-
-        this.bulkInterval = bulkInterval;
-        this.announcementInterval = announcementInterval;
-        this.timers.push(
-            setInterval(() => this.bulkAdd(), this.bulkInterval).unref(),
-        );
-        this.timers.push(
-            setInterval(
-                () => this.announceUnannounced(),
-                this.announcementInterval,
-            ).unref(),
-        );
     }
 
     public async registerInstance(
@@ -248,8 +228,4 @@ export default class ClientInstanceService {
     async removeInstancesOlderThanTwoDays(): Promise<void> {
         return this.clientInstanceStore.removeInstancesOlderThanTwoDays();
     }
-
-    destroy(): void {
-        this.timers.forEach(clearInterval);
-    }
 }

src/lib/services/index.ts

@@ -165,6 +165,18 @@ export const scheduleServices = async (
         'removeInstancesOlderThanTwoDays',
     );
 
+    schedulerService.schedule(
+        clientInstanceService.bulkAdd.bind(clientInstanceService),
+        secondsToMilliseconds(5),
+        'bulkAddInstances',
+    );
+
+    schedulerService.schedule(
+        clientInstanceService.announceUnannounced.bind(clientInstanceService),
+        minutesToMilliseconds(5),
+        'announceUnannounced',
+    );
+
     schedulerService.schedule(
         projectService.statusJob.bind(projectService),
         hoursToMilliseconds(24),

src/lib/services/public-signup-token-service.ts

@@ -30,8 +30,6 @@ export class PublicSignupTokenService {
 
     private logger: Logger;
 
-    private timer: NodeJS.Timeout;
-
     private readonly unleashBase: string;
 
     constructor(
@@ -146,9 +144,4 @@ export class PublicSignupTokenService {
     private getMinimumDate(date1: Date, date2: Date): Date {
         return date1 < date2 ? date1 : date2;
     }
-
-    destroy(): void {
-        clearInterval(this.timer);
-        this.timer = null;
-    }
 }

src/test/e2e/services/client-metrics-service.e2e.test.ts

@@ -12,7 +12,7 @@ const { APPLICATION_CREATED } = require('../../../lib/types/events');
 
 let stores;
 let db;
-let clientInstanceService;
+let clientInstanceService: ClientInstanceService;
 let config: IUnleashConfig;
 beforeAll(async () => {
     db = await dbInit('client_metrics_service_serial', getLogger);
@@ -25,13 +25,10 @@ beforeAll(async () => {
         stores,
         config,
         new FakePrivateProjectChecker(),
-        bulkInterval,
-        announcementInterval,
     );
 });
 
 afterAll(async () => {
-    await clientInstanceService.destroy();
     await db.destroy();
 });
 test('Apps registered should be announced', async () => {
@@ -58,11 +55,11 @@ test('Apps registered should be announced', async () => {
     };
     await clientInstanceService.registerClient(clientRegistration, '127.0.0.1');
     await clientInstanceService.registerClient(differentClient, '127.0.0.1');
-    await new Promise((res) => setTimeout(res, 1200));
+    await clientInstanceService.bulkAdd(); // in prod called by a SchedulerService
     const first = await stores.clientApplicationsStore.getUnannounced();
     expect(first.length).toBe(2);
     await clientInstanceService.registerClient(clientRegistration, '127.0.0.1');
-    await new Promise((res) => setTimeout(res, secondsToMilliseconds(2)));
+    await clientInstanceService.announceUnannounced(); // in prod called by a SchedulerService
     const second = await stores.clientApplicationsStore.getUnannounced();
     expect(second.length).toBe(0);
     const events = await stores.eventStore.getEvents();