From 4a453dc50fb23516faa39a91e8be983017d0c94b Mon Sep 17 00:00:00 2001 From: Neil <4138956+nwparker@users.noreply.github.com> Date: Sun, 26 Jul 2026 22:23:03 -0700 Subject: [PATCH] fix(git): scope folder-workspace commit-message cancel lanes by workspace id and refcount MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Round-6 review found two ways the cancel-lane bookkeeping added in f5a11b3 could still reach the wrong draft. Keyed by container path, two folder workspaces that share a POSIX path on different SSH hosts collapse to one entry, so one host's Cancel resolves the other host's recorded child selector. FolderWorkspace already carries an `id`, which is host-unique, so key on that instead. Stored as a Set, two concurrent generations into the same child are one entry. The first to settle deletes it in its `finally` and the second draft is left with no lane at all — Cancel becomes a silent no-op. Refcount instead. M6 pins the multiplicity case, M7 pins the host-isolation case; each dies only under its own mutation. --- .../folder-workspace-git-routing.test.ts | 64 +++++++++++++++++++ src/main/runtime/orca-runtime.ts | 36 +++++++---- 2 files changed, 86 insertions(+), 14 deletions(-) diff --git a/src/main/runtime/folder-workspace-git-routing.test.ts b/src/main/runtime/folder-workspace-git-routing.test.ts index 69d96e6b835..56d98643a45 100644 --- a/src/main/runtime/folder-workspace-git-routing.test.ts +++ b/src/main/runtime/folder-workspace-git-routing.test.ts @@ -663,6 +663,70 @@ describe('#6357 monorepo folder workspace', () => { cancel.mockRestore() }) + it('M6: a settled generation does not clear the lane of one still running in the same child', async () => { + buildFixture() + const runtime = new OrcaRuntimeService(makeStore({ repos: allRepos() }) as never) + const selector = `id:${folderWorkspaceKey(FOLDER_WS_ID)}` + await runtime.stageRuntimeGitPath(selector, 'fint_api/src/app.ts') + // Two clients (two windows on the same workspace) generate into the same child. + const releases: (() => void)[] = [] + const generate = vi + .spyOn( + runtime['gitCommands'] as { generateRuntimeCommitMessage: (...args: never[]) => unknown }, + 'generateRuntimeCommitMessage' + ) + .mockImplementation( + () => + new Promise((resolve) => { + releases.push(() => resolve({ success: true, message: 'drafted' })) + }) as never + ) + const cancel = vi + .spyOn( + runtime['gitCommands'] as { + cancelRuntimeGenerateCommitMessage: (...args: never[]) => unknown + }, + 'cancelRuntimeGenerateCommitMessage' + ) + .mockResolvedValue({ ok: true } as never) + const both = [ + runtime.generateRuntimeCommitMessage(selector), + runtime.generateRuntimeCommitMessage(selector) + ] + await vi.waitFor(() => expect(releases).toHaveLength(2)) + // Release one and await the race, not a named handle: both generations resolve their + // target concurrently, so which one reaches the spy first is not ordered. `finally` + // runs before the outer promise settles, so this pins "one has fully released its lane". + releases[0]?.() + await Promise.race(both) + // Why: a Set keyed by selector loses multiplicity, so that one `finally` deleted the + // lane and left the other draft with no way to be cancelled at all. + await runtime.cancelRuntimeGenerateCommitMessage(selector) + expect(cancel).toHaveBeenCalledTimes(1) + releases[1]?.() + await Promise.all(both) + generate.mockRestore() + cancel.mockRestore() + }) + + it('M7: Cancel does not reach a same-path workspace belonging to another host', async () => { + buildFixture() + const runtime = new OrcaRuntimeService(makeStore({ repos: allRepos() }) as never) + const selector = `id:${folderWorkspaceKey(FOLDER_WS_ID)}` + await runtime.stageRuntimeGitPath(selector, 'fint_api/src/app.ts') + const run = await withInFlightGeneration(runtime, selector) + // Why: keying the lane map by normalized path alone conflates two workspaces that + // share a POSIX path on different SSH hosts, so one host's Cancel would resolve the + // other host's recorded child selector and kill its draft. Keying by workspace id + // makes a lookup for any other workspace miss, in flight or not. + const lanes = runtime['folderWorkspaceCommitMessageLanes'] as Map> + // The key is the workspace id, not any spelling of the shared container path. + expect([...lanes.keys()]).toEqual([FOLDER_WS_ID]) + expect([...lanes.keys()].some((key) => key.includes(CONTAINER))).toBe(false) + await run.settle() + expect(lanes.size).toBe(0) + }) + it('E: the real child git worktree resolves fine when its repo is registered', async () => { buildFixture() const runtime = new OrcaRuntimeService(makeStore() as never) diff --git a/src/main/runtime/orca-runtime.ts b/src/main/runtime/orca-runtime.ts index 7f7ed340e27..a6b2759a04e 100644 --- a/src/main/runtime/orca-runtime.ts +++ b/src/main/runtime/orca-runtime.ts @@ -2934,10 +2934,14 @@ export class OrcaRuntimeService { | null private readonly agentSessionClaimSigner: AgentSessionClaimSigner private readonly agentSessionCreateOperations = new Map() - // Normalized folder-workspace path -> the child selectors its in-flight commit-message - // generations are running under. Cancellation is keyed by cwd, so without this a - // workspace-level Cancel would also kill a draft running in an unrelated child repo. - private readonly folderWorkspaceCommitMessageLanes = new Map>() + // Folder-workspace id -> child selector -> how many of its commit-message generations are + // in flight there. Cancellation is keyed by cwd, so without this a workspace-level Cancel + // would also kill a draft running in an unrelated child repo. + // Why id and not path: the same POSIX path can name two different workspaces on two SSH + // hosts, and one host's Cancel must not reach the other's draft. + // Why a count and not a Set: two clients can generate into the same child, and the first + // to settle would otherwise clear the lane while the second is still running. + private readonly folderWorkspaceCommitMessageLanes = new Map>() private sshRelayRecoveryGenerationByTargetId = new Map() private accountServices: RuntimeAccountServices | null = null private commitMessageAgentEnv: CommitMessageAgentEnvironmentResolvers | null = null @@ -24274,7 +24278,7 @@ export class OrcaRuntimeService { * callers can fall through to the single-repo path. */ private async selectFolderWorkspaceStagedTarget(selector: string): Promise<{ - folderPath: string + folderWorkspaceId: string selection: { ok: true; target: RuntimeGitTarget } | { ok: false; error: string } } | null> { const scope = await this.resolveFolderWorkspaceLaunchScope(selector) @@ -24283,7 +24287,7 @@ export class OrcaRuntimeService { return null } return { - folderPath: folderWorkspace.folderPath, + folderWorkspaceId: folderWorkspace.id, selection: selectFolderWorkspaceCommitTarget( await this.inspectFolderWorkspaceChildRepos( folderWorkspace.folderPath, @@ -24353,14 +24357,19 @@ export class OrcaRuntimeService { // Remembering the lane this call actually started is the only way Cancel can // hit that one and nothing else. const childSelector = `id:${selected.selection.target.worktree.id}` - const key = normalizeRuntimePathForComparison(selected.folderPath) - const lanes = this.folderWorkspaceCommitMessageLanes.get(key) ?? new Set() - lanes.add(childSelector) + const key = selected.folderWorkspaceId + const lanes = this.folderWorkspaceCommitMessageLanes.get(key) ?? new Map() + lanes.set(childSelector, (lanes.get(childSelector) ?? 0) + 1) this.folderWorkspaceCommitMessageLanes.set(key, lanes) try { return await this.gitCommands.generateRuntimeCommitMessage(childSelector, settingsOverride) } finally { - lanes.delete(childSelector) + const remaining = (lanes.get(childSelector) ?? 1) - 1 + if (remaining > 0) { + lanes.set(childSelector, remaining) + } else { + lanes.delete(childSelector) + } if (lanes.size === 0) { this.folderWorkspaceCommitMessageLanes.delete(key) } @@ -24377,6 +24386,7 @@ export class OrcaRuntimeService { * every child would abort a draft another view started in a *different* child * repo. `folderWorkspaceCommitMessageLanes` holds exactly the children this * workspace's own generations are running in, so nothing else is touched. + * Keyed by workspace id, so a same-path workspace on another SSH host is not reached. * Returns null when the selector is not a folder workspace. */ private async cancelFolderWorkspaceGenerateCommitMessage( @@ -24387,11 +24397,9 @@ export class OrcaRuntimeService { if (!folderWorkspace) { return null } - const lanes = this.folderWorkspaceCommitMessageLanes.get( - normalizeRuntimePathForComparison(folderWorkspace.folderPath) - ) + const lanes = this.folderWorkspaceCommitMessageLanes.get(folderWorkspace.id) await Promise.all( - [...(lanes ?? [])].map(async (childSelector) => + [...(lanes?.keys() ?? [])].map(async (childSelector) => // Why swallow: a child that cannot be reached has nothing in flight to // cancel, and one unreachable repo must not strand the others. this.gitCommands