diff --git a/src/inngest/functions/maintainer-discover.test.ts b/src/inngest/functions/maintainer-discover.test.ts index 602ce195..43cd236d 100644 --- a/src/inngest/functions/maintainer-discover.test.ts +++ b/src/inngest/functions/maintainer-discover.test.ts @@ -12,7 +12,11 @@ vi.mock('@/lib/maintainer/discover', () => ({ decideRepoGrant: vi.fn(), reconcileGrants: vi.fn(), })); -vi.mock('@/lib/cache', () => ({ cacheGet: vi.fn(), cacheSet: vi.fn() })); +vi.mock('@/lib/cache', () => ({ + cacheGet: vi.fn(), + cacheSet: vi.fn(), + cacheDel: vi.fn(), +})); const mockSend = vi.fn(); vi.mock('../client', () => ({ diff --git a/src/inngest/functions/maintainer-discover.ts b/src/inngest/functions/maintainer-discover.ts index 0574c600..f1ceaa64 100644 --- a/src/inngest/functions/maintainer-discover.ts +++ b/src/inngest/functions/maintainer-discover.ts @@ -8,7 +8,7 @@ import { reconcileGrants, type ProposedGrant, } from '@/lib/maintainer/discover'; -import { cacheGet, cacheSet } from '@/lib/cache'; +import { cacheGet, cacheSet, cacheDel } from '@/lib/cache'; /** * Revalidates a user's installs + repos and reconciles the @@ -267,7 +267,7 @@ async function discoverForUser( } await cacheSet(`maint:discovered:${userId}`, { ranAt: Date.now() }, DEDUP_TTL_S); - await cacheSet(`maint:status:${userId}`, false, 1); + await cacheDel(`maint:status:${userId}`); return { user: userId, diff --git a/src/inngest/functions/process-installation-event.ts b/src/inngest/functions/process-installation-event.ts index 564a230b..2e3452ba 100644 --- a/src/inngest/functions/process-installation-event.ts +++ b/src/inngest/functions/process-installation-event.ts @@ -1,6 +1,7 @@ import { inngest } from '../client'; import { getServiceSupabase } from '@/lib/supabase/service'; import { getInstallOctokit } from '@/lib/github/app'; +import { cacheDel } from '@/lib/cache'; /** * GitHub App installation lifecycle: @@ -82,6 +83,10 @@ export const processInstallationEvent = inngest.createFunction( }, { onConflict: 'installation_id,user_id' }, ); + // New grant for this user — drop the cached 1h denial (if any) so + // the grant takes effect immediately instead of after the next + // discovery run. + await cacheDel(`maint:status:${profile.id}`); } // GitHub only includes `repositories` in the payload when the user diff --git a/src/lib/maintainer/detect.test.ts b/src/lib/maintainer/detect.test.ts index 977f1e3c..facf67a7 100644 --- a/src/lib/maintainer/detect.test.ts +++ b/src/lib/maintainer/detect.test.ts @@ -12,18 +12,42 @@ vi.mock('@/lib/cache', () => ({ cacheSet: vi.fn(), })); +const applyFilters = (rows: unknown, filters: Array<[string, unknown]>): unknown => { + if (!Array.isArray(rows)) return rows; + let filtered = rows as Array>; + for (const [col, val] of filters) { + if (col.startsWith('github_installations.')) { + const field = col.split('.')[1] as string; + filtered = filtered.filter((r) => { + const install = r.github_installations; + const joined = (Array.isArray(install) ? install[0] : install) as + | Record + | null + | undefined; + if (!joined) return val === null ? false : true; + return joined[field] === val; + }); + } + } + return filtered; +}; + const mockSupabase = (mockTables: Record) => { const mockClient = { from: vi.fn().mockImplementation((table: string) => { + const eqFilters: Array<[string, unknown]> = []; const chain = { select: vi.fn().mockReturnThis(), - eq: vi.fn().mockReturnThis(), + eq: vi.fn().mockImplementation((col: string, val: unknown) => { + eqFilters.push([col, val]); + return chain; + }), limit: vi.fn().mockReturnThis(), maybeSingle: vi.fn().mockImplementation(() => { - return Promise.resolve({ data: (mockTables[table] as unknown) ?? null }); + return Promise.resolve({ data: applyFilters(mockTables[table], eqFilters) ?? null }); }), then: function (resolve: (value: unknown) => void) { - resolve({ data: (mockTables[table] as unknown) ?? null }); + resolve({ data: applyFilters(mockTables[table], eqFilters) ?? null }); }, }; return chain; @@ -92,13 +116,13 @@ describe('isUserMaintainer', () => { expect(cacheSet).toHaveBeenCalledWith('maint:status:user1', false, 3600); }); - it('returns false when service client is not configured', async () => { + it('returns false when service client is not configured (without caching the denial)', async () => { vi.mocked(cacheGet).mockResolvedValue(null); vi.mocked(getServiceSupabase).mockReturnValue(null); const result = await isUserMaintainer('user1'); expect(result).toBe(false); - expect(cacheSet).toHaveBeenCalledWith('maint:status:user1', false, 3600); + expect(cacheSet).not.toHaveBeenCalled(); }); it('returns cached result when cache is warm (should not hit DB)', async () => { diff --git a/src/lib/maintainer/detect.ts b/src/lib/maintainer/detect.ts index eaa55884..b13b16d9 100644 --- a/src/lib/maintainer/detect.ts +++ b/src/lib/maintainer/detect.ts @@ -8,7 +8,8 @@ import { unwrapJoin } from '@/lib/supabase/inner-join'; * * Cached per user for 1h. Cache is busted by: * - maintainer-discover function (after writing junction changes) - * - process-installation-event (on installation.deleted) + * - process-installation-event (on installation.created / deleted) + * - any grant path that writes junction rows */ const TTL_S = 60 * 60; @@ -19,22 +20,22 @@ export async function isUserMaintainer(userId: string): Promise { if (cached !== null) return cached; const sb = getServiceSupabase(); - if (!sb) { - await cacheSet(cacheKey, false, TTL_S); - return false; - } - + // A missing service client is a transient infra condition — do NOT cache + // the denial, otherwise a blip becomes a 1-hour maintainer lockout. + if (!sb) return false; + + // DB-side existence check: only junction rows joined to an active + // (non-uninstalled) install count. No arbitrary row cap, so a user with + // more than 20 junction rows is still classified correctly. limit(1) just + // bounds the payload — we only need to know whether any active row exists. const { data } = await sb .from('github_installation_users') .select('installation_id, github_installations!inner(uninstalled_at)') .eq('user_id', userId) - .limit(20); + .eq('github_installations.uninstalled_at', null) + .limit(1); - const has = (data ?? []).some((row) => { - const r = row as unknown as { github_installations: unknown }; - const i = unwrapJoin<{ uninstalled_at: string | null }>(r.github_installations); - return i && i.uninstalled_at === null; - }); + const has = (data ?? []).length > 0; await cacheSet(cacheKey, has, TTL_S); return has;