Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
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
4 changes: 2 additions & 2 deletions apps/code/src/main/di/container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -503,10 +503,10 @@ container.bind(GIT_DIFF_SOURCE).toDynamicValue((ctx) => {
}),
getPrTemplate: (directoryPath: string) =>
git().getPrTemplate.query({ directoryPath }),
fetchIfStale: async (directoryPath: string) => {
fetchFromRemote: async (directoryPath: string) => {
await git().getGitSyncStatus.query({
directoryPath,
forceRefresh: true,
fetchFromRemote: true,
});
},
};
Expand Down
4 changes: 2 additions & 2 deletions packages/core/src/git-pr/git-pr.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ function makeDiffSource(over: Partial<GitDiffSource> = {}): GitDiffSource {
getDiffAgainstRemote: vi.fn().mockResolvedValue(""),
getCommitsBetweenBranches: vi.fn().mockResolvedValue([]),
getPrTemplate: vi.fn().mockResolvedValue({ template: null }),
fetchIfStale: vi.fn().mockResolvedValue(undefined),
fetchFromRemote: vi.fn().mockResolvedValue(undefined),
...over,
};
}
Expand Down Expand Up @@ -97,7 +97,7 @@ describe("GitPrService.generatePrTitleAndBody", () => {

expect(result.title).toBe("feat: add widget");
expect(result.body).toBe("TL;DR: adds a widget.");
expect(diffSource.fetchIfStale).toHaveBeenCalledWith("/repo");
expect(diffSource.fetchFromRemote).toHaveBeenCalledWith("/repo");
});
});

Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/git-pr/git-pr.ts
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ ${truncatedDiff}${contextSection}`;
directoryPath: string,
conversationContext?: string,
): Promise<{ title: string; body: string }> {
await this.gitDiff.fetchIfStale(directoryPath);
await this.gitDiff.fetchFromRemote(directoryPath);

const [defaultBranch, currentBranch, prTemplate] = await Promise.all([
this.gitDiff.getDefaultBranch(directoryPath),
Expand Down
2 changes: 1 addition & 1 deletion packages/core/src/git-pr/identifiers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ export interface GitDiffSource {
limit: number,
): Promise<GitCommitSummary[]>;
getPrTemplate(directoryPath: string): Promise<GitPrTemplate>;
fetchIfStale(directoryPath: string): Promise<void>;
fetchFromRemote(directoryPath: string): Promise<void>;
}

export interface GitPrLogger extends SagaLogger {}
Expand Down
5 changes: 4 additions & 1 deletion packages/core/src/git/git-host.ts
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,10 @@ export class GitHostService extends TypedEventEmitter<GitServiceEvents> {
await Promise.allSettled([
this.git.getChangedFilesHead.query({ directoryPath }),
this.git.getDiffStats.query({ directoryPath }),
this.git.getGitSyncStatus.query({ directoryPath, forceRefresh: true }),
this.git.getGitSyncStatus.query({
directoryPath,
fetchFromRemote: true,
}),
this.git.getLatestCommit.query({ directoryPath }),
this.git.getPrStatus.query({ directoryPath }),
]);
Expand Down
4 changes: 2 additions & 2 deletions packages/host-router/src/routers/git.router.ts
Original file line number Diff line number Diff line change
Expand Up @@ -319,14 +319,14 @@ export const gitRouter = router({
.input(
z.object({
directoryPath: z.string(),
forceRefresh: z.boolean().optional(),
fetchFromRemote: z.boolean().optional(),
}),
)
.output(getGitSyncStatusOutput)
.query(({ ctx, input }) =>
getWorkspaceClient(ctx.container).git.getGitSyncStatus.query({
directoryPath: input.directoryPath,
forceRefresh: input.forceRefresh,
fetchFromRemote: input.fetchFromRemote,
}),
),

Expand Down
39 changes: 39 additions & 0 deletions packages/workspace-server/src/services/git/git.integration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -300,5 +300,44 @@ describe("GitService integration (git-read + git-mutate)", () => {
const result = await git.sync(work, "origin");
expect(result.success).toBe(true);
});

it("getGitSyncStatus does not fetch by default, fetches when fetchFromRemote=true", async () => {
// Another clone pushes a commit. The original `work` repo only learns
// about it after a fetch — so this lets us tell whether a sync-status
// read touched the network or not.
const otherClone = await fs.mkdtemp(path.join(os.tmpdir(), "git-other-"));
dirs.push(otherClone);
run(`git clone ${bare} ${otherClone}`, os.tmpdir());
run("git config user.email 'other@test.com'", otherClone);
run("git config user.name 'Other'", otherClone);
run("git config commit.gpgsign false", otherClone);
await fs.writeFile(
path.join(otherClone, "remote-only.txt"),
"from-other\n",
);
commitAll(otherClone, "from other clone");
run("git push origin main", otherClone);

// Default: no fetch, so `work` still thinks it is at the remote tip.
const stale = await git.getGitSyncStatus(work);
expect(stale.behind).toBe(0);

// Explicit fetch: `work` learns it is one commit behind.
const fresh = await git.getGitSyncStatus(work, true);
expect(fresh.behind).toBe(1);

// A second fetch immediately after must still hit the network — the
// staleness throttle must never silently swallow an opt-in
// fetchFromRemote=true call.
await fs.writeFile(
path.join(otherClone, "remote-only-2.txt"),
"from-other-2\n",
);
commitAll(otherClone, "from other clone (second)");
run("git push origin main", otherClone);

const fresher = await git.getGitSyncStatus(work, true);
expect(fresher.behind).toBe(2);
}, 15_000);
});
});
8 changes: 7 additions & 1 deletion packages/workspace-server/src/services/git/schemas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,13 @@ export type DiscardFileChangesOutput = z.infer<typeof discardFileChangesOutput>;

export const getGitSyncStatusInput = z.object({
directoryPath: z.string(),
forceRefresh: z.boolean().optional(),
/**
* Whether to run `git fetch` before reading sync status. Defaults to false:
* background pollers should read local refs only so that idle UI does not
* keep hitting the network (and, transitively, ssh-agent). Set true at the
* few callsites that genuinely need an up-to-date view of `origin/*`.
*/
fetchFromRemote: z.boolean().optional(),
});

