coalesce concurrent secret-provider probes

src/lib/server/secretproviders/probe-cache.ts

@@ -1,10 +1,14 @@
 /**
  * Short-lived cache for live provider probes.
  *
- * The stack editor probes the bound provider on every debounced keystroke to
- * classify a compose ${VAR} as present ("IN VAULT") or missing. Without a cache
- * a fast typist would hammer the provider's API. This caches ONLY the KEY NAMES
- * of a bulk selector for a few seconds - never the secret values.
+ * The stack editor probes the bound provider to classify a compose ${VAR} as
+ * present ("IN VAULT") or missing. Without protection a busy editor page (debounced
+ * keystrokes + the list auto-refresh) fires overlapping probes, and each one would
+ * hit the provider: wasted 1Password/Vault API calls, and for the CLI providers a
+ * burst of keepassxc-cli/bws spawns (each re-deriving the key). Two guards:
+ *   - a ~30s result cache (KEY NAMES only, never values), and
+ *   - single-flight: probes for the same (providerId, selector) that arrive while a
+ *     call is already in flight share that one Promise instead of spawning their own.
  */
 
 import type { SecretProvider, SecretProviderConfig } from './shared';
@@ -16,14 +20,20 @@ interface Entry {
 	at: number;
 }
 
-// Keyed by `${providerId}:${selector}`. Bounded by (provider count * distinct
-// selectors seen in the TTL window); stale entries are dropped lazily on read.
+// Keyed by `${providerId}:${selector}`. A stale entry is deleted when read, so the
+// map self-trims to selectors probed within the TTL window rather than growing
+// forever across a live editing session (every keystroke is a distinct selector).
 const cache = new Map<string, Entry>();
 
+// In-flight probes, keyed the same way, so concurrent callers coalesce onto one
+// provider call. Cleared when the call settles (success or failure).
+const inflight = new Map<string, Promise<string[]>>();
+
 /**
  * Returns the KEY NAMES available under a provider's bulk selector, cached for
  * ~30s per (providerId, selector). Values are discarded immediately - only the
- * names are stored and returned. Propagates the provider's errors to the caller.
+ * names are stored and returned. Concurrent callers for the same key coalesce onto
+ * a single provider call. Propagates the provider's errors to the caller.
  */
 export async function probeBulkKeysCached(
 	providerId: number,
@@ -34,11 +44,24 @@ export async function probeBulkKeysCached(
 	const cacheKey = `${providerId}:${selector}`;
 	const now = Date.now();
 	const hit = cache.get(cacheKey);
-	if (hit && now - hit.at < TTL_MS) {
-		return hit.keys;
+	if (hit) {
+		if (now - hit.at < TTL_MS) return hit.keys;
+		cache.delete(cacheKey); // stale: drop it so the map doesn't grow unbounded
 	}
-	const bulk = await provider.resolveBulk(config, selector);
-	const keys = Object.keys(bulk);
-	cache.set(cacheKey, { keys, at: now });
-	return keys;
+
+	// Coalesce onto an existing in-flight call for the same key.
+	const pending = inflight.get(cacheKey);
+	if (pending) return pending;
+
+	const call = (async () => {
+		const bulk = await provider.resolveBulk(config, selector);
+		const keys = Object.keys(bulk);
+		cache.set(cacheKey, { keys, at: Date.now() });
+		return keys;
+	})().finally(() => {
+		inflight.delete(cacheKey);
+	});
+
+	inflight.set(cacheKey, call);
+	return call;
 }

tests/secret-provider-probe-cache.test.ts

@@ -0,0 +1,90 @@
+/**
+ * probeBulkKeysCached: result cache (~30s, names only) + single-flight coalescing.
+ * The single-flight guard stops overlapping stack-editor probes from each hitting the
+ * provider (wasted 1Password/Vault API calls; a burst of keepassxc-cli/bws spawns).
+ */
+import { describe, it, expect } from 'bun:test';
+import { probeBulkKeysCached } from '../src/lib/server/secretproviders/probe-cache';
+
+// A provider whose resolveBulk is slow and counts its own calls. Cast to the
+// SecretProvider shape (only resolveBulk is exercised here).
+function countingProvider(delayMs = 30) {
+	let calls = 0;
+	const provider: any = {
+		supportsBulk: true,
+		async resolveBulk() {
+			calls++;
+			await new Promise((r) => setTimeout(r, delayMs));
+			return { KEY_A: 'a', KEY_B: 'b' };
+		}
+	};
+	return { provider, calls: () => calls };
+}
+
+describe('probeBulkKeysCached', () => {
+	it('coalesces concurrent probes for the same (provider, selector) into ONE call', async () => {
+		const { provider, calls } = countingProvider();
+		const id = 1001; // unique id so cache/inflight don't collide with other tests
+		const results = await Promise.all(
+			Array.from({ length: 10 }, () => probeBulkKeysCached(id, provider, {} as any, 'grp'))
+		);
+		expect(calls()).toBe(1); // single-flight: 10 concurrent -> 1 provider call
+		// every caller still gets the same key names
+		for (const r of results) expect(r.sort()).toEqual(['KEY_A', 'KEY_B']);
+	});
+
+	it('serves the cached result on a later call without re-calling the provider', async () => {
+		const { provider, calls } = countingProvider();
+		const id = 1002;
+		await probeBulkKeysCached(id, provider, {} as any, 'grp');
+		await probeBulkKeysCached(id, provider, {} as any, 'grp');
+		expect(calls()).toBe(1); // second call hit the ~30s cache
+	});
+
+	it('does NOT coalesce different selectors', async () => {
+		const { provider, calls } = countingProvider();
+		const id = 1003;
+		await Promise.all([
+			probeBulkKeysCached(id, provider, {} as any, 'grp-a'),
+			probeBulkKeysCached(id, provider, {} as any, 'grp-b')
+		]);
+		expect(calls()).toBe(2); // distinct keys -> distinct calls
+	});
+
+	it('re-probes (does not serve a stale entry) once the TTL has elapsed', async () => {
+		// The cache TTL is 30s; we can't wait that long, so drive it by advancing the
+		// clock via a stubbed Date.now. A stale hit must be dropped and re-fetched.
+		const { provider, calls } = countingProvider();
+		const id = 1005;
+		const realNow = Date.now;
+		try {
+			let t = 1_000_000;
+			(Date as any).now = () => t;
+			await probeBulkKeysCached(id, provider, {} as any, 'grp'); // call 1, cached at t
+			t += 31_000; // jump past the 30s TTL
+			await probeBulkKeysCached(id, provider, {} as any, 'grp'); // stale -> call 2
+			expect(calls()).toBe(2);
+		} finally {
+			(Date as any).now = realNow;
+		}
+	});
+
+	it('does not cache a failure, and a later probe retries', async () => {
+		let calls = 0;
+		const provider: any = {
+			supportsBulk: true,
+			async resolveBulk() {
+				calls++;
+				if (calls === 1) throw new Error('transient');
+				return { OK: 'v' };
+			}
+		};
+		const id = 1004;
+		await expect(probeBulkKeysCached(id, provider, {} as any, 'grp')).rejects.toThrow('transient');
+		// the in-flight entry must have been cleared, so this retries rather than
+		// returning a stale/failed promise
+		const keys = await probeBulkKeysCached(id, provider, {} as any, 'grp');
+		expect(keys).toEqual(['OK']);
+		expect(calls).toBe(2);
+	});
+});