From fae7c133bfa7b3bc99203d7edb680b5fd2b16c0e Mon Sep 17 00:00:00 2001 From: "user.email" Date: Wed, 6 May 2026 06:55:51 +0200 Subject: [PATCH] patch cache authoritatively after merge/close/toggle-draft MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub's search/list APIs are eventually consistent — for several seconds after a merge or draft-toggle they may still return the old state. The mutation route's fire-and-forget resync was picking up that stale data and overwriting the client's optimistic update on the next 10s poll, so merged PRs reappeared. Now the route patches the cache directly with the known outcome and records a mutation marker (60s TTL). Subsequent resyncs filter their GitHub response through the marker, so a stale fetch can't resurrect the PR. The marker expires once GitHub has caught up. --- packages/server/src/cache.ts | 4 ++ packages/server/src/routes.test.ts | 100 +++++++++++++++++++++++++++++ packages/server/src/routes.ts | 49 ++++++++++++-- packages/server/src/sync.ts | 87 ++++++++++++++++++++++++- 4 files changed, 234 insertions(+), 6 deletions(-) diff --git a/packages/server/src/cache.ts b/packages/server/src/cache.ts index dd140ee..f29644a 100644 --- a/packages/server/src/cache.ts +++ b/packages/server/src/cache.ts @@ -55,6 +55,10 @@ export function setCached(key: string, data: unknown) { persistCache(); } +export function patchCache(key: string, fn: (data: T | null) => T): void { + setCached(key, fn(getCached(key))); +} + export function cacheAge(key: string): number | null { const entry = store.get(key); if (!entry) return null; diff --git a/packages/server/src/routes.test.ts b/packages/server/src/routes.test.ts index 87c30df..921e8b6 100644 --- a/packages/server/src/routes.test.ts +++ b/packages/server/src/routes.test.ts @@ -54,6 +54,9 @@ vi.mock("./cache.js", () => ({ setCached: (key: string, data: unknown) => { cacheStore.set(key, data); }, + patchCache: (key: string, fn: (data: T | null) => T) => { + cacheStore.set(key, fn((cacheStore.get(key) ?? null) as T | null)); + }, })); vi.mock("./config.js", () => configStub); @@ -372,6 +375,41 @@ describe("POST /:instanceId/prs/:owner/:repo/:prNumber/merge", () => { "Repository rule violations found — A conversation must be resolved before this pull request can be merged.", }); }); + + it("removes the merged PR from cached prs/reviews even when GitHub still reports it as open", async () => { + cacheStore.set("github:prs", [ + { id: 1, repo: "o/r", number: 5, title: "x" }, + { id: 2, repo: "o/r", number: 9, title: "other" }, + ]); + cacheStore.set("github:reviews", [ + { id: 11, repo: "o/r", number: 5, title: "x" }, + { id: 12, repo: "o/r", number: 7, title: "other" }, + ]); + cacheStore.set("github:recent-prs", []); + mockOctokit.pulls.merge.mockResolvedValue({}); + + // Simulate GitHub eventual consistency: search index still returns the PR. + fetchersStub.fetchPrs.mockResolvedValue([ + { id: 1, repo: "o/r", number: 5, title: "x" }, + { id: 2, repo: "o/r", number: 9, title: "other" }, + ]); + fetchersStub.fetchReviews.mockResolvedValue([ + { id: 11, repo: "o/r", number: 5, title: "x" }, + { id: 12, repo: "o/r", number: 7, title: "other" }, + ]); + fetchersStub.fetchRecentPrs.mockResolvedValue([]); + + const res = await call("/github/prs/o/r/5/merge", { method: "POST" }); + expect(res.status).toBe(200); + await waitForPendingResyncs(); + + const prs = cacheStore.get("github:prs") as Array<{ number: number }>; + const reviews = cacheStore.get("github:reviews") as Array<{ + number: number; + }>; + expect(prs.map((p) => p.number)).toEqual([9]); + expect(reviews.map((r) => r.number)).toEqual([7]); + }); }); describe("POST /:instanceId/prs/:owner/:repo/:prNumber/close", () => { @@ -395,6 +433,38 @@ describe("POST /:instanceId/prs/:owner/:repo/:prNumber/close", () => { expect(fetchersStub.fetchReviews).toHaveBeenCalledWith("github"); expect(fetchersStub.fetchRecentPrs).toHaveBeenCalledWith("github"); }); + + it("removes the closed PR from cached prs/reviews even when GitHub still reports it as open", async () => { + cacheStore.set("github:prs", [ + { id: 1, repo: "o/r", number: 5, title: "x" }, + { id: 2, repo: "o/r", number: 9, title: "other" }, + ]); + cacheStore.set("github:reviews", [ + { id: 11, repo: "o/r", number: 5, title: "x" }, + ]); + cacheStore.set("github:recent-prs", []); + mockOctokit.pulls.update.mockResolvedValue({}); + + fetchersStub.fetchPrs.mockResolvedValue([ + { id: 1, repo: "o/r", number: 5, title: "x" }, + { id: 2, repo: "o/r", number: 9, title: "other" }, + ]); + fetchersStub.fetchReviews.mockResolvedValue([ + { id: 11, repo: "o/r", number: 5, title: "x" }, + ]); + fetchersStub.fetchRecentPrs.mockResolvedValue([]); + + const res = await call("/github/prs/o/r/5/close", { method: "POST" }); + expect(res.status).toBe(200); + await waitForPendingResyncs(); + + const prs = cacheStore.get("github:prs") as Array<{ number: number }>; + const reviews = cacheStore.get("github:reviews") as Array<{ + number: number; + }>; + expect(prs.map((p) => p.number)).toEqual([9]); + expect(reviews.map((r) => r.number)).toEqual([]); + }); }); describe("PATCH /:instanceId/prs/:owner/:repo/:prNumber", () => { @@ -466,6 +536,36 @@ describe("POST /:instanceId/prs/:owner/:repo/:prNumber/toggle-draft", () => { { id: "PR_1" }, ); }); + + it("flips draft on the cached PR even when GitHub still reports the old draft state", async () => { + cacheStore.set("github:prs", [ + { id: 1, repo: "o/r", number: 5, title: "x", draft: true }, + { id: 2, repo: "o/r", number: 9, title: "other", draft: false }, + ]); + mockOctokit.pulls.get.mockResolvedValue({ + data: { draft: true, node_id: "PR_1" }, + }); + mockOctokit.graphql.mockResolvedValue({}); + + // GitHub eventual consistency: still says draft=true. + fetchersStub.fetchPrs.mockResolvedValue([ + { id: 1, repo: "o/r", number: 5, title: "x", draft: true }, + { id: 2, repo: "o/r", number: 9, title: "other", draft: false }, + ]); + + const res = await call("/github/prs/o/r/5/toggle-draft", { + method: "POST", + }); + expect(res.status).toBe(200); + await waitForPendingResyncs(); + + const prs = cacheStore.get("github:prs") as Array<{ + number: number; + draft: boolean; + }>; + expect(prs.find((p) => p.number === 5)?.draft).toBe(false); + expect(prs.find((p) => p.number === 9)?.draft).toBe(false); + }); }); describe("POST /:instanceId/prs/:owner/:repo/:prNumber/rerun-ci", () => { diff --git a/packages/server/src/routes.ts b/packages/server/src/routes.ts index f38234f..701a99a 100644 --- a/packages/server/src/routes.ts +++ b/packages/server/src/routes.ts @@ -1,6 +1,6 @@ import { Hono } from "hono"; import { Octokit } from "@octokit/rest"; -import { getCached, setCached } from "./cache.js"; +import { getCached, patchCache, setCached } from "./cache.js"; import { type ConfigSchema, configExists, @@ -17,7 +17,27 @@ import { latestCheckRunsByName, } from "./fetchers.js"; import { clearClients, getClient, getInstance } from "./github-client.js"; -import { scheduleResync } from "./sync.js"; +import { recordMutation, scheduleResync } from "./sync.js"; + +interface CachedListItem { + repo: string; + number: number; + draft?: boolean; +} + +function removeFromList(repo: string, number: number) { + return (data: CachedListItem[] | null): CachedListItem[] => + (data ?? []).filter( + (item) => !(item.repo === repo && item.number === number), + ); +} + +function setDraftInList(repo: string, number: number, draft: boolean) { + return (data: CachedListItem[] | null): CachedListItem[] => + (data ?? []).map((item) => + item.repo === repo && item.number === number ? { ...item, draft } : item, + ); +} const api = new Hono(); @@ -279,13 +299,15 @@ api.post("/:instanceId/prs/:owner/:repo/:prNumber/auto-merge", async (c) => { // Merge a PR directly (squash) api.post("/:instanceId/prs/:owner/:repo/:prNumber/merge", async (c) => { const { instanceId, owner, repo, prNumber } = c.req.param(); + const num = Number(prNumber); + const fullRepo = `${owner}/${repo}`; const client = await getClient(instanceId); try { await client.pulls.merge({ owner, repo, - pull_number: Number(prNumber), + pull_number: num, merge_method: "squash", }); } catch (err) { @@ -305,6 +327,9 @@ api.post("/:instanceId/prs/:owner/:repo/:prNumber/merge", async (c) => { return c.json({ error: "merge_rejected", message }, 422); } + patchCache(`${instanceId}:prs`, removeFromList(fullRepo, num)); + patchCache(`${instanceId}:reviews`, removeFromList(fullRepo, num)); + recordMutation(instanceId, { kind: "removed", repo: fullRepo, number: num }); scheduleResync(instanceId, ["prs", "reviews", "recent-prs"]); return c.json({ ok: true }); @@ -313,15 +338,20 @@ api.post("/:instanceId/prs/:owner/:repo/:prNumber/merge", async (c) => { // Close a PR api.post("/:instanceId/prs/:owner/:repo/:prNumber/close", async (c) => { const { instanceId, owner, repo, prNumber } = c.req.param(); + const num = Number(prNumber); + const fullRepo = `${owner}/${repo}`; const client = await getClient(instanceId); await client.pulls.update({ owner, repo, - pull_number: Number(prNumber), + pull_number: num, state: "closed", }); + patchCache(`${instanceId}:prs`, removeFromList(fullRepo, num)); + patchCache(`${instanceId}:reviews`, removeFromList(fullRepo, num)); + recordMutation(instanceId, { kind: "removed", repo: fullRepo, number: num }); scheduleResync(instanceId, ["prs", "reviews", "recent-prs"]); return c.json({ ok: true }); @@ -355,6 +385,7 @@ api.post("/:instanceId/prs/:owner/:repo/:prNumber/toggle-draft", async (c) => { const { instanceId, owner, repo, prNumber } = c.req.param(); const client = await getClient(instanceId); const num = Number(prNumber); + const fullRepo = `${owner}/${repo}`; const { data: pr } = await client.pulls.get({ owner, @@ -369,9 +400,17 @@ api.post("/:instanceId/prs/:owner/:repo/:prNumber/toggle-draft", async (c) => { await client.graphql(mutation, { id: pr.node_id }); + const newDraft = !pr.draft; + patchCache(`${instanceId}:prs`, setDraftInList(fullRepo, num, newDraft)); + recordMutation(instanceId, { + kind: "draft", + repo: fullRepo, + number: num, + draft: newDraft, + }); scheduleResync(instanceId, ["prs"]); - return c.json({ ok: true, draft: !pr.draft }); + return c.json({ ok: true, draft: newDraft }); }); // Rerun CI for a PR diff --git a/packages/server/src/sync.ts b/packages/server/src/sync.ts index 1d2b41f..349b76d 100644 --- a/packages/server/src/sync.ts +++ b/packages/server/src/sync.ts @@ -25,6 +25,88 @@ const ALL_KEYS: ResyncKey[] = ["prs", "recent-prs", "reviews", "notifications"]; const pending = new Set>(); +// Tracks recent client-driven mutations so a stale resync (GitHub's search +// index lags by seconds after a merge/close/draft toggle) doesn't re-introduce +// the old state. Entries expire after MUTATION_TTL. +type MutationRecord = + | { kind: "removed"; repo: string; number: number; expiresAt: number } + | { + kind: "draft"; + repo: string; + number: number; + draft: boolean; + expiresAt: number; + }; + +const MUTATION_TTL = 60_000; +const mutations = new Map(); + +function mutationKey(instanceId: string, repo: string, number: number) { + return `${instanceId}:${repo}:${number}`; +} + +export function recordMutation( + instanceId: string, + m: + | { kind: "removed"; repo: string; number: number } + | { kind: "draft"; repo: string; number: number; draft: boolean }, +): void { + mutations.set(mutationKey(instanceId, m.repo, m.number), { + ...m, + expiresAt: Date.now() + MUTATION_TTL, + }); +} + +function activeMutations(instanceId: string): MutationRecord[] { + const now = Date.now(); + const out: MutationRecord[] = []; + for (const [k, v] of mutations) { + if (v.expiresAt <= now) { + mutations.delete(k); + continue; + } + if (k.startsWith(`${instanceId}:`)) out.push(v); + } + return out; +} + +interface ListItem { + repo: string; + number: number; + draft?: boolean; +} + +function applyMutations( + instanceId: string, + key: ResyncKey, + data: unknown, +): unknown { + if (key !== "prs" && key !== "reviews") return data; + const muts = activeMutations(instanceId); + if (muts.length === 0) return data; + const items = data as ListItem[]; + + const filtered = items.filter( + (item) => + !muts.some( + (m) => + m.kind === "removed" && + m.repo === item.repo && + m.number === item.number, + ), + ); + + if (key === "reviews") return filtered; + + return filtered.map((item) => { + const draftMut = muts.find( + (m): m is Extract => + m.kind === "draft" && m.repo === item.repo && m.number === item.number, + ); + return draftMut ? { ...item, draft: draftMut.draft } : item; + }); +} + export async function resyncInstance( instanceId: string, keys: ResyncKey[] = ALL_KEYS, @@ -33,7 +115,10 @@ export async function resyncInstance( keys.map(async (key) => { try { const data = await RESYNC_FETCHERS[key](instanceId); - setCached(`${instanceId}:${key}`, data); + setCached( + `${instanceId}:${key}`, + applyMutations(instanceId, key, data), + ); } catch (err) { console.error( `Sync failed for ${instanceId}:${key}:`,