fix(graphile-cache): prevent double-disposal of PostGraphile instances during shutdown

graphile/graphile-cache/__tests__/graphile-cache.test.ts

@@ -0,0 +1,133 @@
+import { createServer } from 'http';
+import express from 'express';
+import { LRUCache } from 'lru-cache';
+
+/**
+ * Regression test for double-disposal of PostGraphile instances.
+ *
+ * The bug: closeAllCaches() manually disposed each entry via disposeEntry(),
+ * which tracked disposal by key string in a Set and deleted the key in a
+ * `finally` block. Then graphileCache.clear() triggered the LRU dispose
+ * callback for the same entries — but the guard key was already gone, so
+ * pgl.release() was called a second time on an already-released instance.
+ *
+ * The fix: track disposal by entry object reference (WeakSet) so the guard
+ * persists across the manual-dispose → clear() sequence.
+ */
+
+// ── Helpers ──────────────────────────────────────────────────────────
+
+/** Minimal mock that records release() calls and throws on double-release */
+function createMockPgl() {
+  let released = false;
+  return {
+    release: jest.fn(async () => {
+      if (released) {
+        throw new Error('PostGraphile instance has been released');
+      }
+      released = true;
+    }),
+    get isReleased() {
+      return released;
+    }
+  };
+}
+
+function createMockEntry(key: string) {
+  const pgl = createMockPgl();
+  const handler = express();
+  const httpServer = createServer(handler);
+
+  return {
+    entry: {
+      pgl: pgl as any,
+      serv: {} as any,
+      handler,
+      httpServer,
+      cacheKey: key,
+      createdAt: Date.now()
+    },
+    pgl
+  };
+}
+
+// ── Tests ────────────────────────────────────────────────────────────
+
+// We import the real module so the WeakSet-based guard is exercised.
+// pg-cache is a workspace dependency that may not resolve in isolation,
+// so we mock it along with @pgpmjs/logger.
+jest.mock('pg-cache', () => ({
+  pgCache: {
+    registerCleanupCallback: jest.fn(() => jest.fn()),
+    close: jest.fn(async () => {})
+  }
+}));
+
+jest.mock('@pgpmjs/logger', () => ({
+  Logger: class {
+    info = jest.fn();
+    warn = jest.fn();
+    error = jest.fn();
+    debug = jest.fn();
+    success = jest.fn();
+  }
+}));
+
+import {
+  graphileCache,
+  closeAllCaches,
+  type GraphileCacheEntry
+} from '../src/graphile-cache';
+
+describe('graphile-cache', () => {
+  afterEach(async () => {
+    // Ensure the cache is clean between tests.
+    // closeAllCaches resets the singleton promise, so we can call it again.
+    graphileCache.clear();
+  });
+
+  describe('closeAllCaches – double-disposal regression', () => {
+    it('should call pgl.release() exactly once per entry', async () => {
+      const { entry: entry1, pgl: pgl1 } = createMockEntry('api-one');
+      const { entry: entry2, pgl: pgl2 } = createMockEntry('api-two');
+
+      graphileCache.set('api-one', entry1 as GraphileCacheEntry);
+      graphileCache.set('api-two', entry2 as GraphileCacheEntry);
+
+      await closeAllCaches();
+
+      // Each release should be called exactly once — not twice.
+      expect(pgl1.release).toHaveBeenCalledTimes(1);
+      expect(pgl2.release).toHaveBeenCalledTimes(1);
+    });
+
+    it('should not throw when closeAllCaches triggers LRU dispose after manual disposal', async () => {
+      const { entry, pgl } = createMockEntry('api-disposable');
+      graphileCache.set('api-disposable', entry as GraphileCacheEntry);
+
+      // This should complete without the mock throwing
+      // "PostGraphile instance has been released"
+      await expect(closeAllCaches()).resolves.toBeUndefined();
+      expect(pgl.release).toHaveBeenCalledTimes(1);
+    });
+
+    it('should handle empty cache gracefully', async () => {
+      await expect(closeAllCaches()).resolves.toBeUndefined();
+    });
+  });
+
+  describe('LRU eviction – single disposal', () => {
+    it('should dispose entry once on LRU eviction', async () => {
+      const { entry, pgl } = createMockEntry('api-evict');
+      graphileCache.set('api-evict', entry as GraphileCacheEntry);
+
+      // Delete triggers the LRU dispose callback
+      graphileCache.delete('api-evict');
+
+      // Give the async fire-and-forget disposal time to complete
+      await new Promise((r) => setTimeout(r, 50));
+
+      expect(pgl.release).toHaveBeenCalledTimes(1);
+    });
+  });
+});

graphile/graphile-cache/src/graphile-cache.ts

@@ -88,8 +88,13 @@ export interface GraphileCacheEntry {
   createdAt: number;
 }
 
-// Track disposed entries to prevent double-disposal
-const disposedKeys = new Set<string>();
+// Track disposed entries by reference to prevent double-disposal.
+// Using a WeakSet keyed on the entry object (rather than the string key)
+// avoids the race where closeAllCaches() manually disposes an entry,
+// the guard key is cleaned up, and then graphileCache.clear() triggers
+// the LRU dispose callback which attempts to release the same
+// PostGraphile instance a second time.
+const disposedEntries = new WeakSet<GraphileCacheEntry>();
 
 // Track keys that are being manually evicted for accurate eviction reason
 const manualEvictionKeys = new Set<string>();
@@ -101,15 +106,15 @@ const manualEvictionKeys = new Set<string>();
  * 1. Closing the HTTP server if listening
  * 2. Releasing the PostGraphile instance (which internally releases grafserv)
  *
- * Uses disposedKeys set to prevent double-disposal when closeAllCaches()
+ * Uses disposedEntries WeakSet to prevent double-disposal when closeAllCaches()
  * explicitly disposes entries and then clear() triggers the dispose callback.
  */
 const disposeEntry = async (entry: GraphileCacheEntry, key: string): Promise<void> => {
-  // Prevent double-disposal
-  if (disposedKeys.has(key)) {
+  // Prevent double-disposal (tracked by object reference, not key string)
+  if (disposedEntries.has(entry)) {
     return;
   }
-  disposedKeys.add(key);
+  disposedEntries.add(entry);
 
   log.debug(`Disposing PostGraphile[${key}]`);
   try {
@@ -125,8 +130,6 @@ const disposeEntry = async (entry: GraphileCacheEntry, key: string): Promise<voi
     }
   } catch (err) {
     log.error(`Error disposing PostGraphile[${key}]:`, err);
-  } finally {
-    disposedKeys.delete(key);
   }
 };
 
@@ -268,11 +271,9 @@ export const closeAllCaches = async (verbose = false): Promise<void> => {
       // Wait for all disposals to complete
       await Promise.allSettled(disposePromises);
 
-      // Clear the cache after disposal (dispose callback will no-op due to disposedKeys)
+      // Clear the cache after disposal (dispose callback will no-op due to disposedEntries)
       graphileCache.clear();
 
-      // Clear disposed keys tracking after full cleanup
-      disposedKeys.clear();
       manualEvictionKeys.clear();
 
       // Close pg pools