fix: 🐛 implemented up front circular dependency check in the BackendInitializer

Signed-off-by: Marley Powell <marley.powell@exclaimer.com>

.changeset/tricky-melons-fold.md

@@ -2,5 +2,5 @@
 '@backstage/backend-app-api': patch
 ---
 
-fix: :bug: implemented a circular dependency check in the `ServiceRegistry`
+fix: :bug: implemented up front circular dependency check in the `BackendInitializer`
 fix: :bug: updated `detectCircularDependency` in `DependencyGraph` to return circular dependencies starting from the first node

packages/backend-app-api/src/wiring/BackendInitializer.test.ts

@@ -324,4 +324,25 @@ describe('BackendInitializer', () => {
       "Extension point registered for plugin 'testA' may not be used by module for plugin 'testB'",
     );
   });
+
+  it('should throw if circular dependency cycles are detected', async () => {
+    const refA = createServiceRef<string>({ id: 'a' });
+    const refB = createServiceRef<string>({ id: 'b' });
+    const init = new BackendInitializer([
+      createServiceFactory({
+        service: refA,
+        deps: { b: refB },
+        factory: async ({ b }) => b,
+      })(),
+      createServiceFactory({
+        service: refB,
+        deps: { a: refA },
+        factory: async ({ a }) => a,
+      })(),
+    ]);
+    await expect(init.start()).rejects.toThrow(
+      `Circular dependencies detected:
+  'a' -> 'b' -> 'a'`,
+    );
+  });
 });

packages/backend-app-api/src/wiring/BackendInitializer.ts

@@ -172,6 +172,8 @@ export class BackendInitializer {
       ...this.#providedServiceFactories,
     ]);
 
+    this.#serviceHolder.checkForCircularDeps();
+
     const featureDiscovery = await this.#serviceHolder.get(
       featureDiscoveryServiceRef,
       'root',

packages/backend-app-api/src/wiring/ServiceRegistry.test.ts

@@ -330,159 +330,222 @@ describe('ServiceRegistry', () => {
     );
   });
 
