diff --git a/packages/server-core/src/github-team.test.ts b/packages/server-core/src/github-team.test.ts index 0d53bfe6..bf6daef0 100644 --- a/packages/server-core/src/github-team.test.ts +++ b/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 & Pick, @@ -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((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((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); + }); +}); diff --git a/packages/server-core/src/github-team.ts b/packages/server-core/src/github-team.ts index 6c49609b..5feaf732 100644 --- a/packages/server-core/src/github-team.ts +++ b/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 { 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>(); +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 = fetchTeamGitHubSnapshotUncached, +): Promise { + 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 { + 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 {