server-core: cache + share the team GitHub snapshot (2.5min TTL, SWR)

packages/server-core/src/github-team.test.ts

@@ -1,16 +1,21 @@
 import { mkdtempSync, rmSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
-import { afterAll, beforeAll, describe, expect, it } from "bun:test";
+import { afterAll, afterEach, beforeAll, describe, expect, it } from "bun:test";
 import {
 	type AgentRef,
 	type MailEvent,
 	type TeamGitHubSnapshot,
+	clearTeamGitHubSnapshotCache,
 	deriveActivityFeed,
 	deriveWorkBoard,
 	fetchTeamGitHubSnapshot,
+	refreshTeamGitHubSnapshotCache,
+	seedTeamGitHubSnapshotCacheForTest,
 } from "./github-team";
 
+const SNAPSHOT_CACHE_TTL_MS = 150_000;
+
 function issue(
 	over: Partial<TeamGitHubSnapshot["issues"][number]> &
 		Pick<TeamGitHubSnapshot["issues"][number], "number">,
@@ -270,3 +275,111 @@ describe("fetchTeamGitHubSnapshot", () => {
 		expect(snap).toEqual({ issues: [], prs: [] });
 	});
 });
+
+describe("fetchTeamGitHubSnapshot cache (issue #66)", () => {
+	afterEach(() => {
+		clearTeamGitHubSnapshotCache();
+	});
+
+	function snapshot(n: number): TeamGitHubSnapshot {
+		return { issues: [issue({ number: n })], prs: [] };
+	}
+
+	it("fresh miss awaits the live fetch", async () => {
+		let calls = 0;
+		const fetcher = async () => {
+			calls++;
+			return snapshot(1);
+		};
+		const result = await refreshTeamGitHubSnapshotCache("repo-a", fetcher);
+		expect(result).toEqual(snapshot(1));
+		expect(calls).toBe(1);
+	});
+
+	it("hit within TTL returns cached without spawning", async () => {
+		seedTeamGitHubSnapshotCacheForTest("repo-b", snapshot(1), 0);
+
+		// A fresh-TTL entry means fetchTeamGitHubSnapshot must return straight
+		// from the cache without ever touching the (real, `gh`-backed) fetcher —
+		// there is no injectable-fetcher seam on this path to spy on, so the
+		// absence of a spawn is exactly what "resolves without needing gh" means
+		// here: the call resolves immediately with the seeded data.
+		const result = await fetchTeamGitHubSnapshot("repo-b");
+		expect(result).toEqual(snapshot(1));
+	});
+
+	it("stale hit returns stale immediately and triggers exactly one refresh under concurrent calls", async () => {
+		let calls = 0;
+		let resolveRefresh: (() => void) | undefined;
+		const fetcher = async () => {
+			calls++;
+			await new Promise<void>((resolve) => {
+				resolveRefresh = resolve;
+			});
+			return snapshot(2);
+		};
+
+		// Three concurrent low-level refresh calls for the same key must share
+		// a single in-flight fetch (this is the exact primitive a stale hit in
+		// fetchTeamGitHubSnapshot delegates to).
+		const first = refreshTeamGitHubSnapshotCache("repo-c", fetcher);
+		const second = refreshTeamGitHubSnapshotCache("repo-c", fetcher);
+		const third = refreshTeamGitHubSnapshotCache("repo-c", fetcher);
+		expect(calls).toBe(1);
+
+		resolveRefresh?.();
+		const [r1, r2, r3] = await Promise.all([first, second, third]);
+		expect(r1).toEqual(snapshot(2));
+		expect(r2).toEqual(snapshot(2));
+		expect(r3).toEqual(snapshot(2));
+		expect(calls).toBe(1);
+	});
+
+	it("stale read through the public API returns stale data without waiting on the refresh", async () => {
+		let resolveSlow: ((value: TeamGitHubSnapshot) => void) | undefined;
+		seedTeamGitHubSnapshotCacheForTest(
+			"repo-d",
+			snapshot(1),
+			SNAPSHOT_CACHE_TTL_MS + 1,
+		);
+
+		// Kick a slow low-level refresh under the same key first, so the public
+		// call below observes it already in flight (it would otherwise start
+		// its own refresh against the real `gh`-backed fetcher).
+		const refreshPromise = refreshTeamGitHubSnapshotCache("repo-d", () => {
+			return new Promise<TeamGitHubSnapshot>((resolve) => {
+				resolveSlow = resolve;
+			});
+		});
+
+		const result = await fetchTeamGitHubSnapshot("repo-d");
+		expect(result).toEqual(snapshot(1));
+
+		resolveSlow?.(snapshot(2));
+		await refreshPromise;
+	});
+
+	it("refresh failure keeps serving stale (cache untouched, no throw to the caller)", async () => {
+		const seeded = await refreshTeamGitHubSnapshotCache(
+			"repo-e",
+			async () => snapshot(1),
+		);
+		expect(seeded).toEqual(snapshot(1));
+
+		// A failing refresh rejects its own promise (a caller who explicitly
+		// awaits a refresh can observe the failure)...
+		const failing = refreshTeamGitHubSnapshotCache("repo-e", async () => {
+			throw new Error("gh exploded");
+		});
+		await expect(failing).rejects.toThrow("gh exploded");
+
+		// ...but must not clobber the cached entry: the failed refresh's `.then`
+		// (where snapshotCache.set happens) never ran, so the still-fresh entry
+		// from the successful seed above is untouched. Reading through the
+		// public API confirms it — a fresh-TTL hit, no fetcher involved at all,
+		// still returns the last good snapshot rather than throwing or going
+		// empty.
+		const stillCached = await fetchTeamGitHubSnapshot("repo-e");
+		expect(stillCached).toEqual(seeded);
+	});
+});

