From ef3058328a10efdf1df1def400f426c0d115d379 Mon Sep 17 00:00:00 2001 From: Dan Lynch Date: Fri, 27 Mar 2026 20:28:03 +0000 Subject: [PATCH 1/2] fix(graphile-cache): prevent double-disposal of PostGraphile instances during shutdown Replace key-based disposedKeys Set with entry-based WeakSet to track disposed PostGraphile instances. The previous approach used a string key guard with a finally block that deleted the key after disposal completed. This caused a race condition in closeAllCaches(): 1. closeAllCaches() manually calls disposeEntry() which adds key to disposedKeys, releases pgl, then deletes key in finally block 2. graphileCache.clear() triggers LRU dispose callback for same entry 3. Key is no longer in disposedKeys, so disposeEntry() tries to release the already-released PostGraphile instance 4. PostGraphile throws 'instance has been released' Using a WeakSet keyed on the entry object reference: - Prevents double-disposal without needing cleanup in finally block - Avoids key-reuse edge cases (same cache key, different entry object) - Allows GC to clean up naturally when entries are no longer referenced --- graphile/graphile-cache/src/graphile-cache.ts | 23 ++++++++++--------- 1 file changed, 12 insertions(+), 11 deletions(-) diff --git a/graphile/graphile-cache/src/graphile-cache.ts b/graphile/graphile-cache/src/graphile-cache.ts index 139e80431..2425d178f 100644 --- a/graphile/graphile-cache/src/graphile-cache.ts +++ b/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(); +// 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(); // Track keys that are being manually evicted for accurate eviction reason const manualEvictionKeys = new Set(); @@ -101,15 +106,15 @@ const manualEvictionKeys = new Set(); * 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 => { - // 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 => { // 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 From 6a80cab76719eb70cb28773809cfff691ad58b7d Mon Sep 17 00:00:00 2001 From: Dan Lynch Date: Fri, 27 Mar 2026 20:41:21 +0000 Subject: [PATCH 2/2] test(graphile-cache): add regression test for double-disposal prevention Adds unit tests that verify: - pgl.release() is called exactly once per entry during closeAllCaches() - closeAllCaches() does not throw when LRU dispose callback fires after manual disposal (the original bug scenario) - Empty cache is handled gracefully - Single LRU eviction disposes exactly once --- .../__tests__/graphile-cache.test.ts | 133 ++++++++++++++++++ 1 file changed, 133 insertions(+) create mode 100644 graphile/graphile-cache/__tests__/graphile-cache.test.ts diff --git a/graphile/graphile-cache/__tests__/graphile-cache.test.ts b/graphile/graphile-cache/__tests__/graphile-cache.test.ts new file mode 100644 index 000000000..11835901a --- /dev/null +++ b/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); + }); + }); +});