History

fix(server): return fresh live pull request reads (#6472)

Adamulek123 committed 2 months ago

Commit 229b05d1 parent ba301772 files · +51 −51Browse files

Unified · Split

2 files
  1. apps/server/src/pullRequest/PullRequestService.test.ts +37 −0
  2. apps/server/src/pullRequest/PullRequestService.ts +14 −51
apps/server/src/pullRequest/PullRequestService.test.ts+37−0
@@ -1,6 +1,7 @@
11 import { assert, it } from "@effect/vitest";
22 import * as Effect from "effect/Effect";
33 import * as Layer from "effect/Layer";
4+import * as TestClock from "effect/testing/TestClock";
45 import type {
56 OrchestrationProjectShell,
67 ProjectId,
@@ -2292,6 +2293,42 @@ it.effect("answers a repeated listing from cache, and concurrent readers share o · 2285 unmodified lines
22922293 }),
22932294 );
22942295
2296+it.effect("returns the refreshed listing on the first read after its cache expires", () =>
2297+ Effect.gen(function* () {
2298+ let hostCalls = 0;
2299+ const service = yield* makeService({
2300+ projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })],
2301+ providers: [
2302+ fakeProvider("github", {
2303+ listChangeRequests: () => {
2304+ hostCalls += 1;
2305+ return Effect.succeed({
2306+ items: [changeRequest(hostCalls, "2026-07-02T00:00:00Z")],
2307+ truncated: false,
2308+ continues: false,
2309+ });
2310+ },
2311+ }),
2312+ ],
2313+ });
2314+
2315+ const first = yield* service.list({ state: "open" });
2316+ assert.deepStrictEqual(
2317+ first.entries.map((entry) => entry.number),
2318+ [1],
2319+ );
2320+
2321+ yield* TestClock.adjust("31 seconds");
2322+ const refreshed = yield* service.list({ state: "open" });
2323+
2324+ assert.strictEqual(hostCalls, 2);
2325+ assert.deepStrictEqual(
2326+ refreshed.entries.map((entry) => entry.number),
2327+ [2],
2328+ );
2329+ }),
2330+);
2331+
22952332 it.effect("a listing narrowed to some projects is its own cache entry", () =>
22962333 Effect.gen(function* () {
22972334 const asked: ReadonlyArray<string>[] = [];
apps/server/src/pullRequest/PullRequestService.ts+14−51
@@ -102,16 +102,7 @@ const DIFF_CACHE_TTL = Duration.seconds(60); · 101 unmodified lines
102102 const COMMIT_DIFF_CACHE_TTL = Duration.minutes(10);
103103 /** Sized like the client's own stale time; a row's counts move only when somebody pushes. */
104104 const LIST_STATS_CACHE_TTL = Duration.seconds(60);
105−/**
106− * How long a cache's last success may still be served while a fresh read runs behind it.
107− * Bounded by how the page actually revalidates: clients re-read on mount and once a minute
108− * while open, and every one of those reads repopulates the cache in the background — so in
109− * steady use a "stale" answer is at most a refresh cycle old, and the window only stretches
110− * that far when nobody has looked at the page for minutes. An explicit refresh or a mutation
111− * bumps the epochs and skips held answers entirely.
112− */
113−const LIST_STALE_WINDOW = Duration.minutes(10);
114−const DETAIL_STALE_WINDOW = Duration.minutes(5);
105+/** A diff can stay interactive while its next cached value is fetched off the critical path. */
115106 const DIFF_STALE_WINDOW = Duration.minutes(10);
116107 /** How long one host's signed-in login is believed without asking its CLI again. */
117108 const VIEWER_CACHE_TTL = Duration.minutes(10);
@@ -1826,30 +1817,22 @@ export const make = Effect.gen(function* () { · 1708 unmodified lines
18261817 const runFork = Effect.runForkWith(context);
18271818
18281819 /**
1829− * Stale answers served while a fresh one is fetched behind them. Every read here leaves the
1830− * process for a CLI whose wall clock is the host's — seconds on a good day, tens of them on a
1831− * slow network — and the short cache windows below mean almost every page visit pays that
1832− * clock again. The last success per key is therefore held a while longer: a read inside the
1833− * window answers with it at once and refreshes the cache in the background, so the next read
1834− * is fresh without anyone having waited on it.
1835− *
1836− * Correctness leans on the epochs: an explicit refresh or a mutation bumps them, the epoch is
1837− * part of every key, and a held answer under the old key is simply never asked for again — so
1838− * "give me truly fresh" still means exactly that.
1820+ * The diff is not live-polled and is expensive enough to keep its stale-while-revalidate path.
1821+ * Explicit refreshes and mutations still strand held values through the reference epoch.
18391822 */
1840− const staleWhileRevalidate = <A>(staleFor: Duration.Duration, capacity: number) => {
1841− const staleMs = Duration.toMillis(staleFor);
1842− const held = new Map<string, { readonly at: number; readonly value: A }>();
1843− const record = (key: string, value: A) =>
1823+ const staleDiff = (() => {
1824+ const staleMs = Duration.toMillis(DIFF_STALE_WINDOW);
1825+ const held = new Map<string, { readonly at: number; readonly value: PullRequestDiffResult }>();
1826+ const record = (key: string, value: PullRequestDiffResult) =>
18441827 Effect.map(Clock.currentTimeMillis, (at) => {
18451828 held.delete(key);
1846− if (held.size >= capacity) {
1829+ if (held.size >= DIFF_CACHE_CAPACITY) {
18471830 const oldest = held.keys().next().value;
18481831 if (oldest !== undefined) held.delete(oldest);
18491832 }
18501833 held.set(key, { at, value });
18511834 });
1852− return <E>(key: string, read: Effect.Effect<A, E>): Effect.Effect<A, E> => {
1835+ return <E>(key: string, read: Effect.Effect<PullRequestDiffResult, E>) => {
18531836 const recorded = read.pipe(Effect.tap((value) => record(key, value)));
18541837 return Effect.flatMap(Clock.currentTimeMillis, (now) => {
18551838 const snapshot = held.get(key);
@@ -1860,7 +1843,7 @@ export const make = Effect.gen(function* () { · 4 unmodified lines
18601843 return Effect.sync(() => runFork(Effect.ignore(recorded))).pipe(Effect.as(snapshot.value));
18611844 });
18621845 };
1863− };
1846+ })();
18641847
18651848 // Epochs are the invalidation mechanism: a key carries its scope's epoch, so bumping the
18661849 // epoch strands every entry made under the old one — no enumerating a cache whose keys
@@ -1946,10 +1929,6 @@ export const make = Effect.gen(function* () { · 79 unmodified lines
19461929 timeToLive: (exit) => (Exit.isSuccess(exit) ? LIST_CACHE_TTL : Duration.zero),
19471930 },
19481931 );
1949− const staleList = staleWhileRevalidate<PullRequestListResult>(
1950− LIST_STALE_WINDOW,
1951− LIST_CACHE_CAPACITY,
1952− );
19531932 const list: PullRequestService["Service"]["list"] = (input) => {
19541933 const key = JSON.stringify([
19551934 listingsEpoch,
@@ -1976,7 +1955,7 @@ export const make = Effect.gen(function* () { · 20 unmodified lines
19761955 ? null
19771956 : Object.entries(input.cursors).toSorted(([left], [right]) => left.localeCompare(right)),
19781957 ]);
1979− return staleList(key, Cache.get(listCache, key));
1958+ return Cache.get(listCache, key);
19801959 };
19811960
19821961 const detailCache = yield* Cache.makeWith(
@@ -1989,13 +1968,9 @@ export const make = Effect.gen(function* () { · 6 unmodified lines
19891968 timeToLive: (exit) => (Exit.isSuccess(exit) ? DETAIL_CACHE_TTL : Duration.zero),
19901969 },
19911970 );
1992− const staleDetail = staleWhileRevalidate<PullRequestDetail>(
1993− DETAIL_STALE_WINDOW,
1994− DETAIL_CACHE_CAPACITY,
1995− );
19961971 const detail: PullRequestService["Service"]["detail"] = (input) => {
19971972 const key = JSON.stringify([refEpoch(input), input.projectId, input.repository, input.number]);
1998− return staleDetail(key, Cache.get(detailCache, key));
1973+ return Cache.get(detailCache, key);
19991974 };
20001975
20011976 const activityCache = yield* Cache.makeWith(
@@ -2008,13 +1983,9 @@ export const make = Effect.gen(function* () { · 6 unmodified lines
20081983 timeToLive: (exit) => (Exit.isSuccess(exit) ? DETAIL_CACHE_TTL : Duration.zero),
20091984 },
20101985 );
2011− const staleActivity = staleWhileRevalidate<PullRequestActivity>(
2012− DETAIL_STALE_WINDOW,
2013− DETAIL_CACHE_CAPACITY,
2014− );
20151986 const activity: PullRequestService["Service"]["activity"] = (input) => {
20161987 const key = JSON.stringify([refEpoch(input), input.projectId, input.repository, input.number]);
2017− return staleActivity(key, Cache.get(activityCache, key));
1988+ return Cache.get(activityCache, key);
20181989 };
20191990
20201991 const diffCache = yield* Cache.makeWith(
@@ -2044,10 +2015,6 @@ export const make = Effect.gen(function* () { · 23 unmodified lines
20442015 },
20452016 },
20462017 );
2047− const staleDiff = staleWhileRevalidate<PullRequestDiffResult>(
2048− DIFF_STALE_WINDOW,
2049− DIFF_CACHE_CAPACITY,
2050− );
20512018 const diff: PullRequestService["Service"]["diff"] = (input) => {
20522019 const key = JSON.stringify([
20532020 refEpoch(input),
@@ -2076,10 +2043,6 @@ export const make = Effect.gen(function* () { · 22 unmodified lines
20762043 // shares between clients like every other read. Refs are sorted so one page's worth of rows
20772044 // is one key however the client assembled them, and the listings epoch rides along so the
20782045 // refresh that forgets the listing forgets its decorations with it.
2079− const staleListStats = staleWhileRevalidate<PullRequestListStatsResult>(
2080− LIST_STALE_WINDOW,
2081− LIST_STATS_CACHE_CAPACITY,
2082− );
20832046 const listStats: PullRequestService["Service"]["listStats"] = (input) => {
20842047 if (input.refs.length === 0) return Effect.succeed({ stats: [] });
20852048 const key = JSON.stringify([
@@ -2090,7 +2053,7 @@ export const make = Effect.gen(function* () { · 4 unmodified lines
20902053 `${left[0]} ${left[1]} ${left[2]}`.localeCompare(`${right[0]} ${right[1]} ${right[2]}`),
20912054 ),
20922055 ]);
2093− return staleListStats(key, Cache.get(listStatsCache, key));
2056+ return Cache.get(listStatsCache, key);
20942057 };
20952058
20962059 const invalidate: PullRequestService["Service"]["invalidate"] = (input) =>

Keyboard shortcuts

General

Show keyboard shortcuts
?
Focus the current search
/

Issues and pull requests

Open filters
f

Issues, pull requests and Inbox

Select next item
j↓
Select previous item
k↑
Open selected item
Enter
Dismiss filters or clear selection
Esc

Repository code

Go to file
t