-  it('should throw if there are shallow circular dependencies', async () => {
-    const refA = createServiceRef<string>({ id: 'a' });
-    const refB = createServiceRef<string>({ id: 'b' });
+  describe('checkForCircularDeps', () => {
+    it('should throw if there are shallow circular dependencies', async () => {
+      const refA = createServiceRef<string>({ id: 'a' });
+      const refB = createServiceRef<string>({ id: 'b' });
 
-    const factoryA = createServiceFactory({
-      service: refA,
-      deps: { b: refB },
-      factory: async ({ b }) => b,
+      const factoryA = createServiceFactory({
+        service: refA,
+        deps: { b: refB },
+        factory: async ({ b }) => b,
+      });
+
+      const factoryB = createServiceFactory({
+        service: refB,
+        deps: { a: refA },
+        factory: async ({ a }) => a,
+      });
+
+      const registry = new ServiceRegistry([factoryA(), factoryB()]);
+
+      expect(() => registry.checkForCircularDeps()).toThrow(
+        `Circular dependencies detected:
+  'a' -> 'b' -> 'a'`,
+      );
     });
 
-    const factoryB = createServiceFactory({
-      service: refB,
-      deps: { a: refA },
-      factory: async ({ a }) => a,
+    it('should throw if there are multiple circular dependency cycles', async () => {
+      const refA = createServiceRef<string>({ id: 'a' });
+      const refB = createServiceRef<string>({ id: 'b' });
+      const refC = createServiceRef<string>({ id: 'c' });
+      const refD = createServiceRef<string>({ id: 'd' });
+
+      const factoryA = createServiceFactory({
+        service: refA,
+        deps: { b: refB },
+        factory: async ({ b }) => b,
+      });
+
+      const factoryB = createServiceFactory({
+        service: refB,
+        deps: { a: refA },
+        factory: async ({ a }) => a,
+      });
+
+      const factoryC = createServiceFactory({
+        service: refC,
+        deps: { d: refD },
+        factory: async ({ d }) => d,
+      });
+
+      const factoryD = createServiceFactory({
+        service: refD,
+        deps: { c: refC },
+        factory: async ({ c }) => c,
+      });
+
+      const registry = new ServiceRegistry([
+        factoryA(),
+        factoryB(),
+        factoryC(),
+        factoryD(),
+      ]);
+
+      expect(() => registry.checkForCircularDeps()).toThrow(
+        `Circular dependencies detected:
+  'a' -> 'b' -> 'a'
+  'c' -> 'd' -> 'c'`,
+      );
     });
 
-    const registry = new ServiceRegistry([factoryA(), factoryB()]);
+    it('should throw if there are deep circular dependencies', async () => {
+      const refA = createServiceRef<string>({ id: 'a' });
+      const refB = createServiceRef<string>({ id: 'b' });
+      const refC = createServiceRef<string>({ id: 'c' });
 
-    await expect(registry.get(refA, 'catalog')).rejects.toThrow(
-      "Failed to instantiate service 'a' for 'catalog' because of the following circular dependency: 'a' -> 'b' -> 'a'",
-    );
-  });
+      const factoryA = createServiceFactory({
+        service: refA,
+        deps: { b: refB },
+        factory: async ({ b }) => b,
+      });
 
-  it('should throw if there are deep circular dependencies', async () => {
-    const refA = createServiceRef<string>({ id: 'a' });
-    const refB = createServiceRef<string>({ id: 'b' });
-    const refC = createServiceRef<string>({ id: 'c' });
+      const factoryB = createServiceFactory({
+        service: refB,
+        deps: { c: refC },
+        factory: async ({ c }) => c,
+      });
 
-    const factoryA = createServiceFactory({
-      service: refA,
-      deps: { b: refB },
-      factory: async ({ b }) => b,
+      const factoryC = createServiceFactory({
+        service: refC,
+        deps: { a: refA },
+        factory: async ({ a }) => a,
+      });
+
+      const registry = new ServiceRegistry([
+        factoryA(),
+        factoryB(),
+        factoryC(),
+      ]);
+
+      expect(() => registry.checkForCircularDeps()).toThrow(
+        `Circular dependencies detected:
+  'a' -> 'b' -> 'c' -> 'a'`,
+      );
     });
 
-    const factoryB = createServiceFactory({
-      service: refB,
-      deps: { c: refC },
-      factory: async ({ c }) => c,
+    it('should throw if there are deep circular dependencies 2', async () => {
+      const refA = createServiceRef<string>({ id: 'a' });
+      const refB = createServiceRef<string>({ id: 'b' });
+      const refC = createServiceRef<string>({ id: 'c' });
+      const refD = createServiceRef<string>({ id: 'd' });
+
+      const factoryA = createServiceFactory({
+        service: refA,
+        deps: { b: refB },
+        factory: async ({ b }) => b,
+      });
+
+      const factoryB = createServiceFactory({
+        service: refB,
+        deps: { c: refC, d: refD },
+        factory: async ({ c, d }) => c + d,
+      });
+
+      const factoryC = createServiceFactory({
+        service: refC,
+        deps: { a: refA },
+        factory: async ({ a }) => a,
+      });
+
+      const factoryD = createServiceFactory({
+        service: refD,
+        deps: {},
+        factory: async () => 'd',
+      });
+
+      const registry = new ServiceRegistry([
+        factoryA(),
+        factoryB(),
+        factoryC(),
+        factoryD(),
+      ]);
+
+      expect(() => registry.checkForCircularDeps()).toThrow(
+        `Circular dependencies detected:
+  'a' -> 'b' -> 'c' -> 'a'`,
+      );
     });
 
-    const factoryC = createServiceFactory({
-      service: refC,
-      deps: { a: refA },
-      factory: async ({ a }) => a,
+    it('should throw if there are circular dependencies', async () => {
+      const refA = createServiceRef<string>({ id: 'a' });
+      const refB = createServiceRef<string>({ id: 'b' });
+      const refC = createServiceRef<string>({ id: 'c' });
+
+      const factoryA = createServiceFactory({
+        service: refA,
+        deps: { b: refB, c: refC },
+        factory: async ({ b, c }) => b + c,
+      });
+
+      const factoryB = createServiceFactory({
+        service: refB,
+        deps: {},
+        factory: async () => 'b',
+      });
+
+      const factoryC = createServiceFactory({
+        service: refC,
+        deps: { a: refA },
+        factory: async ({ a }) => a,
+      });
+
+      const registry = new ServiceRegistry([
+        factoryA(),
+        factoryB(),
+        factoryC(),
+      ]);
+
+      expect(() => registry.checkForCircularDeps()).toThrow(
+        `Circular dependencies detected:
+  'a' -> 'c' -> 'a'`,
+      );
     });
 
-    const registry = new ServiceRegistry([factoryA(), factoryB(), factoryC()]);
+    it('should not infinitely loop if there are circular dependencies where not all nodes are in the cycle', async () => {
+      const refA = createServiceRef<string>({ id: 'a' });
+      const refB = createServiceRef<string>({ id: 'b' });
+      const refC = createServiceRef<string>({ id: 'c' });
 
-    await expect(registry.get(refA, 'catalog')).rejects.toThrow(
-      "Failed to instantiate service 'a' for 'catalog' because of the following circular dependency: 'a' -> 'b' -> 'c' -> 'a'",
-    );
-  });
+      const factoryA = createServiceFactory({
+        service: refA,
+        deps: { b: refB },
+        factory: async ({ b }) => b,
+      });
 
-  it('should throw if there are deep circular dependencies 2', async () => {
-    const refA = createServiceRef<string>({ id: 'a' });
-    const refB = createServiceRef<string>({ id: 'b' });
-    const refC = createServiceRef<string>({ id: 'c' });
-    const refD = createServiceRef<string>({ id: 'd' });
+      const factoryB = createServiceFactory({
+        service: refB,
+        deps: { c: refC },
+        factory: async ({ c }) => c,
+      });
 
-    const factoryA = createServiceFactory({
-      service: refA,
-      deps: { b: refB },
-      factory: async ({ b }) => b,
+      const factoryC = createServiceFactory({
+        service: refC,
+        deps: { b: refB },
+        factory: async ({ b }) => b,
+      });
+
+      const registry = new ServiceRegistry([
+        factoryA(),
+        factoryB(),
+        factoryC(),
+      ]);
+
+      expect(() => registry.checkForCircularDeps()).toThrow(
+        `Circular dependencies detected:
+  'b' -> 'c' -> 'b'`,
+      );
     });
-
-    const factoryB = createServiceFactory({
-      service: refB,
-      deps: { c: refC, d: refD },
-      factory: async ({ c, d }) => c + d,
-    });
-
-    const factoryC = createServiceFactory({
-      service: refC,
-      deps: { a: refA },
-      factory: async ({ a }) => a,
-    });
-
-    const factoryD = createServiceFactory({
-      service: refD,
-      deps: {},
-      factory: async () => 'd',
-    });
-
-    const registry = new ServiceRegistry([
-      factoryA(),
-      factoryB(),
-      factoryC(),
-      factoryD(),
-    ]);
-
-    await expect(registry.get(refA, 'catalog')).rejects.toThrow(
-      "Failed to instantiate service 'a' for 'catalog' because of the following circular dependency: 'a' -> 'b' -> 'c' -> 'a'",
-    );
-  });
-
-  it('should throw if there are circular dependencies', async () => {
-    const refA = createServiceRef<string>({ id: 'a' });
-    const refB = createServiceRef<string>({ id: 'b' });
-    const refC = createServiceRef<string>({ id: 'c' });
-
-    const factoryA = createServiceFactory({
-      service: refA,
-      deps: { b: refB, c: refC },
-      factory: async ({ b, c }) => b + c,
-    });
-
-    const factoryB = createServiceFactory({
-      service: refB,
-      deps: {},
-      factory: async () => 'b',
-    });
-
-    const factoryC = createServiceFactory({
-      service: refC,
-      deps: { a: refA },
-      factory: async ({ a }) => a,
-    });
-
-    const registry = new ServiceRegistry([factoryA(), factoryB(), factoryC()]);
-
-    await expect(registry.get(refC, 'catalog')).rejects.toThrow(
-      "Failed to instantiate service 'c' for 'catalog' because of the following circular dependency: 'a' -> 'c' -> 'a'",
-    );
-  });
-
-  it('should not infinitely loop if there are circular dependencies where not all nodes are in the cycle', async () => {
-    const refA = createServiceRef<string>({ id: 'a' });
-    const refB = createServiceRef<string>({ id: 'b' });
-    const refC = createServiceRef<string>({ id: 'c' });
-
-    const factoryA = createServiceFactory({
-      service: refA,
-      deps: { b: refB },
-      factory: async ({ b }) => b,
-    });
-
-    const factoryB = createServiceFactory({
-      service: refB,
-      deps: { c: refC },
-      factory: async ({ c }) => c,
-    });
-
-    const factoryC = createServiceFactory({
-      service: refC,
-      deps: { b: refB },
-      factory: async ({ b }) => b,
-    });
-
-    const registry = new ServiceRegistry([factoryA(), factoryB(), factoryC()]);
-
-    await expect(registry.get(refA, 'catalog')).rejects.toThrow(
-      "Failed to instantiate service 'a' for 'catalog' because of the following circular dependency: 'b' -> 'c' -> 'b'",
-    );
   });
 
   it('should decorate error messages thrown by the top-level factory function', async () => {

packages/backend-app-api/src/wiring/ServiceRegistry.ts

@@ -20,7 +20,7 @@ import {
   coreServices,
   createServiceFactory,
 } from '@backstage/backend-plugin-api';
-import { stringifyError } from '@backstage/errors';
+import { ConflictError, stringifyError } from '@backstage/errors';
 import { EnumerableServiceHolder } from './types';
 // Direct internal import to avoid duplication
 // eslint-disable-next-line @backstage/no-forbidden-package-imports
@@ -74,6 +74,7 @@ export class ServiceRegistry implements EnumerableServiceHolder {
     InternalServiceFactory,
     Promise<unknown>
   >();
+  readonly #dependencyGraph: DependencyGraph<string>;
 
   constructor(factories: Array<ServiceFactory>) {
     this.#providedFactories = new Map(
@@ -81,6 +82,15 @@ export class ServiceRegistry implements EnumerableServiceHolder {
     );
     this.#loadedDefaultFactories = new Map();
     this.#implementations = new Map();
+    this.#dependencyGraph = DependencyGraph.fromIterable(
+      Array.from(this.#providedFactories).map(
+        ([serviceId, serviceFactory]) => ({
+          value: serviceId,
+          provides: [serviceId],
+          consumes: Object.values(serviceFactory.deps).map(d => d.id),
+        }),
+      ),
+    );
   }
 
   #resolveFactory(
@@ -147,25 +157,17 @@ export class ServiceRegistry implements EnumerableServiceHolder {
     }
   }
 
-  #checkForCircularDeps(factory: InternalServiceFactory, pluginId: string) {
-    const tree = DependencyGraph.fromIterable(
-      Array.from(this.#providedFactories).map(
-        ([serviceId, serviceFactory]) => ({
-          value: { serviceId, serviceFactory },
-          provides: [serviceId],
-          consumes: serviceFactory ? Object.keys(serviceFactory.deps) : [],
-        }),
-      ),
+  checkForCircularDeps(): void {
+    const circularDependencies = Array.from(
+      this.#dependencyGraph.detectCircularDependencies(),
     );
-    const circular = tree.detectCircularDependency();
 
-    if (circular) {
-      const circularDepChain = circular
-        .map(({ serviceId }) => `'${serviceId}'`)
-        .join(' -> ');
-      throw new Error(
-        `Failed to instantiate service '${factory.service.id}' for '${pluginId}' because of the following circular dependency: ${circularDepChain}`,
-      );
+    if (circularDependencies.length) {
+      const cycles = circularDependencies
+        .map(c => c.map(id => `'${id}'`).join(' -> '))
+        .join('\n  ');
+
+      throw new ConflictError(`Circular dependencies detected:\n  ${cycles}`);
     }
   }
 
@@ -179,7 +181,6 @@ export class ServiceRegistry implements EnumerableServiceHolder {
         let existing = this.#rootServiceImplementations.get(factory);
         if (!existing) {
           this.#checkForMissingDeps(factory, pluginId);
-          this.#checkForCircularDeps(factory, pluginId);
           const rootDeps = new Array<Promise<[name: string, impl: unknown]>>();
 
           for (const [name, serviceRef] of Object.entries(factory.deps)) {
@@ -203,7 +204,6 @@ export class ServiceRegistry implements EnumerableServiceHolder {
       let implementation = this.#implementations.get(factory);
       if (!implementation) {
         this.#checkForMissingDeps(factory, pluginId);
-        this.#checkForCircularDeps(factory, pluginId);
         const rootDeps = new Array<Promise<[name: string, impl: unknown]>>();
 
         for (const [name, serviceRef] of Object.entries(factory.deps)) {

packages/backend-app-api/src/wiring/types.ts

@@ -39,6 +39,7 @@ export interface CreateSpecializedBackendOptions {
 
 export interface ServiceHolder {
   get<T>(api: ServiceRef<T>, pluginId: string): Promise<T> | undefined;
+  checkForCircularDeps(): void;
 }
 
 /**