fix: version-service should not use process.nextTick (#6124)

In the beginning we used process.nextTick() as a trick to load some data
initally in the constructor of a service.

This is a bad pattern and we should generally avoid any async operations
in the constructor. Today we have two alternatives:

1. Defer loading until data is needed (wrap it in async)
2. Use the schdule-service.

src/lib/features/instance-stats/instance-stats-service.ts

@@ -211,7 +211,7 @@ export class InstanceStatsService {
      * use getStatsSnapshot for low latency, sacrificing data-freshness
      */
     async getStats(): Promise<InstanceStats> {
-        const versionInfo = this.versionService.getVersionInfo();
+        const versionInfo = await this.versionService.getVersionInfo();
         const [
             featureToggles,
             users,

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

@@ -167,7 +167,7 @@ class ConfigController extends Controller {
             segmentValuesLimit: this.config.segmentValuesLimit,
             strategySegmentsLimit: this.config.strategySegmentsLimit,
             frontendApiOrigins: frontendSettings.frontendApiOrigins,
-            versionInfo: this.versionService.getVersionInfo(),
+            versionInfo: await this.versionService.getVersionInfo(),
             networkViewEnabled: this.config.prometheusApi !== undefined,
             disablePasswordAuth,
             maintenanceMode,

src/lib/services/version-service.test.ts

@@ -44,7 +44,7 @@ test('yields current versions', async () => {
         createFakeGetProductionChanges(),
     );
     await service.checkLatestVersion();
-    const versionInfo = service.getVersionInfo();
+    const versionInfo = await service.getVersionInfo();
     expect(scope.isDone()).toEqual(true);
     expect(versionInfo.current.oss).toBe(version);
     expect(versionInfo.current.enterprise).toBeFalsy();
@@ -83,7 +83,7 @@ test('supports setting enterprise version as well', async () => {
         createFakeGetProductionChanges(),
     );
     await service.checkLatestVersion();
-    const versionInfo = service.getVersionInfo();
+    const versionInfo = await service.getVersionInfo();
     expect(scope.isDone()).toEqual(true);
     expect(versionInfo.current.oss).toBe(version);
     expect(versionInfo.current.enterprise).toBe(enterpriseVersion);
@@ -122,7 +122,7 @@ test('if version check is not enabled should not make any calls', async () => {
         createFakeGetProductionChanges(),
     );
     await service.checkLatestVersion();
-    const versionInfo = service.getVersionInfo();
+    const versionInfo = await service.getVersionInfo();
     expect(scope.isDone()).toEqual(false);
     expect(versionInfo.current.oss).toBe(version);
     expect(versionInfo.current.enterprise).toBe(enterpriseVersion);

src/lib/services/version-service.ts

@@ -178,35 +178,36 @@ export default class VersionService {
         this.telemetryEnabled = telemetry;
         this.versionCheckUrl = versionCheck.url;
         this.isLatest = true;
-        process.nextTick(() => this.setup());
     }
 
-    async setup(): Promise<void> {
+    private async readInstanceId(): Promise<string | undefined> {
-        await this.readInstanceId();
-        await this.checkLatestVersion();
-    }
-
-    private async readInstanceId(): Promise<void> {
         try {
             const { id } = (await this.settingStore.get<{ id: string }>(
                 'instanceInfo',
             )) ?? { id: undefined };
-            this.instanceId = id;
+            return id;
         } catch (err) {
             this.logger.warn('Could not find instanceInfo', err);
         }
     }
 
-    getInstanceId() {
+    async getInstanceId() {
+        if (!this.instanceId) {
+            this.instanceId = await this.readInstanceId();
+        }
         return this.instanceId;
     }
 
     async checkLatestVersion(): Promise<void> {
+        const instanceId = await this.getInstanceId();
+        this.logger.debug(
+            `Checking for newest version for instanceId=${instanceId}`,
+        );
         if (this.enabled) {
             try {
                 const versionPayload: any = {
                     versions: this.current,
-                    instanceId: this.instanceId,
+                    instanceId: instanceId,
                 };
 
                 if (this.telemetryEnabled) {
@@ -282,7 +283,7 @@ export default class VersionService {
             this.userStats(),
             this.productionChanges(),
         ]);
-        const versionInfo = this.getVersionInfo();
+        const versionInfo = await this.getVersionInfo();
         const customStrategies =
             await this.strategyStore.getEditableStrategies();
         const customStrategiesInUse =
@@ -351,15 +352,13 @@ export default class VersionService {
         return settings?.enabled || false;
     }
 
-    getVersionInfo(): IVersionHolder {
+    async getVersionInfo(): Promise<IVersionHolder> {
+        const instanceId = await this.getInstanceId();
         return {
             current: this.current,
             latest: this.latest || {},
             isLatest: this.isLatest,
-            // @ts-ignore instance id can be undefined but not on the version. What should we do is still unclear.
+            instanceId: instanceId || 'unresolved-instance-id',
-            instanceId: this.instanceId,
         };
     }
 }
-
-module.exports = VersionService;