Move error handler to CacheManager from Client.

Signed-off-by: Eric Peterson <ericpeterson@spotify.com>

.changeset/improbable-word-combination.md

@@ -2,15 +2,16 @@
 '@backstage/backend-common': patch
 ---
 
-Plugin developers may now provide handlers for connection errors emitted by cache stores.
+All cache-related connection errors are now handled and logged by the cache manager. App Integrators may provide an optional error handler when instantiating the cache manager if custom error handling is needed.
 
 ```typescript
 // Providing an error handler
-const cacheClient = somePluginCache.getClient({
-  defaultTtl: 3600000,
+const cacheManager = CacheManager.fromConfig(config, {
   onError: e => {
-    logger.error(`There was a cache connection problem: ${e.message}`);
-    execOtherErrorHandlingLogic();
+    if (isSomehowUnrecoverable(e)) {
+      gracefullyShutThingsDown();
+      process.exit(1);
+    }
   },
 });
 ```

packages/backend-common/src/cache/CacheManager.test.ts

@@ -88,13 +88,12 @@ describe('CacheManager', () => {
 
     it('attaches error handler to client', () => {
       const pluginId = 'error-test';
-      const handler = jest.fn();
-      manager.forPlugin(pluginId).getClient({ onError: handler });
+      manager.forPlugin(pluginId).getClient();
 
       const client = DefaultCacheClient as jest.Mock;
       const mockCalls = client.mock.calls.splice(-1);
       const realClient = mockCalls[0][0].client as Keyv;
-      expect(realClient.on).toHaveBeenCalledWith('error', handler);
+      expect(realClient.on).toHaveBeenCalledWith('error', expect.any(Function));
     });
 
     it('provides different plugins different cache clients', async () => {
@@ -174,4 +173,53 @@ describe('CacheManager', () => {
       expect(mockMemcacheCalls[0][0]).toEqual(expectedHost);
     });
   });
+
+  describe('connection errors', () => {
+    it('uses provided logger', () => {
+      // Set up and inject mock logger.
+      const mockLogger = { child: jest.fn(), error: jest.fn() };
+      mockLogger.child.mockImplementation(() => mockLogger as any);
+      const manager = CacheManager.fromConfig(defaultConfig(), {
+        logger: mockLogger as any,
+      });
+
+      // Set up a cache client using the configured manager.
+      manager.forPlugin('error-logger-test').getClient();
+
+      // Retrieve the error handler attached to the cache client.
+      const client = DefaultCacheClient as jest.Mock;
+      const mockCalls = client.mock.calls.splice(-1);
+      const realClient = mockCalls[0][0].client as Keyv;
+      const realOnError = realClient.on as jest.Mock;
+      const realHandler = realOnError.mock.calls.splice(-1)[0][1];
+
+      // Invoke the actual error handler.
+      const expectedError = new Error('some error');
+      realHandler(expectedError);
+      expect(mockLogger.error).toHaveBeenCalledWith(expectedError);
+    });
+
+    it('calls provided handler', () => {
+      // Set up and inject mock logger.
+      const mockHandler = jest.fn();
+      const manager = CacheManager.fromConfig(defaultConfig(), {
+        onError: mockHandler,
+      });
+
+      // Set up a cache client using the configured manager.
+      manager.forPlugin('error-handler-test').getClient();
+
+      // Retrieve the error handler attached to the cache client.
+      const client = DefaultCacheClient as jest.Mock;
+      const mockCalls = client.mock.calls.splice(-1);
+      const realClient = mockCalls[0][0].client as Keyv;
+      const realOnError = realClient.on as jest.Mock;
+      const realHandler = realOnError.mock.calls.splice(-1)[0][1];
+
+      // Invoke the actual error handler.
+      const expectedError = new Error('some error');
+      realHandler(expectedError);
+      expect(mockHandler).toHaveBeenCalledWith(expectedError);
+    });
+  });
 });

