Archived
fix(workspaces): serialize overlapping topology refreshes
The browser-resume path and the plugin-facing app refresh call refreshSelectedProjectTopology independently, so two requests for the same machine and project could be in flight at once. The stale guards check machine and project but not ordering, so a slower earlier response landing last overwrote a newer list, making a just-created worktree disappear again. Route the refresh through TrailingRefreshCoordinator, the primitive already used for browser resume, session refresh, and activity, keyed by machine and project. A second caller no longer opens its own request while one is in flight; it gets a single trailing pass, so the last applied response is the newest one.
This commit is contained in:
@@ -273,6 +273,50 @@ describe("WorkspaceController.refreshSelectedProjectTopology", () => {
|
|||||||
expect(test.state().selectedWorkspace).toBe(selected);
|
expect(test.state().selectedWorkspace).toBe(selected);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("serializes overlapping refreshes so an earlier response cannot overwrite a newer list", async () => {
|
||||||
|
const repo = project("p1", "/repo");
|
||||||
|
const main = workspace(repo.id, repo.path, { isMain: true });
|
||||||
|
const created = workspace(repo.id, "/repo-feature");
|
||||||
|
const gates: ((workspaces: Workspace[]) => void)[] = [];
|
||||||
|
let inFlight = 0;
|
||||||
|
let maxInFlight = 0;
|
||||||
|
const loadWorkspaces = vi.fn(() => {
|
||||||
|
inFlight += 1;
|
||||||
|
maxInFlight = Math.max(maxInFlight, inFlight);
|
||||||
|
return new Promise<Workspace[]>((resolve) => {
|
||||||
|
gates.push((workspaces) => { inFlight -= 1; resolve(workspaces); });
|
||||||
|
});
|
||||||
|
});
|
||||||
|
const test = harness(
|
||||||
|
{
|
||||||
|
selectedMachine: machine("local"),
|
||||||
|
projects: [repo],
|
||||||
|
selectedProject: repo,
|
||||||
|
selectedWorkspace: main,
|
||||||
|
workspaces: [main],
|
||||||
|
workspacesByProjectId: { [repo.id]: [main] },
|
||||||
|
},
|
||||||
|
loadWorkspaces,
|
||||||
|
);
|
||||||
|
|
||||||
|
const resumeRefresh = test.controller.refreshSelectedProjectTopology();
|
||||||
|
await Promise.resolve();
|
||||||
|
const appDataRefresh = test.controller.refreshSelectedProjectTopology();
|
||||||
|
await Promise.resolve();
|
||||||
|
|
||||||
|
// The second caller does not open its own request while the first is in flight; it gets
|
||||||
|
// one trailing pass afterwards. Without this, two responses race and the slower-but-older
|
||||||
|
// one can land last, making a just-created worktree disappear again.
|
||||||
|
expect(maxInFlight).toBe(1);
|
||||||
|
gates[0]?.([main]);
|
||||||
|
await vi.waitFor(() => { expect(gates).toHaveLength(2); });
|
||||||
|
gates[1]?.([main, created]);
|
||||||
|
await Promise.all([resumeRefresh, appDataRefresh]);
|
||||||
|
|
||||||
|
// The last response wins, so the newly created worktree stays visible.
|
||||||
|
expect(test.state().workspaces).toEqual([main, created]);
|
||||||
|
});
|
||||||
|
|
||||||
it("does not request anything when no project is selected", async () => {
|
it("does not request anything when no project is selected", async () => {
|
||||||
const loadWorkspaces = vi.fn();
|
const loadWorkspaces = vi.fn();
|
||||||
const test = harness({ selectedMachine: machine("local") }, loadWorkspaces);
|
const test = harness({ selectedMachine: machine("local") }, loadWorkspaces);
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ import { mergeCachedNewSessions } from "../cachedNewSessions";
|
|||||||
import { machineProjectKey } from "../machineKeys";
|
import { machineProjectKey } from "../machineKeys";
|
||||||
import { selectedMachineId, type GetState, type RouteTarget, type SetState, type UpdateUrl } from "./types";
|
import { selectedMachineId, type GetState, type RouteTarget, type SetState, type UpdateUrl } from "./types";
|
||||||
import type { SessionController } from "./sessionController";
|
import type { SessionController } from "./sessionController";
|
||||||
|
import { TrailingRefreshCoordinator } from "./trailingRefreshCoordinator";
|
||||||
import { InMemoryWorkspaceSelectionMemory, selectPreferredWorkspace, type WorkspaceSelectionMemory } from "./workspaceSelection";
|
import { InMemoryWorkspaceSelectionMemory, selectPreferredWorkspace, type WorkspaceSelectionMemory } from "./workspaceSelection";
|
||||||
|
|
||||||
export interface WorkspaceControllerDependencies {
|
export interface WorkspaceControllerDependencies {
|
||||||
@@ -14,6 +15,7 @@ export interface WorkspaceControllerDependencies {
|
|||||||
export class WorkspaceController {
|
export class WorkspaceController {
|
||||||
private readonly api: Pick<typeof defaultApi, "sessions" | "workspaces">;
|
private readonly api: Pick<typeof defaultApi, "sessions" | "workspaces">;
|
||||||
private readonly onBackgroundError: (message: string, error: unknown) => void;
|
private readonly onBackgroundError: (message: string, error: unknown) => void;
|
||||||
|
private readonly topologyRefreshes = new TrailingRefreshCoordinator<string>();
|
||||||
|
|
||||||
constructor(
|
constructor(
|
||||||
private readonly getState: GetState,
|
private readonly getState: GetState,
|
||||||
@@ -99,6 +101,11 @@ export class WorkspaceController {
|
|||||||
const project = state.selectedProject;
|
const project = state.selectedProject;
|
||||||
if (project === undefined) return;
|
if (project === undefined) return;
|
||||||
const machineId = selectedMachineId(state);
|
const machineId = selectedMachineId(state);
|
||||||
|
// Callers are independent (browser resume and the plugin-facing app refresh), so two
|
||||||
|
// refreshes for the same machine+project can overlap. Sharing one request keeps a slow
|
||||||
|
// earlier response from landing last and overwriting a newer list, which would make a
|
||||||
|
// just-created worktree disappear again.
|
||||||
|
await this.topologyRefreshes.request(machineProjectKey(machineId, project.id), async () => {
|
||||||
try {
|
try {
|
||||||
const workspaces = await this.api.workspaces(project.id, machineId);
|
const workspaces = await this.api.workspaces(project.id, machineId);
|
||||||
const current = this.getState();
|
const current = this.getState();
|
||||||
@@ -107,6 +114,7 @@ export class WorkspaceController {
|
|||||||
} catch (error) {
|
} catch (error) {
|
||||||
this.onBackgroundError(`Failed to refresh workspaces for project ${project.id} on ${machineId}`, error);
|
this.onBackgroundError(`Failed to refresh workspaces for project ${project.id} on ${machineId}`, error);
|
||||||
}
|
}
|
||||||
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
async refreshAfterWorkspaceDeleted(projectId: string, workspaceId: string): Promise<void> {
|
async refreshAfterWorkspaceDeleted(projectId: string, workspaceId: string): Promise<void> {
|
||||||
|
|||||||
Reference in New Issue
Block a user