diff --git a/apps/server/src/git/GitManager.test.ts b/apps/server/src/git/GitManager.test.ts index 600450b4ccec0..cb586a355ed16 100644 --- a/apps/server/src/git/GitManager.test.ts +++ b/apps/server/src/git/GitManager.test.ts @@ -1054,7 +1054,10 @@ it.layer(layerGitManagerTest)("GitManager", (it) => { }), ); - it.effect("a git read in flight during a T3 git action does not outlive it", () => + it.effect.each([ + { action: "a T3 git action", options: undefined }, + { action: "a checkout", options: { keepPullRequestLookups: true } }, + ])("a git read in flight during $action does not outlive it", ({ options }) => Effect.gen(function* () { const repoDir = yield* makeTempDir("t3code-git-manager-"); yield* initRepo(repoDir); @@ -1097,7 +1100,7 @@ it.layer(layerGitManagerTest)("GitManager", (it) => { const inFlight = yield* Effect.forkChild(lookup); yield* Deferred.await(gate.reached); yield* runGit(repoDir, ["branch", "--set-upstream-to=origin/feature/raced-head"]); - yield* manager.invalidateStatus(repoDir); + yield* manager.invalidateStatus(repoDir, options); yield* Deferred.succeed(gate.release, undefined); yield* Fiber.join(inFlight); @@ -3015,6 +3018,42 @@ it.layer(layerGitManagerTest)("GitManager", (it) => { }), ); + it.effect("a checkout's invalidation keeps the branch's cached PR lookup", () => + Effect.gen(function* () { + const repoDir = yield* makeTempDir("t3code-git-manager-"); + yield* initRepo(repoDir); + yield* runGit(repoDir, ["checkout", "-b", "feature/pr-cached"]); + const remoteDir = yield* createBareRemote(); + yield* runGit(repoDir, ["remote", "add", "origin", remoteDir]); + yield* runGit(repoDir, ["push", "-u", "origin", "feature/pr-cached"]); + + const { manager } = yield* makeManager({ + providerScenario: { + prListSequence: [ + [ + { + number: 215, + title: "Cached PR", + url: "https://github.com/pingdotgg/codething-mvp/pull/215", + baseRefName: "main", + headRefName: "feature/pr-cached", + }, + ], + [], + ], + }, + }); + + expect((yield* manager.status({ cwd: repoDir })).pr?.number).toBe(215); + + yield* manager.invalidateStatus(repoDir, { keepPullRequestLookups: true }); + expect((yield* manager.status({ cwd: repoDir })).pr?.number).toBe(215); + + yield* manager.invalidateStatus(repoDir); + expect((yield* manager.status({ cwd: repoDir })).pr).toBeNull(); + }), + ); + it.effect( "status does not reuse a stale PR after the branch is retargeted to a different upstream", () => diff --git a/apps/server/src/git/GitManager.ts b/apps/server/src/git/GitManager.ts index a47a5a2eefb4a..c6e632aa91432 100644 --- a/apps/server/src/git/GitManager.ts +++ b/apps/server/src/git/GitManager.ts @@ -164,7 +164,11 @@ export class GitManager extends Context.Service< }) => Effect.Effect; readonly invalidateLocalStatus: (cwd: string) => Effect.Effect; readonly invalidateRemoteStatus: (cwd: string) => Effect.Effect; - readonly invalidateStatus: (cwd: string) => Effect.Effect; + /** keepPullRequestLookups is for changes that cannot affect a PR, such as a checkout. */ + readonly invalidateStatus: ( + cwd: string, + options?: { readonly keepPullRequestLookups?: boolean }, + ) => Effect.Effect; readonly resolvePullRequest: ( input: GitPullRequestRefInput, ) => Effect.Effect; @@ -1257,6 +1261,15 @@ export const make = Effect.gen(function* () { prLookupEpochByCwd.set(cacheKey, prLookupEpoch(cacheKey) + 1); }), ); + // Separate from the PR lookup epoch because a checkout keeps PR lookups but changes git state. + const prLookupGitStateEpochByCwd = new Map(); + const prLookupGitStateEpoch = (cwd: string) => prLookupGitStateEpochByCwd.get(cwd) ?? 0; + const bumpPrLookupGitStateEpoch = (cwd: string) => + normalizeStatusCacheKey(cwd).pipe( + Effect.map((cacheKey) => { + prLookupGitStateEpochByCwd.set(cacheKey, prLookupGitStateEpoch(cacheKey) + 1); + }), + ); const prLookupCacheKey = ( cwd: string, details: { @@ -2621,10 +2634,13 @@ export const make = Effect.gen(function* () { } // invalidateStatus bumps the epoch. A read that started before a T3 git // action, or before a newer read finished, must not replace their state. - const epoch = prLookupEpoch(cwd); + const epoch = prLookupGitStateEpoch(cwd); const value = yield* read; const latest = prLookupGitStateByKey.get(entryKey); - if (prLookupEpoch(cwd) !== epoch || (latest !== undefined && latest.readAtMs > readAtMs)) { + if ( + prLookupGitStateEpoch(cwd) !== epoch || + (latest !== undefined && latest.readAtMs > readAtMs) + ) { return value; } prLookupGitStateByKey.delete(entryKey); @@ -2964,10 +2980,11 @@ export const make = Effect.gen(function* () { yield* invalidateRemoteStatusResultCache(cwd); }); const invalidateStatus: GitManager["Service"]["invalidateStatus"] = Effect.fn("invalidateStatus")( - function* (cwd) { + function* (cwd, options) { yield* invalidateLocalStatusResultCache(cwd); yield* invalidateRemoteStatusResultCache(cwd); - yield* bumpPrLookupEpoch(cwd); + if (!options?.keepPullRequestLookups) yield* bumpPrLookupEpoch(cwd); + yield* bumpPrLookupGitStateEpoch(cwd); yield* forgetPrLookupGitState(cwd); }, ); diff --git a/apps/server/src/git/GitWorkflowService.ts b/apps/server/src/git/GitWorkflowService.ts index 287207840ad6a..b6234e6e21dc2 100644 --- a/apps/server/src/git/GitWorkflowService.ts +++ b/apps/server/src/git/GitWorkflowService.ts @@ -69,7 +69,7 @@ export class GitWorkflowService extends Context.Service< readonly invalidateLocalStatus: (cwd: string) => Effect.Effect; readonly invalidateRemoteStatus: (cwd: string) => Effect.Effect; readonly readRefSnapshot: (cwd: string) => Effect.Effect; - readonly invalidateStatus: (cwd: string) => Effect.Effect; + readonly invalidateStatus: GitManager.GitManager["Service"]["invalidateStatus"]; /** The checkout's own HEAD file, inside a linked worktree's admin directory when there is one. */ readonly headPath: (cwd: string) => Effect.Effect; readonly pullCurrentBranch: (cwd: string) => Effect.Effect; diff --git a/apps/server/src/vcs/VcsStatusBroadcaster.test.ts b/apps/server/src/vcs/VcsStatusBroadcaster.test.ts index 08fad4ae56aff..622230da42d10 100644 --- a/apps/server/src/vcs/VcsStatusBroadcaster.test.ts +++ b/apps/server/src/vcs/VcsStatusBroadcaster.test.ts @@ -26,6 +26,7 @@ import type { VcsStatusStreamEvent, } from "@t3tools/contracts"; import { GitCommandError, GitManagerError } from "@t3tools/contracts"; +import * as SourceControlRateLimit from "@t3tools/source-control-core/server/SourceControlRateLimit"; import { applyGitStatusStreamEvent } from "@t3tools/shared/git"; import * as VcsAutomaticFetchInterval from "./VcsAutomaticFetchInterval.ts"; @@ -78,6 +79,8 @@ function layerTestFor(state: { localInvalidationCalls: number; remoteInvalidationCalls: number; remoteStatusRefreshUpstreamValues?: Array; + remoteStatusInteractiveValues?: Array; + invalidateStatusKeepLookupValues?: Array; localStatusBranchChanges?: Array; refSnapshot?: string | null; remoteStatusError?: GitManagerError; @@ -163,6 +166,13 @@ function layerTestFor(state: { return state.currentRemoteStatus; }), ), + Effect.tap(() => + Effect.gen(function* () { + state.remoteStatusInteractiveValues?.push( + yield* SourceControlRateLimit.Interactive, + ); + }), + ), Effect.tap(() => state.remoteStatusError ? Effect.fail(state.remoteStatusError) : Effect.void, ), @@ -176,10 +186,11 @@ function layerTestFor(state: { Effect.sync(() => { state.remoteInvalidationCalls += 1; }), - invalidateStatus: () => + invalidateStatus: (_cwd, options) => Effect.sync(() => { state.localInvalidationCalls += 1; state.remoteInvalidationCalls += 1; + state.invalidateStatusKeepLookupValues?.push(options?.keepPullRequestLookups); }), ...(state.pullCurrentBranch ? { pullCurrentBranch: state.pullCurrentBranch } : {}), headPath: () => @@ -1124,6 +1135,118 @@ describe("VcsStatusBroadcaster", () => { }).pipe(Effect.provide(layerTestFor(state)), Effect.scoped); }); + it.effect("a checkout's PR lookup is a user action that reuses cached lookups", () => { + const state: Parameters[0] = { + currentLocalStatus: baseLocalStatus, + currentRemoteStatus: baseRemoteStatus, + localStatusCalls: 0, + remoteStatusCalls: 0, + localInvalidationCalls: 0, + remoteInvalidationCalls: 0, + remoteStatusInteractiveValues: [], + invalidateStatusKeepLookupValues: [], + }; + + return Effect.gen(function* () { + const broadcaster = yield* VcsStatusBroadcaster.VcsStatusBroadcaster; + const remotePublished = yield* Deferred.make(); + const subscribed = yield* Deferred.make(); + yield* broadcaster.streamStatus({ cwd: "/repo", includeRemote: false }).pipe( + Stream.runForEach((event) => { + if (event._tag === "snapshot") return Deferred.succeed(subscribed, undefined); + return event._tag === "remoteUpdated" && event.remote !== null + ? Deferred.succeed(remotePublished, undefined) + : Effect.void; + }), + Effect.forkScoped, + ); + yield* Deferred.await(subscribed); + + yield* broadcaster.refreshAfterCheckout("/repo"); + yield* Deferred.await(remotePublished); + + assert.deepStrictEqual(state.remoteStatusInteractiveValues, [true]); + assert.deepStrictEqual(state.invalidateStatusKeepLookupValues, [true]); + }).pipe(Effect.provide(layerTestFor(state)), Effect.scoped); + }); + + it.effect("a checkout re-reads local status only when a fetch ran", () => { + const state: Parameters[0] = { + currentLocalStatus: baseLocalStatus, + currentRemoteStatus: baseRemoteStatus, + localStatusCalls: 0, + remoteStatusCalls: 0, + localInvalidationCalls: 0, + remoteInvalidationCalls: 0, + }; + + return Effect.gen(function* () { + const broadcaster = yield* VcsStatusBroadcaster.VcsStatusBroadcaster; + const subscribed = yield* Deferred.make(); + const firstRemote = yield* Deferred.make(); + const secondRemote = yield* Deferred.make(); + const thirdRemote = yield* Deferred.make(); + yield* broadcaster.streamStatus({ cwd: "/repo", includeRemote: false }).pipe( + Stream.runForEach((event) => { + if (event._tag === "snapshot") return Deferred.succeed(subscribed, undefined); + if (event._tag !== "remoteUpdated" || event.remote === null) return Effect.void; + if (event.remote.pr === null) return Deferred.succeed(firstRemote, undefined); + return event.remote.pr.number === 2979 + ? Deferred.succeed(thirdRemote, undefined) + : Deferred.succeed(secondRemote, undefined); + }), + Effect.forkScoped, + ); + yield* Deferred.await(subscribed); + const readsBeforeCheckout = state.localStatusCalls; + + yield* broadcaster.refreshAfterCheckout("/repo"); + yield* Deferred.await(firstRemote); + assert.equal(state.localStatusCalls, readsBeforeCheckout + 1); + + const fetchedLocal: VcsStatusLocalResult = { + ...baseLocalStatus, + branchChanges: { baseRef: "origin/main", insertions: 0, deletions: 0 }, + }; + state.currentRemoteStatus = remoteStatusWithPr; + state.beforeRemoteStatus = Effect.sync(() => { + state.refSnapshot = "fetched"; + state.currentLocalStatus = fetchedLocal; + }); + yield* broadcaster.refreshAfterCheckout("/repo"); + yield* Deferred.await(secondRemote); + + assert.equal(state.localStatusCalls, readsBeforeCheckout + 3); + assert.deepStrictEqual( + (yield* broadcaster.getStatus({ cwd: "/repo" })).branchChanges, + fetchedLocal.branchChanges, + ); + + // A fetch finishing after the first local read took its values still needs a re-read. + const lateFetchLocal: VcsStatusLocalResult = { + ...baseLocalStatus, + branchChanges: { baseRef: "origin/main", insertions: 7, deletions: 0 }, + }; + state.beforeRemoteStatus = Effect.void; + state.currentRemoteStatus = { + ...remoteStatusWithPr, + pr: remoteStatusWithPr.pr && { ...remoteStatusWithPr.pr, number: 2979 }, + }; + state.beforeLocalStatus = Effect.sync(() => { + state.beforeLocalStatus = Effect.void; + state.refSnapshot = "fetched late"; + state.currentLocalStatus = lateFetchLocal; + }); + yield* broadcaster.refreshAfterCheckout("/repo"); + yield* Deferred.await(thirdRemote); + + assert.deepStrictEqual( + (yield* broadcaster.getStatus({ cwd: "/repo" })).branchChanges, + lateFetchLocal.branchChanges, + ); + }).pipe(Effect.provide(layerTestFor(state)), Effect.scoped); + }); + it.effect("a remote poll refreshes Changes totals when divergence counts stay the same", () => { const state: Parameters[0] = { currentLocalStatus: { diff --git a/apps/server/src/vcs/VcsStatusBroadcaster.ts b/apps/server/src/vcs/VcsStatusBroadcaster.ts index b691b9d2978a2..89d7907c3f797 100644 --- a/apps/server/src/vcs/VcsStatusBroadcaster.ts +++ b/apps/server/src/vcs/VcsStatusBroadcaster.ts @@ -25,6 +25,7 @@ import type { VcsStatusSubscriptionInput, } from "@t3tools/contracts"; import { DEFAULT_AUTOMATIC_GIT_FETCH_INTERVAL } from "@t3tools/contracts"; +import * as SourceControlRateLimit from "@t3tools/source-control-core/server/SourceControlRateLimit"; import { mergeGitStatusParts } from "@t3tools/shared/git"; import * as KeyedLock from "@t3tools/shared/KeyedLock"; import { resolveProjectSettings } from "@t3tools/shared/projectSettings"; @@ -687,15 +688,46 @@ export const make = Effect.gen(function* () { return yield* refreshRemoteAndLocalTotals(cwd, rawCwd, refsBeforeLocalRead); }, Effect.scoped); + const refreshRemoteAfterCheckout = Effect.fn("VcsStatusBroadcaster.refreshRemoteAfterCheckout")( + function* (cwd: string, policyCwd: string, refsBeforeLocalRead: string | null) { + yield* withRemoteWriteLock( + cwd, + Effect.gen(function* () { + const refreshUpstream = !Duration.isZero(yield* automaticFetchInterval); + yield* workflow.invalidateStatus(cwd, { keepPullRequestLookups: true }); + const remoteRead = yield* readRemoteStatus(cwd, { refreshUpstream }); + // A fetch, ours or not, can move the base used for local Changes totals. + if ( + refsBeforeLocalRead === null || + (yield* workflow.readRefSnapshot(cwd)) !== refsBeforeLocalRead + ) { + yield* refreshLocalStatusCore(cwd); + } + // Without a fetch, the cached behind count cannot justify an automatic pull. + const pulled = refreshUpstream + ? yield* maybeAutoPull(cwd, remoteRead, [policyCwd]) + : null; + if (pulled === null) { + yield* updateCachedRemoteStatus(cwd, remoteRead, { publish: true }); + } + }), + ); + }, + // Someone just picked this branch, so its PR lookup skips the background batching delay. + Effect.provideService(SourceControlRateLimit.Interactive, true), + ); + const refreshAfterCheckout: VcsStatusBroadcaster["Service"]["refreshAfterCheckout"] = Effect.fn( "VcsStatusBroadcaster.refreshAfterCheckout", )(function* (rawCwd) { const cwd = yield* withFileSystem(normalizeCwd(rawCwd)); + // Taken before the local read, so a fetch that finishes after it still triggers a re-read. + const refsBeforeLocalRead = yield* workflow.readRefSnapshot(cwd); const localDone = yield* Deferred.make(); // Owned by the broadcaster so a client disconnecting mid-switch still leaves status correct. yield* refreshLocalStatusCore(cwd).pipe( Effect.ensuring(Deferred.succeed(localDone, undefined)), - Effect.andThen(refreshRemoteAndLocalTotals(cwd, rawCwd, null)), + Effect.andThen(refreshRemoteAfterCheckout(cwd, rawCwd, refsBeforeLocalRead)), Effect.ignoreCause({ log: true }), Effect.forkIn(broadcasterScope), );