diff --git a/apps/server/src/git/GitWorkflowService.ts b/apps/server/src/git/GitWorkflowService.ts index a614ebdfb5687..b5c258dda58c9 100644 --- a/apps/server/src/git/GitWorkflowService.ts +++ b/apps/server/src/git/GitWorkflowService.ts @@ -65,6 +65,7 @@ export class GitWorkflowService extends Context.Service< input: VcsStatusInput, options?: GitManager.GitRemoteStatusOptions, ) => Effect.Effect; + readonly statusFetchCount: Effect.Effect; readonly invalidateLocalStatus: (cwd: string) => Effect.Effect; readonly invalidateRemoteStatus: (cwd: string) => Effect.Effect; readonly invalidateStatus: (cwd: string) => Effect.Effect; @@ -388,6 +389,7 @@ export const make = Effect.gen(function* () { ), ); }), + statusFetchCount: git.statusFetchCount, invalidateLocalStatus: gitManager.invalidateLocalStatus, invalidateRemoteStatus: gitManager.invalidateRemoteStatus, invalidateStatus: gitManager.invalidateStatus, diff --git a/apps/server/src/vcs/GitVcsDriver.ts b/apps/server/src/vcs/GitVcsDriver.ts index 14147854a2ddb..1bf5faee92950 100644 --- a/apps/server/src/vcs/GitVcsDriver.ts +++ b/apps/server/src/vcs/GitVcsDriver.ts @@ -333,6 +333,8 @@ export class GitVcsDriver extends Context.Service< cwd: string, options?: GitRemoteStatusOptions, ) => Effect.Effect; + /** Status fetches so far, in any repository, that moved refs or failed. A change means refs may have moved. */ + readonly statusFetchCount: Effect.Effect; readonly prepareCommitContext: ( cwd: string, filePaths?: readonly string[], diff --git a/apps/server/src/vcs/GitVcsDriverCore.test.ts b/apps/server/src/vcs/GitVcsDriverCore.test.ts index b748d4f364c84..d36c095c82d9f 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.test.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.test.ts @@ -4642,13 +4642,21 @@ it.layer(layerTest)("GitVcsDriver core integration", (it) => { yield* git(updater, ["push", "origin", initialBranch]); const driver = yield* GitVcsDriver.GitVcsDriver; + const fetchesBefore = yield* driver.statusFetchCount; const cachedStatus = yield* driver.statusDetailsRemote(cwd, { refreshUpstream: false, }); + assert.equal(yield* driver.statusFetchCount, fetchesBefore); const refreshedStatus = yield* driver.statusDetailsRemote(cwd); assert.equal(cachedStatus.behindCount, 0); assert.equal(refreshedStatus.behindCount, 1); + assert.equal(yield* driver.statusFetchCount, fetchesBefore + 1); + + // A later fetch that moves no refs leaves the count alone. + yield* TestClock.adjust("16 seconds"); + yield* driver.statusDetailsRemote(cwd); + assert.equal(yield* driver.statusFetchCount, fetchesBefore + 1); }), ); diff --git a/apps/server/src/vcs/GitVcsDriverCore.ts b/apps/server/src/vcs/GitVcsDriverCore.ts index 212ccb3a0b449..b7003f5093bc0 100644 --- a/apps/server/src/vcs/GitVcsDriverCore.ts +++ b/apps/server/src/vcs/GitVcsDriverCore.ts @@ -1374,6 +1374,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* ); }); + let statusFetchCount = 0; const fetchRemoteForStatus = Effect.fn("GitVcsDriver.fetchRemoteForStatus.wait")(function* ( gitCommonDir: string, remoteName: string, @@ -1388,7 +1389,8 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* "--git-dir", gitCommonDir, "fetch", - "--quiet", + // Not `--quiet`: Git prints one line per ref it moved, and nothing when every ref was current. + "--no-progress", "--no-tags", "--no-auto-gc", // FETCH_HEAD is written without a lock; overlapping a user's `git pull` makes that pull fail. @@ -1411,7 +1413,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* fallbackErrorDetail: "Background Git fetch exited with a non-zero status.", timeoutMs: Duration.toMillis(STATUS_UPSTREAM_FETCH_TIMEOUT), }).pipe( - Effect.asVoid, + Effect.map((result) => result.stdout.trim().length + result.stderr.trim().length > 0), Effect.tap(() => Effect.sync(() => { statusRemoteRefreshFailures.delete(key); @@ -1420,11 +1422,14 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* Effect.tapError(() => recordStatusRemoteRefreshFailure(key)), Effect.tapCause((cause) => Effect.logWarning("Background Git fetch failed", cause)), statusFetchProcesses.withPermits(1), - Effect.onExit(() => + Effect.onExit((exit) => Effect.sync(() => { statusFetches.delete(key); + // Counted when git exits, not when a waiter times out; a failed fetch can still move refs. + if (Exit.isFailure(exit) || exit.value) statusFetchCount += 1; }), ), + Effect.asVoid, Effect.forkIn(serviceScope, { startImmediately: false, uninterruptible: false }), ); statusFetches.set(key, started); @@ -5623,6 +5628,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function* statusDetails, statusDetailsLocal, statusDetailsRemote, + statusFetchCount: Effect.sync(() => statusFetchCount), prepareCommitContext, commit: Effect.fnUntraced(function* (cwd, subject, body, options) { return yield* withListRefsInvalidation(cwd, commit(cwd, subject, body, options)); diff --git a/apps/server/src/vcs/VcsStatusBroadcaster.test.ts b/apps/server/src/vcs/VcsStatusBroadcaster.test.ts index 96e29039abbd2..b1b0773b2a334 100644 --- a/apps/server/src/vcs/VcsStatusBroadcaster.test.ts +++ b/apps/server/src/vcs/VcsStatusBroadcaster.test.ts @@ -91,6 +91,7 @@ function layerTestFor(state: { headPathCalls?: number; failLocalStatus?: boolean; onLocalStatusFailure?: Effect.Effect; + statusFetchCount?: number; }) { return VcsStatusBroadcaster.layer.pipe( Layer.provideMerge( @@ -159,6 +160,7 @@ function layerTestFor(state: { ), ); }), + statusFetchCount: Effect.sync(() => state.statusFetchCount ?? 0), invalidateLocalStatus: () => Effect.sync(() => { state.localInvalidationCalls += 1; @@ -745,6 +747,7 @@ describe("VcsStatusBroadcaster", () => { ), Layer.provide( Layer.mock(GitWorkflowService.GitWorkflowService)({ + statusFetchCount: Effect.succeed(0), localStatus: () => Effect.succeed(localStatus), currentBranch: () => Effect.succeed(localStatus.refName), remoteStatus: () => Effect.succeed(remoteStatus), @@ -841,6 +844,7 @@ describe("VcsStatusBroadcaster", () => { Layer.provide(layerBackgroundPolicy(() => true)), Layer.provide( Layer.mock(GitWorkflowService.GitWorkflowService)({ + statusFetchCount: Effect.succeed(0), localStatus: () => Effect.succeed(baseLocalStatus), currentBranch: () => Effect.succeed(baseLocalStatus.refName), remoteStatus: () => @@ -887,6 +891,7 @@ describe("VcsStatusBroadcaster", () => { Layer.provide(layerBackgroundPolicy(() => true)), Layer.provide( Layer.mock(GitWorkflowService.GitWorkflowService)({ + statusFetchCount: Effect.succeed(0), localStatus: () => Effect.succeed(baseLocalStatus), currentBranch: () => Effect.succeed(baseLocalStatus.refName), remoteStatus: () => @@ -936,6 +941,7 @@ describe("VcsStatusBroadcaster", () => { Layer.provide(layerBackgroundPolicy(() => true)), Layer.provide( Layer.mock(GitWorkflowService.GitWorkflowService)({ + statusFetchCount: Effect.succeed(0), localStatus: () => Effect.sync(() => { state.localStatusCalls += 1; @@ -1019,6 +1025,7 @@ describe("VcsStatusBroadcaster", () => { Layer.provide(layerBackgroundPolicy(() => true)), Layer.provide( Layer.mock(GitWorkflowService.GitWorkflowService)({ + statusFetchCount: Effect.succeed(0), localStatus: (input) => Effect.sync(() => { seenCwds.push(input.cwd); @@ -1107,7 +1114,7 @@ describe("VcsStatusBroadcaster", () => { }).pipe(Effect.provide(layerTestFor(state))); }); - it.effect("an explicit refresh reads local totals after the fetch", () => { + it.effect("an explicit refresh reads local totals again only after a fetch", () => { return Effect.gen(function* () { const fetchStarted = yield* Deferred.make(); const finishFetch = yield* Deferred.make(); @@ -1116,7 +1123,14 @@ describe("VcsStatusBroadcaster", () => { branchChanges: { baseRef: "origin/main", insertions: 745, deletions: 24 }, }; let cachedLocal: VcsStatusLocalResult | undefined; - const readLocalStatus = () => Effect.sync(() => (cachedLocal ??= local)); + let localStatusReads = 0; + let fetches = 0; + let fetchNext = true; + const readLocalStatus = () => + Effect.sync(() => { + localStatusReads += 1; + return (cachedLocal ??= local); + }); const layerTest = VcsStatusBroadcaster.layer.pipe( Layer.provideMerge(NodeServices.layer), Layer.provide(layerBackgroundPolicy(() => true)), @@ -1133,11 +1147,19 @@ describe("VcsStatusBroadcaster", () => { }), currentBranch: () => Effect.succeed(baseLocalStatus.refName), invalidateRemoteStatus: () => Effect.void, + statusFetchCount: Effect.sync(() => fetches), remoteStatus: () => - Deferred.succeed(fetchStarted, undefined).pipe( - Effect.andThen(Deferred.await(finishFetch)), - Effect.as(baseRemoteStatus), - ), + fetchNext + ? Deferred.succeed(fetchStarted, undefined).pipe( + Effect.andThen(Deferred.await(finishFetch)), + Effect.andThen( + Effect.sync(() => { + fetches += 1; + }), + ), + Effect.as(baseRemoteStatus), + ) + : Effect.succeed(baseRemoteStatus), }), ), ); @@ -1154,6 +1176,12 @@ describe("VcsStatusBroadcaster", () => { yield* Deferred.succeed(finishFetch, undefined); const status = yield* Fiber.join(refresh); assert.deepStrictEqual(status.branchChanges, local.branchChanges); + + // Without a fetch nothing can move the base, so the working tree is scanned once. + fetchNext = false; + localStatusReads = 0; + yield* broadcaster.refreshStatus("/repo"); + assert.equal(localStatusReads, 1); }).pipe(Effect.provide(layerTest)); }); }); @@ -1761,6 +1789,7 @@ describe("VcsStatusBroadcaster", () => { Layer.provide(layerBackgroundPolicy(() => true)), Layer.provide( Layer.mock(GitWorkflowService.GitWorkflowService)({ + statusFetchCount: Effect.succeed(0), localStatus: () => Effect.sync(() => { state.localStatusCalls += 1; diff --git a/apps/server/src/vcs/VcsStatusBroadcaster.ts b/apps/server/src/vcs/VcsStatusBroadcaster.ts index 06cc4aff7dfa2..bce228ca3ccf6 100644 --- a/apps/server/src/vcs/VcsStatusBroadcaster.ts +++ b/apps/server/src/vcs/VcsStatusBroadcaster.ts @@ -607,6 +607,7 @@ export const make = Effect.gen(function* () { "VcsStatusBroadcaster.refreshStatus", )(function* (rawCwd, options) { const cwd = yield* withFileSystem(normalizeCwd(rawCwd)); + const fetchesBeforeLocalRead = yield* workflow.statusFetchCount; yield* refreshLocalStatusCore(cwd); if (options?.localOnly) { // Never cache a remote here: readers take `value: null` as "read and found no PR". @@ -620,8 +621,10 @@ export const make = Effect.gen(function* () { // Bump the PR lookup epoch after earlier remote writers have finished. yield* workflow.invalidateStatus(cwd); const remoteRead = yield* readRemoteStatus(cwd, { refreshUpstream }); - // Fetch can move the base used for local Changes totals. - yield* refreshLocalStatusCore(cwd); + // A fetch can move the base used for local Changes totals; without one, the first read stands. + if ((yield* workflow.statusFetchCount) !== fetchesBeforeLocalRead) { + yield* refreshLocalStatusCore(cwd); + } // Without a fetch, the cached behind count cannot justify an automatic pull. const pulled = refreshUpstream ? yield* maybeAutoPull(cwd, remoteRead, [rawCwd]) : null; if (pulled !== null) return mergeGitStatusParts(pulled.local, pulled.remote);