packages/server-core/src/github-team.ts

@@ -564,8 +564,13 @@ const GHTeamPRSchema = z.object({
  *
  * NEVER throws: `gh` missing, not a repo, not authenticated, or any parse error
  * all degrade to `{ issues: [], prs: [] }`.
+ *
+ * This is the transport-side boundary (issue #67 owns rewriting what happens
+ * inside it). `fetchTeamGitHubSnapshot` below is the cached, public entry
+ * point — keep the cache wrapped around this function rather than folded
+ * into it, so the two changes compose.
  */
-export async function fetchTeamGitHubSnapshot(
+async function fetchTeamGitHubSnapshotUncached(
 	repoPath: string,
 ): Promise<TeamGitHubSnapshot> {
 	try {
@@ -579,6 +584,92 @@ export async function fetchTeamGitHubSnapshot(
 	}
 }
 
+// ---------------------------------------------------------------------------
+// Per-repoPath cache for fetchTeamGitHubSnapshot (issue #66), stale-while-
+// revalidate with single-flight refresh dedupe. Same shape as the
+// fetchGitHubPRStatus cache above, same TTL (af80ddc): activity(15s) and
+// board(30s) dashboard polls both call this per cycle, so without a shared
+// cache every cycle spawns up to 4 `gh` processes costing 10-35s each on
+// slow-DNS networks.
+const snapshotCache = new Map<
+	string,
+	{ data: TeamGitHubSnapshot; timestamp: number }
+>();
+const snapshotRefreshes = new Map<string, Promise<TeamGitHubSnapshot>>();
+const SNAPSHOT_CACHE_TTL_MS = 150_000;
+
+/**
+ * Kicks (or reuses) a single in-flight refresh for `key`. On success the
+ * cache entry is updated; on failure the cache is left untouched, so a
+ * stale read served alongside a failed refresh just keeps being stale until
+ * a later refresh succeeds. Exported (not just internal) so the caching
+ * behavior itself — dedupe, stale-serving, refresh failure — can be unit
+ * tested with an injected fetcher, without needing to mock `gh`.
+ */
+export function refreshTeamGitHubSnapshotCache(
+	key: string,
+	fetcher: (key: string) => Promise<TeamGitHubSnapshot> = fetchTeamGitHubSnapshotUncached,
+): Promise<TeamGitHubSnapshot> {
+	const inflight = snapshotRefreshes.get(key);
+	if (inflight) {
+		return inflight;
+	}
+	const promise = fetcher(key)
+		.then((data) => {
+			snapshotCache.set(key, { data, timestamp: Date.now() });
+			return data;
+		})
+		.finally(() => {
+			snapshotRefreshes.delete(key);
+		});
+	snapshotRefreshes.set(key, promise);
+	return promise;
+}
+
+/** Test-only: clears the module-level snapshot cache between test cases. */
+export function clearTeamGitHubSnapshotCache(): void {
+	snapshotCache.clear();
+	snapshotRefreshes.clear();
+}
+
+/**
+ * Test-only: seeds the cache with `data` at a given age, so tests can put an
+ * entry past `SNAPSHOT_CACHE_TTL_MS` without waiting 150 real seconds.
+ */
+export function seedTeamGitHubSnapshotCacheForTest(
+	key: string,
+	data: TeamGitHubSnapshot,
+	ageMs: number,
+): void {
+	snapshotCache.set(key, { data, timestamp: Date.now() - ageMs });
+}
+
+/**
+ * Cached, stale-while-revalidate entry point for the team dashboard snapshot.
+ * First-ever call (no cache) awaits the live fetch. A hit within the TTL
+ * returns the cached snapshot without spawning anything. An expired entry is
+ * returned immediately while a single background refresh is kicked off
+ * (concurrent callers share it); a failed refresh just leaves the stale
+ * entry in place for the next attempt.
+ */
+export async function fetchTeamGitHubSnapshot(
+	repoPath: string,
+): Promise<TeamGitHubSnapshot> {
+	const cached = snapshotCache.get(repoPath);
+	if (!cached) {
+		return refreshTeamGitHubSnapshotCache(repoPath);
+	}
+	if (Date.now() - cached.timestamp < SNAPSHOT_CACHE_TTL_MS) {
+		return cached.data;
+	}
+	// Stale: serve immediately, refresh in the background (single-flight).
+	// Nobody awaits this, so a rejection here is intentionally swallowed —
+	// the stale value returned above keeps being served until a refresh
+	// succeeds.
+	refreshTeamGitHubSnapshotCache(repoPath).catch(() => {});
+	return cached.data;
+}
+
 async function fetchTeamIssues(
 	repoPath: string,
 ): Promise<TeamGitHubSnapshot["issues"]> {