diff --git a/src/agents/worktrees/service.ts b/src/agents/worktrees/service.ts index 89b355e08b4d..6e47c17e1d78 100644 --- a/src/agents/worktrees/service.ts +++ b/src/agents/worktrees/service.ts @@ -770,6 +770,11 @@ export class ManagedWorktreeService { return findLiveRegistryWorktreeByOwner(this.env, ownerKind, ownerId); } + findLiveById(id: string): ManagedWorktreeRecord | undefined { + const record = getRegistryWorktree(this.env, id); + return record?.removedAt === undefined ? record : undefined; + } + /** Resolves the canonical registry root and the caller's own checkout root. */ async resolveRepositoryPaths(repoRoot: string): Promise<{ canonicalRoot: string; diff --git a/src/gateway/server-methods/sessions-create.ts b/src/gateway/server-methods/sessions-create.ts index 4afd883614b6..c83df2fac565 100644 --- a/src/gateway/server-methods/sessions-create.ts +++ b/src/gateway/server-methods/sessions-create.ts @@ -3,6 +3,7 @@ import { randomUUID } from "node:crypto"; import fs from "node:fs"; import path from "node:path"; import { expectDefined } from "@openclaw/normalization-core"; +import { err, ok as resultOk } from "@openclaw/normalization-core/result"; import { normalizeOptionalString } from "@openclaw/normalization-core/string-coerce"; import { ErrorCodes, @@ -29,6 +30,7 @@ import { resolveUserPath } from "../../utils.js"; import { generateDashboardSessionTitle } from "../dashboard-session-title.js"; import { ADMIN_SCOPE, authorizeOperatorScopesForRequiredScope } from "../method-scopes.js"; import { buildDashboardSessionKey, createGatewaySession } from "../session-create-service.js"; +import type { PrepareGatewaySessionLifecycle } from "../session-lifecycle-preparation.js"; import { resolveRequestedSessionAgentId as resolveRequestedGlobalAgentId } from "../session-request-agent.js"; import { resolveSessionStoreAgentId } from "../session-store-key.js"; import { readSessionMessageCountAsync } from "../session-transcript-readers.js"; @@ -202,8 +204,7 @@ export const sessionCreateHandlers: GatewayRequestHandlers = { let sessionWorktree: Awaited> | undefined; const sessionExecCwd = requestedExecNode ? requestedCwd : undefined; let sessionCwd = requestedExecNode ? undefined : requestedCwd; - let sessionSourceRoot: string | undefined; - let provisionedSessionWorktree = false; + let prepareLifecycle: PrepareGatewaySessionLifecycle | undefined; let generatedDisplayName: string | undefined; if (requestedCwd && !requestedExecNode && p.worktree !== true) { const targetAgentId = normalizeAgentId( @@ -292,107 +293,9 @@ export const sessionCreateHandlers: GatewayRequestHandlers = { ); return; } + let requestedRepository: Awaited>; try { - const requestedRepository = await managedWorktrees.resolveRepositoryPaths(workspace); - sessionSourceRoot = requestedRepository.sourceRoot; - const existing = managedWorktrees.findLiveByOwner("session", target.canonicalKey); - let existingDirectory = false; - if (existing) { - try { - existingDirectory = fs.lstatSync(existing.path).isDirectory(); - } catch { - // Missing registry targets are replaced; periodic GC retires their stale rows. - } - } - if (existing && existingDirectory) { - if (existing.repoRoot !== requestedRepository.canonicalRoot) { - respond( - false, - undefined, - errorShape( - ErrorCodes.INVALID_REQUEST, - "session worktree belongs to a different repository", - ), - ); - return; - } - // Adopting an existing checkout cannot honor a different name or a - // new base; fail loudly instead of silently ignoring the request. - if ( - (requestedWorktreeName && existing.name !== requestedWorktreeName) || - requestedWorktreeBaseRef - ) { - respond( - false, - undefined, - errorShape( - ErrorCodes.INVALID_REQUEST, - `session is already bound to worktree ${existing.name} (${existing.branch})`, - ), - ); - return; - } - sessionWorktree = existing; - } else { - const scopes = Array.isArray(client?.connect.scopes) ? client.connect.scopes : []; - if ( - !requestedWorktreeName && - !normalizeOptionalString(p.label) && - (initialMessage || initialAttachments) - ) { - try { - const requestedTitleModel = - catalogTarget?.target.model ?? normalizeOptionalString(p.model); - let titleModelEntry: - | Pick - | undefined; - if (requestedTitleModel) { - const defaultModel = resolveDefaultModelForAgent({ - cfg, - agentId: target.agentId, - }); - const selection = resolveSessionPatchModelSelection({ - cfg, - catalog: await context.loadGatewayModelCatalog({ agentId: target.agentId }), - raw: requestedTitleModel, - defaultProvider: defaultModel.provider, - defaultModel: defaultModel.model, - }); - if (selection.ok) { - titleModelEntry = { - providerOverride: selection.provider, - modelOverride: selection.model, - ...(selection.profile ? { authProfileOverride: selection.profile } : {}), - }; - } - } - generatedDisplayName = - (await generateDashboardSessionTitle({ - cfg, - agentId: target.agentId, - entry: titleModelEntry, - userMessage: initialMessage ?? "", - attachments: initialAttachments, - })) ?? undefined; - } catch (error) { - sessionLog.warn(`worktree title generation failed: ${formatErrorMessage(error)}`); - } - } - sessionWorktree = await managedWorktrees.create({ - repoRoot: workspace, - ownerKind: "session", - ownerId: target.canonicalKey, - name: requestedWorktreeName, - suggestedName: slugifyWorktreeTitle( - normalizeOptionalString(p.label) ?? generatedDisplayName ?? "", - ), - baseRef: requestedWorktreeBaseRef, - // Checkout hooks and .openclaw/worktree-setup.sh run repo code; keep them - // admin-only so this write-scoped path cannot execute gated repo scripts. - runSetupScript: scopes.includes(ADMIN_SCOPE), - }); - provisionedSessionWorktree = true; - } + requestedRepository = await managedWorktrees.resolveRepositoryPaths(workspace); } catch (error) { if (error instanceof WorktreeRepositoryError) { respond( @@ -405,22 +308,151 @@ export const sessionCreateHandlers: GatewayRequestHandlers = { respond(false, undefined, errorShape(ErrorCodes.UNAVAILABLE, formatErrorMessage(error))); return; } - // Nested workspaces run from the matching subdirectory inside the worktree, mirroring - // how the session would have run in the source checkout; the worktree root would - // silently change tool/file scope for subdirectory-configured agents. - sessionCwd = sessionWorktree.path; - try { - const relative = path.relative( - sessionSourceRoot ?? fs.realpathSync(sessionWorktree.repoRoot), - fs.realpathSync(workspace), - ); - if (relative && !relative.startsWith("..") && !path.isAbsolute(relative)) { - sessionCwd = path.join(sessionWorktree.path, relative); - fs.mkdirSync(sessionCwd, { recursive: true }); + + if ( + !requestedWorktreeName && + !normalizeOptionalString(p.label) && + (initialMessage || initialAttachments) + ) { + try { + const requestedTitleModel = + catalogTarget?.target.model ?? normalizeOptionalString(p.model); + let titleModelEntry: + | Pick + | undefined; + if (requestedTitleModel) { + const defaultModel = resolveDefaultModelForAgent({ cfg, agentId: target.agentId }); + const selection = resolveSessionPatchModelSelection({ + cfg, + catalog: await context.loadGatewayModelCatalog({ agentId: target.agentId }), + raw: requestedTitleModel, + defaultProvider: defaultModel.provider, + defaultModel: defaultModel.model, + }); + if (selection.ok) { + titleModelEntry = { + providerOverride: selection.provider, + modelOverride: selection.model, + ...(selection.profile ? { authProfileOverride: selection.profile } : {}), + }; + } + } + generatedDisplayName = + (await generateDashboardSessionTitle({ + cfg, + agentId: target.agentId, + entry: titleModelEntry, + userMessage: initialMessage ?? "", + attachments: initialAttachments, + })) ?? undefined; + } catch (error) { + sessionLog.warn(`worktree title generation failed: ${formatErrorMessage(error)}`); } - } catch { - sessionCwd = sessionWorktree.path; } + + const scopes = Array.isArray(client?.connect.scopes) ? client.connect.scopes : []; + prepareLifecycle = async (lifecycleTarget) => { + try { + const boundId = normalizeOptionalString(lifecycleTarget.entry?.worktree?.id); + let existing = boundId ? managedWorktrees.findLiveById(boundId) : undefined; + if ( + existing && + (existing.ownerKind !== "session" || existing.ownerId !== lifecycleTarget.key) + ) { + return err( + errorShape(ErrorCodes.UNAVAILABLE, "session worktree binding has a different owner"), + ); + } + existing ??= managedWorktrees.findLiveByOwner("session", lifecycleTarget.key); + let existingDirectory = false; + if (existing) { + try { + existingDirectory = fs.lstatSync(existing.path).isDirectory(); + } catch { + // Missing registry targets are replaced by create() under its owner lease. + } + } + let provisioned = false; + if (existing && existingDirectory) { + if (existing.repoRoot !== requestedRepository.canonicalRoot) { + return err( + errorShape( + ErrorCodes.INVALID_REQUEST, + "session worktree belongs to a different repository", + ), + ); + } + if ( + (requestedWorktreeName && existing.name !== requestedWorktreeName) || + requestedWorktreeBaseRef + ) { + return err( + errorShape( + ErrorCodes.INVALID_REQUEST, + `session is already bound to worktree ${existing.name} (${existing.branch})`, + ), + ); + } + sessionWorktree = existing; + } else { + sessionWorktree = await managedWorktrees.create({ + repoRoot: workspace, + ownerKind: "session", + ownerId: lifecycleTarget.key, + name: requestedWorktreeName, + suggestedName: slugifyWorktreeTitle( + normalizeOptionalString(p.label) ?? generatedDisplayName ?? "", + ), + baseRef: requestedWorktreeBaseRef, + // Checkout hooks and .openclaw/worktree-setup.sh run repo code; keep them + // admin-only so this write-scoped path cannot execute gated repo scripts. + runSetupScript: scopes.includes(ADMIN_SCOPE), + }); + provisioned = true; + } + // Nested workspaces run from the matching subdirectory inside the worktree. + sessionCwd = sessionWorktree.path; + try { + const relative = path.relative( + requestedRepository.sourceRoot, + fs.realpathSync(workspace), + ); + if (relative && !relative.startsWith("..") && !path.isAbsolute(relative)) { + sessionCwd = path.join(sessionWorktree.path, relative); + fs.mkdirSync(sessionCwd, { recursive: true }); + } + } catch { + sessionCwd = sessionWorktree.path; + } + const preparedWorktree = sessionWorktree; + return resultOk({ + spawnedCwd: sessionCwd, + worktree: { + id: preparedWorktree.id, + branch: preparedWorktree.branch, + repoRoot: preparedWorktree.repoRoot, + }, + ...(provisioned + ? { + rollback: async () => { + await managedWorktrees.remove({ + id: preparedWorktree.id, + reason: "session-create-failed", + force: true, + }); + }, + } + : {}), + }); + } catch (error) { + if (error instanceof WorktreeRepositoryError) { + return err( + errorShape(ErrorCodes.INVALID_REQUEST, "agent workspace is not a git checkout"), + ); + } + return err(errorShape(ErrorCodes.UNAVAILABLE, formatErrorMessage(error))); + } + }; } let runPayload: Record | undefined; let runError: unknown; @@ -498,19 +530,18 @@ export const sessionCreateHandlers: GatewayRequestHandlers = { : {}), } : undefined, - spawnedCwd: sessionCwd, - worktree: sessionWorktree - ? { - id: sessionWorktree.id, - branch: sessionWorktree.branch, - repoRoot: sessionWorktree.repoRoot, - } - : undefined, + spawnedCwd: p.worktree === true ? undefined : sessionCwd, + prepareLifecycle, + onLifecycleCleanupError: (error) => { + sessionLog.warn( + `failed to finalize session worktree lifecycle: ${formatErrorMessage(error)}`, + ); + }, execNode: requestedExecNode, execCwd: sessionExecCwd, clearExecBinding: !requestedExecNode, // A plain New Chat with no cwd must not inherit the prior session cwd. - clearSpawnedCwd: !sessionCwd, + clearSpawnedCwd: p.worktree !== true && !sessionCwd, fork: p.fork, succeedsParent: p.succeedsParent, emitCommandHooks: p.emitCommandHooks, @@ -559,36 +590,9 @@ export const sessionCreateHandlers: GatewayRequestHandlers = { }, }); if (!created.ok) { - if (sessionWorktree && provisionedSessionWorktree) { - try { - await managedWorktrees.remove({ - id: sessionWorktree.id, - reason: "session-create-failed", - force: true, - }); - } catch (error) { - sessionLog.warn( - `failed to clean up worktree after session creation failed: ${formatErrorMessage(error)}`, - ); - } - } respond(false, undefined, created.error); return; } - // Leaving an isolated checkout via a plain New Chat detaches the session from its - // worktree; remove it when lossless so the reset does not orphan a protected worktree. - if (p.worktree !== true) { - try { - const owned = managedWorktrees.findLiveByOwner("session", created.key); - if (owned) { - await managedWorktrees.removeIfLossless(owned.id); - } - } catch (error) { - sessionLog.warn( - `failed to release worktree for reset session ${created.key}: ${formatErrorMessage(error)}`, - ); - } - } if (created.resetExisting) { await captureCreatedSessionBaseline({ key: created.key, diff --git a/src/gateway/server-methods/sessions-delete.ts b/src/gateway/server-methods/sessions-delete.ts index ec2db30648fd..cbac31e36959 100644 --- a/src/gateway/server-methods/sessions-delete.ts +++ b/src/gateway/server-methods/sessions-delete.ts @@ -209,6 +209,8 @@ export const sessionDeleteHandlers: GatewayRequestHandlers = { let deleteBlockedByWorkerPlacement = false; let deleteBlockedByArchiveOrOwnership = false; let preparedDeleteSessionId: string | undefined; + let deletedWorktreeId: string | undefined; + let worktreePreserved: { id: string; branch: string; path: string } | undefined; const deletion = await runExclusiveSessionLifecycleMutation({ scope: storePath, identities: deleteLifecycleIdentities, @@ -381,6 +383,7 @@ export const sessionDeleteHandlers: GatewayRequestHandlers = { ...(requestedAgentId ? { agentId: requestedAgentId } : {}), }); const postCleanupEntry = postCleanupTarget.entry; + deletedWorktreeId = normalizeOptionalString(postCleanupEntry?.worktree?.id); sessionMutationAuthorization?.assertCurrent(); if ( !expectedLifecycleRevisionMatches(postCleanupEntry) || @@ -440,6 +443,44 @@ export const sessionDeleteHandlers: GatewayRequestHandlers = { reason: "session-delete", emitHooks: p.emitLifecycleHooks !== false, }); + // Hooks and unbinding retain their historical post-delete order. The + // generation-scoped purge and checkout cleanup still finish before + // this fence opens, so a same-key successor cannot be mistaken for it. + const deletedSessionKey = target.canonicalKey ?? key; + handleSessionStateSessionDeleted( + deletedSessionKey, + requestedAgentId ?? resolveSessionStoreAgentId(cfg, deletedSessionKey), + ); + const deletedWorktree = deletedWorktreeId + ? managedWorktrees.findLiveById(deletedWorktreeId) + : undefined; + if (deletedWorktree) { + worktreePreserved = { + id: deletedWorktree.id, + branch: deletedWorktree.branch, + path: deletedWorktree.path, + }; + if ( + deletedWorktree.ownerKind !== "session" || + deletedWorktree.ownerId !== deletedSessionKey + ) { + sessionLog.warn( + `refusing to clean up worktree ${deletedWorktree.id} for deleted session ${deletedSessionKey}: registry owner is ${deletedWorktree.ownerKind}${deletedWorktree.ownerId ? ` ${deletedWorktree.ownerId}` : ""}`, + ); + } else { + try { + await managedWorktrees.remove({ + id: deletedWorktree.id, + reason: "session-delete", + }); + worktreePreserved = undefined; + } catch (error) { + sessionLog.warn( + `failed to clean up worktree for deleted session ${deletedSessionKey}: ${formatErrorMessage(error)}`, + ); + } + } + } } return result; }, @@ -451,32 +492,6 @@ export const sessionDeleteHandlers: GatewayRequestHandlers = { const archivedTranscripts = deletion.archivedTranscripts; const archived = archivedTranscripts.map((entryLocal) => entryLocal.archivedPath); - // Session deletion ends worktree ownership. Snapshot before removal so - // inherited unpushed history or local edits do not leave an ownerless checkout. - let worktreePreserved: { id: string; branch: string; path: string } | undefined; - if (deleted) { - // requestedAgentId wins: "global" canonical keys resolve to the default store - // agent, which would purge the wrong agent's rows for explicit-agent deletes. - handleSessionStateSessionDeleted( - target.canonicalKey ?? key, - requestedAgentId ?? resolveSessionStoreAgentId(cfg, target.canonicalKey ?? key), - ); - let worktree: ReturnType = undefined; - try { - worktree = managedWorktrees.findLiveByOwner("session", target.canonicalKey); - if (worktree) { - await managedWorktrees.remove({ id: worktree.id, reason: "session-delete" }); - } - } catch (error) { - if (worktree) { - worktreePreserved = { id: worktree.id, branch: worktree.branch, path: worktree.path }; - } - sessionLog.warn( - `failed to clean up worktree for deleted session ${target.canonicalKey}: ${formatErrorMessage(error)}`, - ); - } - } - respond( true, { diff --git a/src/gateway/server-methods/task-suggestions.test.ts b/src/gateway/server-methods/task-suggestions.test.ts index ae169e53dd41..f455b209957a 100644 --- a/src/gateway/server-methods/task-suggestions.test.ts +++ b/src/gateway/server-methods/task-suggestions.test.ts @@ -1,6 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { GATEWAY_CLIENT_CAPS } from "../../../packages/gateway-protocol/src/client-info.js"; -import { managedWorktrees } from "../../agents/worktrees/service.js"; import { upsertSessionEntry } from "../../config/sessions/session-accessor.js"; import { clearAgentRunContext, registerAgentRunContext } from "../../infra/agent-run-registry.js"; import { closeOpenClawAgentDatabasesForTest } from "../../state/openclaw-agent-db.js"; @@ -17,8 +16,24 @@ import { taskSuggestionsHandlers } from "./task-suggestions.js"; import type { GatewayClient, GatewayRequestContext, RespondFn } from "./types.js"; const mocks = vi.hoisted(() => ({ handleChatSend: vi.fn() })); +const sessionReadState = vi.hoisted(() => ({ mode: "normal" as "normal" | "present" | "throw" })); vi.mock("./chat-send-handler.js", () => ({ handleChatSend: mocks.handleChatSend })); +vi.mock("../session-utils.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + loadSessionEntryReadOnly: (...args: Parameters) => { + if (sessionReadState.mode === "throw") { + throw new Error("session inspection unavailable"); + } + const loaded = actual.loadSessionEntryReadOnly(...args); + return sessionReadState.mode === "present" + ? { ...loaded, entry: { sessionId: "surviving-session", updatedAt: 1 } } + : loaded; + }, + }; +}); type Method = | "taskSuggestions.list" @@ -70,6 +85,7 @@ async function dismissPendingTaskSuggestions(): Promise { } beforeEach(async () => { + sessionReadState.mode = "normal"; await dismissPendingTaskSuggestions(); mocks.handleChatSend.mockReset(); mocks.handleChatSend.mockImplementation(async ({ respond }: { respond: RespondFn }) => { @@ -122,6 +138,18 @@ async function createSourceSuggestion() { return (requirePayload(created) as { taskId: string }).taskId; } +async function createLocalTaskSuggestion() { + const created = await call("taskSuggestions.create", { + title: "Add coverage", + prompt: "Add the missing regression test.", + tldr: "The edge case is untested.", + cwd: GIT_CWD, + sessionKey: "agent:main:main", + agentId: "main", + }); + return (requirePayload(created) as { taskId: string }).taskId; +} + describe("task suggestion gateway methods", () => { it("creates, lists, and resolves an ephemeral suggestion", async () => { const created = await call("taskSuggestions.create", { @@ -688,15 +716,7 @@ describe("task suggestion gateway methods", () => { }); it("rolls back an empty session and keeps a failed seed suggestion pending", async () => { - const created = await call("taskSuggestions.create", { - title: "Add coverage", - prompt: "Add the missing regression test.", - tldr: "The edge case is untested.", - cwd: GIT_CWD, - sessionKey: "agent:main:main", - agentId: "main", - }); - const taskId = (requirePayload(created) as { taskId: string }).taskId; + const taskId = await createLocalTaskSuggestion(); let sessionKey = ""; vi.spyOn(sessionCreateHandlers, "sessions.create").mockImplementation( async ({ params, respond }) => { @@ -741,55 +761,71 @@ describe("task suggestion gateway methods", () => { expect(listed.response?.[1]).toMatchObject({ suggestions: [{ id: taskId }] }); }); - it("rolls back a preallocated session when creation throws after persistence", async () => { - const created = await call("taskSuggestions.create", { - title: "Add coverage", - prompt: "Add the missing regression test.", - tldr: "The edge case is untested.", - cwd: GIT_CWD, - sessionKey: "agent:main:main", - agentId: "main", - }); - const taskId = (requirePayload(created) as { taskId: string }).taskId; - let sessionKey = ""; - vi.spyOn(sessionCreateHandlers, "sessions.create").mockImplementation(async ({ params }) => { - sessionKey = (params as { key: string }).key; - throw new Error("initial dispatch failed"); - }); - const deleteSession = vi - .spyOn(sessionDeleteHandlers, "sessions.delete") - .mockImplementation(async ({ params, respond }) => { - expect(params).toMatchObject({ key: sessionKey, agentId: "main" }); - respond(true, { ok: true, deleted: true }, undefined); + it.each([true, false])( + "rolls back a preallocated session when delete reports deleted=$deleted", + async (deleted) => { + const taskId = await createLocalTaskSuggestion(); + let sessionKey = ""; + vi.spyOn(sessionCreateHandlers, "sessions.create").mockImplementation(async ({ params }) => { + sessionKey = (params as { key: string }).key; + throw new Error("initial dispatch failed"); }); + const deleteSession = vi + .spyOn(sessionDeleteHandlers, "sessions.delete") + .mockImplementation(async ({ params, respond }) => { + expect(params).toMatchObject({ key: sessionKey, agentId: "main" }); + respond(true, { ok: true, deleted }, undefined); + }); - const accepted = await call("taskSuggestions.accept", { taskId }); - const listed = await call("taskSuggestions.list", {}); + const accepted = await call("taskSuggestions.accept", { taskId }); + const listed = await call("taskSuggestions.list", {}); - expect(accepted.response?.[0]).toBe(false); - expect(accepted.response?.[2]).toMatchObject({ message: "initial dispatch failed" }); - expect(deleteSession).toHaveBeenCalledTimes(1); - expect(listed.response?.[1]).toMatchObject({ suggestions: [{ id: taskId }] }); - }); + expect(accepted.response?.[0]).toBe(false); + expect(accepted.response?.[2]).toMatchObject({ message: "initial dispatch failed" }); + expect(deleteSession).toHaveBeenCalledTimes(1); + expect(listed.response?.[1]).toMatchObject({ suggestions: [{ id: taskId }] }); + }, + ); - it("expires a suggestion when partial session rollback cannot finish", async () => { - const created = await call("taskSuggestions.create", { - title: "Add coverage", - prompt: "Add the missing regression test.", - tldr: "The edge case is untested.", - cwd: GIT_CWD, - sessionKey: "agent:main:main", - agentId: "main", - }); - const taskId = (requirePayload(created) as { taskId: string }).taskId; + it.each([ + ["delete rejects", "reject"], + ["delete throws", "throw"], + ["the session row survives", "survives"], + ["delete preserves the worktree", "preserved"], + ["session inspection throws", "inspect-throws"], + ] as const)("expires a suggestion when rollback is incomplete: %s", async (_name, failure) => { + const taskId = await createLocalTaskSuggestion(); vi.spyOn(sessionCreateHandlers, "sessions.create").mockRejectedValue( new Error("initial dispatch failed"), ); + sessionReadState.mode = + failure === "survives" ? "present" : failure === "inspect-throws" ? "throw" : "normal"; vi.spyOn(sessionDeleteHandlers, "sessions.delete").mockImplementation(async ({ respond }) => { - respond(false, undefined, { code: "UNAVAILABLE", message: "still active" }); + if (failure === "throw") { + throw new Error("delete handler failed"); + } + if (failure === "reject") { + respond(false, undefined, { code: "UNAVAILABLE", message: "still active" }); + return; + } + respond( + true, + { + ok: true, + deleted: true, + ...(failure === "preserved" + ? { + worktreePreserved: { + id: "preserved-worktree", + path: "/preserved-worktree", + branch: "openclaw/preserved-worktree", + }, + } + : {}), + }, + undefined, + ); }); - vi.spyOn(managedWorktrees, "findLiveByOwner").mockReturnValue({ id: "wt_partial" } as never); - vi.spyOn(managedWorktrees, "remove").mockRejectedValue(new Error("still active")); const accepted = await call("taskSuggestions.accept", { taskId }); const listed = await call("taskSuggestions.list", {}); @@ -804,47 +840,6 @@ describe("task suggestion gateway methods", () => { expect(listed.response?.[1]).toEqual({ suggestions: [] }); }); - it("abandons an acceptance when rollback inspection throws", async () => { - const created = await call("taskSuggestions.create", { - title: "Add coverage", - prompt: "Add the missing regression test.", - tldr: "The edge case is untested.", - cwd: GIT_CWD, - sessionKey: "agent:main:main", - agentId: "main", - }); - const taskId = (requirePayload(created) as { taskId: string }).taskId; - vi.spyOn(sessionCreateHandlers, "sessions.create").mockRejectedValue( - new Error("initial dispatch failed"), - ); - vi.spyOn(sessionDeleteHandlers, "sessions.delete").mockImplementation(async ({ respond }) => { - respond(true, { ok: true, deleted: true }, undefined); - }); - vi.spyOn(managedWorktrees, "findLiveByOwner").mockImplementation(() => { - throw new Error("worktree registry unavailable"); - }); - const broadcast = vi.fn(); - - await expect(call("taskSuggestions.accept", { taskId }, broadcast)).rejects.toThrow( - "worktree registry unavailable", - ); - - expect(broadcast).toHaveBeenCalledTimes(1); - expect(broadcast).toHaveBeenCalledWith( - "task.suggestion", - { action: "resolved", taskId, resolution: "expired" }, - { dropIfSlow: true }, - ); - const listed = await call("taskSuggestions.list", {}); - expect(listed.response?.[1]).toEqual({ suggestions: [] }); - const retry = await call("taskSuggestions.accept", { taskId }); - expect(retry.response?.[0]).toBe(false); - expect(retry.response?.[2]).toMatchObject({ - code: "INVALID_REQUEST", - message: "task suggestion cannot be accepted: dismissed", - }); - }); - it("rejects a relative cwd before recording or broadcasting", async () => { const result = await call("taskSuggestions.create", { title: "Add coverage", diff --git a/src/gateway/server-methods/task-suggestions.ts b/src/gateway/server-methods/task-suggestions.ts index 0b4a07c8b4b9..622fad48a1e5 100644 --- a/src/gateway/server-methods/task-suggestions.ts +++ b/src/gateway/server-methods/task-suggestions.ts @@ -14,7 +14,6 @@ import { } from "../../../packages/gateway-protocol/src/index.js"; import { resolveDefaultAgentId } from "../../agents/agent-scope.js"; import { insideGitCheckout } from "../../agents/worktrees/git.js"; -import { managedWorktrees } from "../../agents/worktrees/service.js"; import { resolveSessionWorkStartError } from "../../config/sessions.js"; import { formatErrorMessage } from "../../infra/errors.js"; import { normalizeAgentId, parseAgentSessionKey } from "../../routing/session-key.js"; @@ -70,9 +69,13 @@ async function rollbackSuggestedTaskSession(params: { agentId?: string; options: GatewayRequestHandlerOptions; }): Promise { - let deletionConfirmed = false; + let deletionResponse: { ok: true; worktreePreserved: boolean } | { ok: false } | undefined; try { - await sessionDeleteHandlers["sessions.delete"]?.({ + const deleteSession = sessionDeleteHandlers["sessions.delete"]; + if (!deleteSession) { + return false; + } + await deleteSession({ ...params.options, params: { key: params.key, @@ -81,41 +84,33 @@ async function rollbackSuggestedTaskSession(params: { emitLifecycleHooks: false, }, respond: (ok, payload) => { - deletionConfirmed = Boolean( - ok && - payload && - typeof payload === "object" && - typeof (payload as { deleted?: unknown }).deleted === "boolean", - ); + if ( + !ok || + !payload || + typeof payload !== "object" || + typeof (payload as { deleted?: unknown }).deleted !== "boolean" + ) { + deletionResponse = { ok: false }; + return; + } + deletionResponse = { + ok: true, + worktreePreserved: + (payload as { worktreePreserved?: unknown }).worktreePreserved !== undefined, + }; }, }); } catch { - // The state probes below determine whether the preallocated session key - // and its worktree were fully removed despite a handler-level failure. + return false; + } + if (!deletionResponse?.ok || deletionResponse.worktreePreserved) { + return false; } try { - if ( - !deletionConfirmed && - loadSessionEntryReadOnly(params.key, { agentId: params.agentId }).entry - ) { - return false; - } + return !loadSessionEntryReadOnly(params.key, { agentId: params.agentId }).entry; } catch { return false; } - const worktree = managedWorktrees.findLiveByOwner("session", params.key); - if (worktree) { - try { - await managedWorktrees.remove({ - id: worktree.id, - reason: "suggested-task-seed-failed", - force: true, - }); - } catch { - return false; - } - } - return managedWorktrees.findLiveByOwner("session", params.key) === undefined; } async function failSuggestedTaskSession(params: { diff --git a/src/gateway/server.sessions.create.test.ts b/src/gateway/server.sessions.create.test.ts index b25ab1cbe7ee..82cbad597519 100644 --- a/src/gateway/server.sessions.create.test.ts +++ b/src/gateway/server.sessions.create.test.ts @@ -10,6 +10,7 @@ import { useAutoCleanupTempDirTracker } from "../../test/helpers/temp-dir.js"; import { findGitCheckoutRoot } from "../agents/worktrees/git.js"; import { findLiveRegistryWorktreeByOwner, + getRegistryWorktree, listRegistryWorktrees, } from "../agents/worktrees/registry.js"; import { managedWorktrees } from "../agents/worktrees/service.js"; @@ -26,6 +27,7 @@ import { resolveSqliteTargetFromSessionStorePath } from "../config/sessions/sess import type { OpenClawConfig } from "../config/types.openclaw.js"; import { createEmptyPluginRegistry } from "../plugins/registry-empty.js"; import { setActivePluginRegistry } from "../plugins/runtime.js"; +import { isSessionLifecycleMutationActive } from "../sessions/session-lifecycle-admission.js"; import { listSessionStateEventsSince } from "../sessions/session-state-events.js"; import { closeOpenClawAgentDatabasesForTest, @@ -55,6 +57,7 @@ import { sessionHookMocks, sessionLifecycleHookMocks, seedSessionTranscript, + threadBindingMocks, } from "./test/server-sessions.test-helpers.js"; type EnsureSessionDiffBaseline = @@ -893,6 +896,162 @@ test("sessions.create rejects draft visibility when policy disables drafts", asy }); }); +test("sessions.create provisions its worktree inside the target lifecycle fence", async () => { + const openClawState = await createOpenClawTestState({ + layout: "state-only", + prefix: "openclaw-session-worktree-fence-", + }); + const workspace = await initializeGitWorkspace(openClawState.root); + closeOpenClawStateDatabaseForTest(); + testState.agentConfig = { workspace }; + const { storePath } = await createSessionStoreDir(); + const key = "agent:main:dashboard:worktree-fence"; + const originalCreate = managedWorktrees.create.bind(managedWorktrees); + const createSpy = vi.spyOn(managedWorktrees, "create").mockImplementation(async (params) => { + expect(isSessionLifecycleMutationActive(storePath, [key])).toBe(true); + return await originalCreate(params); + }); + let worktreeId: string | undefined; + try { + const created = await directSessionReq<{ + worktree: { id: string; path: string; branch: string }; + }>( + "sessions.create", + { key, agentId: "main", worktree: true }, + { client: { connect: { scopes: ["operator.admin"] } } as never }, + ); + expect(created.ok).toBe(true); + worktreeId = created.payload?.worktree.id; + expect(createSpy).toHaveBeenCalledTimes(1); + } finally { + createSpy.mockRestore(); + if (worktreeId) { + await managedWorktrees.remove({ id: worktreeId, reason: "test-cleanup", force: true }); + } + closeOpenClawStateDatabaseForTest(); + testState.agentConfig = undefined; + await openClawState.cleanup(); + } +}); + +test("sessions.create rolls back failed provisioning before a same-key creator proceeds", async () => { + const openClawState = await createOpenClawTestState({ + layout: "state-only", + prefix: "openclaw-session-worktree-rollback-", + }); + const workspace = await initializeGitWorkspace(openClawState.root); + closeOpenClawStateDatabaseForTest(); + testState.agentConfig = { workspace }; + testState.sessionConfig = { sharing: { drafts: false } }; + const { storePath } = await createSessionStoreDir(); + const key = "agent:main:dashboard:worktree-rollback"; + const adminClient = { connect: { scopes: ["operator.admin"] } } as never; + const originalRemove = managedWorktrees.remove.bind(managedWorktrees); + let failedWorktreeId: string | undefined; + let successorWorktreeId: string | undefined; + let releaseRollback = () => {}; + const rollbackGate = new Promise((resolve) => { + releaseRollback = resolve; + }); + let markRollbackStarted = () => {}; + const rollbackStarted = new Promise((resolve) => { + markRollbackStarted = resolve; + }); + const removeSpy = vi.spyOn(managedWorktrees, "remove").mockImplementation(async (params) => { + if (params.reason === "session-create-failed") { + failedWorktreeId = params.id; + markRollbackStarted(); + expect(isSessionLifecycleMutationActive(storePath, [key])).toBe(true); + await rollbackGate; + } + return await originalRemove(params); + }); + try { + const failedPromise = directSessionReq( + "sessions.create", + { + key, + agentId: "main", + visibility: "draft", + worktree: true, + }, + { client: adminClient }, + ); + await rollbackStarted; + let successorSettled = false; + const successorPromise = directSessionReq<{ + entry: { worktree?: { id: string; branch: string; repoRoot: string } }; + worktree: { id: string; path: string; branch: string }; + }>("sessions.create", { key, agentId: "main", worktree: true }, { client: adminClient }).then( + (result) => { + successorSettled = true; + return result; + }, + ); + await Promise.resolve(); + expect(successorSettled).toBe(false); + + releaseRollback(); + const [failed, successor] = await Promise.all([failedPromise, successorPromise]); + expect(failed).toMatchObject({ + ok: false, + error: { + code: "INVALID_REQUEST", + message: "session visibility is disabled: draft", + details: { code: "SESSION_VISIBILITY_DISABLED", visibility: "draft" }, + }, + }); + expect(failedWorktreeId).toBeTruthy(); + expect(getRegistryWorktree(process.env, failedWorktreeId!)).toMatchObject({ + removedAt: expect.any(Number), + }); + expect(successor.ok).toBe(true); + const successorWorktree = successor.payload!.worktree; + successorWorktreeId = successorWorktree.id; + expect(successorWorktree.id).not.toBe(failedWorktreeId); + await expect(fs.access(successorWorktree.path)).resolves.toBeUndefined(); + expect(loadSessionEntry({ sessionKey: key, storePath })?.worktree).toEqual({ + id: successorWorktree.id, + branch: successorWorktree.branch, + repoRoot: workspace, + }); + + const adoptedFailure = await directSessionReq( + "sessions.create", + { key, agentId: "main", visibility: "draft", worktree: true }, + { client: adminClient }, + ); + expect(adoptedFailure).toMatchObject({ + ok: false, + error: { message: "sessions.create visibility requires a new session" }, + }); + expect( + removeSpy.mock.calls.some( + ([params]) => + params.reason === "session-create-failed" && params.id === successorWorktree.id, + ), + ).toBe(false); + expect(getRegistryWorktree(process.env, successorWorktree.id)?.removedAt).toBeUndefined(); + } finally { + releaseRollback(); + removeSpy.mockRestore(); + if ( + successorWorktreeId && + getRegistryWorktree(process.env, successorWorktreeId)?.removedAt === undefined + ) { + await managedWorktrees.remove({ + id: successorWorktreeId, + reason: "test-cleanup", + force: true, + }); + } + closeOpenClawStateDatabaseForTest(); + testState.agentConfig = undefined; + testState.sessionConfig = undefined; + await openClawState.cleanup(); + } +}); + test("sessions.create provisions and reuses a session worktree for later runs", async () => { const openClawState = await createOpenClawTestState({ layout: "state-only", @@ -1512,6 +1671,8 @@ test("sessions.create reset-in-place persists the returned worktree cwd", async const { storePath } = await createSessionStoreDir(); await writeSessionStore({ entries: { main: sessionStoreEntry("sess-reset-parent") } }); let worktreeId: string | undefined; + let releaseWorktreeRemoval = () => {}; + let restoreRemoveIfLossless = () => {}; try { const created = await directSessionReq<{ key: string; @@ -1542,9 +1703,32 @@ test("sessions.create reset-in-place persists the returned worktree cwd", async worktree?.path, ); - // A later plain New Chat on the same main session must leave the worktree: cwd clears - // and the (clean) session worktree is lossless-removed rather than left orphaned. - const reset = await directSessionReq<{ + // Pause the exact old-binding removal before destructive work. A same-key + // worktree reset must remain fenced until that prior generation is gone. + const originalRemoveIfLossless = managedWorktrees.removeIfLossless.bind(managedWorktrees); + const removalGate = new Promise((resolve) => { + releaseWorktreeRemoval = resolve; + }); + let markRemovalStarted = () => {}; + const removalStarted = new Promise((resolve) => { + markRemovalStarted = resolve; + }); + const removeIfLosslessSpy = vi + .spyOn(managedWorktrees, "removeIfLossless") + .mockImplementation(async (id) => { + if (id === worktree?.id) { + expect(threadBindingMocks.unbindThreadBindingsBySessionKey).toHaveBeenCalledWith({ + targetSessionKey: "agent:main:main", + reason: "session-reset", + }); + markRemovalStarted(); + expect(isSessionLifecycleMutationActive(storePath, ["agent:main:main"])).toBe(true); + await removalGate; + } + return await originalRemoveIfLossless(id); + }); + restoreRemoveIfLossless = () => removeIfLosslessSpy.mockRestore(); + const resetPromise = directSessionReq<{ key: string; entry: { spawnedCwd?: string }; resolved: { modelProvider?: string; model?: string }; @@ -1553,23 +1737,53 @@ test("sessions.create reset-in-place persists the returned worktree cwd", async { agentId: "main", parentSessionKey: "main", emitCommandHooks: true }, { client: { connect: { scopes: ["operator.write"] } } as never }, ); + await removalStarted; + let successorSettled = false; + const successorPromise = directSessionReq<{ + entry: { spawnedCwd?: string; worktree?: { id: string; branch: string; repoRoot: string } }; + worktree: { id: string; path: string; branch: string }; + }>( + "sessions.create", + { + key: "agent:main:main", + agentId: "main", + worktree: true, + }, + { client: { connect: { scopes: ["operator.admin"] } } as never }, + ).then((result) => { + successorSettled = true; + return result; + }); + await Promise.resolve(); + expect(successorSettled).toBe(false); + releaseWorktreeRemoval(); + const [reset, successor] = await Promise.all([resetPromise, successorPromise]); + restoreRemoveIfLossless(); expect(reset.ok).toBe(true); expect(reset.payload?.entry.spawnedCwd).toBeUndefined(); expect(reset.payload?.resolved).toEqual({ modelProvider: "openai", model: "current-model", }); - expect( - listRegistryWorktrees(process.env).filter( - (record) => - record.ownerKind === "session" && - record.ownerId === "agent:main:main" && - record.removedAt === undefined, - ), - ).toHaveLength(0); - worktreeId = undefined; + expect(getRegistryWorktree(process.env, worktree!.id)?.removedAt).toEqual(expect.any(Number)); + expect(successor.ok).toBe(true); + const successorWorktree = successor.payload!.worktree; + expect(successorWorktree.id).not.toBe(worktree?.id); + worktreeId = successorWorktree.id; + await expect(fs.access(successorWorktree.path)).resolves.toBeUndefined(); + expect(loadSessionEntry({ sessionKey: "agent:main:main", storePath })).toMatchObject({ + spawnedCwd: successorWorktree.path, + worktree: { + id: successorWorktree.id, + branch: successorWorktree.branch, + repoRoot: workspace, + }, + }); + expect(getRegistryWorktree(process.env, successorWorktree.id)?.removedAt).toBeUndefined(); } finally { - if (worktreeId) { + releaseWorktreeRemoval(); + restoreRemoveIfLossless(); + if (worktreeId && getRegistryWorktree(process.env, worktreeId)?.removedAt === undefined) { await managedWorktrees.remove({ id: worktreeId, reason: "test-cleanup", force: true }); } closeOpenClawStateDatabaseForTest(); diff --git a/src/gateway/server.sessions.delete-worktree-lifecycle.test.ts b/src/gateway/server.sessions.delete-worktree-lifecycle.test.ts new file mode 100644 index 000000000000..ca2accfbba03 --- /dev/null +++ b/src/gateway/server.sessions.delete-worktree-lifecycle.test.ts @@ -0,0 +1,307 @@ +// Session delete worktree lifecycle tests protect exact-generation cleanup and +// same-key successor admission. +import { execFile } from "node:child_process"; +import fs from "node:fs/promises"; +import path from "node:path"; +import { promisify } from "node:util"; +import { expect, test, vi } from "vitest"; +import { getRegistryWorktree } from "../agents/worktrees/registry.js"; +import { + acquireWorktreeRunLease, + resolveWorktreeIdForPath, +} from "../agents/worktrees/run-lease.js"; +import { managedWorktrees } from "../agents/worktrees/service.js"; +import { loadSessionEntry } from "../config/sessions/session-accessor.js"; +import { isSessionLifecycleMutationActive } from "../sessions/session-lifecycle-admission.js"; +import { listSessionStateEventsSince } from "../sessions/session-state-events.js"; +import { closeOpenClawStateDatabaseForTest } from "../state/openclaw-state-db.js"; +import { createOpenClawTestState } from "../test-utils/openclaw-test-state.js"; +import { testState, writeSessionStore } from "./test-helpers.js"; +import { + directSessionReq, + sessionStoreEntry, + setupGatewaySessionsTestHarness, + threadBindingMocks, +} from "./test/server-sessions.test-helpers.js"; + +const { createSessionStoreDir } = setupGatewaySessionsTestHarness(); +const execFileAsync = promisify(execFile); + +async function initializeRemoteBackedGitWorkspace(root: string): Promise { + const workspace = path.join(root, "workspace"); + const remote = path.join(root, "remote.git"); + await fs.mkdir(workspace, { recursive: true }); + await execFileAsync("git", ["-C", workspace, "init", "-b", "main"]); + await execFileAsync("git", ["-C", workspace, "config", "user.name", "OpenClaw Test"]); + await execFileAsync("git", [ + "-C", + workspace, + "config", + "user.email", + "openclaw-test@example.invalid", + ]); + await fs.writeFile(path.join(workspace, "README.md"), "base\n"); + await execFileAsync("git", ["-C", workspace, "add", "README.md"]); + await execFileAsync("git", ["-C", workspace, "commit", "-m", "initial"]); + await execFileAsync("git", ["clone", "--bare", workspace, remote]); + await execFileAsync("git", ["-C", workspace, "remote", "add", "origin", remote]); + await execFileAsync("git", ["-C", workspace, "push", "-u", "origin", "main"]); + return await fs.realpath(workspace); +} + +test("sessions.delete keeps same-key successor worktree creation behind exact cleanup", async () => { + const openClawState = await createOpenClawTestState({ + layout: "state-only", + prefix: "openclaw-delete-worktree-successor-", + }); + const workspace = await initializeRemoteBackedGitWorkspace(openClawState.root); + closeOpenClawStateDatabaseForTest(); + testState.agentConfig = { workspace }; + const { storePath } = await createSessionStoreDir(); + const key = "agent:main:dashboard:delete-worktree-successor"; + const creatorProfileId = "delete-worktree-successor-creator"; + const adminClient = { + connect: { scopes: ["operator.admin"] }, + authenticatedUserProfile: { + profileId: creatorProfileId, + displayName: "Delete Worktree Test", + hasAvatar: false, + updatedAt: 1, + }, + } as never; + let successorWorktreeId: string | undefined; + let releaseRemoval = () => {}; + const removalGate = new Promise((resolve) => { + releaseRemoval = resolve; + }); + const originalRemove = managedWorktrees.remove.bind(managedWorktrees); + let markRemovalStarted = () => {}; + const removalStarted = new Promise((resolve) => { + markRemovalStarted = resolve; + }); + const removeSpy = vi.spyOn(managedWorktrees, "remove"); + try { + const predecessor = await directSessionReq<{ + sessionId: string; + worktree: { id: string; path: string; branch: string }; + }>("sessions.create", { key, agentId: "main", worktree: true }, { client: adminClient }); + expect(predecessor.ok).toBe(true); + const predecessorSessionId = predecessor.payload!.sessionId; + const predecessorWorktree = predecessor.payload!.worktree; + + removeSpy.mockImplementation(async (params) => { + if (params.id === predecessorWorktree.id && params.reason === "session-delete") { + expect(isSessionLifecycleMutationActive(storePath, [key, predecessorSessionId])).toBe(true); + expect(threadBindingMocks.unbindThreadBindingsBySessionKey).toHaveBeenCalledWith({ + targetSessionKey: key, + reason: "session-delete", + }); + markRemovalStarted(); + await removalGate; + } + return await originalRemove(params); + }); + + const deletion = directSessionReq<{ deleted: boolean }>("sessions.delete", { + key, + expectedSessionId: predecessorSessionId, + }); + await removalStarted; + let successorSettled = false; + const successorPromise = directSessionReq<{ + sessionId: string; + entry: { spawnedCwd?: string; worktree?: { id: string; branch: string; repoRoot: string } }; + worktree: { id: string; path: string; branch: string }; + }>("sessions.create", { key, agentId: "main", worktree: true }, { client: adminClient }).then( + (result) => { + successorSettled = true; + return result; + }, + ); + await Promise.resolve(); + expect(successorSettled).toBe(false); + + releaseRemoval(); + const [deleted, successor] = await Promise.all([deletion, successorPromise]); + expect(deleted).toMatchObject({ ok: true, payload: { deleted: true } }); + expect(successor.ok).toBe(true); + const successorSessionId = successor.payload!.sessionId; + const successorWorktree = successor.payload!.worktree; + successorWorktreeId = successorWorktree.id; + expect(successorSessionId).not.toBe(predecessorSessionId); + expect(successorWorktree.id).not.toBe(predecessorWorktree.id); + await expect(fs.access(successorWorktree.path)).resolves.toBeUndefined(); + expect(getRegistryWorktree(process.env, successorWorktree.id)?.id).toBe(successorWorktree.id); + expect(getRegistryWorktree(process.env, successorWorktree.id)?.removedAt).toBeUndefined(); + const persisted = loadSessionEntry({ sessionKey: key, storePath }); + expect(persisted).toMatchObject({ + sessionId: successorSessionId, + spawnedCwd: successorWorktree.path, + worktree: { + id: successorWorktree.id, + branch: successorWorktree.branch, + repoRoot: workspace, + }, + }); + expect( + listSessionStateEventsSince(key, "main", 0, 20).events.filter( + (event) => event.kind === "created", + ), + ).toEqual([ + expect.objectContaining({ + sessionId: successorSessionId, + actorType: "human", + actorId: creatorProfileId, + }), + ]); + const admittedWorktreeId = await resolveWorktreeIdForPath({ + sessionEntry: persisted, + candidatePaths: [persisted?.spawnedCwd], + }); + expect(admittedWorktreeId).toBe(successorWorktree.id); + const runLease = await acquireWorktreeRunLease(admittedWorktreeId!); + await runLease.release(); + } finally { + releaseRemoval(); + removeSpy.mockRestore(); + if ( + successorWorktreeId && + getRegistryWorktree(process.env, successorWorktreeId)?.removedAt === undefined + ) { + await managedWorktrees.remove({ + id: successorWorktreeId, + reason: "test-cleanup", + force: true, + }); + } + closeOpenClawStateDatabaseForTest(); + testState.agentConfig = undefined; + await openClawState.cleanup(); + } +}); + +test("sessions.delete reports the exact preserved worktree when cleanup fails", async () => { + const openClawState = await createOpenClawTestState({ + layout: "state-only", + prefix: "openclaw-delete-worktree-preserved-", + }); + const workspace = await initializeRemoteBackedGitWorkspace(openClawState.root); + closeOpenClawStateDatabaseForTest(); + testState.agentConfig = { workspace }; + const { storePath } = await createSessionStoreDir(); + const key = "agent:main:dashboard:delete-worktree-preserved"; + const adminClient = { connect: { scopes: ["operator.admin"] } } as never; + const originalRemove = managedWorktrees.remove.bind(managedWorktrees); + let worktreeId: string | undefined; + const removeSpy = vi.spyOn(managedWorktrees, "remove"); + try { + const created = await directSessionReq<{ + worktree: { id: string; path: string; branch: string }; + }>("sessions.create", { key, agentId: "main", worktree: true }, { client: adminClient }); + expect(created.ok).toBe(true); + const worktree = created.payload!.worktree; + worktreeId = worktree.id; + removeSpy.mockImplementation(async (params) => { + if (params.id === worktree.id && params.reason === "session-delete") { + throw new Error("simulated cleanup failure"); + } + return await originalRemove(params); + }); + + const deleted = await directSessionReq<{ + deleted: boolean; + worktreePreserved?: { id: string; path: string; branch: string }; + }>("sessions.delete", { key }); + + expect(deleted).toMatchObject({ + ok: true, + payload: { + deleted: true, + worktreePreserved: { + id: worktree.id, + path: worktree.path, + branch: worktree.branch, + }, + }, + }); + expect(loadSessionEntry({ sessionKey: key, storePath })).toBeUndefined(); + expect(getRegistryWorktree(process.env, worktree.id)?.removedAt).toBeUndefined(); + await expect(fs.access(worktree.path)).resolves.toBeUndefined(); + } finally { + removeSpy.mockRestore(); + if (worktreeId && getRegistryWorktree(process.env, worktreeId)?.removedAt === undefined) { + await managedWorktrees.remove({ id: worktreeId, reason: "test-cleanup", force: true }); + } + closeOpenClawStateDatabaseForTest(); + testState.agentConfig = undefined; + await openClawState.cleanup(); + } +}); + +test("sessions.delete preserves an entry-bound worktree owned by another principal", async () => { + const openClawState = await createOpenClawTestState({ + layout: "state-only", + prefix: "openclaw-delete-worktree-owner-mismatch-", + }); + const workspace = await initializeRemoteBackedGitWorkspace(openClawState.root); + closeOpenClawStateDatabaseForTest(); + testState.agentConfig = { workspace }; + await createSessionStoreDir(); + const key = "agent:main:dashboard:delete-worktree-owner-mismatch"; + const foreignWorktree = await managedWorktrees.create({ + repoRoot: workspace, + ownerKind: "manual", + ownerId: "foreign-owner", + name: "foreign-owner", + }); + await writeSessionStore({ + entries: { + [key]: sessionStoreEntry("session-owner-mismatch", { + spawnedCwd: foreignWorktree.path, + worktree: { + id: foreignWorktree.id, + branch: foreignWorktree.branch, + repoRoot: foreignWorktree.repoRoot, + }, + }), + }, + }); + const removeSpy = vi.spyOn(managedWorktrees, "remove"); + try { + const deleted = await directSessionReq<{ + deleted: boolean; + worktreePreserved?: { id: string; path: string; branch: string }; + }>("sessions.delete", { key, deleteTranscript: false }); + + expect(deleted).toMatchObject({ + ok: true, + payload: { + deleted: true, + worktreePreserved: { + id: foreignWorktree.id, + path: foreignWorktree.path, + branch: foreignWorktree.branch, + }, + }, + }); + expect(removeSpy).not.toHaveBeenCalled(); + expect(getRegistryWorktree(process.env, foreignWorktree.id)).toMatchObject({ + ownerKind: "manual", + ownerId: "foreign-owner", + }); + expect(getRegistryWorktree(process.env, foreignWorktree.id)?.removedAt).toBeUndefined(); + await expect(fs.access(foreignWorktree.path)).resolves.toBeUndefined(); + } finally { + removeSpy.mockRestore(); + if (getRegistryWorktree(process.env, foreignWorktree.id)?.removedAt === undefined) { + await managedWorktrees.remove({ + id: foreignWorktree.id, + reason: "test-cleanup", + force: true, + }); + } + closeOpenClawStateDatabaseForTest(); + testState.agentConfig = undefined; + await openClawState.cleanup(); + } +}); diff --git a/src/gateway/session-create-service.ts b/src/gateway/session-create-service.ts index 2b8630a6af96..8c1194993483 100644 --- a/src/gateway/session-create-service.ts +++ b/src/gateway/session-create-service.ts @@ -76,6 +76,11 @@ import { createLazyRuntimeModule } from "../shared/lazy-runtime.js"; import { normalizeSessionDeliveryState } from "../utils/delivery-context.shared.js"; import { ADMIN_SCOPE } from "./operator-scopes.js"; import { buildForkedGatewaySessionEntry } from "./session-create-fork-entry.js"; +import { + type PreparedGatewaySessionLifecycle, + type PrepareGatewaySessionLifecycle, + rollbackGatewaySessionPreparation, +} from "./session-lifecycle-preparation.js"; import { resolvePluginSessionOwnershipError } from "./session-plugin-ownership.js"; import { resolveRequestedSessionAgentId } from "./session-request-agent.js"; import { isSessionVisibilityAllowed, resolveSessionVisibility } from "./session-sharing.js"; @@ -249,8 +254,9 @@ export async function createGatewaySession(params: { deny: string[]; }; spawnedCwd?: string; - /** Managed worktree bound to the new session; persisted alongside spawnedCwd. */ - worktree?: { id: string; branch: string; repoRoot: string }; + /** Prepares session-owned resources while the target lifecycle fence is held. */ + prepareLifecycle?: PrepareGatewaySessionLifecycle; + onLifecycleCleanupError?: (error: unknown) => void; /** Bind session exec to host=node with this node id; caller scope-checks. */ execNode?: string; /** Working directory interpreted only by execNode. */ @@ -642,7 +648,10 @@ export async function createGatewaySession(params: { commandSource: params.commandSource, ...(params.creation ? { creation: params.creation } : {}), ...(spawnedCwd ? { spawnedCwd } : {}), - ...(params.worktree ? { worktree: params.worktree } : {}), + ...(params.prepareLifecycle ? { prepareLifecycle: params.prepareLifecycle } : {}), + ...(params.onLifecycleCleanupError + ? { onLifecycleCleanupError: params.onLifecycleCleanupError } + : {}), ...(params.execNode ? { execNode: params.execNode } : {}), ...(execCwd ? { execCwd } : {}), ...(params.clearExecBinding ? { clearExecBinding: true } : {}), @@ -670,6 +679,8 @@ export async function createGatewaySession(params: { let createdContext: CreatedGatewaySession | undefined; let createdNewEntry = false; + let preparedLifecycle: PreparedGatewaySessionLifecycle | undefined; + let lifecyclePreparationCommitted = false; const spawnToolPolicy = params.spawnToolPolicy && canonicalParentSessionKey ? { @@ -775,6 +786,22 @@ export async function createGatewaySession(params: { } const target = creationTarget; + const currentTargetEntry = loadSessionEntryReadOnly(target.canonicalKey, { + agentId: target.agentId, + }).entry; + const preparationResult = params.prepareLifecycle + ? await params.prepareLifecycle({ + agentId: target.agentId, + entry: currentTargetEntry, + key: target.canonicalKey, + storePath: target.storePath, + }) + : undefined; + if (preparationResult && !preparationResult.ok) { + return { ok: false, error: preparationResult.error }; + } + preparedLifecycle = preparationResult?.value; + const created = await createSessionEntryWithTranscript( { agentId: target.agentId, @@ -928,7 +955,9 @@ export async function createGatewaySession(params: { return patched; } sessionEntries[target.canonicalKey] = patched.entry; - const spawnedCwd = normalizeOptionalString(params.spawnedCwd); + const spawnedCwd = normalizeOptionalString( + preparedLifecycle?.spawnedCwd ?? params.spawnedCwd, + ); const execNode = normalizeOptionalString(params.execNode); const execCwd = normalizeOptionalString(params.execCwd); const initialAgentHarnessId = params.initialEntry @@ -987,7 +1016,7 @@ export async function createGatewaySession(params: { // Session worktrees adopt cwd only during admin-gated creation; public patching stays // restricted to spawned subagent and ACP lineage. ...(spawnedCwd ? { spawnedCwd } : {}), - ...(params.worktree ? { worktree: params.worktree } : {}), + ...(preparedLifecycle?.worktree ? { worktree: preparedLifecycle.worktree } : {}), ...(execNode ? { execHost: "node", execNode, ...(execCwd ? { execCwd } : {}) } : {}), ...(initialAgentHarnessId ? { agentHarnessId: initialAgentHarnessId } : {}), ...(createdNewEntry && params.authorizedPluginId && !params.catalogTarget @@ -1135,6 +1164,16 @@ export async function createGatewaySession(params: { entry: created.entry, storePath: target.storePath, }; + lifecyclePreparationCommitted = true; + if (createdNewEntry) { + // The created fact belongs to this row generation; record it before a + // same-key delete can acquire the lifecycle fence and purge that state. + recordSessionCreated({ + sessionKey: createdContext.key, + agentId: createdContext.agentId, + entry: createdContext.entry, + }); + } if (canonicalParentSessionKey && parentSessionTarget && params.emitCommandHooks === true) { const parentEntry = currentParentSessionEntry; @@ -1205,15 +1244,16 @@ export async function createGatewaySession(params: { const result = await runExclusiveSessionLifecycleMutation({ targets: lifecycleTargets, run: createChildSession, + finalize: async () => { + if (!lifecyclePreparationCommitted) { + await rollbackGatewaySessionPreparation({ + prepared: preparedLifecycle, + onError: params.onLifecycleCleanupError, + }); + } + }, }); if (result.ok && !result.resetExisting && createdContext) { - if (createdNewEntry) { - recordSessionCreated({ - sessionKey: createdContext.key, - agentId: createdContext.agentId, - entry: createdContext.entry, - }); - } await params.afterCreate?.(createdContext); } return result; diff --git a/src/gateway/session-lifecycle-preparation.ts b/src/gateway/session-lifecycle-preparation.ts new file mode 100644 index 000000000000..bebccc7378c2 --- /dev/null +++ b/src/gateway/session-lifecycle-preparation.ts @@ -0,0 +1,27 @@ +import type { Result } from "@openclaw/normalization-core/result"; +import type { ErrorShape } from "../../packages/gateway-protocol/src/index.js"; +import type { SessionEntry } from "../config/sessions/types.js"; + +export type PreparedGatewaySessionLifecycle = { + spawnedCwd?: string; + worktree?: NonNullable; + rollback?: () => Promise; +}; + +export type PrepareGatewaySessionLifecycle = (target: { + agentId: string; + entry?: SessionEntry; + key: string; + storePath: string; +}) => Promise>; + +export async function rollbackGatewaySessionPreparation(params: { + onError?: (error: unknown) => void; + prepared?: PreparedGatewaySessionLifecycle; +}): Promise { + try { + await params.prepared?.rollback?.(); + } catch (error) { + params.onError?.(error); + } +} diff --git a/src/gateway/session-reset-service.ts b/src/gateway/session-reset-service.ts index 928cb672d35a..69067a78f846 100644 --- a/src/gateway/session-reset-service.ts +++ b/src/gateway/session-reset-service.ts @@ -22,6 +22,7 @@ import { import { clearAllCliSessions } from "../agents/cli-session.js"; import { resetRegisteredAgentHarnessSessions } from "../agents/harness/registry.js"; import { resolveSessionModelRef } from "../agents/session-model-ref.js"; +import { managedWorktrees } from "../agents/worktrees/service.js"; import { stopSubagentsForRequester } from "../auto-reply/reply/abort.js"; import { buildSessionEndHookPayload, @@ -91,6 +92,11 @@ import { noteActiveSessionForShutdown, } from "./active-sessions-shutdown-tracker.js"; import { findDirectChildSessionsForParent } from "./session-child-sessions.js"; +import { + type PreparedGatewaySessionLifecycle, + type PrepareGatewaySessionLifecycle, + rollbackGatewaySessionPreparation, +} from "./session-lifecycle-preparation.js"; import { resolvePluginSessionOwnershipError } from "./session-plugin-ownership.js"; import { notifyGatewaySessionReset } from "./session-reset-notifications.js"; import { @@ -1001,8 +1007,9 @@ export async function performGatewaySessionReset(params: { key: string; agentId?: string; spawnedCwd?: string; - /** Managed worktree adopted by this reset; cleared together with spawnedCwd. */ - worktree?: { id: string; branch: string; repoRoot: string }; + /** Prepares session-owned resources while the target lifecycle fence is held. */ + prepareLifecycle?: PrepareGatewaySessionLifecycle; + onLifecycleCleanupError?: (error: unknown) => void; /** Bind session exec to host=node with this node id; caller scope-checks. */ execNode?: string; /** Working directory interpreted only by execNode. */ @@ -1077,6 +1084,13 @@ export async function performGatewaySessionReset(params: { if (!resetTarget.ok) { return resetTarget; } + const reportLifecycleCleanupError = (error: unknown) => { + if (params.onLifecycleCleanupError) { + params.onLifecycleCleanupError(error); + return; + } + logVerbose(`session lifecycle resource cleanup failed: ${String(error)}`); + }; const initialResetEntry = loadSessionEntry( params.key, resetTarget.requestedAgentId ? { agentId: resetTarget.requestedAgentId } : undefined, @@ -1149,6 +1163,8 @@ export async function performGatewaySessionReset(params: { let admittedWorkReleased = true; let resetPreparationError: ReturnType | undefined; let preparedResetSessionId: string | undefined; + let preparedLifecycle: PreparedGatewaySessionLifecycle | undefined; + let lifecyclePreparationCommitted = false; return await runExclusiveSessionLifecycleMutation({ scope: resetTarget.storePath, identities: resetLifecycleIdentities, @@ -1220,6 +1236,19 @@ export async function performGatewaySessionReset(params: { identities: resetLifecycleIdentities, timeoutMs: SESSION_WORK_ADMISSION_DRAIN_TIMEOUT_MS, }); + if (admittedWorkReleased && params.prepareLifecycle) { + const prepared = await params.prepareLifecycle({ + agentId: resetTarget.target.agentId, + entry: currentEntry, + key: resetTarget.target.canonicalKey, + storePath: resetTarget.storePath, + }); + if (!prepared.ok) { + resetPreparationError = prepared.error; + return; + } + preparedLifecycle = prepared.value; + } }, run: async () => { const { cfg, target, storePath, requestedAgentId } = resetTarget; @@ -1307,6 +1336,9 @@ export async function performGatewaySessionReset(params: { }; } const hadExistingEntry = Boolean(entry); + const detachedWorktreeId = params.clearSpawnedCwd + ? normalizeOptionalString(entry?.worktree?.id) + : undefined; const resetLifecycleRevision = entry?.lifecycleRevision; const agentId = normalizeAgentId(target.agentId ?? resolveDefaultAgentId(cfg)); const workspaceDir = resolveAgentWorkspaceDir(cfg, agentId); @@ -1579,10 +1611,10 @@ export async function performGatewaySessionReset(params: { spawnedWorkspaceDir: currentEntry?.spawnedWorkspaceDir, spawnedCwd: params.clearSpawnedCwd ? undefined - : (params.spawnedCwd ?? currentEntry?.spawnedCwd), + : (preparedLifecycle?.spawnedCwd ?? params.spawnedCwd ?? currentEntry?.spawnedCwd), worktree: params.clearSpawnedCwd ? undefined - : (params.worktree ?? currentEntry?.worktree), + : (preparedLifecycle?.worktree ?? currentEntry?.worktree), parentSessionKey: currentEntry?.parentSessionKey, ...creationStamp, forkSource: currentEntry?.forkSource, @@ -1696,6 +1728,7 @@ export async function performGatewaySessionReset(params: { }); const lifecycle: Awaited> = await lifecyclePromise; + lifecyclePreparationCommitted = !resetSkipped; if (!resetSkipped) { const resetSessionKey = target.canonicalKey ?? params.key; handleSessionStateSessionReset(resetSessionKey); @@ -1746,6 +1779,15 @@ export async function performGatewaySessionReset(params: { reason: "session-reset", }); } + if (!resetSkipped && detachedWorktreeId) { + // Preserve reset notifications and unbinding order, but finalize the exact + // old checkout before the fence opens to same-key successors. + try { + await managedWorktrees.removeIfLossless(detachedWorktreeId); + } catch (error) { + reportLifecycleCleanupError(error); + } + } return { ok: true, key: target.canonicalKey, @@ -1755,6 +1797,14 @@ export async function performGatewaySessionReset(params: { storePath, }; }, + finalize: async () => { + if (!lifecyclePreparationCommitted) { + await rollbackGatewaySessionPreparation({ + prepared: preparedLifecycle, + onError: reportLifecycleCleanupError, + }); + } + }, }); } /* oxlint-disable max-lines -- TODO: split this grandfathered oversized file. */ diff --git a/src/sessions/session-lifecycle-admission.test.ts b/src/sessions/session-lifecycle-admission.test.ts index e68eaa541099..8150525a3a95 100644 --- a/src/sessions/session-lifecycle-admission.test.ts +++ b/src/sessions/session-lifecycle-admission.test.ts @@ -1,6 +1,7 @@ // Tests lifecycle/work admission ordering across canonical keys and backing ids. +import { setImmediate as waitForImmediate } from "node:timers/promises"; import { importFreshModule } from "openclaw/plugin-sdk/test-fixtures"; -import { expect, it } from "vitest"; +import { expect, it, vi } from "vitest"; import { createDeferred } from "../../test/helpers/promise.js"; import { runExclusiveSessionStoreWrite } from "../config/sessions/store-writer.js"; import { @@ -257,6 +258,70 @@ it("counts one multi-identity lifecycle mutation once across module instances", expect(second.getActiveSessionLifecycleMutationCount()).toBe(0); }); +it("keeps a same-identity mutation queued until finalization completes", async () => { + const target = { scope: "store-finalize-order", identities: ["session-finalize-order"] }; + const finalizeStarted = createDeferred(); + const releaseFinalize = createDeferred(); + let secondRan = false; + const first = runExclusiveSessionLifecycleMutation({ + ...target, + run: async () => {}, + finalize: async () => { + finalizeStarted.resolve(); + await releaseFinalize.promise; + }, + }); + await finalizeStarted.promise; + + const second = runExclusiveSessionLifecycleMutation({ + ...target, + run: async () => { + secondRan = true; + }, + }); + await waitForImmediate(); + expect(secondRan).toBe(false); + + releaseFinalize.resolve(); + await Promise.all([first, second]); +}); + +it("finalizes a lifecycle mutation when its run throws", async () => { + const runError = new Error("lifecycle run failed"); + const finalize = vi.fn(async () => {}); + + await expect( + runExclusiveSessionLifecycleMutation({ + scope: "store-finalize-run-error", + identities: ["session-finalize-run-error"], + run: async () => { + throw runError; + }, + finalize, + }), + ).rejects.toBe(runError); + expect(finalize).toHaveBeenCalledOnce(); +}); + +it("releases lifecycle state when finalization throws", async () => { + const target = { scope: "store-finalize-error", identities: ["session-finalize-error"] }; + const finalizeError = new Error("lifecycle finalizer failed"); + + await expect( + runExclusiveSessionLifecycleMutation({ + ...target, + run: async () => {}, + finalize: async () => { + throw finalizeError; + }, + }), + ).rejects.toBe(finalizeError); + expect(isSessionLifecycleMutationActive(target.scope, target.identities)).toBe(false); + await expect( + runExclusiveSessionLifecycleMutation({ ...target, run: async () => "next" }), + ).resolves.toBe("next"); +}); + it("counts a cross-store lifecycle mutation once and fences every target", async () => { const mutationStarted = createDeferred(); const releaseMutation = createDeferred(); diff --git a/src/sessions/session-lifecycle-admission.ts b/src/sessions/session-lifecycle-admission.ts index f2276c0cf30d..3679ca437fb4 100644 --- a/src/sessions/session-lifecycle-admission.ts +++ b/src/sessions/session-lifecycle-admission.ts @@ -48,6 +48,7 @@ type SessionLifecycleMutationTarget = { type SessionLifecycleMutationParams = { kind?: SessionLifecycleMutationKind; prepare?: () => Promise; + finalize?: () => Promise; run: () => Promise; signal?: AbortSignal; } & (SessionLifecycleMutationTarget | { targets: Iterable }); @@ -241,34 +242,40 @@ export async function runExclusiveSessionLifecycleMutation( await params.prepare?.(); return await runWithSessionIdentityLocks(identities, 0, params.run); } finally { - await runWithSessionIdentityLocks(identities, 0, async () => { - for (const identity of identities) { - if (params.kind) { - const kinds = ACTIVE_SESSION_LIFECYCLE_MUTATION_KINDS.get(identity); - const remainingKindCount = (kinds?.get(params.kind) ?? 1) - 1; - if (remainingKindCount > 0) { - kinds?.set(params.kind, remainingKindCount); - } else { - kinds?.delete(params.kind); - if (kinds?.size === 0) { - ACTIVE_SESSION_LIFECYCLE_MUTATION_KINDS.delete(identity); + // Resource finalization is part of the mutation: successors remain + // fenced until rollback or exact-generation cleanup has completed. + try { + await params.finalize?.(); + } finally { + await runWithSessionIdentityLocks(identities, 0, async () => { + for (const identity of identities) { + if (params.kind) { + const kinds = ACTIVE_SESSION_LIFECYCLE_MUTATION_KINDS.get(identity); + const remainingKindCount = (kinds?.get(params.kind) ?? 1) - 1; + if (remainingKindCount > 0) { + kinds?.set(params.kind, remainingKindCount); + } else { + kinds?.delete(params.kind); + if (kinds?.size === 0) { + ACTIVE_SESSION_LIFECYCLE_MUTATION_KINDS.delete(identity); + } } } + const remaining = (ACTIVE_SESSION_LIFECYCLE_MUTATIONS.get(identity) ?? 1) - 1; + if (remaining > 0) { + ACTIVE_SESSION_LIFECYCLE_MUTATIONS.set(identity, remaining); + continue; + } + ACTIVE_SESSION_LIFECYCLE_MUTATIONS.delete(identity); + const waiters = SESSION_LIFECYCLE_IDLE_WAITERS.get(identity); + SESSION_LIFECYCLE_IDLE_WAITERS.delete(identity); + for (const resolve of waiters ?? []) { + resolve(); + } } - const remaining = (ACTIVE_SESSION_LIFECYCLE_MUTATIONS.get(identity) ?? 1) - 1; - if (remaining > 0) { - ACTIVE_SESSION_LIFECYCLE_MUTATIONS.set(identity, remaining); - continue; - } - ACTIVE_SESSION_LIFECYCLE_MUTATIONS.delete(identity); - const waiters = SESSION_LIFECYCLE_IDLE_WAITERS.get(identity); - SESSION_LIFECYCLE_IDLE_WAITERS.delete(identity); - for (const resolve of waiters ?? []) { - resolve(); - } - } - ACTIVE_SESSION_LIFECYCLE_MUTATION_RUNS.delete(mutationRun); - }); + ACTIVE_SESSION_LIFECYCLE_MUTATION_RUNS.delete(mutationRun); + }); + } } }), "mutation",