diff --git a/apps/server/src/pullRequest/PullRequestService.test.ts b/apps/server/src/pullRequest/PullRequestService.test.ts index eb2f913c9206..1b51918901e4 100644 --- a/apps/server/src/pullRequest/PullRequestService.test.ts +++ b/apps/server/src/pullRequest/PullRequestService.test.ts @@ -3351,6 +3351,193 @@ it.effect("answers a known pull request immediately while the host refreshes", ( }), ); +it.effect("an invalidated detail read sees an external change a plain re-read holds back", () => + Effect.gen(function* () { + const gate = yield* Deferred.make(); + let calls = 0; + let hostTitle = "old title"; + let hostChecks: ReadonlyArray<{ readonly name: string; readonly status: "success" }> = []; + const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + getChangeRequest: () => + Effect.gen(function* () { + calls += 1; + // The plain re-read's background refresh stalls here, so the hold stays old + // while the invalidated poll read runs past it. + if (calls === 2) yield* Deferred.await(gate); + return { + ...hostedChangeRequest("polled body", 4), + title: hostTitle, + checks: hostChecks.map((check) => ({ + name: check.name, + status: check.status, + description: null, + url: null, + })), + }; + }), + }), + ], + }); + + const first = yield* service.detail(reference); + assert.strictEqual(first.title, "old title"); + assert.deepStrictEqual(first.checks, []); + + hostTitle = "new title"; + hostChecks = [{ name: "ci", status: "success" }]; + yield* TestClock.adjust("16 seconds"); + + // The old poll path: a plain re-read answers from the hold while refreshing behind it. + const second = yield* service.detail(reference); + assert.strictEqual(second.title, "old title"); + assert.deepStrictEqual(second.checks, []); + yield* Effect.yieldNow; + assert.strictEqual(calls, 2); + + // The poll path the panel now takes: invalidate first so the re-read misses the hold, + // even with that background refresh still in flight. + yield* service.invalidate({ reference }); + const polled = yield* service.detail(reference); + assert.strictEqual(polled.title, "new title"); + assert.deepStrictEqual( + polled.checks.map((check) => check.name), + ["ci"], + ); + + yield* Deferred.succeed(gate, undefined); + yield* Effect.yieldNow; + }), +); + +it.effect("a detail-scoped invalidate refreshes detail without stranding the held diff", () => + Effect.gen(function* () { + const diffGate = yield* Deferred.make(); + let detailCalls = 0; + let diffCalls = 0; + let hostTitle = "old title"; + let hostPatch = "old patch"; + const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + getChangeRequest: () => + Effect.sync(() => { + detailCalls += 1; + return { ...hostedChangeRequest("polled body", 4), title: hostTitle }; + }), + getDiff: () => + Effect.gen(function* () { + diffCalls += 1; + // The stale-while-revalidate background refresh stalls here, so a held diff + // must answer from its snapshot rather than wait on the host. + if (diffCalls === 2) yield* Deferred.await(diffGate); + return { patch: hostPatch, truncated: false, nextCursor: null }; + }), + }), + ], + }); + + const firstDetail = yield* service.detail(reference); + assert.strictEqual(firstDetail.title, "old title"); + const firstDiff = yield* service.diff(reference); + assert.strictEqual(firstDiff.patch, "old patch"); + assert.strictEqual(diffCalls, 1); + + hostTitle = "new title"; + hostPatch = "new patch"; + // Past the diff cache TTL but inside the stale-while-revalidate window, so a held diff + // answers from its snapshot while refreshing behind it. + yield* TestClock.adjust("61 seconds"); + + // The poll path: detail misses the hold while the diff key — and its hold — is untouched. + yield* service.invalidate({ reference, scope: "detail" }); + const polledDetail = yield* service.detail(reference); + assert.strictEqual(polledDetail.title, "new title"); + assert.strictEqual(detailCalls, 2); + + const polledDiff = yield* service.diff(reference); + assert.strictEqual(polledDiff.patch, "old patch"); + yield* Effect.yieldNow; + assert.strictEqual(diffCalls, 2); + + yield* Deferred.succeed(diffGate, undefined); + yield* Effect.yieldNow; + }), +); + +it.effect("a changed revision invalidates every held diff page before reloading", () => + Effect.gen(function* () { + const reference = { projectId: "p1" as ProjectId, repository: "acme/web", number: 1 }; + const calls: Array = []; + let revision = "old"; + let failDetail = false; + const service = yield* makeService({ + projects: [project({ id: "p1", title: "web", workspaceRoot: "/a", repository: "acme/web" })], + providers: [ + fakeProvider("github", { + getChangeRequest: () => + failDetail + ? Effect.fail( + new PullRequestProviderError({ + provider: "github", + operation: "getChangeRequest", + reason: "failed", + detail: "HTTP 503", + }), + ) + : Effect.succeed(hostedChangeRequest("body", 4)), + getDiff: (input) => + Effect.sync(() => { + calls.push(input.cursor); + return { + patch: `${revision}:${input.cursor ?? "first"}`, + truncated: false, + nextCursor: input.cursor ? null : "page-2", + }; + }), + }), + ], + }); + yield* service.detail(reference); + assert.strictEqual((yield* service.diff(reference)).patch, "old:first"); + assert.strictEqual( + (yield* service.diff({ ...reference, cursor: "page-2" })).patch, + "old:page-2", + ); + + revision = "new"; + yield* service.invalidate({ reference, scope: "detail" }); + assert.strictEqual((yield* service.diff(reference)).patch, "old:first"); + assert.strictEqual( + (yield* service.diff({ ...reference, cursor: "page-2" })).patch, + "old:page-2", + ); + assert.deepStrictEqual(calls, [undefined, "page-2"]); + + failDetail = true; + yield* Effect.flip(service.detail(reference)); + assert.strictEqual((yield* service.diff(reference)).patch, "old:first"); + assert.strictEqual( + (yield* service.diff({ ...reference, cursor: "page-2" })).patch, + "old:page-2", + ); + assert.deepStrictEqual(calls, [undefined, "page-2"]); + + yield* service.invalidate({ reference }); + assert.strictEqual((yield* service.diff(reference)).patch, "new:first"); + assert.strictEqual( + (yield* service.diff({ ...reference, cursor: "page-2" })).patch, + "new:page-2", + ); + assert.deepStrictEqual(calls, [undefined, "page-2", undefined, "page-2"]); + }), +); + it.effect("does not ask the host again for a linked summary it already holds", () => Effect.gen(function* () { let calls = 0; diff --git a/apps/server/src/pullRequest/PullRequestService.ts b/apps/server/src/pullRequest/PullRequestService.ts index 2229a4f652c0..0581ed11a0b5 100644 --- a/apps/server/src/pullRequest/PullRequestService.ts +++ b/apps/server/src/pullRequest/PullRequestService.ts @@ -2128,14 +2128,20 @@ export const make = Effect.gen(function* () { // epoch strands every entry made under the old one — no enumerating a cache whose keys // (cursors, commits) nothing holds a list of. The counter is shared and monotonic so a // scope re-entering `refEpochs` after eviction can never mint a key an old entry still has. + // Detail and diff ride separate epochs: a poll only needs title/check freshness, so it + // strands the cheap detail hold while the mounted diff keeps its stale-while-revalidate + // path. Mutations and the manual refresh strand both. let epochCounter = 0; let listingsEpoch = 0; let turnRefreshEpoch = 0; const refEpochs = new Map(); + const diffEpochs = new Map(); const REF_EPOCH_CAPACITY = 2_048; const refScope = (ref: PullRequestRef) => `${ref.projectId} ${ref.repository} ${ref.number}`; const refEpoch = (ref: PullRequestRef) => Math.max(turnRefreshEpoch, refEpochs.get(refScope(ref)) ?? 0); + const diffEpoch = (ref: PullRequestRef) => + Math.max(turnRefreshEpoch, diffEpochs.get(refScope(ref)) ?? 0); const refCacheKey = (ref: PullRequestRef) => JSON.stringify([refEpoch(ref), ref.projectId, ref.repository, ref.number]); // Counts belong to a PR, not a filtered page. Background reads and filter changes reuse @@ -2153,13 +2159,18 @@ export const make = Effect.gen(function* () { if (oldest !== undefined) recentStats.delete(oldest); } }; - const bumpRefEpoch = (ref: PullRequestRef) => { + const bumpMapEpoch = (epochs: Map, ref: PullRequestRef) => { const scope = refScope(ref); - if (!refEpochs.has(scope) && refEpochs.size >= REF_EPOCH_CAPACITY) { - const oldest = refEpochs.keys().next().value; - if (oldest !== undefined) refEpochs.delete(oldest); + if (!epochs.has(scope) && epochs.size >= REF_EPOCH_CAPACITY) { + const oldest = epochs.keys().next().value; + if (oldest !== undefined) epochs.delete(oldest); } - refEpochs.set(scope, ++epochCounter); + epochs.set(scope, ++epochCounter); + }; + const bumpDetailEpoch = (ref: PullRequestRef) => bumpMapEpoch(refEpochs, ref); + const bumpRefEpoch = (ref: PullRequestRef) => { + bumpMapEpoch(refEpochs, ref); + bumpMapEpoch(diffEpochs, ref); }; /** The positional filter slot of a cache key, back as the record `listUncached` takes. */ @@ -2390,19 +2401,28 @@ export const make = Effect.gen(function* () { }, }, ); + const lastGoodDiffRevision = makeLastGoodRead(DIFF_CACHE_CAPACITY); const diff: PullRequestService["Service"]["diff"] = (input) => { + const epoch = diffEpoch(input); + const revisionKey = JSON.stringify([epoch, input.projectId, input.repository, input.number]); + const observedRevision = lastGoodSummary.peek(refCacheKey(input))?.updatedAt; + // Detail invalidation must not erase the revision identifying held diff pages while + // the fresh metadata is pending or fails. Full invalidation changes this key's epoch. + const revision = observedRevision ?? lastGoodDiffRevision.peek(revisionKey) ?? null; + const rememberRevision = + input.commit === undefined && observedRevision !== undefined + ? lastGoodDiffRevision.record(revisionKey, observedRevision) + : Effect.void; const key = JSON.stringify([ - refEpoch(input), + epoch, input.projectId, input.repository, input.number, input.cursor ?? null, input.commit ?? null, - input.commit === undefined - ? (lastGoodSummary.peek(refCacheKey(input))?.updatedAt ?? null) - : null, + input.commit === undefined ? revision : null, ]); - return staleDiff(key, Cache.get(diffCache, key)); + return rememberRevision.pipe(Effect.andThen(staleDiff(key, Cache.get(diffCache, key)))); }; const listStatsCache = yield* Cache.makeWith( @@ -2465,7 +2485,9 @@ export const make = Effect.gen(function* () { const invalidate: PullRequestService["Service"]["invalidate"] = (input) => { const reference = input.reference; if (reference !== undefined) { - return Effect.sync(() => bumpRefEpoch(reference)); + return Effect.sync(() => + input.scope === "detail" ? bumpDetailEpoch(reference) : bumpRefEpoch(reference), + ); } return Effect.sync(() => { listingsEpoch = ++epochCounter; diff --git a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx index f8514f83dcdf..ef347122b4ca 100644 --- a/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx +++ b/apps/web/src/components/pullRequest/PullRequestDetailPanel.tsx @@ -698,32 +698,51 @@ export function PullRequestDetailPanel({ }, [activityQuery.refresh, detailQuery.refresh]); const [refreshToken, setRefreshToken] = useState(0); const codeRefreshToken = refreshToken + (turnRefresh ?? 0); + const invalidate = useAtomCommand(pullRequestEnvironment.invalidate, { reportFailure: false }); const activityRevision = useRef<{ readonly key: string; readonly updatedAt: string } | null>( null, ); useEffect(() => { - if (!coreDetail) return; + if (!coreDetail) { + activityRevision.current = null; + return; + } const next = { key: tabScopeKey, updatedAt: coreDetail.updatedAt }; - if (shouldRefreshPullRequestActivity(activityRevision.current, next)) { + if ( + activityRevision.current?.key === next.key && + activityRevision.current.updatedAt === next.updatedAt + ) + return; + const previous = activityRevision.current; + const changed = shouldRefreshPullRequestActivity(previous, next); + activityRevision.current = next; + if (!changed) return; + // A changed revision must miss the held diff before the Code tab reads its first page. + // Later revisions or another PR supersede this refresh while invalidation is in flight. + void invalidate({ environmentId, input: { reference } }).then((result) => { + if (activityRevision.current !== next) return; + if (result._tag === "Failure") { + activityRevision.current = previous; + toastManager.add({ + type: "error", + title: "The pull request could not be refreshed", + description: readableFailure(squashAtomCommandFailure(result), "Try refreshing again."), + }); + return; + } activityQuery.refresh(); setRefreshToken((token) => token + 1); - } - activityRevision.current = next; - }, [activityQuery.refresh, coreDetail, tabScopeKey]); - // Reuse activity and diff until core detail reports a changed revision. Keyed by - // the pull request rather than by the panel, because this one panel shows a different pull - // request every time it is opened. - useLiveRefresh( - () => { - detailQuery.refresh(); - }, - { key: `pull-request:${environmentId}:${pullRequestKey}` }, - ); - // The button, on the other hand, goes around the server's cache rather than through it: it is - // the answer for a reader who can see that what they are looking at is behind. The - // invalidation goes first so the re-reads miss that cache; if it fails, the reads still run - // and at worst answer from it. - const invalidate = useAtomCommand(pullRequestEnvironment.invalidate, { reportFailure: false }); + }); + }, [activityQuery.refresh, coreDetail, environmentId, invalidate, reference, tabScopeKey]); + // Poll fresh metadata without invalidating cached diff pages. A changed detail revision + // refreshes activity and the Code tab above; unchanged polls preserve loaded slices. + const refreshDetailFromHost = useCallback(async () => { + await invalidate({ environmentId, input: { reference, scope: "detail" } }); + detailQuery.refresh(); + }, [detailQuery.refresh, environmentId, invalidate, reference]); + useLiveRefresh(() => void refreshDetailFromHost(), { + key: `pull-request:${environmentId}:${pullRequestKey}`, + }); const [isInvalidating, setIsInvalidating] = useState(false); const refreshFromHost = useCallback(async () => { setIsInvalidating(true); diff --git a/packages/contracts/src/pullRequest.ts b/packages/contracts/src/pullRequest.ts index 812489a5fb9f..730671c29768 100644 --- a/packages/contracts/src/pullRequest.ts +++ b/packages/contracts/src/pullRequest.ts @@ -687,9 +687,12 @@ export type PullRequestListStatsResult = typeof PullRequestListStatsResult.Type; * forgets that one change request's detail and diff; without one it forgets the listings. * A separate request rather than a flag on the reads, so an explicit "refresh" one person * presses is the only thing that spends host requests — every ordinary read shares. + * The detail scope forgets only the detail (and its summary/activity siblings) while leaving + * the diff's stale-while-revalidate hold alone, for polls that only need title/check freshness. */ export const PullRequestInvalidateInput = Schema.Struct({ reference: Schema.optional(PullRequestRef), + scope: Schema.optional(Schema.Literals(["detail", "all"])), }); export type PullRequestInvalidateInput = typeof PullRequestInvalidateInput.Type;