Skip to content
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions apps/server/src/git/GitWorkflowService.ts
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ export class GitWorkflowService extends Context.Service<
) => Effect.Effect<VcsStatusRemoteResult | null, GitManagerServiceError>;
readonly invalidateLocalStatus: (cwd: string) => Effect.Effect<void, never>;
readonly invalidateRemoteStatus: (cwd: string) => Effect.Effect<void, never>;
readonly readRefSnapshot: (cwd: string) => Effect.Effect<string | null>;
readonly invalidateStatus: (cwd: string) => Effect.Effect<void, never>;
readonly pullCurrentBranch: (cwd: string) => Effect.Effect<VcsPullResult, GitCommandError>;
readonly runStackedAction: (
Expand Down Expand Up @@ -369,6 +370,7 @@ export const make = Effect.gen(function* () {
}),
invalidateLocalStatus: gitManager.invalidateLocalStatus,
invalidateRemoteStatus: gitManager.invalidateRemoteStatus,
readRefSnapshot: git.readRefSnapshot,
invalidateStatus: gitManager.invalidateStatus,
pullCurrentBranch: Effect.fn("GitWorkflowService.pullCurrentBranch")(function (cwd) {
return ensureGitCommand("GitWorkflowService.pullCurrentBranch", cwd).pipe(
Expand Down
2 changes: 2 additions & 0 deletions apps/server/src/vcs/GitVcsDriver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -315,6 +315,8 @@ export class GitVcsDriver extends Context.Service<
cwd: string,
options?: GitRemoteStatusOptions,
) => Effect.Effect<GitRemoteStatusDetails, GitCommandError>;
/** Every ref and HEAD, for comparing across a read. Null when they cannot be listed. */
readonly readRefSnapshot: (cwd: string) => Effect.Effect<string | null>;
readonly prepareCommitContext: (
cwd: string,
filePaths?: readonly string[],
Expand Down
39 changes: 39 additions & 0 deletions apps/server/src/vcs/GitVcsDriverCore.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3477,6 +3477,45 @@ it.layer(layerTest)("GitVcsDriver core integration", (it) => {
}),
);

it.effect("a ref snapshot changes when any fetch or commit moves refs", () =>
Effect.gen(function* () {
const cwd = yield* makeTmpDir();
const remote = yield* makeTmpDir("git-vcs-driver-remote-");
const updater = yield* makeTmpDir("git-vcs-driver-updater-");
const { initialBranch } = yield* initRepoWithCommit(cwd);
yield* git(remote, ["init", "--bare"]);
yield* git(cwd, ["remote", "add", "origin", remote]);
yield* git(cwd, ["push", "-u", "origin", initialBranch]);
yield* git(updater, ["clone", remote, "."]);
yield* git(updater, ["config", "user.email", "test@test.com"]);
yield* git(updater, ["config", "user.name", "Test"]);
const driver = yield* GitVcsDriver.GitVcsDriver;
const snapshot = driver.readRefSnapshot(cwd);

// Newer Git can create origin/HEAD on the first fetch.
yield* git(cwd, ["fetch", "origin"]);
const initial = yield* snapshot;
assert.isNotNull(initial);
yield* git(cwd, ["fetch", "origin"]);
assert.equal(yield* snapshot, initial);

yield* writeTextFile(updater, "remote.txt", "remote\n");
yield* git(updater, ["add", "remote.txt"]);
yield* git(updater, ["commit", "-m", "remote commit"]);
yield* git(updater, ["push", "origin", initialBranch]);
assert.equal(yield* snapshot, initial);
yield* git(cwd, ["fetch", "origin"]);
const fetched = yield* snapshot;
assert.notEqual(fetched, initial);

yield* git(cwd, ["checkout", "--detach"]);
yield* git(cwd, ["commit", "--allow-empty", "-m", "detached"]);
assert.notEqual(yield* snapshot, fetched);

assert.isNull(yield* driver.readRefSnapshot(yield* makeTmpDir()));
}),
);

it.effect("does not start Git auto-maintenance from background upstream fetches", () =>
Effect.gen(function* () {
const cwd = yield* makeTmpDir();
Expand Down
9 changes: 9 additions & 0 deletions apps/server/src/vcs/GitVcsDriverCore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1640,6 +1640,14 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function*
),
});

const readRefSnapshot: GitVcsDriver.GitVcsDriver["Service"]["readRefSnapshot"] = (cwd) =>
executeGit("GitVcsDriver.readRefSnapshot", cwd, ["show-ref", "--head"], {
allowNonZeroExit: true,
Comment thread
Yuruzuu marked this conversation as resolved.
Resolved
}).pipe(
Effect.map((result) => (result.exitCode === 0 ? result.stdout : null)),
Effect.orElseSucceed(() => null),
);

const refreshStatusUpstreamIfStale = Effect.fn("refreshStatusUpstreamIfStale")(function* (
cwd: string,
) {
Expand Down Expand Up @@ -4965,6 +4973,7 @@ export const makeGitVcsDriverCore = Effect.fn("makeGitVcsDriverCore")(function*
statusDetails,
statusDetailsLocal,
statusDetailsRemote,
readRefSnapshot,
prepareCommitContext,
commit: Effect.fnUntraced(function* (cwd, subject, body, options) {
return yield* withListRefsInvalidation(cwd, commit(cwd, subject, body, options));
Expand Down
62 changes: 62 additions & 0 deletions apps/server/src/vcs/VcsStatusBroadcaster.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ function layerTestFor(state: {
localInvalidationCalls: number;
remoteInvalidationCalls: number;
remoteStatusRefreshUpstreamValues?: Array<boolean | undefined>;
refSnapshot?: string | null;
remoteStatusError?: GitManagerError;
backgroundWorkEnabled?: boolean;
automaticFetchInterval?: Effect.Effect<Duration.Duration>;
Expand Down Expand Up @@ -103,6 +104,8 @@ function layerTestFor(state: {
);
}),
currentBranch: () => Effect.sync(() => state.currentLocalStatus.refName),
readRefSnapshot: () =>
Effect.sync(() => (state.refSnapshot === undefined ? "refs" : state.refSnapshot)),
remoteStatus: (_input, options) =>
Effect.suspend(() => {
return (state.beforeRemoteStatus ?? Effect.void).pipe(
Expand Down Expand Up @@ -342,6 +345,7 @@ describe("VcsStatusBroadcaster", () => {
Layer.mock(GitWorkflowService.GitWorkflowService)({
localStatus: () => Effect.succeed(localStatus),
currentBranch: () => Effect.succeed(localStatus.refName),
readRefSnapshot: () => Effect.succeed("refs"),
remoteStatus: () => Effect.succeed(remoteStatus),
invalidateLocalStatus: () => Effect.void,
invalidateRemoteStatus: () => Effect.void,
Expand Down Expand Up @@ -431,6 +435,7 @@ describe("VcsStatusBroadcaster", () => {
Layer.mock(GitWorkflowService.GitWorkflowService)({
localStatus: () => Effect.succeed(baseLocalStatus),
currentBranch: () => Effect.succeed(baseLocalStatus.refName),
readRefSnapshot: () => Effect.succeed("refs"),
remoteStatus: () =>
Effect.gen(function* () {
remoteReads += 1;
Expand Down Expand Up @@ -477,6 +482,7 @@ describe("VcsStatusBroadcaster", () => {
Layer.mock(GitWorkflowService.GitWorkflowService)({
localStatus: () => Effect.succeed(baseLocalStatus),
currentBranch: () => Effect.succeed(baseLocalStatus.refName),
readRefSnapshot: () => Effect.succeed("refs"),
remoteStatus: () =>
Effect.gen(function* () {
remoteReads += 1;
Expand Down Expand Up @@ -530,6 +536,7 @@ describe("VcsStatusBroadcaster", () => {
return state.currentLocalStatus;
}),
currentBranch: () => Effect.succeed(state.currentLocalStatus.refName),
readRefSnapshot: () => Effect.succeed("refs"),
remoteStatus: () =>
Effect.suspend(() => {
state.remoteStatusCalls += 1;
Expand Down Expand Up @@ -704,6 +711,7 @@ describe("VcsStatusBroadcaster", () => {
branchChanges: { baseRef: "origin/main", insertions: 745, deletions: 24 },
};
let cachedLocal: VcsStatusLocalResult | undefined;
let refSnapshot = "before";
const readLocalStatus = () => Effect.sync(() => (cachedLocal ??= local));
const layerTest = VcsStatusBroadcaster.layer.pipe(
Layer.provideMerge(NodeServices.layer),
Expand All @@ -721,6 +729,7 @@ describe("VcsStatusBroadcaster", () => {
}),
currentBranch: () => Effect.succeed(baseLocalStatus.refName),
invalidateRemoteStatus: () => Effect.void,
readRefSnapshot: () => Effect.sync(() => refSnapshot),
remoteStatus: () =>
Deferred.succeed(fetchStarted, undefined).pipe(
Effect.andThen(Deferred.await(finishFetch)),
Expand All @@ -739,13 +748,66 @@ describe("VcsStatusBroadcaster", () => {
...baseLocalStatus,
branchChanges: { baseRef: "origin/main", insertions: 0, deletions: 0 },
};
refSnapshot = "after";
yield* Deferred.succeed(finishFetch, undefined);
const status = yield* Fiber.join(refresh);
assert.deepStrictEqual(status.branchChanges, local.branchChanges);
}).pipe(Effect.provide(layerTest));
});
});

it.effect("an explicit refresh re-reads local totals only after refs moved", () => {
const totals = (insertions: number): VcsStatusLocalResult => ({
...baseLocalStatus,
branchChanges: { baseRef: "origin/main", insertions, deletions: 0 },
});
const state: Parameters<typeof layerTestFor>[0] = {
currentLocalStatus: totals(745),
currentRemoteStatus: remoteStatusWithPr,
localStatusCalls: 0,
remoteStatusCalls: 0,
localInvalidationCalls: 0,
remoteInvalidationCalls: 0,
};
return Effect.gen(function* () {
const broadcaster = yield* VcsStatusBroadcaster.VcsStatusBroadcaster;
const subscribed = yield* Deferred.make<void>();
const events = yield* broadcaster.streamStatus({ cwd: "/repo", includeRemote: false }).pipe(
Stream.tap((event) =>
event._tag === "snapshot" ? Deferred.succeed(subscribed, undefined) : Effect.void,
),
Stream.filter((event) => event._tag !== "snapshot"),
Stream.take(3),
Stream.runCollect,
Effect.forkScoped,
);
yield* Deferred.await(subscribed);

state.localStatusCalls = 0;
yield* broadcaster.refreshStatus("/repo");
assert.equal(state.localStatusCalls, 1);

state.localStatusCalls = 0;
state.currentRemoteStatus = baseRemoteStatus;
state.beforeRemoteStatus = Effect.sync(() => {
state.currentLocalStatus = totals(0);
state.refSnapshot = "moved";
});
const moved = yield* broadcaster.refreshStatus("/repo");
assert.equal(state.localStatusCalls, 2);
assert.deepStrictEqual(moved.branchChanges, totals(0).branchChanges);
assert.deepStrictEqual(
[...(yield* Fiber.join(events))].map((event) => event._tag),
["remoteUpdated", "remoteUpdated", "localUpdated"],
);

state.localStatusCalls = 0;
state.refSnapshot = null;
yield* broadcaster.refreshStatus("/repo");
assert.equal(state.localStatusCalls, 2);
}).pipe(Effect.provide(layerTestFor(state)), Effect.scoped);
});

it.effect("publishes local status while an earlier lookup holds the remote lock", () => {
const state: Parameters<typeof layerTestFor>[0] = {
currentLocalStatus: baseLocalStatus,
Expand Down
10 changes: 8 additions & 2 deletions apps/server/src/vcs/VcsStatusBroadcaster.ts
Original file line number Diff line number Diff line change
Expand Up @@ -495,6 +495,7 @@ export const make = Effect.gen(function* () {
)(function* (rawCwd, options) {
const cwd = yield* withFileSystem(normalizeCwd(rawCwd));
const previous = options?.localOnly ? yield* getCachedStatus(cwd) : null;
const refsBeforeLocalRead = options?.localOnly ? null : yield* workflow.readRefSnapshot(cwd);
const local = yield* refreshLocalStatusCore(cwd);
if (options?.localOnly) {
return yield* withRemoteWriteLock(
Expand All @@ -520,12 +521,17 @@ 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);
// 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);
yield* updateCachedRemoteStatus(cwd, remoteRead, { publish: true });
// 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);
}
const parts = yield* getStatusParts(cwd);
return mergeGitStatusParts(parts.local, parts.remote);
}),
Expand Down
Loading