export const gitBusyStateInput = directoryPathInput;
Expand Down
37 changes: 26 additions & 11 deletions packages/workspace-server/src/services/git/service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -432,25 +432,40 @@ export class GitService extends TypedEventEmitter<GitCloneEvents> {

private readonly lastFetchTime = new Map<string, number>();

/**
* Always runs `git fetch`, bypassing the staleness throttle. Use when the
* caller has explicitly asked for a fresh view of the remote (e.g.,
* `fetchFromRemote: true`) — otherwise a fetch triggered by a preceding
* mutation can silently swallow this one and leave the snapshot stale at
* exactly the moment it mattered.
*/
private async forceFetch(directoryPath: string): Promise<void> {
try {
await gitFetch(directoryPath);
this.lastFetchTime.set(directoryPath, Date.now());
} catch (error) {
const message = error instanceof Error ? error.message : String(error);
process.stderr.write(
`[git-service] fetch failed for ${directoryPath}; using local refs: ${message}\n`,
);
}
}

private async fetchIfStale(directoryPath: string): Promise<void> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this now always force fetches right, e.g. still calling it fetchIfStale is a lie

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good nit — the private fetchIfStale inside GitService still respects the throttle, so the name's accurate there. The actually-lying one was the agent adapter exposed as GitDiffSource.fetchIfStale (in apps/code/src/main/di/container.ts) — it goes through getGitSyncStatus.query({ fetchFromRemote: true }), which since the throttle-bypass fix unconditionally hits forceFetch. Renamed that interface method (and its caller in git-pr.ts) to fetchFromRemote in f4c40b9 so the public contract matches the behavior.

const now = Date.now();
const lastFetch = this.lastFetchTime.get(directoryPath) ?? 0;
if (now - lastFetch > FETCH_THROTTLE_MS) {
try {
await gitFetch(directoryPath);
this.lastFetchTime.set(directoryPath, now);
} catch {}
await this.forceFetch(directoryPath);
}
}

private async getGitSyncStatusInternal(
directoryPath: string,
forceRefresh = false,
fetchFromRemote = false,
): Promise<GitSyncStatus> {
if (forceRefresh) {
this.lastFetchTime.delete(directoryPath);
if (fetchFromRemote) {
await this.forceFetch(directoryPath);
}
Comment thread
dmarticus marked this conversation as resolved.
await this.fetchIfStale(directoryPath);

const status = await getSyncStatus(directoryPath);
return {
Expand Down Expand Up @@ -483,7 +498,7 @@ export class GitService extends TypedEventEmitter<GitCloneEvents> {
includeChangedFiles ? this.getChangedFilesHead(directoryPath) : null,
includeDiffStats ? this.getDiffStats(directoryPath) : null,
includeSyncStatus
? this.getGitSyncStatusInternal(directoryPath, true)
? this.getGitSyncStatusInternal(directoryPath, false)
: null,
includeLatestCommit ? this.getLatestCommit(directoryPath) : null,
]);
Expand All @@ -508,9 +523,9 @@ export class GitService extends TypedEventEmitter<GitCloneEvents> {

async getGitSyncStatus(
directoryPath: string,
forceRefresh = false,
fetchFromRemote = false,
): Promise<GitSyncStatus> {
return this.getGitSyncStatusInternal(directoryPath, forceRefresh);
return this.getGitSyncStatusInternal(directoryPath, fetchFromRemote);
}

async createBranch(directoryPath: string, branchName: string): Promise<void> {
Expand Down
2 changes: 1 addition & 1 deletion packages/workspace-server/src/trpc.ts
Original file line number Diff line number Diff line change
Expand Up @@ -426,7 +426,7 @@ export function createAppRouter({
.query(({ input }) =>
gitService().getGitSyncStatus(
input.directoryPath,
input.forceRefresh,
input.fetchFromRemote,
),
),

Expand Down
Loading