packages/backend-common/src/cache/CacheManager.ts

@@ -18,9 +18,15 @@ import { Config } from '@backstage/config';
 import Keyv from 'keyv';
 // @ts-expect-error
 import KeyvMemcache from 'keyv-memcache';
+import { Logger } from 'winston';
+import { getRootLogger } from '../logging';
 import { DefaultCacheClient, CacheClient } from './CacheClient';
 import { NoStore } from './NoStore';
-import { PluginCacheManager } from './types';
+import {
+  CacheManagerOptions,
+  OptionalOnError,
+  PluginCacheManager,
+} from './types';
 
 /**
  * Implements a Cache Manager which will automatically create new cache clients
@@ -38,8 +44,10 @@ export class CacheManager {
     none: this.getNoneClient,
   };
 
+  private readonly logger: Logger;
   private readonly store: keyof CacheManager['storeFactories'];
   private readonly connection: string;
+  private readonly errorHandler: OptionalOnError;
 
   /**
    * Creates a new CacheManager instance by reading from the `backend` config
@@ -47,21 +55,34 @@ export class CacheManager {
    *
    * @param config The loaded application configuration.
    */
-  static fromConfig(config: Config): CacheManager {
+  static fromConfig(
+    config: Config,
+    options: CacheManagerOptions = {},
+  ): CacheManager {
     // If no `backend.cache` config is provided, instantiate the CacheManager
     // with a "NoStore" cache client.
     const store = config.getOptionalString('backend.cache.store') || 'none';
     const connectionString =
       config.getOptionalString('backend.cache.connection') || '';
-    return new CacheManager(store, connectionString);
+    const logger = (options.logger || getRootLogger()).child({
+      type: 'cacheManager',
+    });
+    return new CacheManager(store, connectionString, logger, options.onError);
   }
 
-  private constructor(store: string, connectionString: string) {
+  private constructor(
+    store: string,
+    connectionString: string,
+    logger: Logger,
+    errorHandler: OptionalOnError,
+  ) {
     if (!this.storeFactories.hasOwnProperty(store)) {
       throw new Error(`Unknown cache store: ${store}`);
     }
+    this.logger = logger;
     this.store = store as keyof CacheManager['storeFactories'];
     this.connection = connectionString;
+    this.errorHandler = errorHandler;
   }
 
   /**
@@ -74,10 +95,16 @@ export class CacheManager {
       getClient: (opts = {}): CacheClient => {
         const concreteClient = this.getClientWithTtl(pluginId, opts.defaultTtl);
 
-        // Attach error handler if provided.
-        if (typeof opts?.onError === 'function') {
-          concreteClient.on('error', opts.onError);
-        }
+        // Always provide an error handler to avoid killing the process.
+        concreteClient.on('error', (err: Error) => {
+          // In all cases, just log the error.
+          this.logger.error(err);
+
+          // Invoke any custom error handler if provided.
+          if (typeof this.errorHandler === 'function') {
+            this.errorHandler(err);
+          }
+        });
 
         return new DefaultCacheClient({
           client: concreteClient,

packages/backend-common/src/cache/types.ts

@@ -14,6 +14,7 @@
  * limitations under the License.
  */
 
+import { Logger } from 'winston';
 import { CacheClient } from './CacheClient';
 
 type ClientOptions = {
@@ -23,12 +24,21 @@ type ClientOptions = {
    * can be configured per entry at set-time).
    */
   defaultTtl?: number;
+};
+
+export type OptionalOnError = ((err: Error) => void) | undefined;
+
+export type CacheManagerOptions = {
+  /**
+   * An optional logger for use by the PluginCacheManager.
+   */
+  logger?: Logger;
 
   /**
    * An optional handler for connection errors emitted from the underlying data
    * store.
    */
-  onError?: (err: Error) => void;
+  onError?: OptionalOnError;
 };
 
 /**